Skip to content

fix: fixed wrong site in django shell#1389

Open
muhammadadeeltajamul wants to merge 2 commits into
overhangio:releasefrom
muhammadadeeltajamul:adeel/fix_wrong_site_in_shell
Open

fix: fixed wrong site in django shell#1389
muhammadadeeltajamul wants to merge 2 commits into
overhangio:releasefrom
muhammadadeeltajamul:adeel/fix_wrong_site_in_shell

Conversation

@muhammadadeeltajamul
Copy link
Copy Markdown
Contributor

Fixes incorrect SITE_ID when site is accessed from management command or celery task.
After this change, it will return LMS_BASE site object when command or task is in lms and CMS_BASE site object when command or task is in cms instead of example.com site object

Comment on lines +20 to +21
from django.conf import settings
from django.contrib.sites.models import Site
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move these imports to top of the file? or any reason to add it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These imports are inside function so that model and settings gets imported once the django.setup is completed. If I move these imports to the top, it will throw error that app is not loaded.

Comment on lines +23 to +26
site, _ = Site.objects.get_or_create(
domain=domain,
defaults={"domain": domain, "name": domain},
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we remove domain from from defaults? as it is already used for lookup.

Copy link
Copy Markdown
Contributor

@Faraz32123 Faraz32123 May 20, 2026

Choose a reason for hiding this comment

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

[Question] Did we tried a fresh setup with it? When there are no migration applied, just to be sure?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

its a small nit. I write this way for better readability but it will work either way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have not tried it with the new setup. I tested it on running instances of tutor local and tutor dev for both cms and lms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed nit.
launch was failing because site model was not migrated. I have fixed this issue

@Faraz32123 Faraz32123 moved this from Pending Triage to In review in Tutor project management May 21, 2026
@Faraz32123 Faraz32123 requested a review from ahmed-arb May 21, 2026 06:33
Copy link
Copy Markdown
Contributor

@Faraz32123 Faraz32123 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Collaborator

@ahmed-arb ahmed-arb left a comment

Choose a reason for hiding this comment

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

I want to push back on the approach here before we merge, given how PR #1323 played out. That PR was reverted because it shifted everyone's SITE_ID from 2 to 1 and broke existing instances where no Site row with id=1 existed. We need to be careful that this fix doesn't introduce a similar class of footgun.

A few concerns with doing this in the Django settings file:

  1. Monkey-patching django.setup() from a settings template is fragile. Settings are supposed to be declarative. Wrapping a core Django bootstrap function so that it does a DB write on every process start is a strong code smell. Any future Django change to the setup flow can break this silently, and it's a surprising thing for an operator to find when they cat lms settings and try to understand their config.
  2. Mutating settings.SITE_ID post-load is racy with imports. Plenty of edx-platform and plugin code reads SITE_ID at module import time or caches derived values. Whatever gets imported and resolved before the patched django.setup finishes running keeps SITE_ID=2; everything after sees the new value. Ordering relative to AppConfig.ready() across plugins isn't guaranteed.
  3. This still treats a data-state problem with a runtime workaround. Per @kdmccormick's analysis in the issue, the actual root cause is that our init creates two Site rows: Django's auto-created example.com at id=SITE_ID, and then a separate my.lms.org row at id=SITE_ID+1. Django's own docs recommend exactly the opposite: use a data migration to modify the existing example Site row in place (https://docs.djangoproject.com/en/5.2/ref/contrib/sites/#enabling-the-sites-framework) so that there's only ever one Site, and SITE_ID naturally points at it.

What I'd propose instead:

Fix the data, once, in the right layer. Tutor's post-migration init step (alongside createsuperuser, oauth client setup, etc.) for lms and cms. A small idempotent step that:

  • Ensures a Site row exists at id=SITE_ID (=2) with domain=LMS_BASE / CMS_BASE.
  • If example.com is squatting on id=2, rename it in place to the real domain (Django's recommended pattern), or move/delete it.
  • Logs what it did.

This way we:

  • Fix both fresh installs and existing instances without an id=1-style breakage.
  • Keep settings declarative.
  • Run once per tutor launch instead of on every process boot.
  • Make it observable in launch logs instead of behind a bare except.
  • Avoids touching django.setup internals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants