[GrandComicsDatabase/gcd-django] Improved handling of selection, ordering, and batch copying of covers and stories in /select_object/ (PR #767)
Simone Nardi
Features
- Added multi-selection with checkboxes and “Select all” / “Clear selection” actions per category.
- Added row drag&drop with automatic order persistence.
- Added single, multiple, per-category, and full cache removal.
- Added explicit primary cover selection for multi-cover selections.
- Added action bar with summary, copy, delete, and cancel actions.
Fixes
- Removed duplicate entries from the cache, including pre-existing ones.
- Unified single and batch copy handling: primary cover at position
0, subsequent reprints before selected stories. - Prevented duplicate copies through transaction handling and persistent
StoryCopyBatchrecords, including the related migration. - Blocked operations while saving the order; required a reload when the outcome is uncertain.
- Validated
cache_sizeas a positive integer and updated resizing logic to preserve the existing cache. - Replaced the hard-coded
2025search limit with the current year.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/767
Commit Summary
- 82e696a Fix full-catalog homepage and series search performance
- 5d7e153 Update apps/gcd/views/search.py
- 1010273 Fix Italian catalog terminology and add gettext to development image
- ef36af6 Merge catalog performance fixes into local Italian development branch
- ee8702a feat(select): add bulk copy and reorderable cover and story cache
File Changes
(20 files)
- M .github/workflows/dev-environment-ci.yml (1)
- M Dockerfile.dev (1)
- M apps/gcd/models/series.py (28)
- A apps/gcd/tests/test_front_page_search_performance.py (158)
- M apps/gcd/views/__init__.py (84)
- M apps/gcd/views/search.py (17)
- M apps/indexer/forms.py (6)
- A apps/oi/migrations/0068_storycopybatch.py (20)
- M apps/oi/models.py (8)
- A apps/oi/tests/test_bulk_copy.py (287)
- M apps/oi/tests/test_select_views.py (273)
- M apps/oi/views.py (262)
- M apps/select/forms.py (5)
- A apps/select/templates/select/cache_row_actions.html (15)
- M apps/select/templates/select/select_object.html (101)
- M apps/select/views.py (162)
- M locale/it/LC_MESSAGES/django.po (54)
- M locale/pt/LC_MESSAGES/django.po (4)
- A static/js/select_cache.js (149)
- M templates/oi/edit/confirm_copy_sequence.html (81)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/767.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/767.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 introduces a robust bulk-copying feature for cached sequences (covers and stories), including UI enhancements for managing and reordering remembered objects, dynamic validation for series years, and performance optimizations for homepage and series searches. The review feedback highlights several critical issues: a database integrity bug in apps/oi/views.py due to using a static hash for StoryCopyBatch keys, a failure to retrieve selected_covers during bulk cache removal in apps/select/views.py, a performance bottleneck in apps/gcd/views/search.py caused by evaluating querysets in memory, and a potential KeyError when sorting cache choices.
In apps/oi/views.py:
> + batch_key = hashlib.sha256(select_key.encode('utf8')).hexdigest()
+ if StoryCopyBatch.objects.filter(key=batch_key).exists():
+ return HttpResponseRedirect(destination)
+ if changeset.state not in (states.OPEN, states.REVIEWING):
+ return render_error(
+ request, 'This changeset is no longer open for editing.')
+
+ confirming = 'confirm_bulk_copy' in request.POST
+ if confirming:
+ try:
+ payload = signing.loads(request.POST.get('copy_batch', ''),
+ salt='copy-cached-sequences', max_age=3600)
Using a static hash of select_key as the primary key for StoryCopyBatch is a critical bug. Since select_key is a static string (e.g., 'batch'), the generated batch_key will always be identical across all users and all copy operations. This will cause database integrity errors (duplicate primary key) or block all subsequent copy operations globally after the first successful copy. Instead, we should check and create the StoryCopyBatch receipt only when confirming is True, using a hash of the unique signed copy_batch payload (which contains a timestamp and is unique to each confirmation form).
batch_key = None if changeset.state not in (states.OPEN, states.REVIEWING): return render_error( request, 'This changeset is no longer open for editing.') confirming = 'confirm_bulk_copy' in request.POST if confirming: copy_batch = request.POST.get('copy_batch', '') batch_key = hashlib.sha256(copy_batch.encode('utf8')).hexdigest() if StoryCopyBatch.objects.filter(key=batch_key).exists(): return HttpResponseRedirect(destination) try: payload = signing.loads(copy_batch, salt='copy-cached-sequences', max_age=3600)
> + if kind == 'all':
+ choices += request.POST.getlist('selected_covers')
When remove_selected_cached_objects is called with kind == 'cover', the selected choices are not retrieved because cover checkboxes are named selected_covers in the HTML template, but the view only retrieves cached_objects. We should ensure that selected_covers are also fetched when kind == 'cover'.
- if kind == 'all':
- choices += request.POST.getlist('selected_covers')
+ if kind == 'all' or kind == 'cover':
+ choices += request.POST.getlist('selected_covers')
> + series_ids_by_name = things.filter(name__icontains=series_name).values_list('pk', flat=True)
+ series_ids_by_issue = Issue.objects.filter(title__icontains=series_name).values_list('series_id', flat=True).distinct()
+ matching_ids = set(series_ids_by_name) | set(series_ids_by_issue)
+ things = things.filter(pk__in=matching_ids)
Evaluating the querysets into memory using set() can cause severe performance issues and high memory usage if the search term matches a large number of series or issues. It also results in a massive IN clause in the final SQL query. Instead, we should combine the subqueries using Django's Q objects with __in to let the database handle the filtering efficiently without loading all IDs into Python memory.
- series_ids_by_name = things.filter(name__icontains=series_name).values_list('pk', flat=True)
- series_ids_by_issue = Issue.objects.filter(title__icontains=series_name).values_list('series_id', flat=True).distinct()
- matching_ids = set(series_ids_by_name) | set(series_ids_by_issue)
- things = things.filter(pk__in=matching_ids)
+ series_ids_by_name = things.filter(name__icontains=series_name).values_list('pk', flat=True)
+ series_ids_by_issue = Issue.objects.filter(title__icontains=series_name).values_list('series_id', flat=True)
+ things = things.filter(Q(pk__in=series_ids_by_name) | Q(pk__in=series_ids_by_issue))
> + cache_choices.sort(key=lambda choice: {
+ 'cover': 0, 'story': 1, 'issue': 2}[choice['kind']])
Accessing the dictionary directly with choice['kind'] can raise a KeyError if there is any unexpected or malformed kind in the cache. Using .get() with a default fallback value is safer and prevents potential 500 errors.
- cache_choices.sort(key=lambda choice: {
- 'cover': 0, 'story': 1, 'issue': 2}[choice['kind']])
+ cache_choices.sort(key=lambda choice: {
+ 'cover': 0, 'story': 1, 'issue': 2}.get(choice['kind'], 3))
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- f76a20f fix(select): accept cover checkboxes when removing cached covers
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- 8a0d4dc fix: address bulk copy identity and search review findings
—
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.
Simone Nardi
@ProfNardi commented on this pull request.
> + cache_choices.sort(key=lambda choice: {
+ 'cover': 0, 'story': 1, 'issue': 2}[choice['kind']])
Fixed in 8a0d4dc. Unsupported cache categories are filtered out before sorting. The rendering regression test now includes an unknown category and verifies that it is omitted without affecting supported entries.
—
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.
Simone Nardi
@ProfNardi commented on this pull request.
In apps/oi/views.py:
> + batch_key = hashlib.sha256(select_key.encode('utf8')).hexdigest()
+ if StoryCopyBatch.objects.filter(key=batch_key).exists():
+ return HttpResponseRedirect(destination)
+ if changeset.state not in (states.OPEN, states.REVIEWING):
+ return render_error(
+ request, 'This changeset is no longer open for editing.')
+
+ confirming = 'confirm_bulk_copy' in request.POST
+ if confirming:
+ try:
+ payload = signing.loads(request.POST.get('copy_batch', ''),
+ salt='copy-cached-sequences', max_age=3600)
Fixed in 8a0d4dc. Each copy operation now has a UUID included in the signed confirmation payload and preserved across updated confirmations. The receipt is checked only after the confirmation is validated, so repeated submissions cannot duplicate the copy.
Regression tests cover independent operations sharing the same selection key, repeated submissions, and preservation of the operation ID across reconfirmations.
—
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.
Simone Nardi
@ProfNardi commented on this pull request.
> + if kind == 'all':
+ choices += request.POST.getlist('selected_covers')
Fixed in f76a20f. Bulk removal now reads selected_covers for both cover and all, while retaining support for cached_objects.
Regression tests cover both field names, repeated removal, and validation of the entire selection before modifying the session.
—
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.
Simone Nardi
@ProfNardi commented on this pull request.
> + series_ids_by_name = things.filter(name__icontains=series_name).values_list('pk', flat=True)
+ series_ids_by_issue = Issue.objects.filter(title__icontains=series_name).values_list('series_id', flat=True).distinct()
+ matching_ids = set(series_ids_by_name) | set(series_ids_by_issue)
+ things = things.filter(pk__in=matching_ids)
Fixed in 8a0d4dc. Matching IDs now stay in the database through a derived-table UNION subquery, eliminating the Python sets and expanded IN list.
The suggested OR query exceeded a 10-second limit on the local MySQL catalog, so the derived-table approach was used instead. Three search terms returned equivalent results without duplicates. A regression test also verifies that constructing the search does not evaluate the ID querysets.
—
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.
> @@ -1012,6 +1012,14 @@ def __str__(self):
return "Changeset"
+class StoryCopyBatch(models.Model):
Why add a model for temp data that can be put into the cache ?
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- d981f3a fix(oi): use cache for bulk copy tokens without schema 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.
Simone Nardi
@ProfNardi commented on this pull request.
> @@ -1012,6 +1012,14 @@ def __str__(self):
return "Changeset"
+class StoryCopyBatch(models.Model):
You’re right; a new model was unnecessary here. Removed StoryCopyBatch and its migration in d981f3a and switched to the existing cache.
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- d98a997 Refine object selector with compact search panels and cache actions
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- 8ac8abd Align ID selection buttons with their input fields
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- 88bc43d Merge beta into PR 767 and resolve imports and Italian metadata
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- cdee1a9 Improve object selection and batch copying of covers and stories
—
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 left a comment (GrandComicsDatabase/gcd-django#767)I see you are editing css, we use tailwind as a css framework, so no written css.
I am behind in documenting its usage.
—
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.
In templates/oi/edit/confirm_copy_sequence.html:
> +{% if bulk_copy %}
+<h2>Copy {{ stories|length }} {{ label }} into {{ issue_revision }}</h2>
+{% else %}
+<h2>Confirm to copy the following story into {{ issue_revision }}:</h2>
+{% endif %}
+{% if not needs_main_cover %}
+{% if copy_plan_changed %}
+<p role="alert">The copy destination or confirmation has changed. Review the updated placement and confirm again. Nothing has been copied.</p>
+{% endif %}
+{% if cover_mode == 'main' %}
+<p>Copy as main cover at position 0.</p>
+{% elif cover_mode == 'reprint' %}
+<p>This issue already has a cover. Copy as cover reprint (on interior page) at position {{ cover_position }}. The main cover stays in place.</p>
+{% endif %}
+{% endif %}
+{% if bulk_copy %}
We use two-space indentation for html files.
—
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 left a comment (GrandComicsDatabase/gcd-django#767)Functionality looks good.
The colors and stylings need to use the tailwind colors.
—
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 left a comment (GrandComicsDatabase/gcd-django#767)select_object is also used for adding reprints, which with this change doesn't work anymore.
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- ea4d798 Use shared Tailwind styles for object selection
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- 9a3feba Preserve the original selector for reprint links
—
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.
Simone Nardi
ProfNardi left a comment (GrandComicsDatabase/gcd-django#767)Thanks, Jochen. Addressed in ea4d798 and 9a3feba:
- Removed the custom stylesheets and inline CSS, replacing them with Tailwind utilities and existing shared button styles. HTML indentation now uses two spaces.
- Restricted the redesigned selector to sequence copying. “Add Reprint” and other selection workflows now use the original page again.
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- ad49b4c Keep selection action bar above the footer with Tailwind sticky utilities
—
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.
Simone Nardi
ProfNardi left a comment (GrandComicsDatabase/gcd-django#767)The current fix keeps their templates separate, but both still use /select_object/, with the template chosen from the operation stored in the session.
I think a dedicated URL and view for sequence copying would make the separation clearer, while reusing the existing search and cache helpers. /select_object/ would retain its original behavior for reprints and other selections.
Would you prefer that approach for this PR?
—
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 left a comment (GrandComicsDatabase/gcd-django#767)This is for sequences only, right ? We can make it under /select_multiple_sequences/ for the selection functionality, we might can use it for other purposes later.
The left/right border/margin on the page is larger then one other pages, should be consistent.
The text is smaller, I see a text-sm ? Should be consistent to other pages so that the user can consistently decide via zoom.
Do we need to show the order column, or can we hide it ? "Order" also line breaks before r on my window, which can be prevented.
The text on the buttons should all be capitalized, e.g. Copy Selected, again for consistency (not all buttons are made consistent, but all being edited should be made so)
Specific reason for covers above stories ? So far we had first stories, with the idea that the more often used object is on top. Should be consistent on the two select screens.
I am not sure about the red color for the clear-buttons. Don't think we have similar buttons so far ?
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- 33f1be6 Separate sequence selection and align UI with review 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.
Simone Nardi
ProfNardi left a comment (GrandComicsDatabase/gcd-django#767)Removed the “risky” changes while keeping the style 100% consistent:
- Added a dedicated
/select_multiple_sequences/URL and view, reusing the existing search and cache logic. - Preserved the original selector for reprints and other selections.
- Removed the extra horizontal margins and smaller text overrides.
- Hid the order column while keeping drag-and-drop ordering.
- Put stories before covers and capitalized the button labels.
- Replaced the red clear-cache buttons with muted Tailwind styling.
—
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 left a comment (GrandComicsDatabase/gcd-django#767)Thanks for the update.
On the confirmation page we so far show the sequence data. For me this is key to make sure that I got the right sequence, also the "Copy credits with credited/signed qualifiers" and "Copy characters" depends on the data of the sequence, which is now not accessible. I think we need to show this as before ?
Since I know these question will come from the users:
Is there a way to remember the order in which one selects the sequences and thereby order them ?
Do we want "Copy credits with credited/signed qualifiers" and "Copy characters" per sequence ?
—
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.
Simone Nardi
@ProfNardi pushed 1 commit.
- b96ac7f Restore sequence details and individual options in copy confirmation
—
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.
Simone Nardi
ProfNardi left a comment (GrandComicsDatabase/gcd-django#767)Ok. Updated in b96ac7fe:
- Restored full sequence details on the confirmation page.
- Added independent credit qualifier and character options for each sequence, with “select all” controls for multiple selections.
- Added a sticky action bar using the existing Tailwind styles.
- The behavior for a single sequence remains unchanged: once confirmed, it opens the copied sequence’s editor directly.
- All 35 targeted tests pass.
- Ordering still follows the cache’s order, which can be adjusted by dragging; selection click order is not tracked.
—
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.