|
Description
Emily Lamancusa (emlam)
2023-06-15 14:51:45 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. 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. (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? Created attachment 154105 [details] [review] Bug 34024: Unit tests 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 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. (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 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> Created attachment 154127 [details] [review] Bug 34024: Unit tests Signed-off-by: Sam Lau <samalau@gmail.com> 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. I second "allowing a staff member to mess with transit data as a workaround is riskier." (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. 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> 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 (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?) 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> 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> 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> Solid fix with unit tests included. QA scripts happy. Passing QA Pushed to master for 23.11. Nice work everyone, thanks! Pushed to 23.05.x for 23.05.03 Nice work everyone! Pushed to 22.11.x for next release |