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.

Sign up for GitHub

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

Jump to bottom

fix: course progress url based on whether the user has learning MFE enabled (for master) #34344

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

fix: course progress url based on whether the user has learning MFE enabled (for master) #34344

wants to merge 1 commit into from
+66 −1

Conversation

FatemeKhodayari
Copy link

Currently, as we have in Quince, when a user hasn't passed the course, the "View Grades" link / button is displayed to him on the dashboard. Clicking on this link / button, takes the user to the old progress page. The URL of the old progress page is returned in the /api/learner_home/init response. In this change, course progress URL is returned based on whether the user has learning MFE enabled or not.

This issue was originally created in this forum topic and later discussed a little bit in the Maintenance WG meeting.

Closes #34323

@FatemeKhodayari FatemeKhodayari requested a review from a team as a code owner March 9, 2024 09:26
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 9, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @FatemeKhodayari! 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.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 11, 2024
@FatemeKhodayari FatemeKhodayari force-pushed the fix/course-progress-page-url-for-master branch from d564408 to 64653f3 Compare March 12, 2024 05:41
@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 13, 2024
@FatemeKhodayari FatemeKhodayari mentioned this pull request Mar 16, 2024
Open
@jsnwesson jsnwesson added the needs maintainer attention Issue or PR specifically needs the attention of the maintainer. label Mar 18, 2024
@FatemeKhodayari FatemeKhodayari force-pushed the fix/course-progress-page-url-for-master branch 2 times, most recently from 7caba09 to 395085e Compare March 21, 2024 11:31
feanil
feanil reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FatemeKhodayari changes look good to me, can you add a test case to validate the change also, then I think this looks good to merge.

@justinhynes
Copy link
Contributor

Hi there -- just mentioning that this is on our radar too for review. I should be able to take a look shortly.

@FatemeKhodayari
Copy link
Author

@FatemeKhodayari changes look good to me, can you add a test case to validate the change also, then I think this looks good to merge.

Sure! I'll try to add the test by Saturday.

@FatemeKhodayari FatemeKhodayari force-pushed the fix/course-progress-page-url-for-master branch from 395085e to 8a1deb1 Compare March 30, 2024 13:46
@FatemeKhodayari
Copy link
Author

Hi @feanil! Just added the test. Please let me know if anything needs changing.

@FatemeKhodayari FatemeKhodayari force-pushed the fix/course-progress-page-url-for-master branch from 8a1deb1 to fad574f Compare March 31, 2024 02:01
feanil
feanil reviewed Apr 8, 2024
View reviewed changes
lms/djangoapps/learner_home/test_serializers.py Outdated
@@ -224,6 +225,18 @@ def test_missing_resume_url(self):
# Then the resumeUrl is None, which is allowed
self.assertIsNone(output_data["resumeUrl"])

def test_progress_url(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test won't always test the code we're trying to test, we should test both possibilities by mocking the course_home_mfe_progress_tab_is_active so that it returns true and false and make sure that the correct expected URL is provided in either case.

See the quick guide if you haven't played with mocking before, and I'm happy to help if you're not sure how to proceed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad. Had totally forgotten about mocking. I'll provide the fix by tomorrow or the day after. Thanks for the review and mentioning @feanil.

@deborahgu deborahgu added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 10, 2024
@FatemeKhodayari FatemeKhodayari force-pushed the fix/course-progress-page-url-for-master branch from fad574f to 5ceb5f1 Compare April 15, 2024 16:46
@FatemeKhodayari
Copy link
Author

Hi @feanil. Could you please have a look at this when possible? I last updated this repo about 3 week ago. Thanks in advance :)

feanil
feanil reviewed May 9, 2024
View reviewed changes
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of final comments then this is good to merge. Soooo sorry for the slow review, the Python 3.11 upgrade ate up all my time.

lms/djangoapps/learner_home/test_utils.py
"""
if course_home_mfe_progress_tab_is_active:
return f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_id}/progress'
return f'/courses/{course_id}/progress'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return f'/courses/{course_id}/progress'
return reverse("progress", kwargs={"course_id": course_id})

You should use the reverse function here if possible.

lms/djangoapps/learner_home/test_utils.py
:return: The course progress URL.
"""
if course_home_mfe_progress_tab_is_active:
return f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_id}/progress'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_id}/progress'
return get_learning_mfe_home_url(course_id, url_fragment='progress')

Why not use the same underlying function, you're testing if the routing logic works so I don't see a reason to hardcode the URL here instead of using the function.

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

@feanil feanil feanil requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs maintainer attention Issue or PR specifically needs the attention of the maintainer. open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
Certificates / Credentials
Status: Triage
Contributions
Status: In Eng Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Course Progress URL points to the old progress page
7 participants
@FatemeKhodayari @openedx-webhooks @justinhynes @feanil @deborahgu @jsnwesson @mphilbrick211