Merge pull request #37510 from openedx/feanil/fix_branding_redirect_loop

[ktyagiapphelix2u]

feanil/fix branding redirect loop

Description

Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.

Useful information to include:

• Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
  "Developer", and "Operator".
• Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
• Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
  changes.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for testing this change.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

Include anything else that will help reviewers and consumers understand the change.

• Does this change depend on other changes elsewhere?
• Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
• If your database migration can't be rolled back easily.

[Copilot]

Pull Request Overview

This PR refactors configuration access patterns in the branding views to use getattr() instead of settings.FEATURES.get() for settings that have been moved from the FEATURES dictionary to top-level settings attributes. It also adds logic to prevent unnecessary redirects when the marketing root URL matches the LMS root URL.

Key changes:

• Replaced settings.FEATURES.get() with getattr() for ALWAYS_REDIRECT_HOMEPAGE_TO_DASHBOARD_FOR_AUTHENTICATED_USER and ENABLE_MKTG_SITE settings
• Added a condition to avoid redirecting to marketing root if it matches LMS_ROOT_URL
• Added test coverage for the new redirect logic

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
lms/djangoapps/branding/views.py	Refactored settings access from FEATURES dictionary to top-level attributes and added redirect prevention logic
lms/djangoapps/branding/tests/test_views.py	Added tests for the new redirect behavior when marketing root matches LMS root

Comments suppressed due to low confidence (2)

lms/djangoapps/branding/views.py:101

• The refactoring to replace settings.FEATURES.get() with getattr() is incomplete. The same change should be applied to lines 101 and 107 in the courses function for consistency with the refactoring done in the index function. Consider changing line 101 to getattr(settings, 'ENABLE_MKTG_SITE', False) and line 107 to getattr(settings, 'COURSES_ARE_BROWSABLE', False) if these settings have also been moved out of the FEATURES dictionary.

        settings.FEATURES.get('ENABLE_MKTG_SITE', False)

lms/djangoapps/branding/views.py:107

• The refactoring to replace settings.FEATURES.get() with getattr() is incomplete. The same change should be applied to lines 101 and 107 in the courses function for consistency with the refactoring done in the index function. Consider changing line 101 to getattr(settings, 'ENABLE_MKTG_SITE', False) and line 107 to getattr(settings, 'COURSES_ARE_BROWSABLE', False) if these settings have also been moved out of the FEATURES dictionary.

    if not settings.FEATURES.get('COURSES_ARE_BROWSABLE'):

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Using getattr() with a default of None for LMS_ROOT_URL is inconsistent with how this setting is accessed elsewhere in the codebase. Throughout the project (e.g., in cms/djangoapps/contentstore/helpers.py and common/djangoapps/student/helpers.py), settings.LMS_ROOT_URL is accessed directly or via configuration_helpers.get_value(). Additionally, there's a system check in openedx/core/djangoapps/common_initialization/checks.py that validates LMS_ROOT_URL is defined. Change this to settings.LMS_ROOT_URL for consistency.

Suggested change

	if root_url != getattr(settings, "LMS_ROOT_URL", None):
	if root_url != settings.LMS_ROOT_URL: