https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42643 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42643 [Bug 42643] [OMNIBUS] Assorted performance and stability work -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Sponsorship status|--- |Sponsored Comma delimited| |ByWater Solutions list of Sponsors| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200146 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200146&action=edit Bug 42404: Add REST API endpoint for batch cover image import This patch adds a REST API endpoint for batch importing cover images: POST /api/v1/cover_images/batch It accepts a zip file via multipart/form-data and enqueues a BatchImportCoverImages background job to process it. Query parameters: - replace: boolean, delete existing images before import - target: enum (biblio_id, item_id), specifies what the first column of the mapping file represents Requires tools: upload_local_cover_images permission. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Tests cover: - Authentication required (401) - Authorization required (403) - Successful upload enqueues job (201) - Replace flag forwarded correctly - Missing file returns 400 4. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200150&action=edit Bug 42404: Add BatchImportCoverImages background job This patch adds a background job that processes a zip file containing cover images and an idlink.txt/datalink.txt mapping file. Each line in the mapping file maps a biblio_id (or item_id) to an image filename. Features: - Supports both biblio_id and item_id targets - Replace mode (deletes existing images before import) - Uses Archive::Zip instead of system unzip - Path traversal protection on extracted files - Reports progress via the background jobs infrastructure Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/BackgroundJob/BatchImportCoverImages.t => SUCCESS: Tests pass! 3. Tests cover: - enqueue() sets correct job size, status, and queue - Single and multiple image imports - Replace mode behavior - Bad delimiter handling in mapping files - Cancelled job is skipped - Upload cleanup after processing 4. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #5 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200152&action=edit Bug 42404: (follow-up) Add background job detail template This patch adds the TT include used by the background jobs detail page to display the results of a batch cover image import job. Test plan: 1. Apply patch 2. Upload a zip file with cover images via the upload tool 3. Navigate to Administration > Background jobs 4. Click on the batch_import_cover_images job => SUCCESS: Job detail page shows imported/failed counts and messages 5. Sign off :-D Sponsored-by: ByWater Solutions -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|42643 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42643 [Bug 42643] [OMNIBUS] Assorted performance and stability work -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |andrew@bywatersolutions.com --- Comment #7 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Agreed, this is not actually enqueueing a background job, it's just processing the uploaded file. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #9 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- I'd be tempted to return a 202 Accepted as apposed to a 201 Created here? The task is transient isn't it.. the final result is the processed covers.. I'd also be tempted to set the Location headers to the job tracking API endpoint? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #11 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 200611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200611&action=edit Bug 42404: (follow-up) Return 202 Accepted with Location header The batch cover image import endpoint enqueues a background job for processing. A 201 Created is incorrect because no resource is durably created at response time - the work is deferred. This patch changes the response to 202 Accepted, which is the correct HTTP status for async job submission. Additionally, the Location header is set to the job tracking endpoint so clients can poll for completion status without out-of-band knowledge. Changes: - Return 202 instead of 201 from batch_import controller - Set Location header to /api/v1/jobs/{job_id} - Update OpenAPI spec to document 202 response and Location header - Update tests to assert 202 status and Location header Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Verify the OpenAPI spec is valid: k$ cd /kohadevbox/koha && perl t/db_dependent/api/v1/spec.t => SUCCESS: Spec validates! 4. Sign off :-D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42869 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42869 [Bug 42869] Adjust async job endpoints to coding guideline (202 Accepted) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch Status|Needs Signoff |Signed Off QA Contact|martin.renvoize@openfifth.c |dcook@prosentient.com.au |o.uk | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200832 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200832&action=edit Bug 42404: Add BatchImportCoverImages background job This patch adds a background job that processes a zip file containing cover images and an idlink.txt/datalink.txt mapping file. Each line in the mapping file maps a biblio_id (or item_id) to an image filename. Features: - Supports both biblio_id and item_id targets - Replace mode (deletes existing images before import) - Uses Archive::Zip instead of system unzip - Path traversal protection on extracted files - Reports progress via the background jobs infrastructure Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/BackgroundJob/BatchImportCoverImages.t => SUCCESS: Tests pass! 3. Tests cover: - enqueue() sets correct job size, status, and queue - Single and multiple image imports - Replace mode behavior - Bad delimiter handling in mapping files - Cancelled job is skipped - Upload cleanup after processing 4. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200834 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200834&action=edit Bug 42404: (follow-up) Add background job detail template This patch adds the TT include used by the background jobs detail page to display the results of a batch cover image import job. Test plan: 1. Apply patch 2. Upload a zip file with cover images via the upload tool 3. Navigate to Administration > Background jobs 4. Click on the batch_import_cover_images job => SUCCESS: Job detail page shows imported/failed counts and messages 5. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #17 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200836 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200836&action=edit Bug 42404: (follow-up) Fix GD exception handling, logger, and empty mapping detection - Wrap GD::Image->new() in eval{} so a die from a malformed image is caught per-image rather than aborting the entire job via the outer catch block; images_failed and step() are now always updated correctly on GD errors. - Add Koha::Logger->get->error/warn calls in all catch blocks so operators have server-side detail while users see a sanitized message. - Add _finish_failed() helper that correctly stores messages and the report before setting status to 'failed', fixing the pre-existing issue where early-exit paths called status('failed')->store without calling finish(). - After mapping parsing, fail the job (not silently succeed with 0 imported) when no valid mappings are found, including the case where all lines had unrecognised delimiters. - Update tests accordingly. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #19 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200838 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200838&action=edit Bug 42404: (follow-up) Wrap per-image replace+store in a transaction Without a transaction, the delete-then-store sequence in replace mode is non-atomic: if ->store() fails the old images are already gone and cannot be recovered. Wrap each image's delete (optional) and store in a txn_do() block so both either succeed together or roll back together. Each image is processed independently so a failure on one does not affect others. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #21 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200840 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200840&action=edit Bug 42404: (follow-up) Handle move failure and empty filename in _store_upload - Check the return value of move_to(); die if the file could not be written so the caller's catch block returns a 500 rather than silently creating an UploadedFile record pointing to a missing file. - After filename sanitization, fall back to 'upload.zip' if all characters were stripped, preventing an empty path component. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #23 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200842 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200842&action=edit Bug 42404: (follow-up) Canonicalize extraction directory before path-traversal check The path-traversal guard compares Cwd::abs_path(\$full_path) against \$dirname using a regex anchor. If \$dirname itself contains unresolved symlink components the comparison could be fooled. Resolve \$dirname via Cwd::abs_path() immediately after extraction (once the directory exists on disk) so both sides of the comparison are fully canonicalized. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #25 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 200844 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=200844&action=edit Bug 42404: (follow-up) Ensure upload file is cleaned up on unexpected exception The upload variable was declared inside the outer try block, making it invisible to any cleanup code after the try/catch. Move the declaration outside the try so a guard after the try/catch can delete the upload if an unexpected exception fired before the normal cleanup path ran. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #200832|0 |1 is obsolete| | Attachment #200833|0 |1 is obsolete| | Attachment #200834|0 |1 is obsolete| | Attachment #200835|0 |1 is obsolete| | Attachment #200836|0 |1 is obsolete| | Attachment #200837|0 |1 is obsolete| | Attachment #200838|0 |1 is obsolete| | Attachment #200839|0 |1 is obsolete| | Attachment #200840|0 |1 is obsolete| | Attachment #200841|0 |1 is obsolete| | Attachment #200842|0 |1 is obsolete| | Attachment #200843|0 |1 is obsolete| | Attachment #200844|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #27 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201279&action=edit Bug 42404: Add REST API endpoint for batch cover image import This patch adds a REST API endpoint for batch importing cover images: POST /api/v1/cover_images/batch It accepts a zip file via multipart/form-data and enqueues a BatchImportCoverImages background job to process it. Query parameters: - replace: boolean, delete existing images before import - target: enum (biblio_id, item_id), specifies what the first column of the mapping file represents Requires tools: upload_local_cover_images permission. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Tests cover: - Authentication required (401) - Authorization required (403) - Successful upload enqueues job (201) - Replace flag forwarded correctly - Missing file returns 400 4. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #29 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201281 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201281&action=edit Bug 42404: (follow-up) Return 202 Accepted with Location header The batch cover image import endpoint enqueues a background job for processing. A 201 Created is incorrect because no resource is durably created at response time - the work is deferred. This patch changes the response to 202 Accepted, which is the correct HTTP status for async job submission. Additionally, the Location header is set to the job tracking endpoint so clients can poll for completion status without out-of-band knowledge. Changes: - Return 202 instead of 201 from batch_import controller - Set Location header to /api/v1/jobs/{job_id} - Update OpenAPI spec to document 202 response and Location header - Update tests to assert 202 status and Location header Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Verify the OpenAPI spec is valid: k$ cd /kohadevbox/koha && perl t/db_dependent/api/v1/spec.t => SUCCESS: Spec validates! 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #31 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201283 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201283&action=edit Bug 42404: (follow-up) Reject symlinks in image file paths The existing path-traversal check guards against filenames that resolve outside the extraction directory, but did not guard against symlinks that point to arbitrary host filesystem paths (e.g. /etc/passwd). Add an explicit -l check for each resolved image path before passing it to GD::Image->new(); symlinks are treated as path_traversal errors and the image is skipped. The mapping-file symlink checks introduced earlier are therefore consistent across all file reads in the job. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #33 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201285 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201285&action=edit Bug 42404: (follow-up) Reuse item object to avoid redundant fetch in replace mode In item_id target mode, the item was fetched once to obtain the biblionumber, then fetched again inside the replace block via Koha::Items->find(). Store the first result and pass it through to avoid the N+1 pattern. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #35 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201287 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201287&action=edit Bug 42404: (follow-up) Validate IDs are numeric before database lookup IDs from the mapping file are used directly in Koha::Items->find() and Koha::Biblios->find(). Add an explicit check that the value is all digits before passing it to the ORM layer so that non-numeric values produce a clear 'invalid_id' message rather than an ORM exception or silent failure. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #37 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201289 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201289&action=edit Bug 42404: (follow-up) Die on upload directory creation failure The mkdir call in _store_upload silently continued if the directory could not be created, leading to a confusing move_to failure later. Die immediately with a descriptive message; the caller's catch block will return a 500 to the client. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #39 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi reviewed Martin's follow-ups, they look ok and make sense. I also squashed some QA check failures in the follow-ups. Thanks! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #40 from David Cook <dcook@prosentient.com.au> --- A consolidated testing plan should be provided... especially including rebuilding the API and restarting Plack... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|dcook@prosentient.com.au |testopia@bugs.koha-communit | |y.org --- Comment #42 from David Cook <dcook@prosentient.com.au> --- Going to reset QA for now... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #44 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to David Cook from comment #43)

