Add acquisitions table and interface for Trusted Book Providers#12851
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new “acquisitions” persistence layer to store trusted book provider metadata per (edition, provider), along with schema and tests.
Changes:
- Introduces
openlibrary.core.acquisitions.Acquisitionwith query/upsert and edition-merge remapping logic. - Adds
acquisitionstable + indexes to the core Postgres schema. - Adds sqlite-backed unit tests for acquisitions CRUD/merge behaviors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| openlibrary/core/acquisitions.py | New model helpers for reading/upserting acquisitions and handling edition merges. |
| openlibrary/core/schema.sql | Adds acquisitions table and supporting indexes in the main schema. |
| openlibrary/tests/core/test_acquisitions.py | Adds sqlite-based tests validating acquisitions behavior. |
|
Thanks for the contribution, @ronibhakta1. Copilot has been assigned for an initial review. The linked issue (#12844) hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 73 open non-draft PRs ahead of yours. Possible improvements for this PR
PR triage checklist (maintainers / Pam)
Note This comment was automatically generated by Pam, Open Library's Project AI Manager, on behalf of @mekarpeles. Pam is designed to provide status visibility, perform basic project management functions and relevant codebase research, and provide actionable feedback so contributors aren't left waiting. |
cdrini
left a comment
There was a problem hiding this comment.
Niiiice looks great Roni! I made a few improvements, namely:
- Use (provider_name, local_id) as the unique constraint for acquisitions to allow for a provider to have multiple acquisitions for the same edition
- Remove update_edition_id; out-of-scope, should be done once edition merging is implemented
- Use modern postgres ON CONFLICT for upserting
To avoid git conflict nightmares, I'm actually going to close this one and push my changes to your aggregate PR, #12852 .
| def get_by_edition(edition_id: int, provider_name: str | None = None) -> list[Acquisition]: | ||
| if provider_name is None: | ||
| rows: ResultSet = db.query( | ||
| "SELECT * FROM acquisitions WHERE edition_id=$edition_id ORDER BY provider_name", |
There was a problem hiding this comment.
Is there a reason these are ordered by default?
| "SELECT * FROM acquisitions WHERE edition_id=$edition_id ORDER BY provider_name", | |
| "SELECT * FROM acquisitions WHERE edition_id=$edition_id", |
|
That will be a great help. I was expecting a few changes cause I didn't have overall edge cases understanding but covered most of them I know |
- Use (provider_name, local_id) as the unique constraint for acquisitions to allow for a provider to have multiple acquisitions for the same edition - Remove update_edition_id; out-of-scope, should be done once edition merging is implemented - Use modern postgres ON CONFLICT for upserting Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4079ba5 to
f9f8c9c
Compare
|
Actually, the rebase was pretty simple! So I'm going to go ahead and merge this one as-is, and then force push the rebase to your aggregate PR 👍 And remove the commit I added to that one :P |
Closes #12844
Feature:
Subtask 2 #12844
Technical
Testing
Screenshot
Stakeholders
@mekarpeles @cdrini