Skip to content

feat: Adds disable_progress_graph attribute to the returned course_me… - #33308

Merged
navinkarkera merged 1 commit into
openedx:masterfrom
RafayGhafoor:fix-progress-graph
Oct 11, 2023
Merged

feat: Adds disable_progress_graph attribute to the returned course_me…#33308
navinkarkera merged 1 commit into
openedx:masterfrom
RafayGhafoor:fix-progress-graph

Conversation

@RafayGhafoor

@RafayGhafoor RafayGhafoor commented Sep 21, 2023

Copy link
Copy Markdown
Contributor

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:

CleanShot 2023-09-21 at 17 47 17@2x

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 21, 2023
@openedx-webhooks

openedx-webhooks commented Sep 21, 2023

Copy link
Copy Markdown

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:

  • 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.

@RafayGhafoor

Copy link
Copy Markdown
Contributor Author

@navinkarkera, please check when you get time.

@RafayGhafoor
RafayGhafoor force-pushed the fix-progress-graph branch 2 times, most recently from 654e59c to 29bbb2c Compare September 22, 2023 07:53
@RafayGhafoor

Copy link
Copy Markdown
Contributor Author

@ormsbee, Please review.

@openedx/cla-problems, I have signed the CLA but still no response have been received. Please check, Thank you.

@RafayGhafoor
RafayGhafoor force-pushed the fix-progress-graph branch 2 times, most recently from 6b479ab to 57ede02 Compare September 24, 2023 17:48

@navinkarkera navinkarkera left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread lms/djangoapps/course_home_api/course_metadata/views.py Outdated
Comment thread openedx/core/djangoapps/content/course_overviews/models.py Outdated
Comment thread lms/djangoapps/course_home_api/course_metadata/serializers.py Outdated
@RafayGhafoor

Copy link
Copy Markdown
Contributor Author

@navinkarkera, I have addressed the changes. PTAL.

@navinkarkera navinkarkera left a comment

Copy link
Copy Markdown
Contributor

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's configuration-secure repository.

@RafayGhafoor
RafayGhafoor force-pushed the fix-progress-graph branch 3 times, most recently from cb1a021 to 0d56501 Compare September 26, 2023 11:56
@mphilbrick211

Copy link
Copy Markdown

Hi @openedx/content-aurora! Would someone be able to take a look at this for us? Thanks!

@farhaanbukhsh farhaanbukhsh left a comment

Copy link
Copy Markdown
Member

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.

@RafayGhafoor

Copy link
Copy Markdown
Contributor Author

@mphilbrick211, can we please get this PR reviewed? Thanks.

@mphilbrick211

Copy link
Copy Markdown

@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!

@navinkarkera

Copy link
Copy Markdown
Contributor

@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.
@navinkarkera
navinkarkera merged commit b14396b into openedx:master Oct 11, 2023
@openedx-webhooks

Copy link
Copy Markdown

@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.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Can't hide progress graph from Studio

6 participants