fix: point SITE_ID at the LMS domain during init (supersedes #1389) - #1431
Open
ahmed-arb wants to merge 1 commit into
Open
fix: point SITE_ID at the LMS domain during init (supersedes #1389)#1431ahmed-arb wants to merge 1 commit into
ahmed-arb wants to merge 1 commit into
Conversation
Django auto-creates an "example.com" Site at id=SITE_ID during the first migration. Code paths that run outside of an HTTP request (e.g. bulk emails sent from Celery workers) cannot infer the site from the request and fall back to SITE_ID, so they render links and branding for "example.com". Fix this in the init layer instead of at runtime: the LMS init job renames the "example.com" site in place to the LMS domain (Django's recommended pattern), so there is a single Site and SITE_ID naturally points at it. Installations that already have both an "example.com" site and a site for their domain are left untouched, with a warning explaining how to consolidate them. The CMS init job ensures a Site exists for the CMS domain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1182
Supersedes #1389 (and replaces the reverted #1323).
Problem
Django auto-creates an
example.comSiteatid=SITE_IDduring the first migration. Code paths that run outside of an HTTP request — e.g. bulk emails sent from Celery workers — cannot infer the site from the request host, so Django falls back to theSITE_IDsetting and renders links/branding forexample.cominstead of the real domain.Approach
This takes the init-layer, data-fix approach discussed on #1389 (rather than the reverted
SITE_ID=1change, or a runtimedjango.setupmonkeypatch):jobs/init/lms.sh(aftermigrate): renames the auto-createdexample.comsite in place to the LMS domain (settings.LMS_BASE). This is Django's recommended pattern — there is then a singleSite, andSITE_ID(still a plain= 2) naturally points at it. Idempotent.example.comsite and a separate site for the domain already exist — are left untouched, with a warning printing both site IDs and how to consolidate. This addresses @kdmccormick's concern (fix: fixed wrong site in django shell #1389 (comment)) that data may be tied to either site.jobs/init/cms.sh: ensures aSiteexists for the CMS domain. The sharedSITE_IDrow is owned by the LMS, since the LMS and CMS share onedjango_sitetable.SITE_IDremains declarative (= 2); no runtime mutation, nodjango.setuppatching, no per-process DB writes.Why not #1389's approach
#1389 wraps
django.setup()from the settings template to rewritesettings.SITE_IDon every process boot. As raised in review, that is fragile (patches a core Django bootstrap hook, DB write on every start), racy with import-time reads ofSITE_ID, and treats a data-state problem as a runtime workaround. Fixing the data once, in the init step, avoids all of that.Testing
Verified on the Teak (
openedx-dev) image viatutor dev:example.com@2): renamed in place → single site(2, <LMS domain>),Site.objects.get_current()returns the real site.example.com@2+ real site@3): warning printed, database left unchanged.migrateunaffected by the step; corelms.shruns before Indigo'ssettheme(so the fresh+theme case consolidates cleanly).tests/commands/test_jobs.py) asserting the rendered init scripts;unittest,mypy --strict,black,isortpass.🤖 Generated with Claude Code