Reuse existing season items across library buckets on completion - #760
Reuse existing season items across library buckets on completion#760JoshDaFishy wants to merge 2 commits into
Conversation
ryan-winkler
left a comment
ryan-winkler
left a comment
There was a problem hiding this comment.
The reported failure is real and the PR is aimed at the correct user problem. The concrete imported-library evidence in the description is especially useful. I found three blocking correctness points that need to be resolved before this can merge:
max_watchedis an episode position, not a completed-episode count. Sparse history can therefore becomeCompletedincorrectly.- The new lookup-then-
create()path removes the database-safeget_or_create()behavior and can fail under concurrent completion requests. - The unbounded cross-bucket fallback can select an anime season for a normal TV parent, or the reverse, when the preferred bucket is absent.
I added exact code suggestions for the two changed paths. Equivalent code is fine if it preserves these rules. I also created #812 as the regression contract and enriched this PR with the full test matrix, relationships, /qa result, interaction-scope statement, and post-mortem.
Please add focused tests for:
- reuse of an imported
tv-bucket season without creating a secondItem,Season, or episode set; - preservation of the existing episode dates;
- exclusion of anime candidates for normal TV and normal-TV candidates for anime;
- one late episode out of ten remaining
In progress; - all ten distinct completed episodes becoming
Completed; - repeated plays not increasing the distinct completion count;
- a deliberate
In progressrewatch remainingIn progress.
The branch also needs an update from current latest before validation. Record the targeted tests, fast suite, Ruff, migration dry-run, and diff check after that update. The AI disclosure also needs the exact provider and model identifier; the current wording is not specific enough to verify.
No screenshot is required for this backend-only change. A manual data check should still confirm that old dates remain visible after whole-show completion.
Thank you for isolating the bucket mismatch and for opening a draft rather than presenting this as merge-ready. The issue is worth fixing; these changes will make the repair safe for imported libraries and future identity variants.
| from integrations.imports.helpers import find_item_across_buckets | ||
|
|
||
| season_identity = { | ||
| "media_id": self.item.media_id, | ||
| "source": self.item.source, | ||
| "media_type": MediaTypes.SEASON.value, | ||
| "season_number": season_number, | ||
| } | ||
| item = find_item_across_buckets( | ||
| preferred_bucket=self.item.library_media_type, | ||
| **season_identity, | ||
| ) | ||
| if item is None: | ||
| item = Item.objects.create( | ||
| **season_identity, | ||
| library_media_type=( | ||
| MediaTypes.ANIME.value | ||
| if self.item.library_media_type == MediaTypes.ANIME.value | ||
| else MediaTypes.SEASON.value | ||
| ), | ||
| **Item.title_fields_from_metadata( | ||
| season_metadata, | ||
| fallback_title=self.item.title, | ||
| ), | ||
| "image": season_image, | ||
| }, | ||
| ) | ||
| image=season_image, | ||
| ) |
There was a problem hiding this comment.
This block needs both an allowed-bucket rule and the existing database-safe create behavior. find_item_across_buckets() can fall back to an anime candidate for a normal TV parent when the preferred bucket is absent, and the following plain create() can race after two requests both observe no item. The replacement below prefers only compatible buckets and uses get_or_create() for the target bucket.
| from integrations.imports.helpers import find_item_across_buckets | |
| season_identity = { | |
| "media_id": self.item.media_id, | |
| "source": self.item.source, | |
| "media_type": MediaTypes.SEASON.value, | |
| "season_number": season_number, | |
| } | |
| item = find_item_across_buckets( | |
| preferred_bucket=self.item.library_media_type, | |
| **season_identity, | |
| ) | |
| if item is None: | |
| item = Item.objects.create( | |
| **season_identity, | |
| library_media_type=( | |
| MediaTypes.ANIME.value | |
| if self.item.library_media_type == MediaTypes.ANIME.value | |
| else MediaTypes.SEASON.value | |
| ), | |
| **Item.title_fields_from_metadata( | |
| season_metadata, | |
| fallback_title=self.item.title, | |
| ), | |
| "image": season_image, | |
| }, | |
| ) | |
| image=season_image, | |
| ) | |
| target_bucket = ( | |
| MediaTypes.ANIME.value | |
| if self.item.library_media_type == MediaTypes.ANIME.value | |
| else MediaTypes.SEASON.value | |
| ) | |
| season_identity = { | |
| "media_id": self.item.media_id, | |
| "source": self.item.source, | |
| "media_type": MediaTypes.SEASON.value, | |
| "season_number": season_number, | |
| } | |
| allowed_buckets = [self.item.library_media_type] | |
| if target_bucket not in allowed_buckets: | |
| allowed_buckets.append(target_bucket) | |
| item = None | |
| for bucket in allowed_buckets: | |
| item = ( | |
| Item.objects.filter( | |
| **season_identity, | |
| library_media_type=bucket, | |
| ) | |
| .order_by("id") | |
| .first() | |
| ) | |
| if item is not None: | |
| break | |
| if item is None: | |
| item, _ = Item.objects.get_or_create( | |
| **season_identity, | |
| library_media_type=target_bucket, | |
| defaults={ | |
| **Item.title_fields_from_metadata( | |
| season_metadata, | |
| fallback_title=self.item.title, | |
| ), | |
| "image": season_image, | |
| }, | |
| ) |
| # count when we have one; only assume "still watching" when we | ||
| # genuinely have no episode total from any local source. | ||
| local_total = getattr(self.item, "local_season_episode_count", None) | ||
| if local_total and max_watched >= local_total: |
There was a problem hiding this comment.
max_watched is the highest episode position. It is not the number of completed episodes. With local_total=10, a single watch of episode 10 satisfies this condition and marks the season complete. The model already provides a distinct completed-episode count that ignores repeated plays and non-completed rows.
| if local_total and max_watched >= local_total: | |
| if local_total and self.completed_episode_count >= local_total: |
Please cover the sparse E10-only case, duplicate plays, full distinct completion, and the manual In progress rewatch override.
ryan-winkler
added
the
bug
ChatGPT Codex Connector
|
One important correction after checking the current After updating the branch, reuse the existing count-based model behavior instead of adding another status algorithm: local_total = getattr(self.item, "local_season_episode_count", None) or 0
known_total = total_eps or local_total or None
desired_status = self.derived_status_from_episode_progress(
max_progress=known_total,
)
if (
desired_status == Status.COMPLETED.value
and self.status == Status.IN_PROGRESS.value
):
# Preserve a deliberate rewatch override.
desired_status = Status.IN_PROGRESS.value
Please add both regression cases:
Also retain the full-completion, duplicate-play, dropped-episode, and manual-rewatch cases in #812. An equivalent implementation is acceptable if it preserves these outcomes. |
959d815 to
12b4ac6
Compare
|
Thanks for the detailed review — all three findings were real defects in my draft.
One deliberate deviation: I kept an explicit PLANNING result when no episodes are logged. derived_status_from_episode_progress() returns the current status in that case, which would leave a season reading Completed after unwatch() removes the last episode — a regression, since this method is called from that path. Happy to drop it if you'd rather that case were handled elsewhere.
Branch updated from current latest. tv.py was unchanged across those commits, so the update was conflict-free. Validation on the updated branch: app.tests.models.test_tv_completed_on_create test_season test_episode_bucket — 47 tests, OK Full suite and the #812 regression module still to come; I'll push those before marking this ready for review. |
|
Regression tests added in src/app/tests/models/test_tv_completion_identity.py — 13 tests covering the #812 matrix. Verified in both directions: against this branch: 13 tests, OK One honest caveat: test_late_single_episode_with_local_count_stays_in_progress passes on unpatched code too, because the local-count branch doesn't exist there. It guards the new behaviour rather than proving a fixed defect. Two of the sparse tests initially passed against unpatched code for the wrong reason — Episode.save() leaves the season In progress, and the rewatch override then masks a wrong Completed. They now reset the status via a queryset update() before calling the sync, so the release-event case discriminates properly. Concurrency is covered by patching title_fields_from_metadata to insert the conflicting row; it's evaluated while building the defaults argument, so it lands between the bucket lookup and the create. Without get_or_create this raises IntegrityError. Validation on the updated branch: ruff check src — clean Note: app.tests.models.test_tv.TVModel.test_tv_save fails with AssertionError: 0 != 10 on this branch and on unmodified latest, so it appears pre-existing and unrelated. |
Review & Status CheckThe goal of resolving existing seasons across compatible library buckets on completion (avoiding duplicate season/episode generation) is well aligned with #812 and #623. Required Formatting & CI Fix
Follow the guidance outlined in the review notes (verifying distinct completed count over position and maintaining atomic |
…pisodes Search compatible library buckets in priority order instead of keying get_or_create on the season bucket alone, so an imported tv-bucket season is reused rather than forked. Never cross normal-TV and anime parent identities. Retain get_or_create for the create path. Route both the release-event and local-count branches through derived_status_from_episode_progress(), which counts distinct completed episodes rather than the highest watched position.
2a244f6 to
6b56354
Compare
|
Both addressed: W292 fixed; ruff check src clean. Re-ran validation on the rebased branch: All checks passed. The distinct-count derivation and atomic get_or_create semantics from the earlier review notes are unchanged in this branch. |
2 participants
Summary
Complete a TV show without creating a second season identity when a compatible imported season already exists in another library bucket. Use local episode-count evidence when provider release events are unavailable.
This objective is valid. The current draft needs the corrections in the review section before it is safe to merge.
User-facing problem
TV._completed()creates or resolves seasonItemrows while it expands a completed show into seasons and episodes. Imported seasons can use the parent show'stvbucket, while this path historically looked in theseasonbucket. The mismatch can create a second season and episode set. Existing dates remain on the first set, while the newly generated set receives completion dates from the later action.The draft also uses the highest watched episode number as proof that all episodes were completed when release-event rows are absent. Sparse history makes that unsafe. Watching only episode 10 of a ten-episode season is one completed episode, not ten.
Reported evidence
The contributor observed imported Breaking Bad seasons in the
tvbucket with April 2025 dates. Completing the show created another set in theseasonbucket and duplicated 62 episodes with dates from the completion action.Proposed solution
get_or_create()for the intended target bucket.local_season_episode_countis authoritative, compare it with the count of distinct completed episodes, not the highest episode position.In progressstatus used for a rewatch.Review findings — changes required
1. Sparse history can be marked complete
The new
max_watched >= local_totalcondition is not completion evidence. Existing model behavior deliberately separates progress position from distinct completed-episode count. A season with only episode 10 watched has a progress position of 10 but a completed count of 1.Use
self.completed_episode_count >= local_totalfor this fallback and add tests for sparse history, duplicate plays, full distinct completion, and the manual rewatch override.2. The create path is no longer database-safe
The draft changes an atomic
get_or_create()path into a lookup followed byItem.objects.create(). Two completion requests can both see no row and then race to create the same target identity. One request can fail with a duplicate-key error.Keep the cross-bucket lookup, but use
get_or_create()when the target bucket is absent.3. The fallback lookup is too broad for TV/anime identity
find_item_across_buckets()returns the oldest candidate from any bucket when the preferred bucket is absent. In this model path, that can select an anime-bucket season for a normal TV parent, or the reverse. Existing identity work in #623 shows why those parent identities must stay separate.Restrict the candidate search to an ordered set of allowed buckets. For a normal TV parent, prefer the parent
tvbucket and thenseason. For an anime parent, use the anime identity only unless a separate, tested migration rule says otherwise.4. Regression tests are required
This PR changes persisted item selection, season relationships, episode history visibility, and status reconciliation. It currently adds no tests and records no command results.
Required cases are tracked in #812.
5. Update from current
latestThe branch is 46 commits behind and 1 commit ahead of current
latestas reviewed on 2026-08-16. Current model and importer changes include distinct-episode completion behavior that this PR must preserve. Update the branch before final validation.AI assistance
Provider: Anthropic. Model: Claude Opus 5 (API identifier claude-opus-5).
Validation
No executable validation result is recorded on the current branch. GitHub Actions reports
action_required; App Tests, Lint, Docker Image, and CodeQL have not executed.Run and record at minimum after updating from
latest:scripts/test.sh app.tests.models.test_tv_completed_on_create app.tests.models.test_seasonuv run --no-sync ruff check srcuv run --no-sync python src/manage.py makemigrations --check --dry-runscripts/test.shgit diff --check upstream/latest...HEADDo not mark a command complete unless its result is available from the updated branch.
Contract handoff
makemigrations --check --dry-runmust confirm this.Screenshots and interaction QA
Not applicable for the current scope. This is backend model behavior. It does not change templates, CSS, layout, keyboard behavior, screen-reader output, focus order, visual grouping, or cognitive load.
The user-facing result still needs a manual data check: existing dates and history must remain visible after completing the parent show.
Human review
/qaPost-mortem
Trigger: An imported season and the whole-show completion path used different library buckets for the same provider identity.
Failure: Completion did not find the compatible imported row and created a parallel season/episode set. A second fallback then treated the highest watched episode number as a completed-episode count.
Impact: Users can see duplicated seasons or episodes, split date history, and an incorrect completed status.
Why the defect escaped: Tests did not combine imported bucket identity, whole-show completion, existing watch dates, sparse episode history, and anime/TV identity separation.
Corrective action: Use an ordered compatible-bucket lookup, retain the atomic create path, derive completion from distinct completed episodes, and add focused model tests.
Prevention: Keep the #812 acceptance matrix as the regression contract for future import and completion changes.
Relationships