enable link to placeholder preview in frontend editing#328
Open
wfehr wants to merge 1 commit intodjango-cms:masterfrom
Open
enable link to placeholder preview in frontend editing#328wfehr wants to merge 1 commit intodjango-cms:masterfrom
wfehr wants to merge 1 commit intodjango-cms:masterfrom
Conversation
Contributor
Reviewer's GuideAdds an optional Sequence diagram for rendering static_alias with placeholder_preview_linksequenceDiagram
actor StaffUser
participant Browser
participant CMSFrontend
participant StaticAliasTag
participant Toolbar
participant AdminURL as admin_view_url
participant AdminPlaceholderPreview
StaffUser->>Browser: Request page with static_alias placeholder_preview_link
Browser->>CMSFrontend: HTTP GET /page/
CMSFrontend->>Toolbar: Initialize toolbar (edit mode or preview mode)
CMSFrontend->>StaticAliasTag: render_tag(context, static_code, extra_bits, nodelist)
StaticAliasTag->>Toolbar: check edit_mode_active
Toolbar-->>StaticAliasTag: edit_mode_active true
StaticAliasTag->>Toolbar: check preview_mode_active
Toolbar-->>StaticAliasTag: preview_mode_active false
StaticAliasTag->>Toolbar: check is_staff
Toolbar-->>StaticAliasTag: is_staff true
StaticAliasTag->>AdminURL: admin_view_url(alias)
AdminURL-->>StaticAliasTag: href to placeholder preview
StaticAliasTag-->>CMSFrontend: HTML with preview link prepended to placeholder content
CMSFrontend-->>Browser: Rendered HTML
Browser->>StaffUser: Shows placeholder content with preview icon link
StaffUser->>Browser: Click preview link
Browser->>AdminPlaceholderPreview: HTTP GET href
AdminPlaceholderPreview-->>Browser: Placeholder preview page
Class diagram for updated StaticAlias tag and DeclaredStaticAliasclassDiagram
class StaticAlias {
+string name
+Toolbar toolbar
+Renderer renderer
+render_tag(context, static_code, extra_bits, nodelist) str
+get_declaration() DeclaredStaticAlias
}
class DeclaredStaticAlias {
+string static_code
+bool site
+bool placeholder_preview_link
}
StaticAlias ..> DeclaredStaticAlias : returns
File-Level Changes
Possibly linked issues
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The HTML for the preview link is embedded as a large inline string with inline styles; consider moving styling to CSS and building the markup via
format_html(or a small template/utility) to improve readability and avoid unintended whitespace. - In
DeclaredStaticAlias, the newplaceholder_preview_linkfield is set but not referenced elsewhere; if it's not needed for downstream consumers, you might remove it to keep the declaration minimal and avoid confusion. - When generating the
hrefwithadmin_view_url(alias)inside the f-string, usingformat_htmlfor the whole link block would be safer and more consistent with Django’s escaping conventions rather than interpolating raw strings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The HTML for the preview link is embedded as a large inline string with inline styles; consider moving styling to CSS and building the markup via `format_html` (or a small template/utility) to improve readability and avoid unintended whitespace.
- In `DeclaredStaticAlias`, the new `placeholder_preview_link` field is set but not referenced elsewhere; if it's not needed for downstream consumers, you might remove it to keep the declaration minimal and avoid confusion.
- When generating the `href` with `admin_view_url(alias)` inside the f-string, using `format_html` for the whole link block would be safer and more consistent with Django’s escaping conventions rather than interpolating raw strings.
## Individual Comments
### Comment 1
<location> `tests/test_templatetags.py:454-461` </location>
<code_context>
+
+ request.toolbar.preview_mode_active = False
+
+ with self.subTest("preview-link enabled in edit-mode"):
+ output = self.render_template_obj(
+ alias_link_enabled_template,
+ {},
+ request,
+ )
+ self.assertNotEqual(alias_content, output)
+ self.assertIn(alias_preview_link, output)
+
+ # check that other "extra_bits" don't interfere with functionality
</code_context>
<issue_to_address>
**suggestion (testing):** Add a more specific assertion on the preview link element to ensure we’re testing the exact behavior and not just partial HTML.
Currently we only check that `alias_preview_link` is a substring of the rendered output. Please tighten this by asserting that the full `<a>` element is present with the expected CSS class, and that its `href` targets the alias admin/preview URL (e.g., contains the alias ID or a known path segment). This will make the test more robust and clearly specify the expected markup.
Suggested implementation:
```python
with self.subTest("preview-link enabled in edit-mode"):
output = self.render_template_obj(
alias_link_enabled_template,
{},
request,
)
self.assertNotEqual(alias_content, output)
# basic sanity check that the preview link markup is present
self.assertIn(alias_preview_link, output)
# assert that the full <a> element is rendered with the expected class and href
preview_link_match = re.search(
r'<a[^>]+class="[^"]*\bplaceholder_preview_link\b[^"]*"[^>]+href="(?P<href>[^"]+)"',
output,
)
self.assertIsNotNone(
preview_link_match,
msg="Expected an <a> tag with class containing 'placeholder_preview_link' in the rendered output.",
)
preview_href = preview_link_match.group("href")
self.assertIn(
"/admin/cms/alias/",
preview_href,
msg="Preview link href should point to the alias admin preview URL.",
)
# ensure the href targets this specific alias
self.assertIn(
str(alias.pk),
preview_href,
msg="Preview link href should contain the alias primary key.",
)
# check that other "extra_bits" don't interfere with functionality
output = self.render_template_obj(
alias_link_enabled_template.replace(
"placeholder_preview_link",
"site placeholder_preview_link",
),
{},
request,
)
self.assertNotEqual(alias_content, output)
# basic sanity check that the preview link markup is present
self.assertIn(alias_preview_link, output)
# assert that the full <a> element is still rendered correctly
preview_link_match = re.search(
r'<a[^>]+class="[^"]*\bplaceholder_preview_link\b[^"]*"[^>]+href="(?P<href>[^"]+)"',
output,
)
self.assertIsNotNone(
preview_link_match,
msg=(
"Expected an <a> tag with class containing 'placeholder_preview_link' "
"in the rendered output when extra_bits are present."
),
)
preview_href = preview_link_match.group("href")
self.assertIn(
"/admin/cms/alias/",
preview_href,
msg="Preview link href should point to the alias admin preview URL.",
)
self.assertIn(
str(alias.pk),
preview_href,
msg="Preview link href should contain the alias primary key.",
)
```
1. Ensure `re` is imported at the top of `tests/test_templatetags.py`:
- If not already present, add `import re`.
2. The above assertions assume an `alias` object (with `.pk`) is already available in the test method’s scope, which is typical for this kind of test. If the variable name differs (e.g., `alias_obj` or `self.alias`), replace `alias.pk` with the correct reference.
3. If the actual admin preview URL pattern differs from `/admin/cms/alias/`, adjust the `self.assertIn("/admin/cms/alias/", preview_href, ...)` check to match the real path segment used in your project’s URL configuration.
</issue_to_address>
### Comment 2
<location> `tests/test_templatetags.py:497-505` </location>
<code_context>
+
+ request.toolbar = toolbar
+
+ with self.subTest("preview-link hidden for anonymous user"):
+ anon_request = self.get_request("/")
+ anon_request.user = AnonymousUser()
+ output = self.render_template_obj(
+ alias_link_enabled_template,
+ {},
+ anon_request,
+ )
+ self.assertNotIn(alias_preview_link, output)
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert the alias content is still rendered for anonymous users, not only that the link is hidden.
This only asserts that the preview link is absent. Please also assert that the alias content is present (e.g. `self.assertEqual(alias_content, output)` or a suitable containment check) so we verify the link is hidden without suppressing the content for anonymous users.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
@wfehr This is certainly an interesting proposal. I like the idea that you can go to the edit(?) view of a static placeholder similarly as to editing a plugin alternatively to using the structure board (I assume you do not use it for static aliases?). I think we would need some discussion on how to consistently implement this, and have a consistent UI here. Maybe the right place for that would be the djangocms-alias workgroup on Discord? But first I need to understand your use case better.
I'm sorry if these questions are strange. I just want to wrap my head around what you're trying to achieve. |
Merged
4 tasks
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
2 participants
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.
Similar to #282.
I would like to enable a little link tooltip which points to the placeholder of a static-alias.
Little context:
Little hacky - but since we had this behaviour in cms 3 - we are using a similar approach in cms 4 / 5.
We got apphooks which have a content placeholder - in the old cms, this was possible (though not a wanted behaviour).
-> normal content placeholder which could be filled with plugins and the apphook could dynamically throw in data which the plugins could use.
Example: dynamic page output based on given sub paths of the url.
(e.g. /a/ = apphook url -> /a/foo and /a/bar could show different output)
With cms 4 / 5 I can maintain this behaviour by using "dynamic static-alias placeholders" which are built via apphook namespace.
Like
{% static_alias "some-prefix-<slugified namespace>" %}.And to provide the user a better interface in terms of "edit the content" -> with the changes made here they can click the added link and land on the placeholder preview page where they can create/edit drafts.
Summary by Sourcery
Enable optional placeholder preview link for static alias placeholders in frontend editing.
New Features:
placeholder_preview_linkflag on thestatic_aliastemplate tag to navigate directly to the alias placeholder's admin view from the frontend.Enhancements:
Tests: