feat: Allow djangocms-core to offer a "New draft" button for published placeholders #503
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
Contributor
Reviewer's GuideAdds logic in is_content_editable to expose "new draft" metadata on placeholders for non-draft, editable versions, and introduces tests ensuring these properties are only set for published placeholders with edit-redirect enabled while keeping behavior unchanged for other states. Sequence diagram for exposing new draft metadata on placeholderssequenceDiagram
actor Editor
participant CMSView as CMSView
participant Placeholder as Placeholder
participant Helpers as is_content_editable
participant Versionables as versionables
participant VersionModel as Version
participant UrlResolver as reverse
Editor->>CMSView: Request published page
CMSView->>Placeholder: Iterate placeholders
CMSView->>Helpers: is_content_editable(placeholder, user)
Helpers->>Versionables: for_content(placeholder.source)
Versionables-->>Helpers: proxy_model
Helpers->>VersionModel: get_for_content(placeholder.source)
VersionModel-->>Helpers: version
alt version.state is DRAFT
Helpers-->>CMSView: True
CMSView->>Placeholder: Render edit UI (no new draft button)
else version.check_edit_redirect.as_bool(user) is True
Helpers->>UrlResolver: reverse(admin:proxy_model_edit_redirect, version.pk)
UrlResolver-->>Helpers: new_draft_url
Helpers->>Placeholder: Set new_draft
Helpers->>Placeholder: Set new_draft_method
Helpers->>Placeholder: Set new_draft_url
Helpers-->>CMSView: False
CMSView->>Placeholder: Render toolbar with Create new draft button
else other states or no redirect
Helpers-->>CMSView: False
CMSView->>Placeholder: Render without new draft button
end
CMSView-->>Editor: Response HTML
Class diagram for updated is_content_editable placeholder and version interactionsclassDiagram
class Placeholder {
+object source
+str new_draft
+str new_draft_method
+str new_draft_url
}
class Version {
+int pk
+str state
+CheckEditRedirect check_edit_redirect
+Version get_for_content(object content)
}
class CheckEditRedirect {
+bool as_bool(object user)
}
class VersionablesRegistry {
+Versionable for_content(object content)
}
class Versionable {
+ProxyModel version_model_proxy
}
class ProxyModel {
+Meta _meta
}
class Meta {
+str app_label
+str model_name
}
class Helpers {
+bool is_content_editable(Placeholder placeholder, object user)
}
Placeholder --> Version : source_version_resolved_by
Helpers --> Placeholder : reads_and_sets_metadata
Helpers --> VersionablesRegistry : uses_for_content
VersionablesRegistry --> Versionable : returns
Versionable --> ProxyModel : provides_version_model_proxy
ProxyModel --> Meta : has_meta
Helpers --> Version : uses_get_for_content
Version --> CheckEditRedirect : has_check_edit_redirect
Helpers --> CheckEditRedirect : calls_as_bool
Helpers --> ProxyModel : uses_for_admin_url_generation
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}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
is_content_editable, the draft-state check is now performed twice (earlyif version.state == DRAFT: return Trueand the finalreturn version.state == DRAFT), so you can simplify this logic and make the return behavior more explicit around the new side-effectful block that setsnew_draftattributes. - The four
NewDraftPropertiesTestCasemethods share very similar setup and assertions; consider extracting a small helper or using parametrization to reduce repetition and make the intent of the different state-dependent behaviors clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `is_content_editable`, the draft-state check is now performed twice (early `if version.state == DRAFT: return True` and the final `return version.state == DRAFT`), so you can simplify this logic and make the return behavior more explicit around the new side-effectful block that sets `new_draft` attributes.
- The four `NewDraftPropertiesTestCase` methods share very similar setup and assertions; consider extracting a small helper or using parametrization to reduce repetition and make the intent of the different state-dependent behaviors clearer.
## Individual Comments
### Comment 1
<location> `djangocms_versioning/helpers.py:258-261` </location>
<code_context>
"""
try:
- versionables.for_content(placeholder.source)
+ proxy_model = versionables.for_content(placeholder.source).version_model_proxy
except KeyError:
return True
from .models import Version
version = Version.objects.get_for_content(placeholder.source)
+ if version.state == DRAFT:
+ return True
+ if version.check_edit_redirect.as_bool(user):
+ placeholder.new_draft = _("Create new draft to edit")
+ placeholder.new_draft_method = "cms-form-post-method"
+ placeholder.new_draft_url = reverse(
+ f"admin:{proxy_model._meta.app_label}_{proxy_model.__name__.lower()}_edit_redirect",
+ args=(version.pk,),
</code_context>
<issue_to_address>
**suggestion:** Prefer using `_meta.model_name` instead of `__name__.lower()` when building the admin URL.
Using `proxy_model._meta.model_name` is more robust, especially if the model customizes `object_name` or related internals, and it better reflects Django’s conventions. For example:
```python
placeholder.new_draft_url = reverse(
f"admin:{proxy_model._meta.app_label}_{proxy_model._meta.model_name}_edit_redirect",
args=(version.pk,),
)
```
```suggestion
placeholder.new_draft_url = reverse(
f"admin:{proxy_model._meta.app_label}_{proxy_model._meta.model_name}_edit_redirect",
args=(version.pk,),
)
```
</issue_to_address>
### Comment 2
<location> `tests/test_checks.py:55-64` </location>
<code_context>
+
+
+class NewDraftPropertiesTestCase(CMSTestCase):
+ def test_new_draft_properties_set_for_published_placeholder(self):
+ """Test that new_draft properties are set on published placeholders"""
+ user = self.get_superuser()
+ version = PageVersionFactory(state=PUBLISHED)
+ placeholder = PlaceholderFactory(source=version.content)
+
+ # Call is_content_editable which should set the new_draft properties
+ is_content_editable(placeholder, user)
+
+ # Check that new_draft properties are set
+ self.assertTrue(hasattr(placeholder, "new_draft"))
+ self.assertTrue(hasattr(placeholder, "new_draft_method"))
+ self.assertTrue(hasattr(placeholder, "new_draft_url"))
+ self.assertEqual(placeholder.new_draft_method, "cms-form-post-method")
+ self.assertIn("edit-redirect", placeholder.new_draft_url)
+
+ def test_new_draft_properties_not_set_for_archived_placeholder(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for when `check_edit_redirect.as_bool(user)` returns `False` to ensure no `new_draft` attributes are set in that branch.
Right now we only exercise the `True` branch of `check_edit_redirect.as_bool(user)` via the published case. Please add a test where it returns `False` for a published version and assert that:
- `is_content_editable` returns the expected value for that scenario, and
- `new_draft`, `new_draft_method`, and `new_draft_url` are not set on the placeholder.
This will cover both sides of the conditional and protect against regressions where `new_draft` attributes are incorrectly added when the redirect check fails.
Suggested implementation:
```python
class NewDraftPropertiesTestCase(CMSTestCase):
def test_new_draft_properties_set_for_published_placeholder(self):
"""Test that new_draft properties are set on published placeholders"""
user = self.get_superuser()
version = PageVersionFactory(state=PUBLISHED)
placeholder = PlaceholderFactory(source=version.content)
# Call is_content_editable which should set the new_draft properties
is_content_editable(placeholder, user)
# Check that new_draft properties are set
self.assertTrue(hasattr(placeholder, "new_draft"))
self.assertTrue(hasattr(placeholder, "new_draft_method"))
self.assertTrue(hasattr(placeholder, "new_draft_url"))
self.assertEqual(placeholder.new_draft_method, "cms-form-post-method")
self.assertIn("edit-redirect", placeholder.new_draft_url)
def test_new_draft_properties_not_set_for_published_when_redirect_check_false(self):
"""
When check_edit_redirect.as_bool(user) returns False for a published version,
is_content_editable should not inject any new_draft attributes.
"""
user = self.get_superuser()
version = PageVersionFactory(state=PUBLISHED)
placeholder = PlaceholderFactory(source=version.content)
with patch("djangocms_versioning.checks.check_edit_redirect.as_bool", return_value=False):
editable = is_content_editable(placeholder, user)
# Assert the expected is_content_editable result for this scenario
self.assertFalse(editable)
# Check that new_draft properties are NOT set when redirect check is False
self.assertFalse(hasattr(placeholder, "new_draft"))
self.assertFalse(hasattr(placeholder, "new_draft_method"))
self.assertFalse(hasattr(placeholder, "new_draft_url"))
def test_new_draft_properties_not_set_for_archived_placeholder(self):
"""Test that new_draft properties are NOT set on archived placeholders"""
user = self.get_superuser()
version = PageVersionFactory(state=ARCHIVED)
placeholder = PlaceholderFactory(source=version.content)
# Call is_content_editable which should NOT set the new_draft properties for archived
is_content_editable(placeholder, user)
# Check that new_draft properties are NOT set
self.assertFalse(hasattr(placeholder, "new_draft"))
```
1. Ensure `patch` is imported at the top of `tests/test_checks.py`, for example:
`from unittest.mock import patch`.
2. The patch target string `"djangocms_versioning.checks.check_edit_redirect.as_bool"` assumes that:
- `is_content_editable` is defined in `djangocms_versioning.checks`, and
- `check_edit_redirect` is referenced from that module.
If the actual module path differs, adjust the patch target to point to the module where
`is_content_editable` is defined (i.e., patch where it is *used*, not necessarily where
`check_edit_redirect` is originally defined).
3. If the expected return value of `is_content_editable` in the "redirect check False" scenario
should be `True` in your implementation (instead of `False` as asserted above), update
`self.assertFalse(editable)` accordingly to match the existing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #503 +/- ##
==========================================
+ Coverage 90.55% 93.65% +3.10%
==========================================
Files 72 76 +4
Lines 2732 2697 -35
Branches 322 0 -322
==========================================
+ Hits 2474 2526 +52
+ Misses 182 171 -11
+ Partials 76 0 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
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
Enable placeholders backed by published versions to expose metadata for creating a new draft when content is not directly editable.
New Features:
new_draftlabel, method, and URL on placeholders whose content requires a new draft to be created before editing.Tests:
new_draftproperties on placeholders across draft, published, unpublished, and archived version states.Fixes django-cms/django-cms#8415
Related resources
static_aliasintegration into structure board django-cms#8415Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.