[Bug 40855] New: Standard backend uses plain SQL
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Bug ID: 40855 Summary: Standard backend uses plain SQL Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: ILL Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org CC: lisette@bywatersolutions.com, pedro.amorim@openfifth.co.uk, tomascohen@gmail.com 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Depends on| |40850 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40850 [Bug 40850] Add `Koha::ILL::Request->add_or_update_attributes` -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |pedro.amorim@openfifth.co.u |y.org |k -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|pedro.amorim@openfifth.co.u |testopia@bugs.koha-communit |k |y.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |38341 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38341 [Bug 38341] Standard backend is missing unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 186740 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186740&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 186741 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186741&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |40856 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40856 [Bug 40856] Improve Standard backend metadata retrieval -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186740|0 |1 is obsolete| | --- Comment #3 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 186758 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186758&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #186741|0 |1 is obsolete| | --- Comment #4 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 186759 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186759&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk CC| |martin.renvoize@openfifth.c | |o.uk --- Comment #5 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Simple internal API improvement, well covered by tests. Straight for QA, Passed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |25.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 --- Comment #6 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Bug 40855 depends on bug 40850, which changed state. Bug 40850 Summary: Add `Koha::ILL::Request->add_or_update_attributes` https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40850 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Laura.escamilla@bywatersolu | |tions.com Status|Pushed to main |Needs documenting --- Comment #7 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- Not backported to 25.05.x. This patch depends on Bug 40850, which adds `Koha::ILL::Request->add_or_update_attributes`. Bug 40850 was not backported to 25.05.x because it is an enhancement, so this patch is not compatible with 25.05.x without also backporting that enhancement dependency. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40855 Aude Charillon <aude.charillon@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Needs documenting |RESOLVED CC| |aude.charillon@openfifth.co | |.uk --- Comment #8 from Aude Charillon <aude.charillon@openfifth.co.uk> --- No need for any update to the Manual. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org