fix: make progress graph respect course settings - #1194
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. |
|
@Agrendalath, Please check when you have time. Thank you. |
|
@RafayGhafoor, I believe @navinkarkera is the reviewer here. |
|
Thank you, I will mention him here. @navinkarkera, Please check when you time. |
navinkarkera
left a comment
There was a problem hiding this comment.
@RafayGhafoor I have left some suggestions in edx-platform PR which will require some changes here as well.
b807547 to
c064e07
Compare
|
@navinkarkera, I have made the changes. |
navinkarkera
left a comment
There was a problem hiding this comment.
- 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-securerepository.
farhaanbukhsh
left a comment
There was a problem hiding this comment.
👍
✅ 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.
|
@RafayGhafoor Thank you for this contribution! @Agrendalath @farhaanbukhsh Once tests are green, will we need another review from the Aurora team to be able to merge these changes, or are your approvals binding? |
|
@brian-smith-tcril Could you please help us triggering the tests as well as review and merge this? |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1194 +/- ##
==========================================
+ Coverage 87.87% 87.91% +0.04%
==========================================
Files 263 263
Lines 4485 4486 +1
Branches 1133 1134 +1
==========================================
+ Hits 3941 3944 +3
+ Misses 530 528 -2
Partials 14 14
☔ View full report in Codecov by Sentry. |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
This makes sense!
Once https://github.com/openedx/edx-platform/pull/33308/files lands, we will be getting back a boolean value for disable_progress_graph from the progress endpoint.
This MFE will grab that as a part of getProgressTabData
frontend-app-learning/src/course-home/data/api.js
Lines 223 to 224 in 5604def
It will be camelCased and turn into disableProgressGraph
And then we will be able to use it in ProgressTab
LGTM!
|
@brian-smith-tcril I have merged openedx/openedx-platform#33308, this PR can be merged now. |
|
@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. |
(cherry picked from commit 391ea08)
Closes #517
Description:
Currently, #517 faces an issue when the progress graph toggle is enabled/disabled but the settings are not respected, this PR requires merge of #33308 as a prerequisite so the disableProgressGraph attribute is available through course_metadata endpoint.
Testing Instructions:
Perform
git pullon edx-platform repository followed bygit checkout fix-progress-graph, it allows the backend to send disableProgressGraph attribute to our frontend-learning-app.Login as edx user on studio.
Navigate to admin > waffle_utils > Waffle flag org overrides
Add and enable these flags:

Navigate to Pages and Resources Tab in frontend-course-authoring app Demo Course page and toggle Enable Progress Graph to test the settings. Make sure you save the settings to reflect the changes.
Sign out on studio and login as normal user (honor) to perform the check.
Visit demo course progress tab to see if the progress graph is respecting the configuration.
Results:
Disabled Progress Graph:
Toggle Settings:
Result:
Enabled Progress Graph:
Toggle Settings:
Result: