Repository navigation
feat(api): auto-approve requests per requester account (#2718) - #2761
Conversation
A requester can only ask, and every ask waits for an admin. With the lean
requester UI that turns into a stream of approvals, which is the opposite
of what the role is for.
Add a per-account setting, off by default so no existing install changes
behaviour. It lives on the user row (migration 090) rather than in the
settings table because it is a per-account permission like role, and it is
set from the Users page or PUT /auth/users/{id}/auto-approve.
When it is on, Create claims the new request and runs the same add an
admin's Approve runs, with no deciding user recorded. The
already-in-the-library check, the stored-payload revalidation and the
pending cap all still apply. A book request searches on add; an author
request runs the ordinary catalogue sync. If the add fails the request
stays pending, so nothing is lost. The switch only affects the next
request; anything already queued stays for a human.
Signed-off-by: Tung Lam <lamphamabtung96@gmail.com>
github-actions
Bot
added
the
bindery-notified
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
vavallee
left a comment
vavallee
left a comment
There was a problem hiding this comment.
Nicely built. Reusing runClaimedApproval rather than forking a second approval pipeline is the right instinct and it is what makes the feature safe to reason about. The feature belongs too: this is the natural completion of the requester role, and per account is the right granularity, since flaevers' own "some of my users are adults" argument is exactly the case a global toggle would get wrong.
Two things block, and one of them changes what the feature is.
1. Migration 090 collides three ways. Main tops out at 089, and across the open PRs: 090 is claimed by this PR, by #2750 and by #2713. assertUniqueMigrationVersions (internal/db/db.go:685) refuses to boot when two files share a prefix, so whichever lands second bricks startup for anyone who takes both. Separately 086 is claimed twice, by #2673 and #2633.
Full map right now: 086 → #2673, #2633. 090 → #2761, #2750, #2713. 091 → #2714. 092 → #2737. Next free is 093, so please rename to 093_requests_auto_approve.sql.
2. Auto-approve removes the pending cap, so there is no ceiling left at all. The cap is CountPendingByOwner >= limit (internal/api/requests.go:500-506, default 25). An auto-approved request never sits in pending, so that count is permanently zero. With the cap set to 3 and the flag on, 20 of 20 creates were accepted and the adder ran 20 times.
The only remaining brake is the requester provider limiter, 20 burst then one every 3s, which is roughly 1200 adds an hour, each one dispatching an indexer search and a grab, and each author request running a full catalogue sync.
The PR body notes "the pending cap is unchanged since an auto-approved request never sits in pending". That is accurate, and it is the problem: the requester role is defined by two limits, an admin decides and 25 pending, and this switch removes both at once.
I do not want to block the feature over this, so concretely: a per account "auto-approve up to N per day, then fall back to queuing" keeps the midnight texts problem solved, leaves the admin a floor, and costs one counting query. I would rather have that now than ship the unbounded version and make the quota a breaking change later.
Two more worth fixing while you are in here.
The webhook still fires. notifyCreated runs before writeCreated on both paths (requests.go:543 and :559), so the admin still gets a requestCreated notification per auto-approved item. The whole point of the issue is not getting pinged, so either skip EventRequestCreated when the request auto-approves or emit a distinct auto-approved event.
The admin gate on the new route is not pinned. Moving r.Put("/auth/users/{id}/auto-approve", ...) (cmd/bindery/main.go:939) out of the RequireAdmin group leaves the whole suite green, because the two new tests call h.SetAutoApprove directly and never see middleware. To be fair the same is true of /auth/users/{id}/role, so this is pre-existing, but this one is a privilege flag and deserves a route level test.
Things I specifically checked that are fine, so you know they were looked at rather than assumed:
A requester cannot grant themselves the flag. SetRequestsAutoApprove (internal/db/auth.go:575) is the only writer, neither INSERT INTO users path touches the column so OIDC and first run provisioning leave it 0, and its one caller sits behind RequireAdmin and off the requester allow list. Blocked twice over.
No new exposure in disabled or local-only mode. Create requires a non zero user id (requests.go:432-436), so the signed out as admin caller those modes admit cannot make a request at all.
The unmonitored author guard that landed in 78aadafa does not silently eat these. The search is tagged OriginAdd, which is classified non automatic, so it is correct by construction rather than by luck.
Migration applied cleanly against a populated database, both existing users defaulted to 0, single statement so no half applied state.
On the issue's actual ask: book requests do search immediately, author requests do not, because autoApprove sets search only for RequestKindBook (requests.go:611), so an author request's catalogue books wait for the sweep. That may well be what you want, but it is worth a line in the docs since flaevers quoted the immediate search behaviour specifically. Also, #2739 is not a dependency here; I checked, it fixes the list sync and monitor flip paths, not add.
Minor, take or leave: SetAutoApprove returns 200 for an unknown user id so an admin typo silently succeeds; users.autoApproveHint is in en.json and never rendered; flipping a requester to user and back silently restores the flag because the column keeps its value while the checkbox is hidden; and there is no way for an admin to list what was auto approved, which matters most when they want to turn the switch back off.
Rename to 093, add the quota, suppress the created webhook, add the route test, and I think this is good to go.
…avallee#2718) Renames 090_requests_auto_approve.sql to 093. Main tops out at 089 and both vavallee#2750 and vavallee#2713 claim 090, so whichever of the three landed second would trip assertUniqueMigrationVersions at boot. Auto-approval removed the pending cap, because that cap counts pending rows and an auto-approved request never sits in pending. The claim now also carries a per-account daily quota, drawn from the same requests.max_pending_per_user limit (25 by default). It is enforced inside the claim statement, the way pendingBelowCap guards Create, so a burst of creates cannot all pass a separate count. Once the account has had that many requests auto-approved since midnight UTC, the next one is left pending for a human, exactly as it is with the setting off. An auto-approved request no longer sends requestCreated, so an admin is not pinged for an item that was added without them. A request that stays pending, whether the add failed or the day's quota is spent, still notifies. The admin user routes move into registerUserAdminRoutes so the route level test drives the same registration the server uses. Moving /auth/users/{id}/auto-approve out of the RequireAdmin group failed nothing before this test; an unknown user id on that route is now a 404 rather than a silent success, and the Users page renders the autoApproveHint key that was already in en.json. Signed-off-by: Tung Lam <lamphamabtung96@gmail.com>
|
Thanks for the careful read. All four are in, pushed as 82ac5b5.
I reproduced all three before fixing: with the flag on a create captured Minors: took the 404 on an unknown user id and rendering No force-push: the branch is still the original head plus this commit, so your review anchor is intact. |
vavallee
left a comment
vavallee
left a comment
There was a problem hiding this comment.
All four done, and done well. Checked each one rather than taking the commit message for it:
- 093 is unique across main and every open PR right now.
- The quota sits inside the claim statement, so a burst can't all pass a separate count. I also checked the one way it could silently do nothing: `decided_at` is a DATETIME written from a raw `time.Time` in `Complete`, and `since` is bound the same way, so both sides are the same text shape in UTC and `>=` orders them correctly. Disabling the predicate fails `TestRequestRepo_AutoApproveQuota` and `TestRequestsCreate_AutoApproveDailyQuota`.
- The webhook. Moving `notifyCreated` back above the auto approve fails `TestRequestsCreate_AutoApproveSendsNoRequestCreated`.
- The route test. Dropping `RequireAdmin` from `registerUserAdminRoutes` fails `TestUserAdminRoutesRequireAdmin`, and moving the registration into one function the server and the test both call is the right way to make that test mean something. It covers `/role` too now, which fixes the pre-existing gap as a side effect.
Merged onto current main it builds, vets, and `./internal/api`, `./internal/db` and `./cmd/bindery` pass.
Summary
A requester can only ask, and every ask waits for an admin. On a lean requester UI that turns into a stream of approvals, which is exactly what the reporter wants to switch off for adults they trust. Closes #2718.
The switch is per account, on the user row (
requests_auto_approve, migration 093), and off by default. An admin sets it from a new Auto-approve requests checkbox on the Users page, orPUT /api/v1/auth/users/{id}/auto-approvewith{"enabled": true}. It sits onusersrather than in the settings table because it is a per-account permission like role, and the admin users API already owns that surface. No existing account changes behaviour.The existing flow, for reference:
RequestHandler.Createstores the row aspendingand that is where it waited.Approveclaims the row with a compare-and-swap on its status, runsrunApproval(which revalidates the stored payload, rechecks "already in the library" and calls the same add cores the Add dialog uses with the requester as owner), thenCompletemarks itapproved. Auto-approval hooks in at that first step:Createhands the stored row towriteCreated, which reads the owner's flag and, when it is on, claims the row and runs the same add. There is no second pipeline. I splitApproveso the claim, the renewer, the payload revalidation, the owner in the context and the release-on-failure are one function (runClaimedApproval) that both callers drive.decided_bystays NULL, so the queue can tell an automatic approval from a human one.The approval body is the approve form's own starting point, since there is no form to fill in: a book request searches on add, an author request runs the ordinary catalogue sync. The form defaults author search off too, and auto-approving a batch of author requests with search on add would fan out over every monitored book of each author, so I kept it that way.
Quota
An auto-approved request never sits in
pending, so the pending cap (CountPendingByOwner >= limit, default 25) stopped applying to it, and that was the missing ceiling. The claim now carries a daily quota, taken from the samerequests.max_pending_per_userlimit rather than a second number: once the account has had that many requests auto-approved since midnight UTC, the claim matches nothing and the request is left pending for a human, exactly as it is with the setting off. The count sits inside the claim UPDATE, the waypendingBelowCapguardsCreate, so a burst of creates cannot all pass a separate read. The quota is per account, resets the next day, and a human approval does not spend it.Webhook
notifyCreatedran beforewriteCreatedon both create paths, so an auto-approved request still sentrequestCreatedand the admin still got a ping per item. It now runs after the approval attempt and only when the request actually stayed pending. A request left pending by a failed add or a spent quota still notifies. You offered a distinct auto-approved event as the alternative; I went with suppressing the created ping, since the issue is about not getting pinged.Route gate
The admin user routes now sit in
registerUserAdminRoutes, with their ownRequireAdmingroup, socmd/bindery/user_admin_routes_test.godrives the same registrationmain()uses. Moving/auth/users/{id}/auto-approveout of the group now failsTestUserAdminRoutesRequireAdmin/set_auto_approvewithstatus = 204; want 403; on the reviewed head the equivalent move left./cmd/...and the auto-approve tests green. The test covers the other/auth/usersroutes too, since none of them had a route-level test either.Minors
I took two. An unknown user id on
PUT /auth/users/{id}/auto-approveis now a 404 instead of a silent 200:SetRequestsAutoApprovereturnsdb.ErrUserNotFoundwhen the update matches no row, and the handler maps it. Andusers.autoApproveHintis now rendered, as a line above the users table.I left two. The flag keeps its value when a requester is demoted and re-promoted. The column is a per-account property rather than a property of the role, and clearing it on a role change would be a destructive side effect of an unrelated action; the checkbox stays hidden while the role is
userand comes back with its value. And there is still no dedicated list of what was auto-approved. The queue already lists approvals anddecided_by IS NULLmarks the automatic ones, so the data is there; a filter or a badge is UI work I would rather do on its own than grow this diff.Boundary notes
I asked on the issue whether this should be blanket or limited to some other condition; no answer yet, so it is blanket for now, which is what the issue's Seerr comparison implies. I did not touch the wanted/sweep/scoring paths or the narrator token work.
Checklist
git commit -sdocs/DEPLOYMENT.mdupdated if env vars, config, or upgrade path changed (no env or config change; migration 093 only adds a column with a default)changelog.d/docs/multi-user.mdis the reference and is updated)Test plan
Fail-before as a mutation check. With the flag lookup forced to
falseon the branch, the auto-approval tests fail:TestRequestsCreate_WithoutAutoApproveStillWaitsand the failure-fallback test pass either way, which is the point of both.New in this revision:
TestRequestsCreate_AutoApproveDailyQuota: on a limit of 2, the third request stays pending, the adder is not run for it, a second account is unaffected, and a human can still approve the queued one.TestRequestsCreate_AutoApproveSendsNoRequestCreatedandTestRequestsCreate_AutoApproveFailureStillNotifies: no event when the request is approved, an event when the same setting leaves it pending.TestUserAdminRoutesRequireAdmin/TestUserAdminRoutesAllowAdmin: the route-level gate, verified failing with the route moved out of the group.TestRequestRepo_AutoApproveQuota: the tally ignores a human approval and rolls with the window.TestMigration093_AppliesToPopulatedUsers: drops the column and itsschema_migrationsrow on a populated database, re-runs the migrator, and checks the existing account defaults to 0 and the column still writes.TestUserRepo_SetRequestsAutoApprove_UnknownUser: the 404 path.Reproductions on the reviewed head, before the fix:
OL27482WcapturedrequestCreatedwith{kind: book, requestId: 1, title: The War of the Worlds, username: reader}.requests.max_pending_per_userset to 3 and the flag on, 5 creates all returnedapprovedand the add core ran 5 times.RequireAdmingroup leftgo test ./cmd/...and the auto-approve tests green.Validation on 82ac5b5:
gofmt -l .no output;go build ./...,go vet ./internal/api/... ./internal/db/... ./cmd/...exit 0go test ./cmd/... ./internal/...passes apart from the twelve hardlink/rename/filesystem tests that fail identically on a cleanmainworktree in this sandbox:TestHardlinkFile,TestHardlinkDir,TestDrop_EbookHardlink,TestImportMode_DefaultHardlinkSameDevice,TestImportMode_DefaultHardlinkSameDevice_DstNotExist,TestResolveImportMode,TestStagedImport_RollbackRestoresMoveSourceSameFS,TestFlattenAudiobookDir_Hardlink,TestScannerFlatten_HardlinkMode,TestDiagnose_QbittorrentAllPass,TestMigrateFlatCache_MovesFilesAndSidecars,TestCheckSQLiteFile_AwkwardPath. The failure sets are identical on both trees.npx tsc --noEmitexit 0;npm run lint0 errors (7 pre-existingexhaustive-deps/ unused-disable warnings across the app, one of themUsersPage.tsx);npx vitest run95 files / 1086 tests passed;npm run buildexit 0.I could not run
golangci-lint: the binary available here is v1.64.8 and the repo config is v2, somake lintandmake checkdo not run in this environment. The Go and web steps above are the ones I ran.