Fix multi site #479
Open
Fix multi site #479
Conversation
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
This allows viewing versioned content on different sites
Reviewer's GuideThis PR extends multi-site compatibility by leveraging get_full_url in both the publish view redirect and CMS toolbar, and by preserving the site query parameter in the page admin change form. Sequence diagram for publish view redirect with multi-site supportsequenceDiagram
participant Admin as actor Admin User
participant VersioningAdmin as VersioningAdmin
participant VersionContent as Version Content
participant Site as Site
Admin->>VersioningAdmin: publish_view(request, object_id)
VersioningAdmin->>VersionContent: check for get_full_url
alt get_full_url exists and returns URL
VersionContent-->>VersioningAdmin: full_url
VersioningAdmin->>Admin: redirect(full_url)
else get_absolute_url exists
VersionContent-->>VersioningAdmin: get_absolute_url
VersioningAdmin->>Admin: internal_redirect(requested_redirect)
else
VersioningAdmin->>Admin: internal_redirect(requested_redirect)
end
Sequence diagram for CMS toolbar 'View Published' button with multi-site supportsequenceDiagram
participant User as actor CMS User
participant Toolbar as Toolbar
participant PublishedVersion as Published Version
participant Site as Site
User->>Toolbar: Open toolbar
Toolbar->>PublishedVersion: check for get_full_url
alt get_full_url exists
PublishedVersion-->>Toolbar: full_url
Toolbar->>User: Show 'View Published' button (full_url)
else get_absolute_url exists
PublishedVersion-->>Toolbar: get_absolute_url
Toolbar->>User: Show 'View Published' button (absolute_url)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @stefanw - I've reviewed your changes - here's some feedback:
- Extract the logic for choosing between get_full_url and get_absolute_url into a reusable helper to eliminate duplication in admin.py and cms_toolbars.py.
- When performing an immediate redirect on full_url, ensure that all user messages and context are preserved; centralizing the redirect logic could help maintain consistency.
- The form action now includes many inline query parameters—consider refactoring this into a template tag or helper to improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the logic for choosing between get_full_url and get_absolute_url into a reusable helper to eliminate duplication in admin.py and cms_toolbars.py.
- When performing an immediate redirect on full_url, ensure that all user messages and context are preserved; centralizing the redirect logic could help maintain consistency.
- The form action now includes many inline query parameters—consider refactoring this into a template tag or helper to improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `djangocms_versioning/admin.py:1107` </location>
<code_context>
+ if not requested_redirect and conf.ON_PUBLISH_REDIRECT == "published":
+ if hasattr(version.content, "get_full_url"):
+ full_url = version.content.get_full_url()
+ if full_url:
+ # Can't resolve full_url, redirect directly to it
+ return redirect(full_url)
+ elif hasattr(version.content, "get_absolute_url"):
</code_context>
<issue_to_address>
Redirecting directly to 'full_url' may bypass internal URL resolution.
Validating 'full_url' before redirecting can help prevent open redirect vulnerabilities. Ensure the URL is within the expected domain or document why this approach is secure.
</issue_to_address>
### Comment 2
<location> `djangocms_versioning/cms_toolbars.py:261` </location>
<code_context>
return
- url = published_version.get_absolute_url() if hasattr(published_version, "get_absolute_url") else None
+ url = published_version.get_full_url() if hasattr(published_version, "get_full_url") else None
+ if not url and hasattr(published_version, "get_absolute_url"):
+ url = published_version.get_absolute_url()
if url and (self.toolbar.edit_mode_active or self.toolbar.preview_mode_active):
</code_context>
<issue_to_address>
The fallback logic for URL resolution could be simplified.
If 'get_full_url' exists but returns a falsy value, the code falls back to 'get_absolute_url'. Confirm this is intentional, as it could hide problems with 'get_full_url' implementations.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
url = published_version.get_full_url() if hasattr(published_version, "get_full_url") else None
if not url and hasattr(published_version, "get_absolute_url"):
url = published_version.get_absolute_url()
=======
if hasattr(published_version, "get_full_url"):
url = published_version.get_full_url()
elif hasattr(published_version, "get_absolute_url"):
url = published_version.get_absolute_url()
else:
url = None
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1107
to
+1108
| if full_url: | ||
| # Can't resolve full_url, redirect directly to it |
There was a problem hiding this comment.
🚨 issue (security): Redirecting directly to 'full_url' may bypass internal URL resolution.
Validating 'full_url' before redirecting can help prevent open redirect vulnerabilities. Ensure the URL is within the expected domain or document why this approach is secure.
Comment on lines
+261
to
+263
| url = published_version.get_full_url() if hasattr(published_version, "get_full_url") else None | ||
| if not url and hasattr(published_version, "get_absolute_url"): | ||
| url = published_version.get_absolute_url() |
There was a problem hiding this comment.
suggestion: The fallback logic for URL resolution could be simplified.
If 'get_full_url' exists but returns a falsy value, the code falls back to 'get_absolute_url'. Confirm this is intentional, as it could hide problems with 'get_full_url' implementations.
Suggested change
| url = published_version.get_full_url() if hasattr(published_version, "get_full_url") else None | |
| if not url and hasattr(published_version, "get_absolute_url"): | |
| url = published_version.get_absolute_url() | |
| if hasattr(published_version, "get_full_url"): | |
| url = published_version.get_full_url() | |
| elif hasattr(published_version, "get_absolute_url"): | |
| url = published_version.get_absolute_url() | |
| else: | |
| url = None |
Comment on lines
+1106
to
+1107
| full_url = version.content.get_full_url() | ||
| if full_url: |
There was a problem hiding this comment.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
Suggested change
| full_url = version.content.get_full_url() | |
| if full_url: | |
| if full_url := version.content.get_full_url(): |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #479 +/- ##
==========================================
+ Coverage 90.55% 93.56% +3.01%
==========================================
Files 72 76 +4
Lines 2732 2705 -27
Branches 322 0 -322
==========================================
+ Hits 2474 2531 +57
+ Misses 182 174 -8
+ Partials 76 0 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Description
This builds on django-cms#8303 and copies the changes to page admin change form template.
The URL for published content can now be behind
get_full_urlin order to access a separate domain when the versioned object is associated with a different site.Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Fix multi-site support by using full URLs for versioned content and preserving site context in the page admin interface
Enhancements: