feat(indexer): normalise the audiobook size bonus by runtime and cap grabs (#2740) - #2750
Conversation
github-actions
Bot
added
the
bindery-notified
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks @tunglambk for taking this on. The opt-in profile settings, tolerance band, and adjustable grabs weight provide a useful foundation. I appreciate the focused tests and documentation, and keeping existing ranking unchanged when scoring is disabled. The new controls also saved and reopened correctly during testing and worked across desktop/mobile layouts and both themes. There are two behavior issues I noticed when reviewing this:
A few clarifications on the intended scope:
These two items are non-blocking and can be fixed here or explicitly deferred:
One merge-order note: migration |
|
Heads up on a migration number collision, not a review. This PR adds a migration numbered 090, and so do two others that are open right now: #2761 and #2713 and #2750 all claim 090. Main tops out at 089. Current claim map across the open PRs:
The usual gotcha here is that each of us computes "next free" against a main that does not yet contain anyone else's unpushed work, so this keeps happening. Worth renaming to something above 093 and picking by checking open PRs rather than main. No rush on my side, just flagging it before two of these land in the same window. |
…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>
|
Two things on this PR, one of them my own mistake. Migration number. I told four PRs that 093 was the first free number, and two took it, so I created a fresh collision. Assigning distinct numbers instead:
Please rename to The larger issue, and it is worth knowing before you spend more time here. This PR and #2737 both rewrite More specifically, #2737 replaces the base term this PR's arithmetic is derived from. Today format contributes Two substantive points while you are rebasing, both about the feature rather than the code.
The duration the size per minute term needs is usually zero. The version of this I would take: normalise the existing size term by duration when duration is known, and cap the grabs term, as weight changes inside |
* feat(api): auto-approve requests per requester account (#2718) 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> * fix(api): rename migration 090 to 093 and cap auto-approve per day (#2718) Renames 090_requests_auto_approve.sql to 093. Main tops out at 089 and both #2750 and #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> --------- Signed-off-by: Tung Lam <lamphamabtung96@gmail.com> Co-authored-by: Tung Lam <lamphamabtung96@gmail.com>
bc79a28 to
c3fc10c
Compare
…grabs (vavallee#2740) The size term rewarded total bytes, so of two releases of the same book the larger file always gained more, whatever its bitrate. When the book has a stored runtime an audio release is now scored by its density in MiB per minute instead, at the points-per-MiB the flat term used for a ten-hour book and capped where its 1024 MiB cap lands. A ten-hour book therefore keeps the numbers it had, and a shorter or longer one stops being rewarded or punished for its length. A book with no stored runtime keeps the flat, capped bonus exactly as it was. The popularity term is capped at 100 grabs, where log10(grabs+1)*10 reaches about 20 points, so a release with thousands of downloads can no longer outweigh the format and size terms together. Both are weight changes inside scoreResult. There is no profile block, no setting to turn on, no migration and no per-codec UI. Signed-off-by: Tung Lam <lamphamabtung96@gmail.com>
c3fc10c to
6bfc260
Compare
tunglambk
changed the title
4 participants
Summary
A book's runtime now weights two existing ranking terms, so a long audiobook stops being rewarded for being long and a very popular release can't dominate the format it carries. This is the smaller shape @vavallee described rather than the per-profile
audiobookScoringblock: no setting, no migration, no per-codec UI, and a book with no stored runtime scores exactly what it scored before.Part of #2740.
The size bonus used to be
min(size, 1024 MiB) / 100, which grows with the file. When the book has a storeddurationSeconds, an audio release is now scored by its density instead, in MiB per minute, at the same points-per-MiB the flat term used for a ten-hour book and capped where its 1024 MiB cap lands. The ten-hour reference is deliberate: it is the length that cap implies, so a ten-hour book keeps the numbers it had. Against an ordered audiobook profile (m4b second of five, so 400) the 1,200 / 600 / 300 MiB releases score 440.24 / 436.00 / 433.00, and with no profile, where the format term isQualityRank, 940.24 / 936.00 / 933.00. What changes is that length no longer skews the term: 300 MiB over 5 h, 600 MiB over 10 h and 1,200 MiB over 20 h are all 1.0 MiB/min and all score 436 against that profile.The popularity bonus is capped at 100 grabs, where
log10(grabs+1)*10reaches about 20 points. Popularity is still rewarded, but a release with thousands of downloads can no longer outweigh the format and size terms together. This one applies whether or not a runtime is known.Two limits are worth stating plainly. The runtime describes the book, not the release, so an abridged or differently narrated release is still scored against the full book's runtime; there is no per-candidate runtime to use yet. And
books.duration_secondsis only written from Hardcover, Audible, Audnexus gated on a non-empty ASIN, and the Audiobookshelf importer. Nothing derives it from a file, andmetadata_providerdefaults to OpenLibrary, which supplies neither runtime nor ASIN. On a default install the normalised path is rarely reached and the zero-runtime fallback is the one that runs, so a test pins that fallback bit for bit against the old formula on both ranking bases.The per-codec targets, the settings UI and the score breakdown from the review are what this shape drops. The pending-release ranking path is still untouched: it needs a decision on where the ranking belongs, so it is not in here.
Test plan
gofmt -l .— cleango build ./...— cleango vet ./internal/indexer/... ./internal/models/... ./internal/api/... ./internal/db/... ./internal/scheduler/...— cleango test -count=1 ./internal/indexer/... ./internal/models/... ./internal/scheduler/...— pass.internal/api,internal/dbandinternal/importerfail the same twelve hardlink/rename/filesystem tests on this branch and on unmodifiedmain, checked side by side in two worktrees.cd web && npm run typecheck && npm run lint && npm test && npm run build(Node 24) — pass, and the web side is back tomain's sources.golangci-lintcouldn't run here: the workspace v2.12.2 build panics under Go 1.27 before it reaches a file, and CI pins v2.11.4 on Go 1.26.6, which isn't installed. I'm not claiming it passed.