[GrandComicsDatabase/gcd-django] Avoid duplicate active-name query in creator name handling (PR #737)
Adam Hernandez
Summary
- evaluate the matching active-name queryset once with
.first() - reuse the matching creator name when present
- preserve the official house-name fallback when no match exists
- add focused coverage for both branches
Root cause
CreatorNameDetail.display_credit() evaluated the same filtered active-name queryset once for truthiness and again to retrieve its first result. That issued two equivalent database queries on the matching house-name path.
Impact
House-name credit rendering now performs one matching-name lookup without changing the rendered result or fallback behavior. This PR has no schema or migration changes.
Validation
pytest -q apps/gcd/tests/test_creator.py— 2 passedpytest -q apps/gcd/tests— 183 passed, 9 existing warningspytest -q— 450 passed, 18 existing warnings- Django system check, Python compilation, focused flake8, and
git diff --checkpassed
Closes #732
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/737
Commit Summary
- 385ed20 fix(gcd): avoid duplicate active-name query
File Changes
(2 files)
- M apps/gcd/models/creator.py (10)
- A apps/gcd/tests/test_creator.py (70)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/737.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/737.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 display_credit method in apps/gcd/models/creator.py to use .first() instead of evaluating the queryset and indexing [0], which avoids redundant queries. It also introduces unit tests for this behavior in apps/gcd/tests/test_creator.py. The review feedback suggests importing mock from the standard library unittest module and removing an unused __bool__ mock configuration in the tests that is no longer needed after the refactoring.
In apps/gcd/tests/test_creator.py:
> @@ -0,0 +1,70 @@ +# -*- coding: utf-8 -*- + +from types import SimpleNamespace + +import mock
In Python 3, unittest.mock is part of the standard library. It is recommended to use from unittest import mock instead of importing the third-party mock package to avoid unnecessary external dependencies and ensure compatibility with standard Python environments.
-import mock +from unittest import mock
In apps/gcd/tests/test_creator.py:
> + active_names.filter.return_value.first.return_value = matching_name + active_names.filter.return_value.__bool__.return_value = \ + matching_name is not None
Since the implementation of display_credit was updated to use .first() and check the truthiness of the returned model instance rather than evaluating the queryset directly, the mock configuration for __bool__ on the filtered queryset is no longer called or needed. Removing this dead mock setup improves test maintainability.
- active_names.filter.return_value.first.return_value = matching_name - active_names.filter.return_value.__bool__.return_value = \ - matching_name is not None + active_names.filter.return_value.first.return_value = matching_name
—
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.
Adam Hernandez
@DeusExTaco pushed 1 commit.
- 72e14af test(gcd): remove unused queryset mock setup
—
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.
Adam Hernandez
@DeusExTaco commented on this pull request.
In apps/gcd/tests/test_creator.py:
> @@ -0,0 +1,70 @@ +# -*- coding: utf-8 -*- + +from types import SimpleNamespace + +import mock
Thanks. This repository currently standardizes on the third-party mock package: it remains an explicit requirement and is used across 22 test modules. Changing only this import would not remove the dependency and would make this test inconsistent with its neighbors, so I am keeping import mock here. A repository-wide move to unittest.mock would be better handled separately.
—
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.
Adam Hernandez
> + active_names.filter.return_value.first.return_value = matching_name + active_names.filter.return_value.__bool__.return_value = \ + matching_name is not None
Thanks—agreed. I removed the unused __bool__ configuration in commit 72e14af5. The focused creator tests still pass (2 passed), and flake8 passes for the test module.
—
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
—
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.