-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: The XBlock should work even when max_attempts is null #155
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @xitij2000! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:
Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
7452d76
to
0758b56
Compare
Hi @openedx/2u-arbi-bom! Would someone be able to please review / merge this for us? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- I tested this: Empty max_attempts is working
- I read through the code
0758b56
to
c9f583f
Compare
You need to fix the commit message.
|
f8ff4aa
to
bd9937c
Compare
Done! |
Is this good to merge? |
bd9937c
to
6388a54
Compare
@openedx/2u-arbi-bom hi there! is someone able to take a look at this for us? |
Once someone assigns themselves for review, we can resolve conflicts. |
@itsjeyd @mphilbrick211 Just checking on this, how can we push this forward? |
@xitij2000 @mphilbrick211 It looks like arbi-bom was reviewing this earlier: #155 (review). How about re-requesting a review from |
@awais786 Could you have another look at this PR since you reviewed it a while back? |
6388a54
to
7719c46
Compare
@itsjeyd The PR has already been reviewed and approved by him, it just needs a quick review and approval from someone with merge rights as well. |
This change allows the XBlock to continue working in cases where max_attempts is set to null. Currently Studio and LMS will throw an error in such a case due to a number of places where max_attempts is compared to count_attempts without testing for None first.
7719c46
to
2185505
Compare
@xitij2000 Ah right, I was assuming that |
@xitij2000 I don't have merging rights. Also few jobs are failing in this PR. |
Overview
This change allows the XBlock to continue working in cases where max_attempts is set to null. Currently Studio and LMS will throw an error in such a case due to a number of places where max_attempts is compared to count_attempts without testing for None first.
Test Instructions
TODO
setup.py
edx-platform
to bump the version