-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: Extend MFE Config API for frontend-app-catalog [FC-0086]
#37130
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: Extend MFE Config API for frontend-app-catalog [FC-0086]
#37130
Conversation
|
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. |
|
|
||
| # Based config values, used in tests to build a correct expected response | ||
| default_base_config = { | ||
| 'course_about_twitter_account': '@YourPlatformTwitterAccount', |
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.
Do we still use Twitter even if it is called X now? Just wondering, no actions required.
| "course_about_twitter_account", | ||
| settings.PLATFORM_TWITTER_ACCOUNT | ||
| ), | ||
| "is_cosmetic_price_enabled": settings.FEATURES.get("ENABLE_COSMETIC_DISPLAY_PRICE"), |
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.
Need to clarify whether we need a MFe config here? I believe the preliminary decision was to allow to override from MFE_CONFIG.
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.
This is still marked as "Unknown" in the "Do we want this functionality?" column of the spreadsheet.
I left a comment there asking:
Could you elaborate on this? I found https://github.com/openedx/edx-platform/blob/0bf88d225f563ad5a81205706a5ed9be91fe0794/lms/templates/courseware/course_about.html#L222-L228 and it's not clear to me how the "when there is no registration price" part of this fits in
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.
Removed as discussed in the spreadsheet
| settings.PLATFORM_TWITTER_ACCOUNT | ||
| ), | ||
| "is_cosmetic_price_enabled": settings.FEATURES.get("ENABLE_COSMETIC_DISPLAY_PRICE"), | ||
| "courses_are_browsable": settings.FEATURES.get("COURSES_ARE_BROWSABLE") |
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.
Non clear decision yet but it looks like we are leaning toward the following fallback in frontend-app-learner-dashboard:
NON_BROWSABLE_COURSES fall back to !(not)COURSES_ARE_BROWSABLE
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.
Changed to NON_BROWSABLE_COURSES
| response = self.client.get(self.mfe_config_api_url) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.json(), settings.MFE_CONFIG) | ||
| self.assertEqual(response.json(), settings.MFE_CONFIG | default_base_config) |
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.
Looks like it differs from this order:
# Merge the three configs in the order of precedence
merged_config = base_config | mfe_config | mfe_config_overridesCould you explain why?
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.
This was appended to make the existing tests pass, and in those particular tests the order of precedence didn't matter much, considering the test values returned. But you are right, it is better to use the established order everywhere to avoid confusion. Thanks for noticing. Done.
| "show_homepage_promo_video": configuration_helpers.get_value( | ||
| "show_homepage_promo_video", | ||
| False | ||
| ), |
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 believe we can remove this.
From our conversations on the spreadsheet:
@brian-smith-tcril: I added a "Yes (needs DEPR)" option to this dropdown. This is functionality we want to keep, but I believe the implementation change will require a DEPR.
@PKulkoRaccoonGang: Am I understanding correctly that the approval will come after we pass the DEPR?
@brian-smith-tcril: My understanding is that we should:
- Not expose
show_homepage_promo_videoto the MFE- Ensure the functionality provided by the
show_homepage_promo_videotoggle (having a homepage without a promo video) still exists in the MFE. This will be covered by the suggested check for a validhomepage_promo_video_youtube_idvalue and a plugin slot.- During the release where site operators can choose to use the legacy pages or the MFE, we will have a large DEPR covering the removal of the legacy pages
- We need to make sure to note this change in that DEPR
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.
Removed
| "show_homepage_promo_video", | ||
| False | ||
| ), | ||
| "homepage_promo_video_youtube_id": configuration_helpers.get_value( |
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.
According to the case of other fields in MFE config it makes sense for the new ones to also be in the format upper case
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.
Changed it to uppercase (even though apparently the official docs do not enforce this).
Done.
|
We've updated the spreadsheet and I think we're quite close to this. @Serj-N if you could work on fixing the tests I think we can take this out of draft when they pass and get @feanil 's review |
feanil
left a comment
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.
The general approach and code makes sense to me and I approve of it, I have one question where I'm looking for more information and a bit more documenting and ticketing that I'd like to see before this merges.
I think we should create the relevant DEPR for the dropping of the legacy settings from the MFE config and maybe fully from the settings as well if they're not used anywhere else. That DEPR should be created and linked in the comments here so that it's easy to go find out why we're doing this in the future and come cleanup after ourselves.
I'm assuming the number of settings that are getting pulled here may change so I'm fine with creating the DEPR once that has all been settled but it should be done before we merge this code.
| ), | ||
| "HOMEPAGE_PROMO_VIDEO_YOUTUBE_ID": configuration_helpers.get_value( | ||
| "homepage_promo_video_youtube_id", | ||
| "your-youtube-id" |
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.
Is this a default value that is being set in the code here? Does it makes sense for this to be None by default? I'm not sure if it's important for this to be a valid string and my assumption is that if this is not some sort of null object, the consumer of the API might try to use it thinking it's valid and then have an unexpected failure because it wouldn't know that this is a string that is not a valid id.
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.
Good point, changed to None
| @@ -31,6 +31,13 @@ class MFEConfigView(APIView): | |||
| def get(self, request): | |||
| """ | |||
| Return the MFE configuration, optionally including MFE-specific overrides. | |||
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.
| Return the MFE configuration, optionally including MFE-specific overrides. | |
| Return the MFE configuration, optionally including MFE-specific overrides. | |
| This configuration currently also pulls specific settings from site configuration or | |
| django settings. This is a temporary change as a part of the migration of some legacy | |
| pages to MFEs. This is a temporary compatibility layer which will eventually be deprecated. | |
| See [Link to DEPR ticket] for more details. | |
| The compatability means that settings from the legacy locations will continue to work but | |
| the settings listed below in the `_get_legacy_config` function should be added to the MFE | |
| config by operators. |
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.
Docstring added with a todo (add link)
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.
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.
| return JsonResponse(merged_config, status=status.HTTP_200_OK) | ||
|
|
||
| @staticmethod | ||
| def _get_base_config() -> dict: |
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.
| def _get_base_config() -> dict: | |
| def _get_legacy_config() -> dict: |
You'll need to update the calls as well but these are specifically a compatibility layer that is meant to be temporary, the naming should indicate that as should the docstring.
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.
Done
I know #36785 has been updated to include information about changes from this PR. |
@sarina Now that I added new commits with the requested changes, the pipeline reran and all the tests pass. I went ahead and changed the PR status from draft to open, too. |
brian-smith-tcril
left a comment
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.
This is looking great! I've gone through to check that the implementations match the decisions in the spreadsheet.
Here's an overview of what has been implemented at time of review:
| Status | Legacy Implementation |
|---|---|
| ✅ | settings.FEATURES["ENABLE_COURSE_SORTING_BY_START_DATE"]) |
| 📤 | homepage_overlay_html in Site Configuration or modifying index_overlay.html |
| 📤 | show_partners - in Site Configuration |
| 📤 | show_homepage_promo_video - in Site Configuration |
| ✅ | homepage_promo_video_youtube_id - in Site Configuration |
| ✅ | homepage_course_max - in Site Configuration or Django settings |
| 📤 | sidebar_html_enabled - WaffleSwitch "course_experience.enable_about_sidebar_html" |
| 📤 | course_about_show_social_links - in Site Configuration |
| ✅ | course_about_twitter_account - in Site Configuration or Django settings |
| ❌ | is_cosmetic_price_enabled - settings.FEATURES['ENABLE_COSMETIC_DISPLAY_PRICE'] |
| ✅ | courses_are_browsable - settings.FEATURES['COURSES_ARE_BROWSABLE'] |
| ✅ | enable_course_discovery - settings.FEATURES['ENABLE_COURSE_DISCOVERY'] |
Status Key:
| Emoji | Meaning |
|---|---|
| ✅ | Implementation in this PR matches decision in spreadsheet |
| 📤 | Implementation intentionally not in this PR, matches decision in spreadsheet |
| ❌ | Implementation in this PR does not match decision in spreadsheet |
| "course_about_twitter_account", | ||
| settings.PLATFORM_TWITTER_ACCOUNT | ||
| ), | ||
| "IS_COSMETIC_PRICE_ENABLED": settings.FEATURES.get("ENABLE_COSMETIC_DISPLAY_PRICE"), |
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.
In the spreadsheet the decision is documented as:
We should handle this in the same way as show_homepage_promo_video. If there isn't a price don't show it, and wrap the price in a plugin slot so people can remove it using FPF
so this should be removed.
| "IS_COSMETIC_PRICE_ENABLED": settings.FEATURES.get("ENABLE_COSMETIC_DISPLAY_PRICE"), |
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.
Done.
Thanks for a detailed breakdown!
3445554 to
d0ea0ee
Compare
brian-smith-tcril
left a comment
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.
Thank you so much for your patience throughout the review process on this one!
I'm very happy with how this turned out!
Here's an updated overview of what has been implemented at time of approval:
| Status | Legacy Implementation |
|---|---|
| ✅ | settings.FEATURES["ENABLE_COURSE_SORTING_BY_START_DATE"]) |
| 📤 | homepage_overlay_html in Site Configuration or modifying index_overlay.html |
| 📤 | show_partners - in Site Configuration |
| 📤 | show_homepage_promo_video - in Site Configuration |
| ✅ | homepage_promo_video_youtube_id - in Site Configuration |
| ✅ | homepage_course_max - in Site Configuration or Django settings |
| 📤 | sidebar_html_enabled - WaffleSwitch "course_experience.enable_about_sidebar_html" |
| 📤 | course_about_show_social_links - in Site Configuration |
| ✅ | course_about_twitter_account - in Site Configuration or Django settings |
| 📤 | is_cosmetic_price_enabled - settings.FEATURES['ENABLE_COSMETIC_DISPLAY_PRICE'] |
| ✅ | courses_are_browsable - settings.FEATURES['COURSES_ARE_BROWSABLE'] |
| ✅ | enable_course_discovery - settings.FEATURES['ENABLE_COURSE_DISCOVERY'] |
Status Key:
| Emoji | Meaning |
|---|---|
| ✅ | Implementation in this PR matches decision in spreadsheet |
| 📤 | Implementation intentionally not in this PR, matches decision in spreadsheet |
I also believe all of @feanil's comments have been addressed, so this should be good to merge!
|
@cmltaWt0 have all of your change requests been addressed on this one? |
Looking at it. |
frontend-app-catalog [FC-0086]
|
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. |
…nedx#37130) * feat: return response merged from base config, mfe config and mfe overrides * test: adjust existing tests * test: add new test methods * fix: add blank line * fix: remove homepage_overlay_html * fix: typo in a comment * test: change dict merge order to follow the correct hierarchy * fix: change response keys to uppercase * fix: add enable_course_discovery * fix: change COURSES_ARE_BROWSABLE to NON_BROWSABLE_COURSES * fix: remove show_homepage_promo_video * fix: use None as default for promo video youtube id * docs: expand docstrings, rename method/variables * fix: remove is_cosmetic_price_enabled field
…nedx#37130) * feat: return response merged from base config, mfe config and mfe overrides * test: adjust existing tests * test: add new test methods * fix: add blank line * fix: remove homepage_overlay_html * fix: typo in a comment * test: change dict merge order to follow the correct hierarchy * fix: change response keys to uppercase * fix: add enable_course_discovery * fix: change COURSES_ARE_BROWSABLE to NON_BROWSABLE_COURSES * fix: remove show_homepage_promo_video * fix: use None as default for promo video youtube id * docs: expand docstrings, rename method/variables * fix: remove is_cosmetic_price_enabled field
Summary
This PR extends the MFE Config API in order to return MFE configuration values using the following hierarchy, in the order from most specific to least specific:
The following fields have been added to the configuration being returned:
Usage
Usage remains exactly as before:
Get common config:
GET /api/mfe_config/v1Get app config (common + mfe-specific overrides):
GET /api/mfe_config/v1?mfe=name_of_mfeExample of a GET response
(abbreviated)
Dependent PRs:
feat: [FC-86] Home page banner section
feat: [FC-86] Added Home page courses list
feat: [FC-86] Created Intro section for Course About page
feat: [FC-86] Added Course About sidebar
feat: [FC-86] Added course overview and updated sidebar
feat: [FC-86] Updated Header links
feat: [FC-86] Added DataTable for Catalog page
feat: [FC-86] Added filtration for Catalog page
feat: [FC-86] Added course search for DataTable