Skip to content

fix: improve the redirect mktg url logic #37404

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

Closed
ktyagiapphelix2u wants to merge 4 commits into from
Closed

fix: improve the redirect mktg url logic #37404

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97e26e3
fix: improve the redirect mktg url logic
ktyagiapphelix2u Sep 30, 2025
114cb68
fix: improve the redirect mktg url logic
ktyagiapphelix2u Sep 30, 2025
2528c86
Merge branch 'master' into ktyagi/BOMS-215
ktyagiapphelix2u Sep 30, 2025
393c08e
Merge branch 'openedx:master' into ktyagi/BOMS-215
ktyagiapphelix2u Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion lms/djangoapps/branding/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ def index(request):
'MKTG_URLS',
settings.MKTG_URLS
)
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
Copy link
Contributor

@robrap robrap Oct 10, 2025

Choose a reason for hiding this comment

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

[reminder] Once the dust settles, we'll want to update unit tests.

lms_root_url = getattr(settings, 'LMS_ROOT_URL', '').rstrip('/')
if marketing_root and (marketing_root.rstrip('/') == lms_root_url):
return redirect('signin_user')
Copy link
Contributor

@robrap robrap Oct 10, 2025

Choose a reason for hiding this comment

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

@feanil:

  • 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_AUTHENTICATE or some such?

Thanks for any thoughts on this front. We can't update edge settings until this PR lands in some form.

Copy link
Contributor

@feanil feanil Oct 17, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@robrap robrap Oct 17, 2025

Choose a reason for hiding this comment

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

  1. It sounds like you are confirming that student_views.index is something we can keep, even though the code comment said it would go away. That works in general and is a useful confirmation.
  2. 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. :)

Copy link
Contributor

@feanil feanil Oct 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@robrap robrap Oct 20, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@feanil feanil Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. 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.

  2. 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.

Copy link
Contributor

@robrap robrap Oct 20, 2025

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@feanil feanil Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ktyagiapphelix2u ktyagiapphelix2u Oct 21, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@feanil feanil Oct 21, 2025

Choose a reason for hiding this comment

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

I think #37510 should resolve this issue then. I've marked it ready for review.

return redirect(marketing_root)

domain = request.headers.get('Host')

Expand Down
Loading