refactor: remove legacy Podcast.is_subscribed field#86
Merged
allenhutchison merged 2 commits intomainfrom Jan 16, 2026
Merged
Conversation
Replace system-level is_subscribed field with UserSubscription-based logic. The field was redundant now that the system has per-user subscriptions. Changes: - Add list_podcasts_with_subscribers() repository method - Update SyncWorker and FeedSyncService to use subscriber-based filtering - Update get_overall_stats() to count podcasts by UserSubscription - Update CLI to show subscriber counts instead of subscription status - Remove subscribed_only parameter from list_podcasts() - Add database migration to drop the column
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughRemoves the Podcast.model boolean Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_repository.py`:
- Around line 98-118: In test_list_podcasts_with_subscribers, remove the unused
local variable by changing the second podcast creation from an assigned variable
to a plain call; update the line that currently assigns podcast2 =
repository.create_podcast(...) to just
repository.create_podcast(feed_url="https://example.com/feed2.xml",
title="Podcast 2") so the second podcast still exists for list_podcasts() but
the unused symbol podcast2 is not defined (target function:
test_list_podcasts_with_subscribers, symbol: podcast2).
🧹 Nitpick comments (2)
src/workflow/workers/sync.py (1)
62-74: Consider suppressing the unusedlimitargument lint warning.The
limitparameter is intentionally ignored (as documented in the docstring), likely to comply with theWorkerInterface.process_batchsignature. Consider using an underscore prefix or adding a# noqa: ARG002comment to suppress the lint warning explicitly.♻️ Suggested fix
- def process_batch(self, limit: int = 0) -> WorkerResult: + def process_batch(self, limit: int = 0) -> WorkerResult: # noqa: ARG002Or alternatively, prefix with underscore if the interface allows:
- def process_batch(self, limit: int = 0) -> WorkerResult: + def process_batch(self, _limit: int = 0) -> WorkerResult:src/cli/podcast_commands.py (1)
249-265: Consider batching episode counts to avoid per-podcast queries.
You already havepodcast_ids; using the bulk API keeps list output fast for large datasets.♻️ Proposed refactor
- # Get subscriber counts for all podcasts in one query + # Get subscriber/episode counts for all podcasts in one query each podcast_ids = [p.id for p in podcasts] subscriber_counts = repository.get_podcast_subscriber_counts(podcast_ids) + episode_counts = repository.get_podcast_episode_counts(podcast_ids) @@ - # Get episode count - episodes = repository.list_episodes(podcast_id=podcast.id) - sub_count = subscriber_counts.get(podcast.id, 0) + episode_count = episode_counts.get(podcast.id, 0) + sub_count = subscriber_counts.get(podcast.id, 0) print( f"{podcast.id:<36} " f"{podcast.title[:40]:<40} " - f"{len(episodes):<10} " + f"{episode_count:<10} " f"{sub_count}" )
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
is_subscribedfield from thePodcastmodelUserSubscription-based logic for determining which podcasts to syncChanges
Repository (
src/db/repository.py)list_podcasts_with_subscribers()method - returns podcasts with at least oneUserSubscriptionsubscribed_onlyparameter fromlist_podcasts()get_overall_stats()to count "subscribed_podcasts" based onUserSubscriptiontableModel (
src/db/models.py)is_subscribedfield fromPodcastmodelSyncWorker (
src/workflow/workers/sync.py)list_podcasts_with_subscribers()for determining which podcasts to syncFeedSyncService (
src/podcast/feed_sync.py)sync_podcasts_with_subscribers()method for pipeline usesubscribed_onlyparameter fromsync_all_podcasts()CLI (
src/cli/podcast_commands.py)listcommand to show subscriber counts instead of "Subscribed/Unsubscribed" statusMigration
is_subscribedcolumn frompodcaststableTest plan
alembic upgrade headto apply migrationpodcast listshows subscriber countsSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.