feat [FC-86]: update course_about & catalog link generation - #37342
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Serj-N! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Serj-N
changed the title
f157bdb to
c64ec1f
Compare
8a36d27 to
47f5cb8
Compare
| ZENDESK_API_KEY = "" | ||
| ZENDESK_USER = "" | ||
|
|
||
| CATALOG_MICROFRONTEND_URL = "http://catalog-mfe" |
There was a problem hiding this comment.
Please add with the other mfe urls: https://github.com/openedx/edx-platform/blob/47f5cb84bcca1aa162a55879e0ea675377ee1a17/lms/envs/test.py#L570-L579
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
This needs a few changes but the approach and the places where you've made the code changes make sense to me so I think once we agree on the implementation details, this should be good to go.
There was a problem hiding this comment.
With the change as you have it, when we move to the new MFE, the CourseAboutPageURLRequested filter will stop working. We don't want to do that, but also the fact that this filter only runs in the last conditional is a bug in my opinion. Also, the way the URL is built in the final else clause is nearly the same as in the new clause you're adding. I think you should do the following:
- Pull Lines 66-71 which call the filter, out of the if clause so that it always runs. This is I think the correct behavior long-term.
- Extract the
about_base_urlvalue up from the last clause and set it to the mfe base or lms base based on the conditional. This has a few benefits.
a. When we come back to clean this up as a part of dropping the old LMS implementation, the cleanup is easier because it's obvious what to delete.
b. The hardcoded url structure only lives on one line and isn't duplicated spreading the assumption about about page URL structure.
c. The other two if statements look like legacy 2U specific code and hopefully will be removed in the future in which case all the code worth keeping is localized in the last clause making that future cleanup easier.
There was a problem hiding this comment.
Good points, both changes implemented.
| Returns a boolean = true if the Catalog MFE is enabled. | ||
| """ | ||
| return configuration_helpers.get_value( | ||
| 'ENABLE_CATALOG_MICROFRONTEND', settings.FEATURES['ENABLE_CATALOG_MICROFRONTEND'] | ||
| 'ENABLE_CATALOG_MICROFRONTEND', settings.FEATURES.get('ENABLE_CATALOG_MICROFRONTEND') | ||
| ) |
There was a problem hiding this comment.
You are now saying that this returns a boolean, which means in should never return None which can currently happen if the setting is not set. Either set a default for this value here or in the common.py settings file (I would prefer in the common.py settings file).
Also note, we have stopped using the FEATURES dictionary as a part of the platform settings simplification work. You should not use it in your new code and update anything that you're touching to drop its usage as it makes sense. You don't need to fix everything but do the minimal necessary to not add new code that uses this deprecated setting.
There was a problem hiding this comment.
ENABLE_CATALOG_MICROFRONTEND is already in common.py. I changed this line to look up the key directly in settings, and adjusted the many tests that looked up this value from FEATURES.
|
@Serj-N can you please address failing tests so I can ask Feanil for another review today. I see |
bba5faa to
cb4dcb0
Compare
|
@sarina Done, tests fixed |
github-project-automation
Bot
moved this from Ready for Review
to Done
in Contributions
|
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. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
7 participants
Note
Related to: #37341
Description
This PR updates the logic for generating URLs for the course about page and catalog page.
Key changes:
In
get_link_for_about_page(course), ifuse_catalog_mfe()is true:${CATALOG_MICROFRONTEND_URL}/courses/<course_id>/aboutIn
get_platform_settings(), ifuse_catalog_mfe()is true:${CATALOG_MICROFRONTEND_URL}/coursesConfiguration:
CATALOG_MICROFRONTEND_URLadded as a Django setting for constructing MFE URLs.