Bug 40850 - Add `Koha::ILL::Request->add_or_update_attributes`
Summary: Add `Koha::ILL::Request->add_or_update_attributes`
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 40262
Blocks: 40855
  Show dependency treegraph
 
Reported: 2025-09-22 18:12 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-09-23 22:25 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00
Circulation function:


Attachments
Bug 40850: Add `Koha::ILL::Request->add_or_update_attributes` (6.55 KB, patch)
2025-09-22 18:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40850: (follow-up) Use add_or_update_attributes in copyright clearance methods (1.94 KB, patch)
2025-09-22 18:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40850: (follow-up) Use add_or_update_attributes in Standard backend (3.91 KB, patch)
2025-09-22 18:25 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40850: Add `Koha::ILL::Request->add_or_update_attributes` (6.62 KB, patch)
2025-09-22 20:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40850: (follow-up) Use add_or_update_attributes in copyright clearance methods (2.00 KB, patch)
2025-09-22 20:55 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40850: (follow-up) Use add_or_update_attributes in Standard backend (3.98 KB, patch)
2025-09-22 20:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-09-22 18:12:51 UTC
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.

The proposed `add_or_update_attributes()` method in Koha::ILL::Request would provide a safe, efficient way to add new attributes or update existing ones in a single operation, eliminating duplicate key errors and reducing code duplication. 

This enhancement would immediately benefit the Standard backend (which currently as 6+ locations using the problematic pattern), allow existing plugins to remove their custom implementations, improve performance by only updating attributes when values actually change, and provide a cleaner API for all ILL backend developers. The implementation follows proven patterns already tested in production plugins an d maintains full backward compatibility while solving a common pain point in ILL development.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-09-22 18:25:33 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
Comment 2 Tomás Cohen Arazi (tcohen) 2025-09-22 18:25:35 UTC
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
Comment 3 Tomás Cohen Arazi (tcohen) 2025-09-22 18:25:39 UTC
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
Comment 4 Martin Renvoize (ashimema) 2025-09-22 20:55:56 UTC
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>
Comment 5 Martin Renvoize (ashimema) 2025-09-22 20:55:58 UTC
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>
Comment 6 Martin Renvoize (ashimema) 2025-09-22 20:56:00 UTC
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>
Comment 7 Martin Renvoize (ashimema) 2025-09-22 20:57:37 UTC
Simple internal API improvement, well covered by tests.

Straight for QA, Passed.
Comment 8 Lucas Gass (lukeg) 2025-09-23 22:25:20 UTC
Nice work everyone!

Pushed to main for 25.11