fix: improve the redirect mktg url logic #37404
Conversation
|
@robrap I wanted to explain a recent change in the logic related to how marketing URLs are being handled. Previously, when someone accessed marketing URLs (like footer links) on platforms such as edge.edx.org, they would land directly on the index (home) page, without needing to log in. From there, users had the option to either browse the page, view footer marketing links, or choose to log in if they wanted. With the new logic we’ve added: As a result, users are now immediately redirected to edge.edx.org/login and then to edge.edx.org/dashboard, without seeing the index (home) page first. This removes the previous experience where users could view marketing content without being authenticated. This change doesn't impact edge.edx.org much since that environment didn't show the index page anyway, but in other environments, this is a noticeable change users can no longer land on the home page directly without logging in. |
| return redirect(marketing_urls.get('ROOT')) | ||
| marketing_root = marketing_urls.get('ROOT') | ||
|
|
||
| # Prevent redirect loop: if LMS_ROOT_URL equals marketing ROOT, redirect to sign-in |
There was a problem hiding this comment.
[reminder] Once the dust settles, we'll want to update unit tests.
| # Prevent redirect loop: if LMS_ROOT_URL equals marketing ROOT, redirect to sign-in | ||
| lms_root_url = getattr(settings, 'LMS_ROOT_URL', '').rstrip('/') | ||
| if marketing_root and (marketing_root.rstrip('/') == lms_root_url): | ||
| return redirect('signin_user') |
There was a problem hiding this comment.
- This condition protects against a redirect loop that would have happened on edge (and any deployment), where the LMS index and marketing root match.
- In this case, we have brought forward the code from below that edge used, which basically required users to be authenticated.
- For other deployments, they would have used https://github.com/openedx/edx-platform/blob/393c08ed89c8ea634c4e333bad136a165a265ab5/lms/djangoapps/branding/views.py#L80, but the comments make it seem like this code will be replaced by the marketing urls.
- I don't really know if we need to handle this case (an unauthenticated view), or if the edge solution is good enough for everyone? If you want a special solution for this, we need to determine: a) where you want to redirect the user, and b) how we configure the marketing url settings to choose between the two options. For part b, we might add another setting like
MKTG_URLS_FORCE_AUTHENTICATEor some such?
- I don't really know if we need to handle this case (an unauthenticated view), or if the edge solution is good enough for everyone? If you want a special solution for this, we need to determine: a) where you want to redirect the user, and b) how we configure the marketing url settings to choose between the two options. For part b, we might add another setting like
Thanks for any thoughts on this front. We can't update edge settings until this PR lands in some form.
There was a problem hiding this comment.
So the situation is that you "ENABLE_MKTG_SITE" is set to true because you want to use "MKTG_URLS" for the various places where they are used, but you don't actually want to change the Marketing Root URL?
I think rather than forcing a login flow, we just need to update this so that in the case of the match, we don't do any redirects and just fall through to the rendering of the student_views.index that happens a bit lower in the function. What do you think of that approach?
There was a problem hiding this comment.
- It sounds like you are confirming that
student_views.indexis something we can keep, even though the code comment said it would go away. That works in general and is a useful confirmation. - I'll need to determine if that is also acceptable in Edge, which has its own special code. If not, I'll try to find out why. :)
There was a problem hiding this comment.
Sounds good, yea I think we can code in the assumption that this should work with that, and if we do remove that later, we can make sure that whatever the new default behavior is, will still work. I think the eventual default is going to be the MFE redriect, but the old view will exist until the New catalog MFE is the default.
There was a problem hiding this comment.
@feanil: I'm not comfortable rushing a fix in. I don't yet know how this would affect Edge, and I imagine that the student_view wouldn't be appropriate for non-students.
How would you feel about getting a more complete replacement in for Verawood, including possible new MFE redirects, etc., and then bumping the removal to the W release? I want to avoid having urgent changes around cut dates that don't feel like they need to be urgent.
There was a problem hiding this comment.
-
I don't think pushing it out to W makes sense. I think we can have a transition plan in place before Verawood that allows people to put in the new config before switching to verawood.
-
I think the fix is not that complex, it's falling back to the behavior that existed before, which is to render the student_view.index which is just the index.html that it was previously rendering if you didn't want to redirect to your own home page.
The bug is that now that MARKETING_URLS is a required setting, there will the ROOT will always be set.
There was a problem hiding this comment.
@feanil: Your proposal is not a backward compatible replacement for Edge, but since the code is clearly 2U-specific, I understand that that may not matter to you. So, I'll leave this back in your hands to proceed how and when you wish. You are welcome to take over this PR if you want to have a backward-compatible fix for others in Ulmo. Your proposal won't address our needs, and I don't want orbi-bom to have to work urgently on this, because it doesn't affect us. Thanks and let me know if you have any questions.
There was a problem hiding this comment.
An alternative PR with my suggested fix: #37510 FWIW, the 2U code is still there and should behave as it did before. This PR would just not change the behavior for everyone else also.
There was a problem hiding this comment.
Yes, the main goal here is to enable MKTG_URLS without triggering unnecessary redirects when the ROOT matches LMS_ROOT_URL. We’re not looking to change the marketing root behavior just to prevent regressions when ENABLE_MKTG_SITE is True but the URLs are aligned.
There was a problem hiding this comment.
I think #37510 should resolve this issue then. I've marked it ready for review.
|
This was fixed in another PR #37510 |
3 participants
Description
This PR addresses a potential redirect loop that can occur when the LMS_ROOT_URL is configured to be the same as the marketing site's root URL.
Change Details
Added a conditional check to compare LMS_ROOT_URL and marketing_root.
If both URLs are the same, the user is redirected directly to the signin_user view instead of looping back to the marketing site.
Prevents unnecessary redirects and ensures a smoother user experience
Related Link
https://2u-internal.atlassian.net/jira/software/c/projects/BOMS/boards/3017?assignee=712020%3A61c35560-3472-4e12-b833-884e5c4bbff4&selectedIssue=BOMS-215