[GrandComicsDatabase/gcd-django] Fix cross series variants (PR #717)
jhunterjActual
- Updated
IssueRevision.series_changedto accurately detect when a cross-series variant's parent issue moves to a different series, ensuring the routing engine applies stat deltas correctly. - Updated
scripts/reset_stats.pyto recalculate theSeries.issue_countcache in bulk. The script now mirrors the real-time logic: issues are counted if they are base issues OR variants belonging to a different series than their parent.
See comment for testing hurdles.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/717
Commit Summary
- 4b38446 Fix series issue count for cross-series variants (For #235 and #157)
- 132fae1 Fix cross-series variant issue counting
File Changes
(3 files)
- M apps/gcd/models/issue.py (7)
- M apps/oi/models.py (20)
- M scripts/reset_stats.py (37)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/717.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/717.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 updates the issue counting logic to correctly account for cross-series variants, ensuring they contribute to the issue count of the series they reside in. It modifies the real-time Python logic in Issue.stat_counts(), attempts to detect parent series changes in series_changed, and adds a bulk update script in scripts/reset_stats.py. The review feedback identifies two critical issues: first, the series_changed detection logic will always evaluate to False because both the old and new revisions point to the same updated database row; second, the bulk update script uses a CASE statement with potentially thousands of WHEN clauses, which could crash the database. Suggestions are provided to handle the variant stats adjustment in _handle_dependents and to use a more efficient Subquery with Coalesce for the bulk update.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
> + # Check 1: Standard direct series column change
+ # Catches standard issues or variants directly moved to a new series.
+ if ((not self.deleted) and
(self.previous_revision is not None) and
- self.previous_revision.series != self.series)
+ self.previous_revision.series != self.series):
+ return True
+
+ # Check 2: Cross-series variant parent change detect
+ # IMPORTANT: If an editor moves a base issue to a new series, any cross-series
+ # variants attached to it undergo a relational boundary change, even though
+ # the variant's own `series` field remains untouched. Flag this as a series
+ # change so the _adjust_stats engine routes the delta correctly.
+ if (not self.deleted) and (self.previous_revision is not None) and self.variant_of:
+ old_base_series = getattr(self.previous_revision.variant_of, 'series_id', None)
+ new_base_series = getattr(self.variant_of, 'series_id', None)
+ if old_base_series and new_base_series and old_base_series != new_base_series:
+ return True
+ return False
This check will always evaluate to False when a base issue is moved to a new series. Both self.previous_revision.variant_of and self.variant_of point to the same Issue database row. Since the base issue's row is updated first, accessing series_id on both will fetch the same updated value from the database, making old_base_series != new_base_series always False.
Furthermore, if the variant itself is not in the changeset (which is typical when only the base issue is moved), this property is never evaluated for the variant, leaving its stats out of sync.
Instead of trying to detect this on the variant's revision, we should handle the variant stat adjustments directly when the base issue is committed in _handle_dependents.
return ((not self.deleted) and (self.previous_revision is not None) and self.previous_revision.series != self.series)
> + issue_qs = Issue.objects.filter(deleted=False).filter(
+ Q(variant_of__isnull=True) | ~Q(series=F('variant_of__series'))
+ ).values('series').annotate(c=Count('id'))
+
+ whens = [When(pk=entry['series'], then=entry['c']) for entry in issue_qs]
+ if whens:
+ Series.objects.update(issue_count=Case(*whens, default=0))
+ else:
+ Series.objects.update(issue_count=0)
Using a CASE statement with thousands of WHEN clauses will exceed query size limits or crash the database on large datasets like GCD (which has over 100,000 series). Instead, we can perform this bulk update in a single, highly efficient SQL query using a Subquery with OuterRef and Coalesce.
- issue_qs = Issue.objects.filter(deleted=False).filter(
- Q(variant_of__isnull=True) | ~Q(series=F('variant_of__series'))
- ).values('series').annotate(c=Count('id'))
-
- whens = [When(pk=entry['series'], then=entry['c']) for entry in issue_qs]
- if whens:
- Series.objects.update(issue_count=Case(*whens, default=0))
- else:
- Series.objects.update(issue_count=0)
+ from django.db.models import Subquery, OuterRef
+ from django.db.models.functions import Coalesce
+
+ subquery = Issue.objects.filter(
+ deleted=False,
+ series_id=OuterRef('pk')
+ ).filter(
+ Q(variant_of__isnull=True) | ~Q(series_id=F('variant_of__series_id'))
+ ).values('series_id').annotate(c=Count('id')).values('c')
+
+ Series.objects.update(issue_count=Coalesce(Subquery(subquery), 0))
—
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 left a comment (GrandComicsDatabase/gcd-django#717)While testing the cross-series variant counting fix, I discovered a limitation in our local development environment initialization. Because the setup_initial_changesets.py script limits mock staging history to the first 500 IDs, it is impossible to test some cases (like variant issues) in the UI if they do not occur within those first 500 rows of the database dump.
To test the variant_of logic, I needed an existing variant issue.
The lowest is_variant issue ID existing in my database snapshot was 852842, so setup_initial_changesets.py skipped it during environment setup.
When I tried to click "Edit" on this issue in the local UI, the Online Indexing (oi) workflow engine crashed because it expects a baseline revision to clone from.
From there:
Trying to edit an uninitialized issue triggers a series of DoesNotExist exceptions:
First, it failed looking for a baseline IssueRevision.
After forcing an IssueRevision, it crashed on StoryRevision matching query does not exist.
After forcing the StoryRevisions, it crashed on StoryCreditRevision matching query does not exist.
To test the PR, I had to drop into python manage.py shell and synthesize an APPROVED (state=5) Changeset and recursively build the mock IssueRevision, StoryRevision, and StoryCreditRevision baseline rows for ID 852842; then the "Edit" button worked.
—
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 pushed 1 commit.
- 3d6fc63 Incorporate Gemini Code Assist feedback
—
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 pushed 1 commit.
- 777503b update comment blocks
—
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.
Incorporated Gemini Code Assist feedback
—
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.
JochenGCD
@jochengcd commented on this pull request.
> + # ---------------------------------------------------------------------
+ # When a base issue moves to a new series, its variants do not automatically
+ # follow it. This means a variant left behind in the old series just became
+ # a "cross-series" variant (which contributes +1 to its series issue_count),
+ # or vice-versa. Adjust the cached counts of the affected series.
+ if changes.get('series changed'):
+ old_series = changes.get('old series')
+ new_series = self.issue.series
+
+ IssueClass = type(self.issue)
+ variants = IssueClass.objects.filter(variant_of=self.issue, deleted=False)
+
+ for variant in variants:
+ # 1. Variant left behind: Goes from Standard -> Cross-Series (+1)
+ if variant.series == old_series and variant.series != new_series:
+ variant.series.issue_count += 1
From the other Gemini report:
When updating cached counts like issue_count on Series models, it is safer and more efficient to use update_fields=['issue_count']. This prevents overwriting other fields of the Series model that might have been modified concurrently.
We should use update_fields in other places.
> + new_series = self.issue.series + + IssueClass = type(self.issue) + variants = IssueClass.objects.filter(variant_of=self.issue, deleted=False) + + for variant in variants: + # 1. Variant left behind: Goes from Standard -> Cross-Series (+1) + if variant.series == old_series and variant.series != new_series: + variant.series.issue_count += 1 + variant.series.save() + + # 2. Base issue returns: Goes from Cross-Series -> Standard (-1) + elif variant.series != old_series and variant.series == new_series: + if variant.series.issue_count > 0: + variant.series.issue_count -= 1 + variant.series.save()
see above, the save should only be done if > 0
—
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 pushed 1 commit.
- 5477e94 use update_fields as suggested by Jochen
—
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.
JochenGCD
—
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.