[GrandComicsDatabase/gcd-django] Fix brand emblem (PR #723)

Josef Andersson

unread,

Jul 6, 2026, 4:23:06 AM (4 days ago) Jul 6

to GrandComicsDatabase/gcd-django, Subscribed

Looking at start contributing, mostly in dev-experience, tests and other things. Found some red tests and noticed recent refactorings, here is a try to complete that.

Two bugs from the brand -> brand_emblem change: forked variants copy the base issue's brand emblems, and brand group issue counts are not decremented on issue delete. Fixes both, with tests (test_fork_variant_for_cover_no_reserve, test_delete_issue).

Being new to scheme and domain of gcd.

You can view, comment on, or merge this pull request online at:

https://github.com/GrandComicsDatabase/gcd-django/pull/723

Commit Summary

f3332e6 fix brand emblem regressions from the m2m migration
dc76da6 update issue revision tests for the brand emblem changes

File Changes

(3 files)

M apps/oi/models.py (24)
M apps/oi/tests/conftest.py (8)
M apps/oi/tests/db/test_issue_revision.py (118)

Patch Links:

—
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.

Josef Andersson

unread,

Jul 6, 2026, 10:31:05 AM (4 days ago) Jul 6

to GrandComicsDatabase/gcd-django, Push

@janderssonse pushed 1 commit.

9273b5d update issue revision tests for the brand emblem changes

—
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

unread,

Jul 7, 2026, 5:39:00 PM (2 days ago) Jul 7

to GrandComicsDatabase/gcd-django, Subscribed

jochengcd left a comment (GrandComicsDatabase/gcd-django#723)

/gemini review

—
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]

unread,

Jul 7, 2026, 5:40:33 PM (2 days ago) Jul 7

to GrandComicsDatabase/gcd-django, Subscribed

@gemini-code-assist[bot] commented on this pull request.

Code Review

This pull request refactors the codebase to replace the brand field with brand_emblem across models, tests, and fixtures, and updates the brand group tracking logic in _check_major_change. Feedback on the changes highlights an N+1 query issue and a potential bug with duplicate groups in the newly introduced brand_groups helper function, which could lead to incorrect cached counts. Using prefetch_related and a set is recommended to resolve these issues.

In apps/oi/models.py:

> +            def brand_groups(issue_or_revision):
+                groups = []
+                for emblem in issue_or_revision.brand_emblem.all():
+                    groups.extend(emblem.group.all())
+                return groups

The current implementation of brand_groups has two issues:

N+1 Query Problem: It performs a database query for each emblem's groups in a loop (emblem.group.all()). This can be optimized by using prefetch_related('group') on the brand_emblem relation.
Duplicate Groups: If an issue has multiple brand emblems that belong to the same brand group, the returned list will contain duplicate BrandGroup objects. This will cause the cached counts of the same brand group to be decremented or incremented multiple times in _adjust_parent_counts. Using a set to collect the groups resolves this issue.

⬇️ Suggested change

-            def brand_groups(issue_or_revision):
-                groups = []
-                for emblem in issue_or_revision.brand_emblem.all():
-                    groups.extend(emblem.group.all())
-                return groups
+            def brand_groups(issue_or_revision):
+                groups = set()
+                for emblem in issue_or_revision.brand_emblem.prefetch_related('group'):
+                    groups.update(emblem.group.all())
+                return groups

—
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

unread,

Jul 7, 2026, 5:44:57 PM (2 days ago) Jul 7

to GrandComicsDatabase/gcd-django, Subscribed

@jochengcd commented on this pull request.

In apps/oi/models.py:

> +            def brand_groups(issue_or_revision):
+                groups = []
+                for emblem in issue_or_revision.brand_emblem.all():
+                    groups.extend(emblem.group.all())
+                return groups

the N+1 Query is more nice to have, we rarely have more than one or two emblems

the double counts appear more relevant

—
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.

Josef Andersson

unread,

Jul 9, 2026, 8:22:42 AM (16 hours ago) Jul 9

to GrandComicsDatabase/gcd-django, Push

@janderssonse pushed 2 commits.

0a57c02 fix: brand emblem regressions from the m2m migration
bfd2a59 test: update issue revision tests for the brand emblem changes

—
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.

Josef Andersson

unread,

Jul 9, 2026, 8:30:45 AM (16 hours ago) Jul 9

to GrandComicsDatabase/gcd-django, Subscribed

@janderssonse commented on this pull request.

In apps/oi/models.py:

> +            def brand_groups(issue_or_revision):
+                groups = []
+                for emblem in issue_or_revision.brand_emblem.all():
+                    groups.extend(emblem.group.all())
+                return groups

Thanks for looking, let's see if this is better, I have updated and rebased the PR ->> the double count is the issue, a group shared by two emblems gets adjusted twice in _adjust_parent_counts, so I used the set(). The same duplication hit active_issues() too, so that gets .distinct() now. (In a dev/toy/test branch I have another future relpath improvement possibly that generalizes this a bit, but will see, that is for the future possibly)

—
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

unread,

Jul 9, 2026, 5:36:10 PM (7 hours ago) Jul 9

to GrandComicsDatabase/gcd-django, Subscribed

Merged #723 into beta.

—
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.

Reply all

Reply to author

Forward