Bug 36329

Summary: Transfer limits should respect `BranchTransferLimitsType`
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Pushed to oldstable --- QA Contact:
Severity: normal    
Priority: P5 - low CC: david, fridolin.somers, kyle, lucas, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04,23.05.12
Circulation function:
Bug Depends on: 36277    
Bug Blocks:    
Attachments: Bug 36329: Add missing return codes
Bug 36329: Miscelaneous spec fixes
Bug 36329: Make POST /transfer_limits/batch honor BranchTransferLimitsType
Bug 36329: Add missing return codes
Bug 36329: Miscelaneous spec fixes
Bug 36329: Make POST /transfer_limits/batch honor BranchTransferLimitsType
Bug 36329: Add missing return codes
Bug 36329: Miscelaneous spec fixes
Bug 36329: Make POST /transfer_limits/batch honor BranchTransferLimitsType
Bug 36329: (follow-up) Fix error message comparisson in testes

Description Tomás Cohen Arazi (tcohen) 2024-03-15 11:15:26 UTC
The endpoint allows passing both a `collection code` and an `item type` on a batch request, but the UI would never allow such combination.

I wonder if this is a design decision (i.e. supporting both in the future) or just a missing check. My intuition says it is the latter.

I'm submitting a patch just in case.

Kyle?
Comment 1 Tomás Cohen Arazi (tcohen) 2024-03-15 13:01:16 UTC
Created attachment 163232 [details] [review]
Bug 36329: Add missing return codes
Comment 2 Tomás Cohen Arazi (tcohen) 2024-03-15 13:01:19 UTC
Created attachment 163233 [details] [review]
Bug 36329: Miscelaneous spec fixes
Comment 3 Tomás Cohen Arazi (tcohen) 2024-03-15 13:01:21 UTC
Created attachment 163234 [details] [review]
Bug 36329: Make POST /transfer_limits/batch honor BranchTransferLimitsType

This patch adds tests for the different cases of `BranchTransferLimitsType`.
It also adds tests for the situation of the consumer sending both limit
criterias on the request.

The controller gets adjusted for this new behavior and the spec gets
documentation added about this.

Bonus: tests are added the right guidelines code, and
BranchTransferLimitsType gets mocked to avoid failures due to existing
data.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All green, and tests pass!
3. Sign off :-D
Comment 4 David Nind 2024-03-15 21:19:51 UTC
Created attachment 163280 [details] [review]
Bug 36329: Add missing return codes

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2024-03-15 21:19:53 UTC
Created attachment 163281 [details] [review]
Bug 36329: Miscelaneous spec fixes

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-03-15 21:19:56 UTC
Created attachment 163282 [details] [review]
Bug 36329: Make POST /transfer_limits/batch honor BranchTransferLimitsType

This patch adds tests for the different cases of `BranchTransferLimitsType`.
It also adds tests for the situation of the consumer sending both limit
criterias on the request.

The controller gets adjusted for this new behavior and the spec gets
documentation added about this.

Bonus: tests are added the right guidelines code, and
BranchTransferLimitsType gets mocked to avoid failures due to existing
data.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All green, and tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2024-03-15 21:21:43 UTC
Testing notes (using KTD):

1. Tests pass before and after the patches are applied: prove t/db_dependent/api/v1/transfer_limits.t
Comment 8 Kyle M Hall (khall) 2024-03-18 17:48:22 UTC
Created attachment 163365 [details] [review]
Bug 36329: Add missing return codes

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall (khall) 2024-03-18 17:48:29 UTC
Created attachment 163366 [details] [review]
Bug 36329: Miscelaneous spec fixes

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall (khall) 2024-03-18 17:48:31 UTC
Created attachment 163367 [details] [review]
Bug 36329: Make POST /transfer_limits/batch honor BranchTransferLimitsType

This patch adds tests for the different cases of `BranchTransferLimitsType`.
It also adds tests for the situation of the consumer sending both limit
criterias on the request.

The controller gets adjusted for this new behavior and the spec gets
documentation added about this.

Bonus: tests are added the right guidelines code, and
BranchTransferLimitsType gets mocked to avoid failures due to existing
data.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ qa
=> SUCCESS: All green, and tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Katrin Fischer 2024-03-19 09:27:55 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 12 Tomás Cohen Arazi (tcohen) 2024-03-19 15:12:28 UTC
Created attachment 163453 [details] [review]
Bug 36329: (follow-up) Fix error message comparisson in testes

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Katrin Fischer 2024-03-21 06:27:43 UTC
Pushed the follow-up.
Comment 14 Fridolin Somers 2024-03-21 10:19:59 UTC
Pushed to 23.11.x for 23.11.04
Comment 15 Lucas Gass (lukeg) 2024-05-13 14:41:48 UTC
Backported to 23.05.x for upcoming 23.05.12