fix: mktg urls from the configuration#193
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates marketing site URL configurations across Python and YAML configuration files, standardizing URL paths and removing deprecated marketing URLs.
- Simplified MKTG_URLS by removing unused marketing page keys (ACCESSIBILITY, CAREERS, ENTERPRISE, MEDIA_KIT, NEWS, SITE_MAP)
- Updated URL paths for COURSES, PRIVACY, and TOS_AND_HONOR to shorter, more standardized paths
- Added missing marketing URL entries to YAML configuration and cleaned up trailing whitespace
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| py_configuration_files/lms.py | Removed deprecated marketing URL keys and updated paths for COURSES, PRIVACY, and TOS_AND_HONOR |
| configuration_files/lms.yml | Removed FAQ key, updated COURSES path, added BLOG/DONATE/TOS_AND_HONOR keys, and fixed trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| COURSES: /course | ||
| COURSES: /courses | ||
| BLOG: /blog | ||
| DONATE: /donate |
There was a problem hiding this comment.
The FAQ key was removed from configuration_files/lms.yml but is still present in py_configuration_files/lms.py (line 437: 'FAQ': '/student-faq'). These two configuration files should remain consistent. Either add FAQ back to the YAML file or remove it from the Python file as well.
| DONATE: /donate | |
| DONATE: /donate | |
| FAQ: /student-faq |
There was a problem hiding this comment.
This looks relevant comment
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
py_configuration_files/lms.py
Outdated
|
|
||
| ENABLE_MKTG_SITE = os.environ.get('ENABLE_MARKETING_SITE', True) | ||
| MARKETING_SITE_ROOT = os.environ.get('MARKETING_SITE_ROOT', 'http://localhost:8080') | ||
| MARKETING_SITE_ROOT = os.environ.get('MARKETING_SITE_ROOT', '/') |
There was a problem hiding this comment.
Changing MARKETING_SITE_ROOT default from 'http://localhost:8080' to '/' is a significant behavioral change. The value '/' is a relative path rather than a full URL, which may cause issues when constructing absolute URLs for marketing site links.
If this is intentional to support relative paths, ensure that:
- All code using MARKETING_SITE_ROOT can properly handle relative paths
- The change is documented and tested
- Consider if this affects external link generation or redirects
Alternatively, if this should remain an absolute URL, consider using 'http://localhost:18000' to match the typical LMS development environment.
| MARKETING_SITE_ROOT = os.environ.get('MARKETING_SITE_ROOT', '/') | |
| MARKETING_SITE_ROOT = os.environ.get('MARKETING_SITE_ROOT', 'http://localhost:18000') |
There was a problem hiding this comment.
@ktyagiapphelix2u Can this entire setting be removed ?
As we already have MKTG_URLS['ROOT'] so should we rely on this variable anymore ?
Can you check with @robrap or in general with community on this ?
jcapphelix
left a comment
jcapphelix
left a comment
There was a problem hiding this comment.
Minor comments to check a few things.
2 participants
Description
Fixing mktg urls for infinite redirect loop
Ticket
https://2u-internal.atlassian.net/browse/BOMS-215