feat: Setting to block deletion of public versions#501
Open
feat: Setting to block deletion of public versions#501
Conversation
Contributor
Reviewer's GuideIntroduce configurable deletion modes for versioned content (none, any, non-public-only), wire them into the deletion handler and version string representation, and update tests and docs accordingly. Class diagram for version deletion configuration and handlersclassDiagram
class Version {
+pk
+state
+object_id
+__str__() str
+verbose_name() str
}
class Constants {
+VERSION_STATES
+DELETE_NON_PUBLIC_ONLY
+DELETE_ANY
+DELETE_NONE
}
class DeletionHandlers {
+PROTECT_IF_PUBLIC_VERSION(collector, field, sub_objs, using)
+allow_deleting_versions(collector, field, sub_objs, using)
}
Version --> Constants : uses
DeletionHandlers --> Constants : uses
DeletionHandlers ..> Version : acts_on_versions
Flow diagram for allow_deleting_versions deletion modesflowchart TD
A[Start deletion of related versions] --> B[Read ALLOW_DELETING_VERSIONS setting]
B -->|DELETE_NON_PUBLIC_ONLY| C[Filter sub_objs where state == PUBLISHED]
B -->|DELETE_ANY or True| E[Set foreign key to NULL via SET_NULL]
B -->|DELETE_NONE or other| F[Protect all via PROTECT]
C -->|Public versions exist| D[Raise ProtectedError and block deletion]
C -->|No public versions| E
E --> G[Deletion of parent object continues]
F --> H[Deletion of parent object blocked]
D --> H
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #501 +/- ##
==========================================
+ Coverage 90.55% 93.68% +3.12%
==========================================
Files 72 76 +4
Lines 2732 2706 -26
Branches 322 0 -322
==========================================
+ Hits 2474 2535 +61
+ Misses 182 171 -11
+ Partials 76 0 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 4 issues, and left some high level feedback:
- In
PROTECT_IF_PUBLIC_VERSION, relying onsub_objs[0]inside the error message is a bit brittle and can diverge frompublic_exists(or even raise ifsub_objsis empty); consider usingpublic_exists.model/public_exists.first()or guarding against empty collections to make the error construction more robust. - The new
Version.__str__implementation catches a bareException, which can mask unexpected issues in content stringification; consider narrowing this toTypeError/AttributeError(or similar) and/or logging the failure before falling back to the simpler representation. - In
test_version_str_failing, the manual monkeypatch of__str__on the content class is restored only on the happy path; wrapping the patch/restore in atry/finallyor using a context manager/fixture would avoid leaving the class in a modified state if the assertion fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PROTECT_IF_PUBLIC_VERSION`, relying on `sub_objs[0]` inside the error message is a bit brittle and can diverge from `public_exists` (or even raise if `sub_objs` is empty); consider using `public_exists.model` / `public_exists.first()` or guarding against empty collections to make the error construction more robust.
- The new `Version.__str__` implementation catches a bare `Exception`, which can mask unexpected issues in content stringification; consider narrowing this to `TypeError`/`AttributeError` (or similar) and/or logging the failure before falling back to the simpler representation.
- In `test_version_str_failing`, the manual monkeypatch of `__str__` on the content class is restored only on the happy path; wrapping the patch/restore in a `try/finally` or using a context manager/fixture would avoid leaving the class in a modified state if the assertion fails.
## Individual Comments
### Comment 1
<location> `djangocms_versioning/models.py:39-40` </location>
<code_context>
+def PROTECT_IF_PUBLIC_VERSION(collector, field, sub_objs, using):
+ public_exists = sub_objs.filter(state=constants.PUBLISHED)
+ if public_exists:
+ # Any public objects?
+ raise models.ProtectedError(
</code_context>
<issue_to_address>
**issue (bug_risk):** The queryset truthiness check will raise in Django; use `.exists()` (or similar) instead.
Evaluating a QuerySet in a boolean context (`if public_exists:`) raises `ValueError: The truth value of a QuerySet is ambiguous`. Instead, assign the filtered queryset and call `.exists()` before raising:
```python
def PROTECT_IF_PUBLIC_VERSION(collector, field, sub_objs, using):
public_qs = sub_objs.filter(state=constants.PUBLISHED)
if public_qs.exists():
raise models.ProtectedError(..., public_qs)
```
This keeps the behavior but avoids a runtime error on delete.
</issue_to_address>
### Comment 2
<location> `tests/test_settings.py:59-55` </location>
<code_context>
+ @override_settings(DJANGOCMS_VERSIONING_ALLOW_DELETING_VERSIONS=constants.DELETE_NON_PUBLIC_ONLY)
</code_context>
<issue_to_address>
**issue (testing):** This test deletes a published version while the setting name and PR description suggest only non-public deletions should be allowed.
In `test_deletion_non_public_possible`, `version1` is set to `PUBLISHED` before calling `version1.content.delete()`. This makes the test assert that deleting a *public* version is allowed under `DELETE_NON_PUBLIC_ONLY`, which conflicts with both the setting name and the PR description (“allow non-public deletions and protect public versions”). Please confirm the intent: if we want to verify that only non‑public versions can be deleted, the test should instead delete a non‑public object (e.g. `version2.content`, which remains a draft) or otherwise make clear that the deleted object is not public.
</issue_to_address>
### Comment 3
<location> `tests/test_models.py:246-257` </location>
<code_context>
+ expected_str = f"Version #{version.pk} (Draft) of {version.content}"
+ self.assertEqual(str(version), expected_str)
+
+ def test_version_str_failing(self):
+ def failing_str_method(self):
+ raise Exception("Cannot stringify")
+
+ version = factories.PollVersionFactory()
+ original_str_method = version.content.__class__.__str__
+ version.content.__class__.__str__ = failing_str_method
+
+ expected_str = f"Version #{version.pk} (Draft)"
+ self.assertEqual(str(version), expected_str)
+
+ version.content.__class__.__str__ = original_str_method
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use a try/finally or context manager to restore the patched __str__ method even if the test assertion fails.
In `test_version_str_failing`, if the assertion (or any earlier line) raises, the patched `__str__` is never restored and can affect other tests. Wrap the patch and assertion in a `try/finally` so the original method is always restored, or use a monkeypatch fixture that handles cleanup automatically:
```python
original_str_method = version.content.__class__.__str__
try:
version.content.__class__.__str__ = failing_str_method
expected_str = f"Version #{version.pk} (Draft)"
self.assertEqual(str(version), expected_str)
finally:
version.content.__class__.__str__ = original_str_method
```
```suggestion
def test_version_str_failing(self):
def failing_str_method(self):
raise Exception("Cannot stringify")
version = factories.PollVersionFactory()
original_str_method = version.content.__class__.__str__
try:
version.content.__class__.__str__ = failing_str_method
expected_str = f"Version #{version.pk} (Draft)"
self.assertEqual(str(version), expected_str)
finally:
version.content.__class__.__str__ = original_str_method
```
</issue_to_address>
### Comment 4
<location> `docs/api/settings.rst:17` </location>
<code_context>
- rights. Set this to ``True`` if you want users to be able to delete versioned
- objects and you do not need a full history of versions, e.g. for documentation
- purposes.
+ If set to ``constants.DELETE_ANY`` (``"any"``)) users can delete version objects
+ if the have the appropriate rights. Set this to ``"any"`` if you want users to be
+ able to delete versioned objects and you do not need a full history of versions,
</code_context>
<issue_to_address>
**issue (typo):** Remove the extra closing parenthesis after the "any" constant label.
It should read: `If set to ``constants.DELETE_ANY`` (``"any"``) users can...` with only one closing parenthesis.
```suggestion
If set to ``constants.DELETE_ANY`` (``"any"``) users can delete version objects
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -55,3 +55,52 @@ def test_deletion_possible(self): | |||
| # try deleting and see if error is raised | |||
Contributor
There was a problem hiding this comment.
issue (testing): This test deletes a published version while the setting name and PR description suggest only non-public deletions should be allowed.
In test_deletion_non_public_possible, version1 is set to PUBLISHED before calling version1.content.delete(). This makes the test assert that deleting a public version is allowed under DELETE_NON_PUBLIC_ONLY, which conflicts with both the setting name and the PR description (“allow non-public deletions and protect public versions”). Please confirm the intent: if we want to verify that only non‑public versions can be deleted, the test should instead delete a non‑public object (e.g. version2.content, which remains a draft) or otherwise make clear that the deleted object is not public.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
fsbraun
commented
Feb 3, 2026
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
1 participant
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
Add configurable deletion modes for versioned content to allow any deletions, block all deletions, or block only public version deletions; update deletion handlers, version string output, tests, and documentation accordingly.
New Features:
DJANGOCMS_VERSIONING_ALLOW_DELETING_VERSIONSoptions: none, any, and non-public-only deletion modesEnhancements:
PROTECT_IF_PUBLIC_VERSIONhandler and extend allow_deleting_versions to enforce the new deletion modesVersion.__str__to include version state and associated content for clearer representationDocumentation:
Tests:
This is a first step to prevent accidental deletion of large page trees (see #490). Caveat: You still can delete an object that has a single version - independently if it's public or not.
Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Introduce configurable deletion modes for versioned content, including protection for public versions, and improve version string representation.
New Features:
Enhancements:
Documentation:
Tests: