[GrandComicsDatabase/gcd-django] disallow reprint links within an issue (PR #743)
jhunterjActual
Summary
Fixes #708.
Reprint links whose origin and target belong to the same issue are not meaningful reprints. The site previously allowed users to create these internal links, after which several reprint-editing operations could fail with unhandled exceptions.
This change establishes the invariant that a reprint’s origin and target cannot belong to the same issue. It enforces that invariant at the model boundary and throughout the OI workflows that can create or indirectly produce a reprint link.
The change also preserves a limited cleanup path for legacy internal links already present in imported data:
- Change Target remains available so the target can be moved to another issue.
- Mark To Delete remains available.
- Change Origin, Edit Note, Flip Reprint Direction, and Create Corresponding Sequence are disabled.
- Once a legacy internal link is marked for deletion, Restore is disabled.
No existing reprint records are automatically deleted or modified by this PR.
Problem and root cause
As described in #708, the application could accept reprint links between two sequences in the same issue even though those relationships are not reprints.
There was no single invariant preventing this at either of the main persistence boundaries:
Reprint, for records in the display database.ReprintRevision, for changes being processed through OI.
This meant an internal link could arise through several paths:
- Selecting the current issue, one of its sequences, or one of its covers while adding or editing a reprint.
- Entering the current issue ID or one of its story IDs manually.
- Flipping or restoring a legacy internal link.
- Creating a corresponding sequence from an internal link.
- Moving a story or sequence between issues when an existing external reprint would become internal after the move.
- Calling model or view code directly without passing through the selector UI.
Some of these paths reached the persistence layer and raised an unhandled ValueError; others could partially save related revisions before the invalid relationship was discovered.
Resulting behavior
| Operation | Result |
|---|---|
| Add a reprint to the current issue | Rejected |
| Add a reprint to a sequence in the current issue | Rejected |
| Add a reprint to a cover in the current issue | Rejected |
| Manually enter the current issue or story ID | Rejected before the reprint edit/save workflow proceeds |
Save an internal Reprint directly |
Rejected by the model |
Save a new or modified internal ReprintRevision |
Rejected by the model |
| Change the target of a legacy internal link | Allowed, so the link can be repaired |
| Change the origin of a legacy internal link | Disabled |
| Edit the note on a legacy internal link | Disabled |
| Flip a legacy internal link | Disabled and rejected server-side |
| Create a corresponding sequence from an internal link | Disabled and rejected server-side |
| Mark a legacy internal link for deletion | Allowed |
| Restore a deleted internal link | Disabled and rejected server-side |
| Move a story or sequence when the move would make a reprint internal | Rejected before reservation or mutation |
| Create or edit a normal cross-issue reprint | Unchanged |
The common user-facing validation message is:
Reprint origin and target cannot be in the same issue.
Implementation details
Model-level invariant
Reprint and ReprintRevision expose an is_internal() helper and validate the issue boundary during save().
The display model rejects any persisted reprint whose resolved origin and target issue IDs are equal.
The revision model rejects new or modified non-deleted internal revisions. It has one deliberately narrow exception: OI may create the initial revision history for an already-existing legacy internal Reprint.
That exception:
- Requires an existing display-database reprint.
- Only applies while creating its first cloned revision.
- Does not permit creation of a new internal reprint.
- Does not permit subsequent edits that leave the revision internal.
- Allows imported legacy data to be reserved and then repaired or deleted through OI.
OI workflow validation
The affected reprint views check the invariant before performing related writes. This includes:
- Reprint selection and confirmation.
- Saving reprint changes.
- Flipping the relationship.
- Restoring a deleted relationship.
- Creating a corresponding sequence.
- Moving a story or sequence between issues.
The save flow validates the assembled reprint revision before saving an associated StoryRevision. An invalid request therefore does not leave behind an unrelated story revision.
Story and sequence moves are checked before objects are reserved or mutated, preventing an existing cross-issue link from silently becoming internal as a side effect of a move.
Server-side checks remain in place even when the corresponding control is disabled in the template. Stale pages, direct POST requests, and hand-entered IDs therefore return a normal validation response rather than a server error.
Object selector
The object selector accepts the current issue as an exclusion and disables choices representing:
- The current issue.
- Stories or sequences belonging to the current issue.
- Covers belonging to the current issue.
The disabled choices are maintained as an immutable tuple on the form instance.
Users can still enter IDs manually, so the selector behavior is backed by server-side validation during selection confirmation and save.
Legacy-link controls
For an existing internal reprint link:
- Change Target uses the normal global object selector and remains enabled.
- Mark To Delete remains enabled.
- Change Origin, Edit Note, Flip Reprint Direction, and Create Corresponding Sequence are disabled.
- Restore is disabled after the link has been marked for deletion.
This behavior is applied consistently to:
- Display-database reprints.
- Revisions in the current changeset.
- Approved revision-history rows.
- Both origin-side and target-side renderings.
Accessibility and disabled-state UX
Disabled controls use the native disabled attribute and a visibly inactive button style. They do not retain the active blue color or active hover treatment.
Each disabled control includes:
- Hover text explaining why it is unavailable.
aria-describedbypointing to the visible explanation.- A smaller, italicized explanation alongside the relevant controls.
Redundant aria-disabled="true" attributes were removed where the native disabled attribute already supplies the disabled state.
Tests
Coverage was added for:
- The display-model invariant.
- The revision-model invariant.
- Initial revision cloning for legacy internal links.
- Rejection when a previously valid source is changed into an internal link.
- Rejection of manually entered current-issue and current-story IDs.
- Disabled issue, story, and cover choices in cached selectors.
- The immutable disabled-choice collection.
- Selector exclusions when adding a reprint.
- Graceful rejection of flip, restore, and matching-sequence requests.
- Guarding both matching-sequence endpoints.
- Validation before saving a related
StoryRevision. - Story and sequence moves that would create an internal link.
- The complete legacy-link button matrix.
- Display, current-changeset, and approved-revision template branches.
- Origin-side and target-side rendering.
- Disabling Restore after marking an internal link for deletion.
Validation performed
- Focused pytest selection: 52 passed, 5 deselected
python manage.py check --settings=settings_dev: no issues 12 pre-existing silenced messages comprise one development reCAPTCHA test-key check and eleven intentional OI
preview-proxy accessor clashes.- Flake8 on the new and changed Python code: clean
git diff --check: clean- Docker runtime smoke check: HTTP 200
- Live UAT confirmed:
- Current-issue selector choices are disabled.
- Change Target opens the global selector.
- The remaining unsafe internal-link actions are disabled.
- Matching-sequence requests are rejected gracefully.
- Rejected saves do not save a
StoryRevision. - Accessibility description IDs are unique.
A full-file Flake8 run on apps/oi/models.py reports two pre-existing, unrelated violations:
apps/oi/models.py:6031:27 E122apps/oi/models.py:7685:80 E501
The lines added by this PR are clean.
Reviewer UAT setup
The development database dump contains useful legacy internal-reprint fixtures, but those objects do not necessarily have the OI revision history required to reserve and edit them.
The separately attached seed_issue_708_history.py helper creates the missing development-only history for issue 26941 and the reprints touching it. It is intentionally not included in the branch.
Run the helper against a fresh development import before reserving issue 26941.
Prerequisites
Follow the Docker repository’s normal database-import procedure, then apply the existing migrations and load the development users:
docker compose up -d --build web docker compose run --rm web python gcd-django/manage.py migrate docker compose run --rm web python gcd-django/manage.py loaddata gcd-django/apps/indexer/fixtures/users.yaml
This PR contains no schema changes or new migrations.
Run the attached helper
From PowerShell:
Get-Content -Raw .\seed_issue_708_history.py | docker compose exec -T ` -e DJANGO_SETTINGS_MODULE=settings ` -e PYTHONPATH=/code/gcd-django ` web python -
From a POSIX shell:
docker compose exec -T \ -e DJANGO_SETTINGS_MODULE=settings \ -e PYTHONPATH=/code/gcd-django \ web python - < seed_issue_708_history.py
On the current UAT data, the two internal fixture IDs are:
14873751487376
The helper is transactional and idempotent. Running it again after a successful setup should report that no additional histories were created. It also refuses to seed missing history when one of the affected objects is already locked.
Manual UAT
- Start with a fresh imported development database.
- Apply the existing migrations and load the development users.
- Run the attached history setup helper before reserving issue
26941. - Sign in as
dexter_1234with the standard development password. - Reserve and edit issue
26941. - Open its reprint-link editing page.
- Start adding or changing a reprint and inspect the cached-object selector:
- The current issue’s Select button should be disabled.
- Its sequences and covers should also have disabled Select buttons.
- Disabled buttons should have an inactive color and no active hover treatment.
- Hover text and the smaller italicized explanation should identify why the choice is unavailable.
- Enter issue ID
26941, or a story ID belonging to it, manually:- The request should be rejected before an internal reprint can reach the edit/save workflow.
- The response should not be a server error.
- Inspect legacy internal links
1487375and1487376:- Change Target should remain enabled and open the global selector.
- Change Origin should be disabled.
- Edit Note should be disabled.
- Flip Reprint Direction should be disabled.
- Create Corresponding Sequence should be disabled where applicable.
- Mark To Delete should remain enabled.
- Mark one of the legacy internal links for deletion:
- The operation should succeed.
- Restore should then be disabled.
- Verify that a normal reprint between different issues can still be added and edited.
- Optionally submit stale or direct requests for flip, restore, or matching-sequence actions:
- They should return the validation message instead of an unhandled exception.
- Optionally attempt a story or sequence move that would make an existing reprint internal:
- The move should be rejected before the object is reserved or changed.
Scope and follow-up
This PR does not:
- Automatically delete or migrate existing internal reprint links.
- Change the existing behavior that may remove internal links when an issue is edited.
The implementation is limited to preventing new internal reprint links, preventing unsafe modifications of legacy ones, and preserving explicit repair and deletion paths.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/743
Commit Summary
- 35a800f disallow reprint links within an issue
File Changes
(10 files)
- M apps/gcd/models/reprint.py (11)
- M apps/gcd/tests/test_reprint.py (11)
- M apps/oi/models.py (29)
- M apps/oi/tests/test_reprint_revision.py (48)
- A apps/oi/tests/test_reprint_views.py (403)
- M apps/oi/views.py (76)
- M apps/select/forms.py (19)
- M apps/select/templates/select/select_object.html (13)
- M apps/select/views.py (12)
- M templates/oi/bits/reprint_type_list.html (58)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/743.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/743.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 prevents the creation of internal reprint links (where both origin and target are in the same issue) by adding validation checks in models, views, and forms, and disabling invalid options in the UI. The review feedback is highly constructive, identifying three opportunities to optimize database performance by using foreign key ID fields directly instead of triggering implicit queries on related model instances.
In apps/gcd/models/reprint.py:
> + def is_internal(self): + """Return whether both ends of the link are in the same issue.""" + origin_issue = self.origin.issue if self.origin else self.origin_issue + target_issue = self.target.issue if self.target else self.target_issue + return (origin_issue is not None and target_issue is not None and + origin_issue == target_issue)
In is_internal(), accessing self.origin.issue and self.target.issue triggers database queries to fetch the related Issue model instances if they are not already prefetched. Since we only need to compare the issue IDs, we can use the _id fields (issue_id on the Story model, and origin_issue_id/target_issue_id on the Reprint model) to perform the comparison entirely without fetching the Issue rows from the database.
- def is_internal(self): - """Return whether both ends of the link are in the same issue.""" - origin_issue = self.origin.issue if self.origin else self.origin_issue - target_issue = self.target.issue if self.target else self.target_issue - return (origin_issue is not None and target_issue is not None and - origin_issue == target_issue) + def is_internal(self): + """Return whether both ends of the link are in the same issue.""" + origin_issue_id = self.origin.issue_id if self.origin else self.origin_issue_id + target_issue_id = self.target.issue_id if self.target else self.target_issue_id + return (origin_issue_id is not None and target_issue_id is not None and + origin_issue_id == target_issue_id)
> + def is_internal(self): + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue = self.origin_revision.issue + elif self.origin: + origin_issue = self.origin.issue + else: + origin_issue = self.origin_issue + + if self.target_revision: + target_issue = self.target_revision.issue + elif self.target: + target_issue = self.target.issue + else: + target_issue = self.target_issue + + return (origin_issue is not None and target_issue is not None and + origin_issue == target_issue)
Similar to the display model, we can optimize is_internal() in ReprintRevision by using the _id fields (issue_id on StoryRevision and Story, and origin_issue_id/target_issue_id on ReprintRevision) to avoid fetching the related Issue rows from the database.
- def is_internal(self): - """Return whether both ends of the link are in the same issue.""" - if self.origin_revision: - origin_issue = self.origin_revision.issue - elif self.origin: - origin_issue = self.origin.issue - else: - origin_issue = self.origin_issue - - if self.target_revision: - target_issue = self.target_revision.issue - elif self.target: - target_issue = self.target.issue - else: - target_issue = self.target_issue - - return (origin_issue is not None and target_issue is not None and - origin_issue == target_issue) + def is_internal(self): + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + return (origin_issue_id is not None and target_issue_id is not None and + origin_issue_id == target_issue_id)
In apps/oi/views.py:
> +def _story_move_creates_internal_reprint(story, new_issue): + if not story.story_id or not new_issue.issue_id: + return False + return ( + story.story.from_all_reprints.filter( + origin_issue_id=new_issue.issue_id).exists() or + story.story.to_all_reprints.filter( + target_issue_id=new_issue.issue_id).exists())
In _story_move_creates_internal_reprint(), accessing story.story triggers a database query to fetch the Story model instance. We can completely avoid this query by querying the Reprint model directly using story.story_id (which is already populated on the StoryRevision instance without triggering a query). This reduces the number of database queries and avoids instantiating the Story model.
-def _story_move_creates_internal_reprint(story, new_issue): - if not story.story_id or not new_issue.issue_id: - return False - return ( - story.story.from_all_reprints.filter( - origin_issue_id=new_issue.issue_id).exists() or - story.story.to_all_reprints.filter( - target_issue_id=new_issue.issue_id).exists()) +def _story_move_creates_internal_reprint(story, new_issue): + if not story.story_id or not new_issue.issue_id: + return False + return ( + Reprint.objects.filter( + target_id=story.story_id, + origin_issue_id=new_issue.issue_id + ).exists() or + Reprint.objects.filter( + origin_id=story.story_id, + target_issue_id=new_issue.issue_id + ).exists() + )
—
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 3 commits.
- 779e377 avoid issue queries in reprint validation
- 5ed78d2 avoid issue queries in reprint revision validation
- 529cc1e avoid loading stories for reprint move checks
—
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.
In apps/gcd/models/reprint.py:
> + def is_internal(self): + """Return whether both ends of the link are in the same issue.""" + origin_issue = self.origin.issue if self.origin else self.origin_issue + target_issue = self.target.issue if self.target else self.target_issue + return (origin_issue is not None and target_issue is not None and + origin_issue == target_issue)
Addressed in 779e377. The normal path now compares issue IDs without loading Issue rows. I retained an object-comparison fallback when both IDs are absent to preserve validation for shared unsaved Issue objects.
—
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 commented on this pull request.
> + def is_internal(self): + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue = self.origin_revision.issue + elif self.origin: + origin_issue = self.origin.issue + else: + origin_issue = self.origin_issue + + if self.target_revision: + target_issue = self.target_revision.issue + elif self.target: + target_issue = self.target.issue + else: + target_issue = self.target_issue + + return (origin_issue is not None and target_issue is not None and + origin_issue == target_issue)
Addressed in 5ed78d2, with the same ID-based optimization and unsaved-object compatibility.
—
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 commented on this pull request.
In apps/oi/views.py:
> +def _story_move_creates_internal_reprint(story, new_issue): + if not story.story_id or not new_issue.issue_id: + return False + return ( + story.story.from_all_reprints.filter( + origin_issue_id=new_issue.issue_id).exists() or + story.story.to_all_reprints.filter( + target_issue_id=new_issue.issue_id).exists())
Addressed in 529cc1e. The move guard now queries Reprint directly using story_id and combines both directions into one exists() query.
—
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.
Thanks for tackling this.
I am not sure if we really want to all this checking in the form.
These are very limited cases, so the check on save could be enough ? It is a reasonable amount of code to avoid coming to that, which makes maintenance difficult.
In apps/gcd/models/reprint.py:
> @@ -24,6 +24,25 @@ class Meta:
notes = models.TextField(max_length=255)
+ def is_internal(self):
Why do we need this on the model ?
origin_issue_id is always set, so if we keep this, it can be simplified
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
it is not evident how this can happen, needs documentation
—
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.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
Instead of checking for existing internal reprint links in the code, we should edit the existing 90 occurrences and focus on the prevention of new ones. These are the key problem since ISEs arise, which don't for existing.
—
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.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
We might need to check on main on this, people might see this internal ones as relevant. Maybe the preview bug can be fixed 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.
jhunterjActual
@jhunterjActual commented on this pull request.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
Instead of checking for existing internal reprint links in the code, we should edit the existing 90 occurrences and focus on the prevention of new ones. These are the key problem since ISEs arise, which don't for existing.
Understood. The legacy-clone exception and special button behavior were added so imported internal links could be repaired or deleted after the new invariant landed. If the existing links will be corrected before deployment, that compatibility layer is unnecessary. I can remove the legacy-specific code and focus the PR on preventing new links, but the cleanup would need to be finished before deployment because the new Save guard would prevent those links from being cloned into OI afterward.
—
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 commented on this pull request.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
We might need to check on main on this, people might see this internal ones as relevant. Maybe the preview bug can be fixed instead.
I suspect this means I should suspend tweaks on the rest of this pending the answer to that question.
—
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.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
Yes, this should wait a few days.
—
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.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
seems like we don't want internal reprint 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.
jhunterjActual
@jhunterjActual commented on this pull request.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
seems like we don't want internal reprint links
I'll pick up the refactor work 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.
jhunterjActual
jhunterjActual left a comment (GrandComicsDatabase/gcd-django#743)Thanks for tackling this.
I am not sure if we really want to all this checking in the form. These are very limited cases, so the check on save could be enough ? It is a reasonable amount of code to avoid coming to that, which makes maintenance difficult.
I'm going to try to refactor the checking to reduce the maintenance headache -- I find it irritating when forms accept things they could have been designed against and just error out at the end, but if the reduced refactor is still onerous, I can remove it.
—
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 commented on this pull request.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
I'm going
it is not evident how this can happen, needs documentation
I deliberately preserved a synthetic unsaved-object test after Gemini’s suggestion. There is no demonstrated supported workflow requiring it.
—
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 commented on this pull request.
> + """Return whether both ends of the link are in the same issue.""" + if self.origin_revision: + origin_issue_id = self.origin_revision.issue_id + elif self.origin: + origin_issue_id = self.origin.issue_id + else: + origin_issue_id = self.origin_issue_id + + if self.target_revision: + target_issue_id = self.target_revision.issue_id + elif self.target: + target_issue_id = self.target.issue_id + else: + target_issue_id = self.target_issue_id + + if origin_issue_id is None and target_issue_id is None:
Instead of checking for existing internal reprint links in the code, we should edit the existing 90 occurrences and focus on the prevention of new ones. These are the key problem since ISEs arise, which don't for existing.
If we're going to pre-clean the current internal reprints, the prevention code can be simplified, yes.
—
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.