The edititem() method in the Standard ILL backend uses raw SQL queries with manual delete-all-and-recreate patterns instead of leveraging Koha's ORM. This approach creates several problems: it bypasses ORM validation and relationships, uses inefficient delete-all-then-insert operations that update unchanged attributes, includes complex conditional SQL based on column existence checks [1], and makes the code harder to maintain and debug. The method contains over 40 lines of raw SQL that could be replaced with a single call to add_or_update_attributes() (from bug 40850), which would be more efficient (only updating changed values), safer (using ORM validation), more maintainable (no raw SQL). [1] For backwards compatibility with older Koha versions? Inherited from the time it was an external backend.
Created attachment 186740 [details] [review] Bug 40855: Add unit tests for Standard ILL backend edititem method This patch adds comprehensive unit tests for the edititem() method in the Standard ILL backend to establish baseline behavior before refactoring. The tests cover: - Form stage (initial load) - displays current attributes for editing - Commit stage (form submission) - processes updates with validation - Core field updates (title, author, year, etc.) - Custom field handling (null-delimited key/value pairs) - Attribute persistence after updates These tests demonstrate the current raw SQL implementation behavior and will ensure the upcoming refactoring maintains compatibility. Current behavior tested: - edititem() uses multi-stage workflow (init -> form -> commit) - Validation requires 'type' and 'branchcode' fields - Custom fields use null-delimited strings for multiple values - Method returns 'create' (not 'edititem') in commit stage - Raw SQL delete-all-and-recreate pattern for attribute updates Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Backend/Standard.t => SUCCESS: Tests pass! Current edititem behavior documented 3. Tests establish baseline for upcoming refactoring 4. Sign off :-D
Created attachment 186741 [details] [review] Bug 40855: Replace raw SQL in Standard backend edititem with add_or_update_attributes This patch refactors the edititem() method in the Standard ILL backend to use the modern add_or_update_attributes() method instead of raw SQL delete-all-and-recreate operations. Problems with the old approach: - 40+ lines of raw SQL bypassing ORM validation - Inefficient delete-all-then-insert pattern - Complex conditional SQL based on column existence - Manual transaction handling and error-prone bind parameters - Difficult to maintain and debug Benefits of the new approach: - Single line replaces 40+ lines of raw SQL - Uses ORM validation and relationships - Only updates attributes when values actually change - Automatic backend column handling - Consistent with other modernized Standard backend methods - Better performance and maintainability The refactoring: - Removes all raw SQL queries (DELETE and INSERT statements) - Removes manual transaction handling - Removes conditional column existence checks - Replaces with single add_or_update_attributes() call - Maintains identical functionality and behavior Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Backend/Standard.t => SUCCESS: Tests pass! Refactoring maintains compatibility 3. Test ILL request editing in Standard backend: - Edit existing ILL request metadata - Add/modify custom fields - Verify attributes updated correctly => SUCCESS: Same functionality, cleaner implementation 4. Sign off :-D
Created attachment 186758 [details] [review] Bug 40855: Add unit tests for Standard ILL backend edititem method This patch adds comprehensive unit tests for the edititem() method in the Standard ILL backend to establish baseline behavior before refactoring. The tests cover: - Form stage (initial load) - displays current attributes for editing - Commit stage (form submission) - processes updates with validation - Core field updates (title, author, year, etc.) - Custom field handling (null-delimited key/value pairs) - Attribute persistence after updates These tests demonstrate the current raw SQL implementation behavior and will ensure the upcoming refactoring maintains compatibility. Current behavior tested: - edititem() uses multi-stage workflow (init -> form -> commit) - Validation requires 'type' and 'branchcode' fields - Custom fields use null-delimited strings for multiple values - Method returns 'create' (not 'edititem') in commit stage - Raw SQL delete-all-and-recreate pattern for attribute updates Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Backend/Standard.t => SUCCESS: Tests pass! Current edititem behavior documented 3. Tests establish baseline for upcoming refactoring 4. Sign off :-D Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 186759 [details] [review] Bug 40855: Replace raw SQL in Standard backend edititem with add_or_update_attributes This patch refactors the edititem() method in the Standard ILL backend to use the modern add_or_update_attributes() method instead of raw SQL delete-all-and-recreate operations. Problems with the old approach: - 40+ lines of raw SQL bypassing ORM validation - Inefficient delete-all-then-insert pattern - Complex conditional SQL based on column existence - Manual transaction handling and error-prone bind parameters - Difficult to maintain and debug Benefits of the new approach: - Single line replaces 40+ lines of raw SQL - Uses ORM validation and relationships - Only updates attributes when values actually change - Automatic backend column handling - Consistent with other modernized Standard backend methods - Better performance and maintainability The refactoring: - Removes all raw SQL queries (DELETE and INSERT statements) - Removes manual transaction handling - Removes conditional column existence checks - Replaces with single add_or_update_attributes() call - Maintains identical functionality and behavior Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/ILL/Backend/Standard.t => SUCCESS: Tests pass! Refactoring maintains compatibility 3. Test ILL request editing in Standard backend: - Edit existing ILL request metadata - Add/modify custom fields - Verify attributes updated correctly => SUCCESS: Same functionality, cleaner implementation 4. 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