[Bug 34024] New: REST API should not allow changing the pickup location on a waiting hold
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Bug ID: 34024 Summary: REST API should not allow changing the pickup location on a waiting hold Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: REST API Assignee: koha-bugs@lists.koha-community.org Reporter: emily.lamancusa@montgomerycountymd.gov CC: tomascohen@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Summary|REST API should not allow |REST API should not allow |changing the pickup |changing the pickup |location on a waiting hold |location on found holds --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- 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. -- 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=34024 --- Comment #2 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- 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. -- 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=34024 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- 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=34024 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 154105 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154105&action=edit Bug 34024: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 154106 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154106&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #6 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com, | |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Sam Lau <samalau@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Sam Lau <samalau@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154106|0 |1 is obsolete| | --- Comment #8 from Sam Lau <samalau@gmail.com> --- Created attachment 154126 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154126&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Sam Lau <samalau@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154105|0 |1 is obsolete| | --- Comment #9 from Sam Lau <samalau@gmail.com> --- Created attachment 154127 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154127&action=edit Bug 34024: Unit tests Signed-off-by: Sam Lau <samalau@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #10 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Felicity Brown <felicity.brown@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |felicity.brown@montgomeryco | |untymd.gov --- Comment #11 from Felicity Brown <felicity.brown@montgomerycountymd.gov> --- I second "allowing a staff member to mess with transit data as a workaround is riskier." -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 154132 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154132&action=edit Bug 34024: (QA follow-up) Allow pickup location changing for in transit holds Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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?) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=34458 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Rebecca Coert <rcoert@arlingtonva.us> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |rcoert@arlingtonva.us -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154126|0 |1 is obsolete| | --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 154513 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154513&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154127|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 154514 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154514&action=edit Bug 34024: Unit tests Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154132|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 154515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154515&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |martin.renvoize@ptfs-europe | |.com CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Passed QA --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Solid fix with unit tests included. QA scripts happy. Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable Version(s)|23.11.00 |23.11.00,23.05.03 released in| | --- Comment #21 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00,23.05.03 |23.11.00,23.05.03,22.11.09 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34024 --- Comment #22 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Nice work everyone! Pushed to 22.11.x for next release -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org