-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Community Batch Imports endpoint #8122
Community Batch Imports endpoint #8122
Conversation
c721508
to
fb0f567
Compare
Converting this to a draft for now since it's kind of in limbo! |
fb0f567
to
f0ce028
Compare
This: - still has debugging lines that need to be removed; - creates an endpoint at /api/import?batch=true that reads JSON data; - uses an ISBN (preferring ISBN 13) for the ia_id; - uses the `Batch` class for imports; - relies on there being a 'submitter' column in thte import_item table; - only works for people with `can_write()` privileges; and - generates a hash based on the incoming JSON byte string to use as the name of the import batch. To use: curl -X POST http://localhost:8080/api/import\?batch\=true -H \ "Content-Type: application/json" -H "Cookie: $OL_COOKIE" -d \ '[{"title": "test book 1", "isbn_10": "test_1"}, {"title": "test book 2", "isbn_13": "test_2"}]'
f0ce028
to
23a7dd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This looks great! A few code fixes, and one annoying rename :P
] | ||
|
||
# Create the batch | ||
batch = Batch.find(batch_name) or Batch.new(name=batch_name, submitter=username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the future we should move away from treating batch_name
like a unqiue string; it should just be effectively a comment. The batch id is what should be... the id :P But future PR problem, I think there were some good reasons why we did this.
@@ -31,17 +30,28 @@ | |||
|
|||
|
|||
class Batch(web.storage): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo in a future PR: make this not extend web.storage
Adding feedback from CR Co-authored-by: Drini Cami <cdrini@gmail.com> [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
042d27f
to
ce52c90
Compare
ec84040
to
fdcf2c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice lgtm! Tested it erroring correctly; and tested it actually queueing items 🥳 Note I didn't see it actually get imported; caused around this time of a month we have a big import that goes through, so it'll take a whiiile for it to actually go through (note not ideal UX :/ but problem for another time and should only affect ~15-20th of every month).
Closes #7705
Feature.
This adds a (currently admin-only) endpoint at
/import/batch/new
for community batch imports.Technical
This:
/import/batch/new
that takes JSONL data via a multipart POST;#
as a comment in JSONL, just to make life a bit easier.rec
for import viaload()
;source_records[0]
for theia_id
;Batch
class for imports;import_item
table;/usergroup/admin
group; andname
of the import batch (so that an attempted import of the same items will get the same name). Note: this isn't super sophisticated as the hash would change if the order changed.Some outstanding questions:
/import/batch/new
and/api/import/bulk
; which do we want to use, if either?Batch.add_items()
, what should that limit be?This was also tested against some of the data from @Billa05's work in #8551, found here: #8551 (comment)
Testing
Visit http://localhost:8080/import/batch/new

Try to upload a JSONL file with some errors:
See that the validation errors are displayed to the patrons:

Fix the errors by... commenting out the problematic lines:
Verify the records are not in the
import_item
table:Then try to upload again:

See that the items ended up in
import_item
:Try to upload the same file again and see that the duplicates are caught and reported, and that the total tried, added, and not queued are correct, along with the duplicate items:
