diff --git a/app/main.py b/app/main.py index d995feb..12e473b 100644 --- a/app/main.py +++ b/app/main.py @@ -917,9 +917,6 @@ async def subscribe(request): raise web.HTTPBadRequest(reason='check_interval_minutes must be an integer') from exc if cic < 1: raise web.HTTPBadRequest(reason='check_interval_minutes must be at least 1') - if o.get('clip_start') is not None or o.get('clip_end') is not None: - raise web.HTTPBadRequest(reason='clip options are not supported for subscriptions') - try: skip_subscriber_only = coerce_optional_bool( post.get('skip_subscriber_only'), @@ -929,6 +926,19 @@ async def subscribe(request): except ValueError as exc: raise web.HTTPBadRequest(reason=str(exc)) from exc + # A t= timestamp in the URL means "start playing here" and parse_download_options + # turns it into a clip start, which is right for a one-off download of that video. + # A subscription URL is a channel or playlist, so a timestamp left on it says + # nothing about the videos it will yield — honour clip fields only when the + # caller supplied them explicitly, rather than silently clipping every future + # download. The t= param is still stripped from the stored URL. + clip_given = ( + _clip_field_provided_in_post(post.get('clip_start')) + or _clip_field_provided_in_post(post.get('clip_end')) + ) + sub_clip_start = o['clip_start'] if clip_given else None + sub_clip_end = o['clip_end'] if clip_given else None + result = await submgr.add_subscription( o['url'], check_interval_minutes=cic, @@ -948,6 +958,8 @@ async def subscribe(request): ytdl_options_overrides=o['ytdl_options_overrides'], title_regex=post.get('title_regex'), skip_subscriber_only=skip_subscriber_only, + clip_start=sub_clip_start, + clip_end=sub_clip_end, ) return web.Response(text=serializer.encode(result)) diff --git a/app/subscriptions.py b/app/subscriptions.py index 45cc293..1cf7764 100644 --- a/app/subscriptions.py +++ b/app/subscriptions.py @@ -189,6 +189,13 @@ class SubscriptionInfo: ytdl_options_overrides: dict[str, Any] = field(default_factory=dict) title_regex: str = "" skip_subscriber_only: bool = False + # A fixed range applied to every video the subscription downloads. Only + # sensible for channels with a consistent format (a standing intro, a fixed + # sponsor read); left unset, videos download whole. Old stored records + # predate these fields and take the defaults — _from_stored filters by + # field name, so nothing needs migrating. + clip_start: Optional[float] = None + clip_end: Optional[float] = None last_checked: Optional[float] = None seen_ids: list[str] = field(default_factory=list) error: Optional[str] = None @@ -211,6 +218,8 @@ class SubscriptionInfo: "folder": self.folder, "title_regex": self.title_regex, "skip_subscriber_only": self.skip_subscriber_only, + "clip_start": self.clip_start, + "clip_end": self.clip_end, "last_checked": self.last_checked, "seen_count": len(self.seen_ids), "error": self.error, @@ -240,6 +249,8 @@ def _subscription_to_record(sub: SubscriptionInfo) -> dict[str, Any]: "ytdl_options_overrides": sub.ytdl_options_overrides, "title_regex": sub.title_regex, "skip_subscriber_only": sub.skip_subscriber_only, + "clip_start": sub.clip_start, + "clip_end": sub.clip_end, "last_checked": sub.last_checked, "seen_ids": list(sub.seen_ids), "error": sub.error, @@ -474,6 +485,8 @@ class SubscriptionManager: subtitle_mode: str, ytdl_options_presets: Optional[list[str]] = None, ytdl_options_overrides: Optional[dict[str, Any]] = None, + clip_start: Optional[float] = None, + clip_end: Optional[float] = None, ) -> tuple[list[str], list[str]]: queued_ids: list[str] = [] queue_errors: list[str] = [] @@ -504,6 +517,8 @@ class SubscriptionManager: subtitle_mode, presets, ytdl_options_overrides, + clip_start, + clip_end, ) if isinstance(result, dict) and result.get("status") == "error": msg = str(result.get("msg") or f"Queueing failed for {vurl}") @@ -593,6 +608,8 @@ class SubscriptionManager: ytdl_options_overrides: Optional[dict[str, Any]] = None, title_regex: Any = None, skip_subscriber_only: Any = None, + clip_start: Optional[float] = None, + clip_end: Optional[float] = None, ) -> dict: url = self._normalize_url(url) if not url: @@ -679,6 +696,8 @@ class SubscriptionManager: ytdl_options_overrides=dict(ytdl_options_overrides or {}), title_regex=title_regex_stored, skip_subscriber_only=skip_so, + clip_start=clip_start, + clip_end=clip_end, last_checked=time.time(), seen_ids=list(dict.fromkeys(all_ids)), error=None, @@ -930,6 +949,8 @@ class SubscriptionManager: dl_ytdl_overrides = dict(cur.ytdl_options_overrides) dl_title_regex = cur.title_regex or "" dl_skip_subscriber_only = bool(cur.skip_subscriber_only) + dl_clip_start = cur.clip_start + dl_clip_end = cur.clip_end new_entries: list[dict] = [] for ent in entries: @@ -994,6 +1015,8 @@ class SubscriptionManager: subtitle_mode=dl_submode, ytdl_options_presets=dl_ytdl_presets, ytdl_options_overrides=dl_ytdl_overrides, + clip_start=dl_clip_start, + clip_end=dl_clip_end, ) log.info( "Subscription check finished for %s: %d new, %d filtered, %d subscriber_skipped, %d queued, %d failed", diff --git a/app/tests/test_api.py b/app/tests/test_api.py index 9779769..ceb4124 100644 --- a/app/tests/test_api.py +++ b/app/tests/test_api.py @@ -349,17 +349,70 @@ async def test_add_passes_clip_bounds_to_queue(mock_dqueue): @pytest.mark.asyncio -async def test_subscribe_rejects_clip_options(mock_dqueue, monkeypatch): - monkeypatch.setattr(main.submgr, "add_subscription", AsyncMock()) +async def test_subscribe_passes_clip_bounds(mock_dqueue, monkeypatch): + """Issue #1049: a subscription's options apply to every future download, and + clip bounds were the one option carved out of that.""" + monkeypatch.setattr(main.submgr, "add_subscription", AsyncMock(return_value={"status": "ok"})) req = _json_request( { - **_valid_video_add_body(clip_start="10"), + **_valid_video_add_body(clip_start="2:26", clip_end="3:24"), "check_interval_minutes": 60, } ) + resp = await main.subscribe(req) + assert resp.status == 200 + kwargs = main.submgr.add_subscription.await_args.kwargs + assert kwargs["clip_start"] == pytest.approx(146.0) + assert kwargs["clip_end"] == pytest.approx(204.0) + + +@pytest.mark.asyncio +async def test_subscribe_without_clip_fields_stores_none(mock_dqueue, monkeypatch): + monkeypatch.setattr(main.submgr, "add_subscription", AsyncMock(return_value={"status": "ok"})) + req = _json_request({**_valid_video_add_body(), "check_interval_minutes": 60}) + await main.subscribe(req) + kwargs = main.submgr.add_subscription.await_args.kwargs + assert kwargs["clip_start"] is None + assert kwargs["clip_end"] is None + + +@pytest.mark.asyncio +async def test_subscribe_ignores_t_param_in_url(mock_dqueue, monkeypatch): + """A t= timestamp means "start here" for a one-off download of that video. + On a channel or playlist URL it says nothing about the videos it yields, so + it must not silently clip every future download.""" + monkeypatch.setattr(main.submgr, "add_subscription", AsyncMock(return_value={"status": "ok"})) + body = _valid_video_add_body() + # t= is only honoured on YouTube hosts, so this must be one to exercise it. + body["url"] = "https://www.youtube.com/@somechannel?t=90" + req = _json_request({**body, "check_interval_minutes": 60}) + await main.subscribe(req) + kwargs = main.submgr.add_subscription.await_args.kwargs + assert kwargs["clip_start"] is None + assert kwargs["clip_end"] is None + # The timestamp is still stripped from the URL that gets stored. + assert "t=90" not in main.submgr.add_subscription.await_args.args[0] + + +@pytest.mark.asyncio +async def test_subscribe_explicit_clip_wins_over_t_param(mock_dqueue, monkeypatch): + monkeypatch.setattr(main.submgr, "add_subscription", AsyncMock(return_value={"status": "ok"})) + body = _valid_video_add_body(clip_start="30") + body["url"] = "https://www.youtube.com/@somechannel?t=90" + req = _json_request({**body, "check_interval_minutes": 60}) + await main.subscribe(req) + kwargs = main.submgr.add_subscription.await_args.kwargs + assert kwargs["clip_start"] == pytest.approx(30.0) + + +@pytest.mark.asyncio +async def test_subscribe_still_rejects_clips_for_non_media(mock_dqueue, monkeypatch): + monkeypatch.setattr(main.submgr, "add_subscription", AsyncMock(return_value={"status": "ok"})) + body = _valid_video_add_body(clip_start="10") + body["download_type"] = "thumbnail" + req = _json_request({**body, "check_interval_minutes": 60}) with pytest.raises(web.HTTPBadRequest): await main.subscribe(req) - main.submgr.add_subscription.assert_not_awaited() @pytest.mark.asyncio diff --git a/app/tests/test_subscriptions.py b/app/tests/test_subscriptions.py index d5d5ac5..1934d36 100644 --- a/app/tests/test_subscriptions.py +++ b/app/tests/test_subscriptions.py @@ -409,6 +409,76 @@ class SubscriptionPersistenceTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(sub.seen_ids[:2], ["v2", "v1"]) self.assertEqual([entry["webpage_url"] for entry, _, _ in queue.entries], ["https://example.com/v2"]) + async def test_check_now_applies_subscription_clip_bounds(self): + """Issue #1049: clip bounds were the one download option a subscription + could not carry, so they must reach every entry it queues.""" + with tempfile.TemporaryDirectory() as tmp: + queue = _Queue() + mgr = SubscriptionManager(_Config(tmp), queue, _Notifier()) + + with patch( + "subscriptions.extract_flat_playlist", + side_effect=[ + ( + {"_type": "channel", "title": "Channel"}, + [{"id": "v1", "title": "One", "webpage_url": "https://example.com/v1"}], + ), + ( + {"_type": "channel", "title": "Channel"}, + [ + {"id": "v2", "title": "Two", "webpage_url": "https://example.com/v2"}, + {"id": "v1", "title": "One", "webpage_url": "https://example.com/v1"}, + ], + ), + ], + ): + result = await mgr.add_subscription( + "https://example.com/channel", + check_interval_minutes=60, + download_type="video", + codec="auto", + format="any", + quality="best", + folder="", + custom_name_prefix="", + auto_start=True, + playlist_item_limit=0, + split_by_chapters=False, + chapter_template="", + subtitle_language="en", + subtitle_mode="prefer_manual", + clip_start=30.0, + clip_end=90.0, + ) + sub_id = result["subscription"]["id"] + self.assertEqual(mgr.get(sub_id).clip_start, 30.0) + self.assertEqual(mgr.get(sub_id).clip_end, 90.0) + await mgr.check_now([sub_id]) + + self.assertEqual(len(queue.entries), 1) + _entry, args, _kwargs = queue.entries[0] + # add_entry(entry, download_type, ..., ytdl_options_overrides, clip_start, clip_end) + self.assertEqual(args[-2], 30.0) + self.assertEqual(args[-1], 90.0) + + async def test_clip_bounds_survive_reload_and_default_to_none(self): + with tempfile.TemporaryDirectory() as tmp: + cfg = _Config(tmp) + queue = _Queue() + mgr = SubscriptionManager(cfg, queue, _Notifier()) + sub_id = await self._add_one_subscription(mgr) + # Records written before these fields existed simply take the defaults. + self.assertIsNone(mgr.get(sub_id).clip_start) + self.assertIsNone(mgr.get(sub_id).clip_end) + + mgr.get(sub_id).clip_start = 12.5 + async with mgr._lock: + mgr._save_locked() + + reloaded = SubscriptionManager(cfg, _Queue(), _Notifier()) + self.assertEqual(reloaded.get(sub_id).clip_start, 12.5) + self.assertIsNone(reloaded.get(sub_id).clip_end) + async def test_check_now_queues_subscriber_only_when_skip_disabled(self): with tempfile.TemporaryDirectory() as tmp: queue = _Queue() diff --git a/ui/src/app/app.spec.ts b/ui/src/app/app.spec.ts index 6bd6ed7..fb529e8 100644 --- a/ui/src/app/app.spec.ts +++ b/ui/src/app/app.spec.ts @@ -254,7 +254,9 @@ describe('App', () => { expect(payload.skipSubscriberOnly).toBe(true); }); - it('omits clip fields from subscribe payload', () => { + it('passes clip fields through to the subscribe payload', () => { + // #1049: a subscription's options apply to all its future downloads, and + // clip bounds used to be stripped out on the way. const fixture = TestBed.createComponent(App); const app = fixture.componentInstance; const subs = TestBed.inject(SubscriptionsService) as unknown as SubscriptionsServiceStub; @@ -264,8 +266,8 @@ describe('App', () => { app.addSubscription(); expect(subs.subscribeCalls.length).toBe(1); const payload = subs.subscribeCalls[0] as Record; - expect('clipStart' in payload).toBe(false); - expect('clipEnd' in payload).toBe(false); + expect(payload['clipStart']).toBe('1:00'); + expect(payload['clipEnd']).toBe('2:00'); }); it('buildAddPayload includes clip times', () => { diff --git a/ui/src/app/app.ts b/ui/src/app/app.ts index 99b112b..d88cc9d 100644 --- a/ui/src/app/app.ts +++ b/ui/src/app/app.ts @@ -601,14 +601,10 @@ export class App implements AfterViewInit, OnInit, OnDestroy { if (!this.validateYtdlOptionsOverrides(payload.ytdlOptionsOverrides)) { return; } - // Subscriptions do not support clip ranges (backend rejects clip fields). - const { clipStart: _clipStart, clipEnd: _clipEnd, ...subscribeBase } = payload; - void _clipStart; - void _clipEnd; this.subscribeInProgress = true; this.subscriptionsSvc .subscribe({ - ...subscribeBase, + ...payload, checkIntervalMinutes: this.checkIntervalMinutes, titleRegex: tr, skipSubscriberOnly: this.skipSubscriberOnly, diff --git a/ui/src/app/interfaces/subscription.ts b/ui/src/app/interfaces/subscription.ts index 99b012e..007ea78 100644 --- a/ui/src/app/interfaces/subscription.ts +++ b/ui/src/app/interfaces/subscription.ts @@ -11,6 +11,8 @@ export interface SubscriptionRow { folder: string; title_regex?: string; skip_subscriber_only?: boolean; + clip_start?: number | null; + clip_end?: number | null; last_checked: number | null; seen_count: number; error: string | null; diff --git a/ui/src/app/services/subscriptions.service.ts b/ui/src/app/services/subscriptions.service.ts index a50be8e..3199db8 100644 --- a/ui/src/app/services/subscriptions.service.ts +++ b/ui/src/app/services/subscriptions.service.ts @@ -81,28 +81,34 @@ export class SubscriptionsService { } subscribe(payload: SubscribePayload) { - return this.http - .post('subscribe', { - url: payload.url, - download_type: payload.downloadType, - codec: payload.codec, - quality: payload.quality, - format: payload.format, - folder: payload.folder, - custom_name_prefix: payload.customNamePrefix, - playlist_item_limit: payload.playlistItemLimit, - auto_start: payload.autoStart, - split_by_chapters: payload.splitByChapters, - chapter_template: payload.chapterTemplate, - subtitle_language: payload.subtitleLanguage, - subtitle_mode: payload.subtitleMode, - ytdl_options_presets: payload.ytdlOptionsPresets, - ytdl_options_overrides: payload.ytdlOptionsOverrides, - check_interval_minutes: payload.checkIntervalMinutes, - title_regex: payload.titleRegex, - skip_subscriber_only: payload.skipSubscriberOnly, - }) - .pipe(catchError((err) => this.handleHTTPError(err))); + const body: Record = { + url: payload.url, + download_type: payload.downloadType, + codec: payload.codec, + quality: payload.quality, + format: payload.format, + folder: payload.folder, + custom_name_prefix: payload.customNamePrefix, + playlist_item_limit: payload.playlistItemLimit, + auto_start: payload.autoStart, + split_by_chapters: payload.splitByChapters, + chapter_template: payload.chapterTemplate, + subtitle_language: payload.subtitleLanguage, + subtitle_mode: payload.subtitleMode, + ytdl_options_presets: payload.ytdlOptionsPresets, + ytdl_options_overrides: payload.ytdlOptionsOverrides, + check_interval_minutes: payload.checkIntervalMinutes, + title_regex: payload.titleRegex, + skip_subscriber_only: payload.skipSubscriberOnly, + }; + // Send the clip fields only when actually filled in. The backend treats an + // absent field as "not requested", which is what stops a t= timestamp on the + // subscribed URL from clipping every future download. + const cs = payload.clipStart?.trim(); + const ce = payload.clipEnd?.trim(); + if (cs) body['clip_start'] = cs; + if (ce) body['clip_end'] = ce; + return this.http.post('subscribe', body).pipe(catchError((err) => this.handleHTTPError(err))); } delete(ids: string[]) {