I don't love the idea of signing off on a bug report just because it's passing tests though...

What I expect testers is: - You apply 42404, 42405 and 42804. - Tests from all bugs pass. - All cover image-related features in the UI work as expected. I know splitting like this might have not been the best. On the other hand, the endpoints are features by themselves third parties couldrely on, appart from the internal use in Koha itself! It can be tested using Postman, but I haven't really played with it for this use case myself. When you choose `raw` for the request body format, it lets you choose a file to upload. But you are still able to test from the tools section! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This work adds an endpoint release notes| |for handling batch cover | |images uploads on the REST | |API. It accepts uploaded | |files and schedules a | |background job for | |processing them. it | |conveniently returns a 202 | |with the effective job | |endpoint in the Location | |header so consumers can | |query progress afterwards. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #45 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- I found what I think is a problem: To recreate: 1. Enable LocalCoverImages 2. Enable AllowMultipleCovers 3. Go to a record -> Uplaod image ( upload any image ) 4. In the holdings table click Edit -> Upload image ( any image ) 5. Do a POST to `/api/v1/cover_images/batch?replace=true` with the zip file. The biblio level image is replaced. The item level image is just gone. Questions, not necessarily blockers: -Should this ever replace item level images? Or are we waiting on Bug 42405 to do that? -When I put two images in a ZIP only the first one is ever used to replace at `/api/v1/cover_images/batch?replace=true`, even when AllowMultipleCovers is on. It should honor AllowMultipleCovers, right? -Maybe I missed this, or there is a separate bug report, will the staff image upload tool use the API? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #47 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209366 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209366&action=edit Bug 42404: (follow-up) Finish BatchImportCoverImages jobs exactly once The early returns in process() (upload not found, unreadable zip, no valid mappings) only returned from the Try::Tiny block, so the job was finished by _finish_failed() and then a second time after the block. Unexpected exceptions, on the other hand, ended up with the 'finished' status. This patch makes the try block return whether the job failed, and the job is finished once, after the block. The uploaded zip is deleted in a single place, and unexpected exceptions now mark the job as 'failed'. The extraction directory is now a File::Temp->newdir object, so it is removed when process() is done instead of when the process exits. File::Temp is now explicitly loaded, it was only available because another module loaded it. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/BackgroundJob/BatchImportCoverImages.t => SUCCESS: Tests pass! 3. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #49 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209368 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209368&action=edit Bug 42404: (follow-up) Fix replace behaviour in BatchImportCoverImages The way BatchImportCoverImages replaced existing covers had three problems. The first two were reported by Lucas on comment #45: 1. Replacing a biblio's covers used $biblio->cover_images, which also includes the covers of its items, so the item level covers were deleted too. tools/upload-cover-image.pl only deleted the biblio level ones when uploading a single image. 2. The existing covers were deleted for every line of the mapping file, so when several images were mapped to the same record, each one deleted the previous one and only one image was kept, even with AllowMultipleCovers enabled. 3. tools/upload-cover-image.pl always replaced existing covers when the AllowMultipleCovers system preference was disabled. The job only looked at the 'replace' parameter, so API consumers could attach several covers to a record when the library does not allow it. This patch: - Only deletes the biblio level covers (itemnumber IS NULL) when replacing a biblio's covers. Item covers are only replaced when importing with target=item_id. - Removes the existing covers of a record only once per job when AllowMultipleCovers is enabled, so all the images in the file are kept. When it is disabled each image replaces the previous one, so the record ends up with the last one. - Replaces existing covers when AllowMultipleCovers is disabled, whatever the 'replace' parameter says. The check lives in the job so every caller gets it. The API spec documents the behaviour. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/BackgroundJob/BatchImportCoverImages.t \ xt/api.t => SUCCESS: Tests pass! 3. Revert the changes to Koha/BackgroundJob/BatchImportCoverImages.pm and repeat 2 => FAIL: Item level covers are deleted, only one image is kept when several are mapped to the same record, and two covers are attached to a record with AllowMultipleCovers disabled 4. Restore the changes and restart services: k$ restart_all 5. Enable LocalCoverImages, AllowMultipleCovers and RESTBasicAuth 6. Go to record 1 and upload a cover image (Images tab -> Upload) 7. In the holdings table, pick an item: Edit -> Upload image, upload another image 8. Create a zip with two images and an idlink.txt mapping both to record 1: k$ mkdir /tmp/covers && cd /tmp/covers k$ cp /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/img/koha-logo.png a.png k$ cp a.png b.png k$ printf "1\ta.png\n1\tb.png\n" > idlink.txt k$ python3 -m zipfile -c covers.zip idlink.txt a.png b.png 9. Upload it, replacing existing covers: k$ curl -u koha:koha -F file=@covers.zip \ "http://localhost:8081/api/v1/cover_images/batch?replace=true" => SUCCESS: A job id is returned 10. Check the job in Administration -> Manage jobs => SUCCESS: 2 images imported 11. Go to record 1 => SUCCESS: The record cover uploaded in 6 is gone, the two new images are there => SUCCESS: The item cover uploaded in 7 is still there 12. Disable AllowMultipleCovers and repeat 9 => SUCCESS: Record 1 has a single record level cover 13. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 --- Comment #51 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209370 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209370&action=edit Bug 42404: (follow-up) Remove the upload if the batch import job cannot be enqueued If enqueueing the BatchImportCoverImages job failed, the zip stored by _store_upload was left behind until cleanup_database.pl purged it. This patch deletes it before rethrowing the exception. The API tests are also improved: - 'replace' and 'target' were sent as form fields while the spec declares them as query parameters, and the mocked enqueue ignored its arguments, so nothing checked they reached the job. The tests now use query parameters and check the job arguments, including the defaults. - The stored upload is checked (temporary, file on disk). - An invalid 'target' is rejected with a 400. - An enqueue failure returns a 500 and leaves no upload behind. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/cover_images.t => SUCCESS: Tests pass! 3. Revert the changes to Koha/REST/V1/CoverImages.pm and repeat 2 => FAIL: The upload is left behind when the job cannot be enqueued 4. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42404 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #53 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- 1. AllowMultipleCovers = off 2. Create a zip with two images and an idlink.txt mapping both to record 1 3. Do a POST to /api/v1/cover_images/batch ( I used cURL ) 4. Go to Administration -> Manage jobs 5. Find you job ID and click on it 6. Only 1 of the 2 images is on the record; the other was replaced. Nothing on the job page says so. -- You are receiving this mail because: You are watching all bug changes.