fix(playlists): a refused push spends none of the push allowance
The cooldown was checked before the list was found pushable and before its sync claim, so Watch later or a list another sync held used up the push allowance. It now counts only a push that runs. The docstring that called a YouTube mirror unpushable is corrected: its local edits sync back.
This commit is contained in:
@@ -261,8 +261,9 @@ def _check_push_cooldown(user: User) -> None:
|
|||||||
|
|
||||||
|
|
||||||
def _pushable(db: Session, user: User, playlist_id: int) -> "Playlist":
|
def _pushable(db: Session, user: User, playlist_id: int) -> "Playlist":
|
||||||
"""A local playlist the user may push to YouTube (create or update). The built-in
|
"""A playlist the user may push to YouTube: a local one (created there on its first push) or a
|
||||||
Watch later and read-only YouTube mirrors are never pushable."""
|
mirror of their own YouTube list (its local edits merged back). Only the built-in Watch later is
|
||||||
|
never pushable."""
|
||||||
pl = _own_playlist(db, user, playlist_id)
|
pl = _own_playlist(db, user, playlist_id)
|
||||||
if pl.kind == "watch_later":
|
if pl.kind == "watch_later":
|
||||||
raise _refused(400, "watch_later_not_pushable")
|
raise _refused(400, "watch_later_not_pushable")
|
||||||
@@ -278,8 +279,8 @@ def push_plan(
|
|||||||
"""Dry run: what a two-way 'Sync to YouTube' merge would do (what each side gains/loses) +
|
"""Dry run: what a two-way 'Sync to YouTube' merge would do (what each side gains/loses) +
|
||||||
the quota estimate, so the UI can show a confirm. For a linked playlist this reads the live
|
the quota estimate, so the UI can show a confirm. For a linked playlist this reads the live
|
||||||
YouTube state (cheap, 1 unit/page). Write scope is required — syncing is a YouTube write."""
|
YouTube state (cheap, 1 unit/page). Write scope is required — syncing is a YouTube write."""
|
||||||
_check_push_cooldown(user)
|
|
||||||
pl = _pushable(db, user, playlist_id)
|
pl = _pushable(db, user, playlist_id)
|
||||||
|
_check_push_cooldown(user) # after the refusal: a list that cannot be pushed spends nothing
|
||||||
try:
|
try:
|
||||||
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
|
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
|
||||||
plan = plan_merge(db, user, pl)
|
plan = plan_merge(db, user, pl)
|
||||||
@@ -299,13 +300,14 @@ def push(
|
|||||||
"""Two-way sync a local playlist with YouTube (create it on first push, else 3-way merge so
|
"""Two-way sync a local playlist with YouTube (create it on first push, else 3-way merge so
|
||||||
neither side loses an item). Refuses if the estimated quota exceeds what's left today, so a
|
neither side loses an item). Refuses if the estimated quota exceeds what's left today, so a
|
||||||
big sync can't strand half-applied. Write scope required (require_write_scope)."""
|
big sync can't strand half-applied. Write scope required (require_write_scope)."""
|
||||||
_check_push_cooldown(user)
|
|
||||||
pl = _pushable(db, user, playlist_id)
|
pl = _pushable(db, user, playlist_id)
|
||||||
# One sync of a list at a time, across tabs, devices and the scheduler (`sync_claim`): the
|
# One sync of a list at a time, across tabs, devices and the scheduler (`sync_claim`): the
|
||||||
# client's own in-flight flag only covers the menus of one page.
|
# client's own in-flight flag only covers the menus of one page.
|
||||||
with sync_claim(db, pl.id) as free:
|
with sync_claim(db, pl.id) as free:
|
||||||
if not free:
|
if not free:
|
||||||
raise _refused(409, "sync_busy")
|
raise _refused(409, "sync_busy")
|
||||||
|
# Spent only by a push that runs, as Revert's pull: a busy list or Watch later costs nothing.
|
||||||
|
_check_push_cooldown(user)
|
||||||
try:
|
try:
|
||||||
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
|
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
|
||||||
# Read the live playlist ONCE (linked playlists only) and share it with both the
|
# Read the live playlist ONCE (linked playlists only) and share it with both the
|
||||||
|
|||||||
@@ -177,6 +177,48 @@ def test_plan_and_push_draw_on_one_allowance(db, as_user, make_user, monkeypatch
|
|||||||
assert r.json()["detail"] == {"code": "playlist", "reason": "push_cooldown"}
|
assert r.json()["detail"] == {"code": "playlist", "reason": "push_cooldown"}
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_refused_push_spends_no_allowance(db, as_user, make_user, monkeypatch):
|
||||||
|
# The allowance counts pushes that run: Watch later (never pushable) and a list another sync
|
||||||
|
# holds are refused before any YouTube call, so they must not leave the next real press at 429.
|
||||||
|
from contextlib import contextmanager
|
||||||
|
|
||||||
|
from app.reader.models import Playlist
|
||||||
|
from app.routes import playlists as plroutes
|
||||||
|
|
||||||
|
user, pl = _pushable_list(db, make_user, monkeypatch, plroutes)
|
||||||
|
wl = Playlist(user_id=user.id, name="Watch later", kind="watch_later")
|
||||||
|
db.add(wl)
|
||||||
|
db.commit()
|
||||||
|
c = as_user(user)
|
||||||
|
over = plroutes.PUSH_ALLOWANCE + 1
|
||||||
|
assert {c.get(f"/api/playlists/{wl.id}/push-plan").status_code for _ in range(over)} == {400}
|
||||||
|
assert {c.post(f"/api/playlists/{wl.id}/push").status_code for _ in range(over)} == {400}
|
||||||
|
|
||||||
|
@contextmanager
|
||||||
|
def busy(_db, _id):
|
||||||
|
yield False
|
||||||
|
|
||||||
|
with monkeypatch.context() as m:
|
||||||
|
m.setattr(plroutes, "sync_claim", busy)
|
||||||
|
assert {c.post(f"/api/playlists/{pl.id}/push").status_code for _ in range(over)} == {409}
|
||||||
|
assert c.post(f"/api/playlists/{pl.id}/push").status_code == 200
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_mirrored_youtube_list_is_pushable(db, as_user, make_user, monkeypatch):
|
||||||
|
# A mirror of the user's own YouTube list takes local edits (`dirty`) and syncs them back; only
|
||||||
|
# Watch later is refused.
|
||||||
|
from app.reader.models import Playlist
|
||||||
|
from app.routes import playlists as plroutes
|
||||||
|
|
||||||
|
user, _ = _pushable_list(db, make_user, monkeypatch, plroutes)
|
||||||
|
mirror = Playlist(
|
||||||
|
user_id=user.id, name="Mine", source="youtube", yt_playlist_id="PLx", dirty=True
|
||||||
|
)
|
||||||
|
db.add(mirror)
|
||||||
|
db.commit()
|
||||||
|
assert as_user(user).get(f"/api/playlists/{mirror.id}/push-plan").status_code == 200
|
||||||
|
|
||||||
|
|
||||||
def test_the_push_cooldown_is_per_user(db, as_user, make_user, monkeypatch):
|
def test_the_push_cooldown_is_per_user(db, as_user, make_user, monkeypatch):
|
||||||
from app.routes import playlists as plroutes
|
from app.routes import playlists as plroutes
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user