-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: Profile MFE redirection issue (URL path override) #37416
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
Conversation
This error was occurring because the way the redirect URL was constructed caused the entire base path to be removed. This commit updates the URL construction method to correctly preserve the MFE's path.
|
Thanks for the pull request, @DeimerM! 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. |
felipemontoya
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 looks good to me. It fixes the redirect for when an instance use the PATH option for hosting the MFEs (as is the default for tutor) and at the same time does not use the indigo-theme (which is not the default, but widely used for instances that predate the existence of indigo).
|
@deborahgu could we request a review from you since you drived the DEPR of the account and profile pages? |
| <div class="mobile-nav-item dropdown-item dropdown-nav-item"><a href="${settings.ENTERPRISE_LEARNER_PORTAL_BASE_URL}/${enterprise_customer_portal.get('slug')}" role="menuitem">${_("Dashboard")}</a></div> | ||
| % endif | ||
|
|
||
| <div class="mobile-nav-item dropdown-item dropdown-nav-item"><a href="${urljoin(settings.PROFILE_MICROFRONTEND_URL, f'/u/{user.username}')}" role="menuitem">${_("Profile")}</a></div> |
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.
It looks like the default profile MFE does have it's path this way; https://github.com/openedx/frontend-app-profile/blob/master/src/routes/AppRoutes.jsx#L11
Where it expects the /u/ so is this an issue where that "requirement" is not being met for some reason?
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 issue comes up when using this MFE together with the per_path install option which is the tutor-mfe default.
When you use tutor-mfe, then domain.com/profile/ is used as the root and a real profile url would look like domain.com/profile/u/<username>. The u/ is to be expected, but having the link start with the leading / makes this replace /profile/u/ with /u/ and thus the MFE root is never reached.
An example from the python console is easier to understand:
>>> from urllib.parse import urljoin
>>> urljoin('https://mydomain.com/profile/', f'/u/<username>') # <-- this is what we are seeing
'https://mydomain.com/u/<username>'
>>> urljoin('https://mydomain.com/profile/u/', f'<username>') # <-- this is what the fix suggests we should do
'https://mydomain.com/profile/u/<username>'
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.
but this change isn't doing what you show in this code sample, unless settings.PROFILE_MICROFRONTEND_URL is doing something I definitely don't understand. It appears to me that the /u/ has been knocked out entirely in the urljoin call. am I missing something?
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.
oh huh I AM missing something, I just went and looked and I see that for our instance that's set as
PROFILE_MICROFRONTEND_URL: https://profile.ourdomain/u/
We surely don't enforce that, but I assume it should work fine even if people don't have it set up that way? Hmm.
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.
Got it that makes sense, I was confused because the defaults in edx-platform also don't have /u/ suffix on the default PROFILE_MICROFRONTEND_URL setting in test and devstack settings. No issues from my perspective on this.
deborahgu
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 makes sense to me, thank you for explaining it so clearly.
This error was occurring because the way the redirect URL was constructed caused the entire base path to be removed. This commit updates the URL construction method to correctly preserve the MFE's path.
Description
Given that the legacy account and profile pages were deprecated from the Teak version of the platform, certain changes were made to the platform to redirect from a legacy page to the respective Account and Profile MFEs (Micro-Frontends).
For more information about the changes that were made, please refer to this PR: #36219
In this case, the specific change affecting the user experience is fixed in this PR. This change involves the logic responsible for rendering the profile URL in the legacy dropdown.
Useful information:
It is happening due to the implementation of the legacy user dropdown, as its implementation did not account for scenarios where PROFILE_MICROFRONTEND_URL contains a path.
Specifically, it occurs when the following function is called:
This is because the
urljoinfunction is used to concatenate an absolute URL with a relative URL. In the case of the path configuration,settings.PROFILE_MICROFRONTEND_URLwould be the absolute URL (e.g.,http://apps.local.openedx.io/profile/u/), and/u/{user.username}would be the relative URL.However, the
urljoinfunction has certain implications. The one that matters in this case is that if the relative URL begins with a/(slash), it replaces the entire path contained within the absolute URL. In our example, this causes the URL to become:http://apps.local.openedx.io/u/{user.username}The correct URL, in this case, should be:
http://apps.local.openedx.io/profile/u/{user.username}This incorrect construction makes it impossible to find the Profile MFE by path when accessing it from a legacy page.
Supporting information
This implementation was based on the solution proposed in tutor-indigo https://github.com/overhangio/tutor-indigo/blob/cf48e296affdfae4d0d0d7b21902d85eb51b739a/tutorindigo/templates/indigo/lms/templates/header/user_dropdown.html#L42 with the aim of making the solution compatible with the default PROFILE_MICROFRONTEND_URL configuration in tutor-mfe: https://github.com/overhangio/tutor-mfe/blob/4e31afa55c3f61a455d6bad3ffdc058e249e47a8/tutormfe/patches/openedx-lms-production-settings#L73
Testing instructions
tutor-indigoplugin and the Indigo theme deactivated.http://local.openedx.io/courses).