Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xitij2000
Copy link

@xitij2000 xitij2000 commented Jan 24, 2024

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

  • Set up this XBlock in a course
  • Set the max_attempts in course advanced setting to 1, save, and then set it to null and save
  • Test that the XBlock is working

TODO

  • Compile static assets
  • Lint all files
  • Pass all tests
  • Bump the version number in setup.py
  • Attach screenshots?
  • Code Reviewer 1:
  • Code Reviewer 2:
  • Submit PR against edx-platform to bump the version
  • Upload to PyPi

@openedx-webhooks
Copy link

Thanks for the pull request, @xitij2000! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 24, 2024
@xitij2000 xitij2000 force-pushed the kshitij/fix-max-attempts-null branch from 7452d76 to 0758b56 Compare January 24, 2024 08:05
@mphilbrick211 mphilbrick211 requested a review from a team January 24, 2024 18:07
@mphilbrick211
Copy link

mphilbrick211 commented Jan 24, 2024

Hi @openedx/2u-arbi-bom! Would someone be able to please review / merge this for us? Thanks!

Copy link

@mavidser mavidser left a 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

@xitij2000 xitij2000 force-pushed the kshitij/fix-max-attempts-null branch from 0758b56 to c9f583f Compare January 30, 2024 03:18
@awais786
Copy link

You need to fix the commit message.

 input: fixup: Update tests
✖   type must be one of [revert, feat, fix, perf, docs, test, build, refactor, style, chore, temp] [type-enum]

@xitij2000 xitij2000 force-pushed the kshitij/fix-max-attempts-null branch from f8ff4aa to bd9937c Compare January 30, 2024 14:51
@xitij2000
Copy link
Author

You need to fix the commit message.

 input: fixup: Update tests
✖   type must be one of [revert, feat, fix, perf, docs, test, build, refactor, style, chore, temp] [type-enum]

Done!

@xitij2000
Copy link
Author

You need to fix the commit message.

 input: fixup: Update tests
✖   type must be one of [revert, feat, fix, perf, docs, test, build, refactor, style, chore, temp] [type-enum]

Is this good to merge?

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.
@xitij2000 xitij2000 force-pushed the kshitij/fix-max-attempts-null branch from bd9937c to 6388a54 Compare April 30, 2024 12:21
@mphilbrick211
Copy link

@openedx/2u-arbi-bom hi there! is someone able to take a look at this for us?

@mphilbrick211 mphilbrick211 added the needs reviewer assigned PR needs to be (re-)assigned a new reviewer label Jul 16, 2024
@mphilbrick211
Copy link

Once someone assigns themselves for review, we can resolve conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviewer assigned PR needs to be (re-)assigned a new reviewer open-source-contribution PR author is not from Axim or 2U
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

5 participants