[GrandComicsDatabase/gcd-django] Remove related revisions when deleting stories (PR #740)
jhunterjActual
Summary
Closes #662.
When a sequence is deleted, this change:
- Removes newly added Credit, Character, and Group revisions.
- Resets edits to existing related revisions from their published source data.
- Marks existing related revisions for deletion.
- Keeps Credit, Character, and Group deletion states synchronized when sequence deletion is toggled.
- Restores many-to-many character/group-name values before deletion.
A regression test covers added and edited related revisions, including many-to-many data.
Validation
- Focused #662 regression test passed.
- Full story-revision test file passed: 62 tests.
- Django system check passed.
- Manual UAT passed for adding and editing related records before deleting a sequence. I used issue record 92: New Fun (1935 series) DC #v1#6 sequence 53 for the check.
git diff --checkpassed.
No database migration is required.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/740
Commit Summary
- 34d507e Remove related revisions when deleting stories
File Changes
(2 files)
- M apps/oi/models.py (41)
- M apps/oi/tests/db/test_story_revision.py (111)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/740.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/740.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request refactors the deletion and value-resetting logic for story revisions to handle story character and story group revisions alongside story credit revisions. It also adds a comprehensive test suite to verify that deleting a story correctly resets these related revisions. The reviewer identified a potential AttributeError in _reset_values if revision.source is None (which can happen if added revisions are cached or prefetched and still present in the relation list during iteration), and suggested adding safety checks before accessing revision.source.
> + for field in revision._get_single_value_fields(): + setattr(revision, field, + getattr(revision.source, field)) + revision.deleted = True + revision.save() + for field in revision._get_multi_value_fields(): + getattr(revision, field).set( + getattr(revision.source, field).all())
If revision.source is None (which is the case for newly added revisions), attempting to access its attributes will raise an AttributeError.
Although _pre_delete is called before _reset_values and deletes added revisions from the database, if the relation (e.g., story_credit_revisions) was prefetched or cached on the story instance, the Python list returned by getattr(self, related_name).all() will still contain these deleted added revisions. When the loop iterates over them, it will crash when trying to access revision.source.
Adding a check to ensure revision.source is not None before attempting to copy its fields prevents this potential crash.
- for field in revision._get_single_value_fields(): - setattr(revision, field, - getattr(revision.source, field)) - revision.deleted = True - revision.save() - for field in revision._get_multi_value_fields(): - getattr(revision, field).set( - getattr(revision.source, field).all()) + if revision.source: + for field in revision._get_single_value_fields(): + setattr(revision, field, + getattr(revision.source, field)) + revision.deleted = True + revision.save() + if revision.source: + for field in revision._get_multi_value_fields(): + getattr(revision, field).set( + getattr(revision.source, field).all())
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
jhunterjActual
@jhunterjActual commented on this pull request.
> + for field in revision._get_single_value_fields(): + setattr(revision, field, + getattr(revision.source, field)) + revision.deleted = True + revision.save() + for field in revision._get_multi_value_fields(): + getattr(revision, field).set( + getattr(revision.source, field).all())
The concern doesn't apply to this workflow, and the suggested change introduces a big bug:
_pre_delete() runs before _reset_values(), and the approval path does not prefetch these related managers, so _reset_values() performs a fresh query after the added revisions are deleted.
Django clears an instance’s primary key after delete(). The suggested guard would still call save() on such a cached instance, reinserting the source-less revision that this change is intended to remove. If we later support prefetched related revisions here, the safe handling would be to skip source-less cached objects entirely rather than save them.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.