get_lms_link_from_course_key fallback to LMS base if no Site found - #813
Conversation
OmarIthawi
left a comment
OmarIthawi
left a comment
There was a problem hiding this comment.
Thanks @bryanlandia. This PR shouldn't go as-is because it could break the feature for SaaS customers.
Please check the notes and let me know what do you think. I'm leaning towards Option 1 and using get_site_for_course since the course.org is not a reliable source of truth imho.
77f8a60 to
35ba62e
Compare
|
Thanks @bryanlandia. Now it looks good. Apparently, we're having failed tests due to (I think) circular dependency. Please refactor to avoid this issue and we'll need to test this helper on staging to see if it works. |
|
Checks have passed now @OmarIthawi. Thanks for investigating the build results. |
OmarIthawi
left a comment
OmarIthawi
left a comment
There was a problem hiding this comment.
Thanks @bryanlandia! Would it be okay to write a test case or two for get_lms_link_from_course_key?
Rework get_lms_link_from_course_key using get_site_for_course @OmarIthawi suggestion Co-authored-by: Omar Al-Ithawi <i@omardo.com> Fix import issue in get_lms_link_for_course_key
5607890 to
2bf1e1e
Compare
2bf1e1e to
f209638
Compare
|
@OmarIthawi I added some tests for this |
|
Thanks @bryanlandia! |
2 participants
The way this is written now is adding a lot of complexity for standalone deployments. There, we have a single Site with multiple course orgs. As far as I can tell, there's no need within Tahoe SaaS multitenant to fall back to {course_org}.LMS_BASE if no Site with the name of the course org is found. There are many other points of the system that enforce or will break unless that Site is named 'chef' or whatever.
As it is, for a single-site standalone, the Site not found fallback then means we have to support wildcard subdomains, as all of our preview and jump_to links will end up going to {course_org}.lms_base.customer_domain.com. That gets complicated, as wildcard LE certs require DNS TXT record validation, etc.
So, putting this together as a Draft for initial feedback. If it looks good to go ahead I'll write tests (there are none yet anyhow).