-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat: Adds disable_progress_graph attribute to the returned course_me… #33308
Conversation
Thanks for the pull request, @RafayGhafoor! 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:
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. |
@navinkarkera, please check when you get time. |
654e59c
to
29bbb2c
Compare
@ormsbee, Please review. @openedx/cla-problems, I have signed the CLA but still no response have been received. Please check, Thank you. |
6b479ab
to
57ede02
Compare
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.
@RafayGhafoor It works nicely, just added some suggestions to move this to progress related API instead of using course metadata view. Please have a look.
PS: While testing, I found a small bug in fronted-app-course-authoring for which I have created a fix PR: openedx/frontend-app-authoring#610
de3a246
to
9d2d126
Compare
@navinkarkera, I have addressed the changes. PTAL. |
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.
👍 @RafayGhafoor Great work 💯
- I tested this: (tested progress graph locally in devstack)
- I read through the code
-
I checked for accessibility issues -
Includes documentation -
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-secure
repository.
cb1a021
to
0d56501
Compare
Hi @openedx/content-aurora! Would someone be able to take a look at 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 on my devstack and it works cleanly.
✅ I read through the code
❌ I checked for accessibility issues
❌ Includes documentation
❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.
@mphilbrick211, can we please get this PR reviewed? Thanks. |
Hi @RafayGhafoor - someone from @openedx/content-aurora needs to review this. I have pinged them again. I apologize for the delay! |
0d56501
to
b2333d6
Compare
@mphilbrick211 I can merge if it is ok with you. |
…tadata response Currently, openedx/frontend-app-authoring#517 faces an issue when the progress graph toggle is enabled/disabled but the settings are not respected, the disable_progress_graph attribute will allow the frontend-app-learning repo to use this attribute to respect the settings authored from frontend-app-course-authoring and ultimately fix openedx/frontend-app-authoring#517.
b2333d6
to
45f5e24
Compare
@RafayGhafoor 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Closes #517
Adds disable_progress_graph attribute to the returned course_metadata response
Description
Currently, #517 faces an issue when the progress graph toggle is enabled/disabled but the settings are not respected, this PR enables the disable_progress_graph attribute to be sent in the response so the front-end can be allowed to display/hide the graph based on the defined course-wide settings.
Testing Instructions:
Please follow instructions from the this PR
Results: