Bug 34024 - REST API should not allow changing the pickup location on found holds
Summary: REST API should not allow changing the pickup location on found holds
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-06-15 14:51 UTC by Emily Lamancusa
Modified: 2023-12-28 20:47 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.03,22.11.09


Attachments
Bug 34024: Unit tests (3.18 KB, patch)
2023-07-31 17:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34024: Block changing pickup location for found holds (2.11 KB, patch)
2023-07-31 17:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34024: Block changing pickup location for found holds (2.15 KB, patch)
2023-08-01 14:26 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 34024: Unit tests (3.22 KB, patch)
2023-08-01 14:27 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 34024: (QA follow-up) Allow pickup location changing for in transit holds (1.66 KB, patch)
2023-08-01 19:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34024: Block changing pickup location for found holds (2.22 KB, patch)
2023-08-17 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34024: Unit tests (3.28 KB, patch)
2023-08-17 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34024: (QA follow-up) Allow pickup location changing for in transit holds (1.72 KB, patch)
2023-08-17 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa 2023-06-15 14:51:45 UTC
If a hold is already set to "waiting", and a request to change the pickup location is sent through the REST API, Koha simply sets the hold to "waiting" at the new location with no notification to staff. This is clearly inaccurate, as the item is still physically at the original location.

Either the API handler or Koha::Hold should have some error-checking to reject a request to change the pickup location on a waiting hold.
Comment 1 Kyle M Hall 2023-07-26 18:38:30 UTC
My testing indicates that Koha doesn't allow changing the pickup location of any found holds, including In Transit holds. The API should reflect this same restriction.
Comment 2 Emily Lamancusa 2023-07-26 19:39:50 UTC
I think those restrictions are coded in at the UI level, so the API provides an unintentional loophole.

To reproduce on KTD:
1. Place a hold for pickup at CPL
2. Check in an item at CPL to fill the hold and set it to Waiting
3. Note that the hold now appears in the Holds Awaiting Pickup table for Centerville
4. Send an API request to /api/vi/holds/:hold_id/pickup_location to change
   the pickup location to MPL
5. Note that the request succeeds and the hold is moved to the Holds Awaiting
   Pickup table for Midway.

