Bug 22379

Summary: ILS-DI Method "CancelHold" doesn't check CanReserveBeCanceledFromOpac
Product: Koha Reporter: Arthur Suzuki <arthur.suzuki>
Component: Web servicesAssignee: Arthur Suzuki <arthur.suzuki>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: arthur.suzuki, david, kyle, martin.renvoize, nick, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Martin Renvoize Documentation submission:
Text to go in the release notes:
This adds checks so that borrowers can't cancel holds that have a transit or waiting status when using the ILS-DI web service's CancelHold method. These checks now reflect the same behaviour seen on the OPAC.
Version(s) released in:
22.05.00,21.11.06
Attachments: Bug 22379 : Fix checks not made by ILS-DI method CancelHold
Bug 22379 : Fix checks not made by ILS-DI method CancelHold
Bug 22379 : Fix checks not made by ILS-DI method CancelHold
Bug 22379: Fix checks not made by ILS-DI method CancelHold
Bug 22379: Unit tests for CancelHold service
Bug 22379: Fix checks not made by ILS-DI method CancelHold
Bug 22379: Unit tests for CancelHold service
Bug 22379: Unit tests for CancelHold service
Bug 22379: Fix checks not made by ILS-DI method CancelHold
Bug 22379: Unit tests for CancelHold service

Description Arthur Suzuki 2019-02-20 20:05:43 UTC
Requests to cancel a hold done through ILS-DI CancelHold method do not check same conditions as cancellation done through the OPAC interface.

Here are the conditions (extract from C4/Reserves.pm) :
Returns 1 if reserve can be cancelled by user from OPAC.
First check if reserve belongs to user, next checks if reserve is not in transfer or waiting status
Comment 1 Arthur Suzuki 2019-02-20 20:21:54 UTC
Created attachment 85413 [details] [review]
Bug 22379 : Fix checks not made by ILS-DI method CancelHold

Disable the possibility for a borrower to cancel a reservation
which is either in a Transit or Waiting state.
This reproduce the behaviour seen on the OPAC.
Also replaces previous checks on the borrowernumber
since CanReserveBeCanceledFromOpac already checks this.
Comment 2 Arthur Suzuki 2019-02-20 20:37:42 UTC
Created attachment 85414 [details] [review]
Bug 22379 : Fix checks not made by ILS-DI method CancelHold

Disable the possibility for a borrower to cancel a reservation
which is either in a Transit or Waiting state.
This reproduce the behaviour seen on the OPAC.
Also replaces previous checks on the borrowernumber
since CanReserveBeCanceledFromOpac already checks this.

--------------------------------

