Add workaround for live button URL in admin panel#549
Add workaround for live button URL in admin panel#549MaciekBaron wants to merge 2 commits intomainfrom
Conversation
| """ | ||
| swap_domains: bool = host is not None and host in settings.CMS_HOSTNAME_ALTERNATIVES.values() | ||
| cache_key = f"cms-{swap_domains}-{SITE_ROOT_PATHS_CACHE_KEY}" | ||
| cache_key = f"cms-{swap_domains}-{default_site_only}-{SITE_ROOT_PATHS_CACHE_KEY}" |
There was a problem hiding this comment.
I augmented the cache key so that if you change the setting, you don't need to flush Redis.
sanjeevz3009
left a comment
sanjeevz3009
left a comment
There was a problem hiding this comment.
Tested it locally, works well! Thanks, @MaciekBaron
| ) | ||
| ) | ||
| if default_site_only: | ||
| root_url = getattr(settings, "WAGTAILADMIN_BASE_URL", site.root_url) |
There was a problem hiding this comment.
Q: Why do we need to do this instead of using the default site’s root_url, can you have multiple default?
If we move away from using subdomains, the “Live” button in the admin will fall back to the admin-facing URL rather than the public site hostname.
In our setup, that’s a problem because we run separate internal and external instances. The admin sits on the internal side, but the live site is served from the external hostname.
So without this change, the “Live” button would point to the internal/admin URL instead of the correct public-facing one. It’s a subtle side effect of the dual-instance architecture rather than a general Wagtail behaviour.
There was a problem hiding this comment.
Ah right, in that case my solution wouldn't work for that scenario.
Might need to sadly close this PR then.
There was a problem hiding this comment.
Noooo, I quite liked it. But I understand that in the future, when we move away from subdomains to path-based, then this change will have a negative effect
There was a problem hiding this comment.
If default_site_only, why not just use site.root_url, as you can only have 1 default site anyway and it should still solve the problem and handle the case for path-based locale fine?. What am I missing?.
MaciekBaron
added
On Hold ✋
3 participants
What is the context of this PR?
This PR addresses the bug raised in CMS-1140 and adds a workaround for the issue.
When
CMS_USE_SUBDOMAIN_LOCALESis disabled (the default for local development), the admin "Live" button on pages pointed to the wrong host (e.g. http://cy.web.ons.orb.local:8000/) because_get_site_root_pathsfell back to Wagtail's defaultSite.get_site_root_paths(), which returned allSiterecords from the database, including those with production/staging hostnames loaded fromCMS_HOSTNAME_LOCALE_MAPin.env. Note that other "Live" buttons would work correctly.The workaround routes the non-subdomain code path through
get_mapped_site_root_paths(default_site_only=True)instead, which filters to the default site and usesWAGTAILADMIN_BASE_URLas the root URL. This ensures the Live button points to http://localhost:8000 (or whatever you use) in development, regardless of what hostnames are stored in the database. This works if and only if one domain is being used to access the site (which should be true whenCMS_USE_SUBDOMAIN_LOCALESis disabled).How to review
CMS_USE_SUBDOMAIN_LOCALESis disabled/admin/pages/3/CMS_USE_SUBDOMAIN_LOCALESand reload the serverDeployment Safety
Bleed and Sandbox deploy automatically on merge, so PRs should be safe to deploy immediately.
Please select one:
Follow-up Actions
Ensure this change introduces no regressions.