[GrandComicsDatabase/gcd-django] Add multi-story copy actions to issue editing (PR #758)
Simone Nardi
Description
This change affects the Online Indexer Copy Sequence workflow.
Start from an issue changeset edit page, then select Copy Sequence.
The change is implemented in the session-based object selection screen,
select_object, used to select stories from the data cache.
Summary
- Allow selecting multiple cached stories and copying them together.
- Add a confirmation step with options to copy credits and characters.
- Preserve story order and avoid duplicate sources.
- Roll back the complete operation if any copy fails.
- Add
Clear Objectsand multiple-object removal actions. - Preserve the existing single-selection flow.
The implementation reuses the existing session-based selection flow and wraps the multiple-copy operation in a Django transaction.
Validation
- Manually verified the multi-selection flow locally.
- Django system check passed.
- Added coverage for selection, validation, ordering, duplicate handling, rollback, and cache actions.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/758
Commit Summary
- cede523 Add multi-story copy actions to issue editing
File Changes
(5 files)
- M apps/oi/tests/test_select_views.py (328)
- M apps/oi/views.py (56)
- M apps/select/templates/select/select_object.html (20)
- M apps/select/views.py (66)
- M templates/oi/edit/confirm_copy_sequence.html (18)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/758.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/758.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 support for copying multiple cached stories or covers at once into an issue revision, including UI updates for multiple selection, confirmation templates, and comprehensive unit tests. Feedback on the changes suggests correcting the indentation of the new _parse_selection function to follow PEP 8, improving the robustness of checkbox boolean checks by verifying key presence in request.POST instead of using .get(), and handling cases where multiple_selection might be explicitly set to None in session data.
> +def _parse_selection(request, choice):
+ try:
+ object_type, selected_id = choice.split('_', 1)
+ except ValueError:
+ return None, None, render_error(
+ request,
+ 'The selected object is invalid. Please return and try again.',
+ redirect=False)
+ if object_type not in ('publisher', 'series', 'issue', 'story', 'cover') \
+ or not selected_id.isdecimal():
+ return None, None, render_error(
+ request,
+ 'The selected object is invalid. Please return and try again.',
+ redirect=False)
+ return object_type, selected_id, None
The indentation of the _parse_selection function uses 8 spaces instead of the standard 4 spaces. According to PEP 8, 4 spaces per indentation level should be used to maintain consistency and readability across the codebase.
⬇️ Suggested change-def _parse_selection(request, choice):
- try:
- object_type, selected_id = choice.split('_', 1)
- except ValueError:
- return None, None, render_error(
- request,
- 'The selected object is invalid. Please return and try again.',
- redirect=False)
- if object_type not in ('publisher', 'series', 'issue', 'story', 'cover') \
- or not selected_id.isdecimal():
- return None, None, render_error(
- request,
- 'The selected object is invalid. Please return and try again.',
- redirect=False)
- return object_type, selected_id, None
+def _parse_selection(request, choice):
+ try:
+ object_type, selected_id = choice.split('_', 1)
+ except ValueError:
+ return None, None, render_error(
+ request,
+ 'The selected object is invalid. Please return and try again.',
+ redirect=False)
+ if (object_type not in ('publisher', 'series', 'issue', 'story', 'cover')
+ or not selected_id.isdecimal()):
+ return None, None, render_error(
+ request,
+ 'The selected object is invalid. Please return and try again.',
+ redirect=False)
+ return object_type, selected_id, None
References
- PEP 8 recommends using 4 spaces per indentation level. (link)
In apps/oi/views.py:
> + copy_credit_info = request.POST.get('copy_credit_info', False)
+ copy_characters = request.POST.get('copy_characters', False)
In Django, checkboxes that are unchecked are not submitted in the POST request, while checked checkboxes submit their value attribute (or 'on'). Using request.POST.get('copy_credit_info', False) can be problematic if a string like 'False' is sent, as any non-empty string is truthy in Python. It is more idiomatic and robust to check for the presence of the key in request.POST.
⬇️ Suggested change- copy_credit_info = request.POST.get('copy_credit_info', False)
- copy_characters = request.POST.get('copy_characters', False)
+ copy_credit_info = 'copy_credit_info' in request.POST
+ copy_characters = 'copy_characters' in request.POST
> + if not data.get('multiple_selection',
+ data.get('return') == '_selected_copy_sequence'):
Using data.get('multiple_selection', default) can lead to unexpected behavior if multiple_selection is explicitly set to None in the session data, as the default fallback will be ignored and None (falsy) will be returned. It is safer to explicitly check if the value is None and fall back to the default, matching the logic used in the GET block on lines 352-354.
⬇️ Suggested change- if not data.get('multiple_selection',
- data.get('return') == '_selected_copy_sequence'):
+ can_copy_multiple = data.get('multiple_selection')
+ if can_copy_multiple is None:
+ can_copy_multiple = data.get('return') == '_selected_copy_sequence'
+ if not can_copy_multiple:
—
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.
- b125002 Address review feedback on multi-story copy support
—
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
—
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 left a comment (GrandComicsDatabase/gcd-django#758)This PR is superseded by #767.
The changes have been incorporated into the newer PR, which should be reviewed and merged instead.
—
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.