Test plan (before patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "RecordNotFound"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

--------------------------------

Test plan (after patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.
Comment 3 Arthur Suzuki 2019-10-18 07:31:45 UTC
Created attachment 94387 [details] [review]
Bug 22379 : Fix checks not made by ILS-DI method CancelHold

Disable the possibility for a borrower to cancel a reservation
which is either in a Transit or Waiting state.
This reproduce the behaviour seen on the OPAC.
Also replaces previous checks on the borrowernumber
since CanReserveBeCanceledFromOpac already checks this.

--------------------------------

Test plan (before patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "RecordNotFound"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

--------------------------------

Test plan (after patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Comment 4 Nick Clemens 2019-10-25 15:45:50 UTC
Unit tests please
Comment 5 Arthur Suzuki 2020-01-27 04:46:16 UTC
Created attachment 97959 [details] [review]
Bug 22379: Fix checks not made by ILS-DI method CancelHold

Disable the possibility for a borrower to cancel a reservation
which is either in a Transit or Waiting state.
This reproduce the behaviour seen on the OPAC.
Also replaces previous checks on the borrowernumber
since CanReserveBeCanceledFromOpac already checks this.

--------------------------------

Test plan (before patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "RecordNotFound"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

--------------------------------

Test plan (after patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Comment 6 Arthur Suzuki 2020-01-27 04:46:41 UTC
Created attachment 97960 [details] [review]
Bug 22379: Unit tests for CancelHold service
Comment 7 David Nind 2020-03-01 04:09:21 UTC
I'm happy to test if you could provide some example URLs for ILS-DI requests and the required parameters.

Notes:
- The patch still applies
- Tests don't pass:

root@722db1e0af2e:koha(bz22379)$ prove t/db_dependent/ILSDI_Services.t
t/db_dependent/ILSDI_Services.t .. 6/10 Use of uninitialized value $charge in numeric gt (>) at /kohadevbox/koha/C4/Circulation.pm line 2991.
t/db_dependent/ILSDI_Services.t .. 8/10     # No tests run!
t/db_dependent/ILSDI_Services.t .. 9/10 
#   Failed test 'No tests run for subtest "CancelHold"'
#   at t/db_dependent/ILSDI_Services.t line 698.
Can't use string ("Su0VSFAs") as a HASH ref while "strict refs" in use at /kohadevbox/koha/C4/Reserves.pm line 175.
# Looks like your test exited with 255 just after 9.
t/db_dependent/ILSDI_Services.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/10 subtests 

Test Summary Report
-------------------
t/db_dependent/ILSDI_Services.t (Wstat: 65280 Tests: 9 Failed: 1)
  Failed test:  9
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 10 tests but ran 9.
Files=1, Tests=9, 14 wallclock secs ( 0.04 usr  0.00 sys + 11.33 cusr  1.57 csys = 12.94 CPU)
Result: FAIL
Comment 8 Arthur Suzuki 2021-09-24 09:07:31 UTC
Created attachment 125230 [details] [review]
Bug 22379: Fix checks not made by ILS-DI method CancelHold

Disable the possibility for a borrower to cancel a reservation
which is either in a Transit or Waiting state.
This reproduce the behaviour seen on the OPAC.
Also replaces previous checks on the borrowernumber
since CanReserveBeCanceledFromOpac already checks this.

--------------------------------

Test plan (before patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "RecordNotFound"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

--------------------------------

Test plan (after patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Comment 9 Arthur Suzuki 2021-09-24 09:07:47 UTC
Created attachment 125231 [details] [review]
Bug 22379: Unit tests for CancelHold service
Comment 10 Arthur Suzuki 2021-09-24 09:10:17 UTC
Hello,
I've rebased and reuploaded the patches.
I also have tests failing but they seem to fail also on master :
<pre>
    #   Failed test 'Record hold, Item cannot be transferred'
    #   at t/db_dependent/ILSDI_Services.t line 558.
    #          got: undef
    #     expected: 'cannotBeTransferred'
</pre>

Hope this will pass QA now!
Arthur
Comment 11 Arthur Suzuki 2021-09-24 09:13:51 UTC
Created attachment 125232 [details] [review]
Bug 22379: Unit tests for CancelHold service
Comment 12 Martin Renvoize 2022-04-26 14:08:07 UTC
Created attachment 133903 [details] [review]
Bug 22379: Fix checks not made by ILS-DI method CancelHold

Disable the possibility for a borrower to cancel a reservation
which is either in a Transit or Waiting state.
This reproduce the behaviour seen on the OPAC.
Also replaces previous checks on the borrowernumber
since CanReserveBeCanceledFromOpac already checks this.

--------------------------------

Test plan (before patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "RecordNotFound"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

--------------------------------

Test plan (after patch) :
-Put a reserve for a borrower
-Try to cancel the reserve providing another borrowernumber as argument
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Try to cancel the reserve providing the borrowernumber the reserve is
for.
	-> Should succeed, reply with "Canceled"
	-> Reserve do not show up in the list of holds for the borrower

-Put a new reserve with a pickup branch != from the homebranch
-Transfer the item to the pickup branch (reserve status = Transit)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

-Checkout the reserved item in the pickup branch (reserve status =
Waiting)
-Try to cancel the reserve (with proper borrowernumber)
	-> Should fail and reply "BorrowerCannotCancelHold"
	-> Reserve still appears in the list of holds.

Signed-off-by: Laurence Rault <laurence.rault@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-04-26 14:08:11 UTC
Created attachment 133904 [details] [review]
Bug 22379: Unit tests for CancelHold service

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2022-04-26 14:08:49 UTC
Nice one Arthur, great to see these getting some love

All working, Unit tests reads well and passes.. QA script happy

PQA
Comment 15 Fridolin Somers 2022-05-02 21:24:02 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 16 Kyle M Hall 2022-05-13 15:50:18 UTC
Pushed to 21.11.x for 21.11.06
Comment 17 Victor Grousset/tuxayo 2022-06-23 01:45:23 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.
Comment 18 Martin Renvoize 2022-06-23 13:48:30 UTC
API code correction, no documentation change required.