For In Transit holds, the API should respect OPACAllowUserToChangeBranch, when applicable.
Comment 3 Tomás Cohen Arazi 2023-07-31 13:25:23 UTC
(In reply to Emily Lamancusa from comment #2)
> For In Transit holds, the API should respect OPACAllowUserToChangeBranch,
> when applicable.

I don't think we should enforce it on the staff-only endpoint. Is this used by Aspen?
Comment 4 Tomás Cohen Arazi 2023-07-31 17:50:46 UTC
Created attachment 154105 [details] [review]
Bug 34024: Unit tests
Comment 5 Tomás Cohen Arazi 2023-07-31 17:50:49 UTC
Created attachment 154106 [details] [review]
Bug 34024: Block changing pickup location for found holds

This patch adds a check on holds for 'found' statuses. Holds with the
following statuses have special workflows in Koha, and overwritting
their pickup locations yields uncertain scenarios:

* Waiting: the item has probably been put on a special shelve
* In transit: the item is travelling to a new destination, and changing
  the pickup location should include some validations, and probably
  generate some notifications. At least.
* In processing: the item has already been taken out of the shelve for
  some internal processing. Changing the pickup location could interfere
  with this workflow. Overriding this limitation could be introduced
  with care on a separate report.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests don't pass. Holds go through (200) instead of being
rejected (409).
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 6 Emily Lamancusa 2023-07-31 17:57:18 UTC
I'm not sure which API endpoint Aspen uses, so it's very possible I used the wrong one in my steps to reproduce (my current understanding of the API doesn't go very far beyond looking up an endpoint at https://api.koha-community.org/ and sending a query to KTD by command line). My concern is indeed the Aspen behavior, though, so please double-check me if I have the wrong endpoint!

My main point was that the fix here shouldn't block changing the pickup location on an in-transit hold entirely, but rather should be consistent with the OPAC setting, so that Koha/Aspen libraries (or, presumably, libraries with other discovery layers?) can still use the feature.
Comment 7 Tomás Cohen Arazi 2023-07-31 18:28:13 UTC
(In reply to Emily Lamancusa from comment #6)
> I'm not sure which API endpoint Aspen uses, so it's very possible I used the
> wrong one in my steps to reproduce (my current understanding of the API
> doesn't go very far beyond looking up an endpoint at
> https://api.koha-community.org/ and sending a query to KTD by command line).
> My concern is indeed the Aspen behavior, though, so please double-check me
> if I have the wrong endpoint!
> 
> My main point was that the fix here shouldn't block changing the pickup
> location on an in-transit hold entirely, but rather should be consistent
> with the OPAC setting, so that Koha/Aspen libraries (or, presumably,
> libraries with other discovery layers?) can still use the feature.

I asked about Aspen in particular because I know it connects to Koha as a privileged user (as opposed to doing it with the patron's credentials). So, as a general rule, such system will be using a privileged set of endpoints and needs the right permissions. And the endpoint's behavior should respect the permissions/configurations limitations the UI has for such permissions/configurations.

I've read holds_table.inc lines 134-161, to identify what's the underlying logic for (not) allowing the pickup location change (on the staff interface). And that's what my patch implements at this time.

As I mention on the commit message: we can add 'overrides' to some situations, with a more evolved design. But, as you said, the current implementation leaves Koha in unhandled situations that need to be avoided.

Best regards
Comment 8 Sam Lau 2023-08-01 14:26:20 UTC
Created attachment 154126 [details] [review]
Bug 34024: Block changing pickup location for found holds

This patch adds a check on holds for 'found' statuses. Holds with the
following statuses have special workflows in Koha, and overwritting
their pickup locations yields uncertain scenarios:

* Waiting: the item has probably been put on a special shelve
* In transit: the item is travelling to a new destination, and changing
  the pickup location should include some validations, and probably
  generate some notifications. At least.
* In processing: the item has already been taken out of the shelve for
  some internal processing. Changing the pickup location could interfere
  with this workflow. Overriding this limitation could be introduced
  with care on a separate report.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests don't pass. Holds go through (200) instead of being
rejected (409).
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 9 Sam Lau 2023-08-01 14:27:31 UTC
Created attachment 154127 [details] [review]
Bug 34024: Unit tests

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 10 Emily Lamancusa 2023-08-01 17:46:30 UTC
Is changing the pickup location on a hold that's in transit truly an unhandled situation? 

Looking at bug 30500 and bug 14783, if the syspref is set to allow it, opac-modrequest.pl calls $hold->set_pickup_location without extra handling, because AddReturn already has logic to handle a situation where a transfer completes and a new transfer needs to be initiated based on hold pickup location. 

It's true the functionality to directly change the pickup location on an in-transit hold wasn't added to the staff interface when it was added to the OPAC, but is there a practical reason it shouldn't be permitted, or more just that there hasn't been a need to add it yet (in part because staff have the capability to revert the transit status on a hold in order to make those types of changes, while the OPAC and API do not)?

I'm not sure I fully understand the implications of a privileged API endpoint being able to change the pickup location on an in-transit hold, versus allowing the OPAC to do it, versus allowing a person in the staff interface to revert the transit status, change the pickup location, and then put the hold back in transit. (though actually I'd say the existing behavior allowing a staff member to mess with transit data as a workaround is riskier than adding the ability for them to change the pickup location directly, but that's beyond the scope of this bug) If there's a true risk to allowing the API to change the pickup location on an in-transit hold without an override, then it makes sense to be cautious. But if that's the case, please open a follow-up bug to add such an override route and restore the functionality.
Comment 11 Felicity Brown 2023-08-01 17:52:10 UTC
I second "allowing a staff member to mess with transit data as a workaround is riskier."
Comment 12 Tomás Cohen Arazi 2023-08-01 19:00:18 UTC
(In reply to Emily Lamancusa from comment #10)
> Is changing the pickup location on a hold that's in transit truly an
> unhandled situation? 

The code for handling it in the OPAC is defined in:

sub can_update_pickup_location_opac {
    my ($self) = @_;

    my @statuses = split /,/, C4::Context->preference("OPACAllowUserToChangeBranch");
    foreach my $status ( @statuses ){
        return 1 if ($status eq 'pending' && !$self->is_found && !$self->is_suspended );
        return 1 if ($status eq 'intransit' && $self->is_in_transit);
        return 1 if ($status eq 'suspended' && $self->is_suspended);
    }
    return 0;
}

I assume it should be safe to allow in_transit. I just don't know the area (staff UI-wise) enough to feel confident about enabling. Some thought on the initial report was needed though, as I found the in processing use case while digging.

I'm open to change it to allow changing the pickup location for in transit holds. I just thougth it deserved it's own bug report in which people involved in the area could weight in.
Comment 13 Tomás Cohen Arazi 2023-08-01 19:43:11 UTC
Created attachment 154132 [details] [review]
Bug 34024: (QA follow-up) Allow pickup location changing for in transit holds

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Nick Clemens (kidclamp) 2023-08-02 14:18:04 UTC
This seems correct. We should allow the API to update pickup location for in-transit holds. IMHO I think we should also add the ability to the staff interface
Comment 15 Tomás Cohen Arazi 2023-08-02 14:21:52 UTC
(In reply to Nick Clemens from comment #14)
> This seems correct. We should allow the API to update pickup location for
> in-transit holds. IMHO I think we should also add the ability to the staff
> interface

I agree. I'll file a separate report so we discuss the implementation details (logs? notifications?)
Comment 16 Martin Renvoize 2023-08-17 10:21:09 UTC
Created attachment 154513 [details] [review]
Bug 34024: Block changing pickup location for found holds

This patch adds a check on holds for 'found' statuses. Holds with the
following statuses have special workflows in Koha, and overwritting
their pickup locations yields uncertain scenarios:

* Waiting: the item has probably been put on a special shelve
* In transit: the item is travelling to a new destination, and changing
  the pickup location should include some validations, and probably
  generate some notifications. At least.
* In processing: the item has already been taken out of the shelve for
  some internal processing. Changing the pickup location could interfere
  with this workflow. Overriding this limitation could be introduced
  with care on a separate report.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/holds.t
=> FAIL: Tests don't pass. Holds go through (200) instead of being
rejected (409).
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2023-08-17 10:21:12 UTC
Created attachment 154514 [details] [review]
Bug 34024: Unit tests

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2023-08-17 10:21:15 UTC
Created attachment 154515 [details] [review]
Bug 34024: (QA follow-up) Allow pickup location changing for in transit holds

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-08-17 10:21:58 UTC
Solid fix with unit tests included. QA scripts happy. Passing QA
Comment 20 Tomás Cohen Arazi 2023-08-18 07:35:37 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 21 Fridolin Somers 2023-08-22 06:25:59 UTC
Pushed to 23.05.x for 23.05.03
Comment 22 Pedro Amorim 2023-08-28 10:22:55 UTC
Nice work everyone!

Pushed to 22.11.x for next release