Skip to content

Fix: Include tvdb in .ids (omitted from the ids accessor list)#127

Merged
glensc merged 2 commits into
glensc:mainfrom
yitzhaq:fix-ids-include-tvdb
Jul 16, 2026
Merged

Fix: Include tvdb in .ids (omitted from the ids accessor list)#127
glensc merged 2 commits into
glensc:mainfrom
yitzhaq:fix-ids-include-tvdb

Conversation

@yitzhaq

@yitzhaq yitzhaq commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Problem

IdsMixin.ids builds {"ids": {...}} from a curated key list that omits tvdb, even though the mixin already exposes a tvdb property and Trakt returns tvdb for shows. So obj.ids["ids"] never includes the tvdb id (while obj.tvdb is populated).

This breaks tvdb-based id matching in downstream consumers. Concretely, PlexTraktSync's TraktApi.search_by_id validates the lookup result via m.ids["ids"].get(id_type); for id_type="tvdb" that is always None, so every show in a Plex library using the legacy TheTVDB agent is rejected as "not found on Trakt" — even though the show exists on Trakt and m.tvdb is correct. Refs Taxel/PlexTraktSync#2394, Taxel/PlexTraktSync#2360, Taxel/PlexTraktSync#2312.

Change

  • Add tvdb to the ids accessor list.
  • Filter None values from the dict, so media types without a given id (e.g. movies have no tvdb) don't gain a null key. Existing output stays stable — all prior tests pass unchanged — while shows now expose their tvdb id.
  • Add a regression test; update the docstring.

Testing

pytest green — 166 passed. The new test asserts a show's .ids["ids"]["tvdb"] is populated; the existing movie .ids equality tests still pass, confirming no null tvdb leaks into media that lack it.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 976a226b-dec5-4cb1-a2a3-bd4d7c66fee2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@glensc glensc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yitzhaq why did your agent ignore commit guidelines?

at minimum tests must be a separate commit.

also, v := requires py 3.8 syntax, but this project supports 3.6

and why this change is neccessary? "Only ids present on the object are included", could be a lot easier code without this restriction.

@glensc glensc changed the title Include tvdb in .ids (omitted from the ids accessor list) Fix: Include tvdb in .ids (omitted from the ids accessor list) Jul 14, 2026
@yitzhaq

yitzhaq commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Good question. My guess would be because it wasn't in the project root; it started out with the PTS repo before drilling down to where the fix actually should live, and something else even before that. I'll point it to it explicitly, and instruct it to take another pass. Sorry about that, and thanks for the heads up!

yitzhaq and others added 2 commits July 16, 2026 01:42
IdsMixin.ids built its dict from a curated key list that omitted tvdb,
even though a tvdb property already exists on the mixin and Trakt returns
tvdb for shows. So obj.ids["ids"] never contained tvdb, breaking
tvdb-based id matching downstream: PlexTraktSync's search_by_id checks
m.ids["ids"].get("tvdb"), which was always None, so every show in a
library using the legacy TheTVDB Plex agent is rejected as "not found on
Trakt" even though m.tvdb is populated.

Add tvdb to the accessor list. tvdb is a TV-only id, so filter out None
values: shows/episodes gain tvdb while movies, people and other media
that lack it keep their existing .ids output unchanged (no null tvdb key).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Regression for tvdb being omitted from the ids accessor list: assert a
show exposes its tvdb both via .tvdb and in .ids["ids"].

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yitzhaq
yitzhaq force-pushed the fix-ids-include-tvdb branch from d91181a to 49d93db Compare July 15, 2026 23:48
@yitzhaq

yitzhaq commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — addressed all three:

  1. Commits: rebuilt atomically — the behavior change (trakt/mixins.py) and the test (tests/test_shows.py) are now separate commits, imperative subjects, Co-authored-by trailer.
  2. Python 3.6: dropped the walrus; the None-filter is a plain two-step dict comprehension now.
  3. Why the None-filter: it's what scopes this to TV. tvdb is a TV-only id, but IdsMixin is shared by all media types — filtering absent ids keeps tvdb on shows/episodes while leaving movies, people, etc. with unchanged .ids output (no tvdb: null). Without it, every non-TV object gains a spurious tvdb: null key and the existing exact-equality tests (test_get_movie, and the people[0].ids assertion in test_shows) break. Happy to drop it if you'd rather have the simpler shape and accept null keys on non-TV media (updating those tests) — your call.

pytest: 166 passed; isort clean.

@glensc

glensc commented Jul 16, 2026

Copy link
Copy Markdown
Owner

I'd accept null keys, because the other id's were not filtered for null, and now doing so, could be a breaking change. but i don't think anybody filters object like this. so, the current changes are LGTM as well.

@glensc
glensc merged commit 4d63543 into glensc:main Jul 16, 2026
8 checks passed
@yitzhaq
yitzhaq deleted the fix-ids-include-tvdb branch July 17, 2026 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants