Description
Tomás Cohen Arazi (tcohen)
2025-09-22 18:12:51 UTC
Created attachment 186734 [details] [review] Bug 40850: Add `Koha::ILL::Request->add_or_update_attributes` Currently, ILL backends and plugins must implement custom logic to update existing extended attributes on ILL requests, as the core extended_attributes() method only supports adding new attributes and throws duplicate key errors when attempting to update existing ones. This has led to code duplication across multiple ILL plugins (Rapido, INNReach, SLNP) that all implement identical add_or_update_attributes() methods, and forces the Standard backend to use error-prone patterns when managing request metadata. This patch adds add_or_update_attributes() method to Koha::ILL::Request that provides a safe, efficient way to add new attributes or update existing ones in a single operation, eliminating duplicate key errors and reducing code duplication. The method: - Accepts a hashref of attribute type => value pairs - Updates existing attributes only when values actually change - Creates new attributes when they don't exist - Skips undefined or empty values to avoid constraint errors - Uses transactions for atomicity - Returns the request object for method chaining Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Request.t => SUCCESS: Tests pass! New method works correctly 3. Test adding new attributes works 4. Test updating existing attributes works 5. Test mixed add/update operations work 6. Test empty/undefined values are skipped 7. Sign off :-D Created attachment 186735 [details] [review] Bug 40850: (follow-up) Use add_or_update_attributes in copyright clearance methods This patch simplifies the set_copyright_clearance_confirmed() method to use the new add_or_update_attributes() method instead of custom update/create logic. Benefits: - Reduces code complexity from 20+ lines to 6 lines - Eliminates duplicate update/create logic - Uses the new standardized method for attribute management - Maintains all existing functionality and test coverage Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Request.t => SUCCESS: Tests pass! Copyright clearance methods work with new implementation 3. Test copyright clearance workflow still works in ILL module 4. Sign off :-D Created attachment 186736 [details] [review] Bug 40850: (follow-up) Use add_or_update_attributes in Standard backend This patch updates the Standard ILL backend to use the new add_or_update_attributes() method instead of the problematic extended_attributes() method that only supports adding new attributes. Changes: - Request creation: Use add_or_update_attributes() for setting metadata - Request migration: Use add_or_update_attributes() for copying attributes Benefits: - Eliminates duplicate key errors when updating existing requests - Cleaner code - removes array-to-hash conversion boilerplate - More efficient - only updates attributes when values change - Consistent with new Koha::ILL::Request API patterns Locations updated: - add_request(): Request creation with metadata (line ~1017) - migrate(): Request migration copying attributes (line ~731) Test plan: 1. Apply patch 2. Test ILL request creation in Standard backend: - Create new ILL request with metadata => SUCCESS: Request created with attributes 3. Test ILL request migration: - Migrate existing request to new request => SUCCESS: Attributes copied correctly 4. Test updating existing request metadata: - Edit request and update metadata fields => SUCCESS: No duplicate key errors 5. Sign off :-D Created attachment 186755 [details] [review] Bug 40850: Add `Koha::ILL::Request->add_or_update_attributes` Currently, ILL backends and plugins must implement custom logic to update existing extended attributes on ILL requests, as the core extended_attributes() method only supports adding new attributes and throws duplicate key errors when attempting to update existing ones. This has led to code duplication across multiple ILL plugins (Rapido, INNReach, SLNP) that all implement identical add_or_update_attributes() methods, and forces the Standard backend to use error-prone patterns when managing request metadata. This patch adds add_or_update_attributes() method to Koha::ILL::Request that provides a safe, efficient way to add new attributes or update existing ones in a single operation, eliminating duplicate key errors and reducing code duplication. The method: - Accepts a hashref of attribute type => value pairs - Updates existing attributes only when values actually change - Creates new attributes when they don't exist - Skips undefined or empty values to avoid constraint errors - Uses transactions for atomicity - Returns the request object for method chaining Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Request.t => SUCCESS: Tests pass! New method works correctly 3. Test adding new attributes works 4. Test updating existing attributes works 5. Test mixed add/update operations work 6. Test empty/undefined values are skipped 7. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186756 [details] [review] Bug 40850: (follow-up) Use add_or_update_attributes in copyright clearance methods This patch simplifies the set_copyright_clearance_confirmed() method to use the new add_or_update_attributes() method instead of custom update/create logic. Benefits: - Reduces code complexity from 20+ lines to 6 lines - Eliminates duplicate update/create logic - Uses the new standardized method for attribute management - Maintains all existing functionality and test coverage Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Request.t => SUCCESS: Tests pass! Copyright clearance methods work with new implementation 3. Test copyright clearance workflow still works in ILL module 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186757 [details] [review] Bug 40850: (follow-up) Use add_or_update_attributes in Standard backend This patch updates the Standard ILL backend to use the new add_or_update_attributes() method instead of the problematic extended_attributes() method that only supports adding new attributes. Changes: - Request creation: Use add_or_update_attributes() for setting metadata - Request migration: Use add_or_update_attributes() for copying attributes Benefits: - Eliminates duplicate key errors when updating existing requests - Cleaner code - removes array-to-hash conversion boilerplate - More efficient - only updates attributes when values change - Consistent with new Koha::ILL::Request API patterns Locations updated: - add_request(): Request creation with metadata (line ~1017) - migrate(): Request migration copying attributes (line ~731) Test plan: 1. Apply patch 2. Test ILL request creation in Standard backend: - Create new ILL request with metadata => SUCCESS: Request created with attributes 3. Test ILL request migration: - Migrate existing request to new request => SUCCESS: Attributes copied correctly 4. Test updating existing request metadata: - Edit request and update metadata fields => SUCCESS: No duplicate key errors 5. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Simple internal API improvement, well covered by tests. Straight for QA, Passed. Nice work everyone! Pushed to main for 25.11 |