Bug 40855 - Standard backend uses plain SQL
Summary: Standard backend uses plain SQL
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 40850
Blocks: 38341 40856
  Show dependency treegraph
 
Reported: 2025-09-22 19:05 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 40855: Add unit tests for Standard ILL backend edititem method (6.00 KB, patch)
2025-09-22 19:15 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40855: Replace raw SQL in Standard backend edititem with add_or_update_attributes (4.11 KB, patch)
2025-09-22 19:15 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40855: Add unit tests for Standard ILL backend edititem method (6.06 KB, patch)
2025-09-22 20:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40855: Replace raw SQL in Standard backend edititem with add_or_update_attributes (4.17 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 19:05:03 UTC
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.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-09-22 19:15:22 UTC
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
Comment 2 Tomás Cohen Arazi (tcohen) 2025-09-22 19:15:25 UTC
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
Comment 3 Martin Renvoize (ashimema) 2025-09-22 20:56:25 UTC
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>
Comment 4 Martin Renvoize (ashimema) 2025-09-22 20:56:28 UTC
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>
Comment 5 Martin Renvoize (ashimema) 2025-09-22 20:58:03 UTC
Simple internal API improvement, well covered by tests.

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

Pushed to main for 25.11