Bug 24620 - Existing transfers not closed when hold is set to waiting
Summary: Existing transfers not closed when hold is set to waiting
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 24839
Blocks: 22160
  Show dependency treegraph
 
Reported: 2020-02-10 18:18 UTC by Nick Clemens
Modified: 2020-05-18 13:45 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 24620: Unit tests (2.92 KB, patch)
2020-02-10 18:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24620: Close existing transfers when setting item to waiting (1.70 KB, patch)
2020-02-10 18:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24620: Close existing transfers when setting item to waiting (1.77 KB, patch)
2020-02-11 15:18 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24620: Close existing transfers when setting item to waiting (1.82 KB, patch)
2020-02-13 10:45 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24620: Remove original call to notify reserve (804 bytes, patch)
2020-03-10 11:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24620: Fix test, remove replaced code, use dt_from_string (2.87 KB, patch)
2020-03-10 11:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24620: Unit tests (2.93 KB, patch)
2020-04-14 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24620: Close existing transfers when setting item to waiting (1.89 KB, patch)
2020-04-14 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24620: Fix test, remove replaced code, use dt_from_string (2.95 KB, patch)
2020-04-14 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24620: [19.11.x] Close existing transfer when hold set to waiting (squashed) (4.77 KB, patch)
2020-05-06 15:15 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-02-10 18:18:25 UTC
To recreate:
1 - Set AutomaticItemReturn to Do
2 - Checkin an item from Library B at Library A
3 - Confirm item is in transfer (check the details page)
4 - Place a item level hold for pickup at library A
5 - Checkin the item at Library A
6 - Confirm the hold
7 - View the details page
8 - Note the item is in transit and waiting
Comment 1 Nick Clemens 2020-02-10 18:24:03 UTC
Created attachment 98678 [details] [review]
Bug 24620: Unit tests
Comment 2 Nick Clemens 2020-02-10 18:24:05 UTC
Created attachment 98679 [details] [review]
Bug 24620: Close existing transfers when setting item to waiting

This patch adds a clause in ModReserveAffect to check if there
are existing transfers and close them when setting a hold to waiting

To test:
 1 - Set AutomaticItemReturn to Do
 2 - Checkin an item from Library B at Library A
 3 - Confirm item is in transfer (check the details page)
 4 - Place a item level hold for pickup at library A
 5 - Checkin the item at Library A
 6 - Confirm the hold
 7 - View the details page
 8 - Note the item is in transit and waiting
 9 - Apply patch
10 - Delete hold and repeat
11 - Confirm that transfer is closed when hold marked waiting
Comment 3 ByWater Sandboxes 2020-02-11 15:18:43 UTC
Created attachment 98711 [details] [review]
Bug 24620: Close existing transfers when setting item to waiting

This patch adds a clause in ModReserveAffect to check if there
are existing transfers and close them when setting a hold to waiting

To test:
 1 - Set AutomaticItemReturn to Do
 2 - Checkin an item from Library B at Library A
 3 - Confirm item is in transfer (check the details page)
 4 - Place a item level hold for pickup at library A
 5 - Checkin the item at Library A
 6 - Confirm the hold
 7 - View the details page
 8 - Note the item is in transit and waiting
 9 - Apply patch
10 - Delete hold and repeat
11 - Confirm that transfer is closed when hold marked waiting

Signed-off-by: Sally <sally.healey@cheshirewestandchester.gov.uk>
Comment 4 ByWater Sandboxes 2020-02-13 10:45:07 UTC
Created attachment 98805 [details] [review]
Bug 24620: Close existing transfers when setting item to waiting

This patch adds a clause in ModReserveAffect to check if there
are existing transfers and close them when setting a hold to waiting

To test:
 1 - Set AutomaticItemReturn to Do
 2 - Checkin an item from Library B at Library A
 3 - Confirm item is in transfer (check the details page)
 4 - Place a item level hold for pickup at library A
 5 - Checkin the item at Library A
 6 - Confirm the hold
 7 - View the details page
 8 - Note the item is in transit and waiting
 9 - Apply patch
10 - Delete hold and repeat
11 - Confirm that transfer is closed when hold marked waiting

Signed-off-by: Sally <sally.healey@cheshirewestandchester.gov.uk>

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>
Comment 5 Jonathan Druart 2020-03-10 11:06:01 UTC
Nick, did not you forget to remove the existing call to _koha_notify_reserve 2 lines before?

now we have:

1140     _koha_notify_reserve( $hold->reserve_id )
1141       if ( !$transferToDo && !$already_on_shelf );
1142 
1143     if( !$transferToDo ){
1144         _koha_notify_reserve( $hold->reserve_id ) unless $already_on_shelf;
Comment 6 Jonathan Druart 2020-03-10 11:09:02 UTC
Also:
1. ->store not needed on ->build_object
2. DateTime->now should not be used directly (or add arguments)
3. Tests are failing:
t/db_dependent//Circulation.t .. 45/47     # Looks like you planned 4 tests but ran 1.

#   Failed test 'Filling a hold should cancel existing transfer'
#   at t/db_dependent//Circulation.t line 3783.
Can't use string ("ZSpzGDUl") 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 47.
Comment 7 Nick Clemens 2020-03-10 11:12:51 UTC
Created attachment 100448 [details] [review]
Bug 24620: Remove original call to notify reserve
Comment 8 Nick Clemens 2020-03-10 11:25:31 UTC
Created attachment 100451 [details] [review]
Bug 24620: Fix test, remove replaced code, use dt_from_string
Comment 9 Jonathan Druart 2020-03-10 11:49:20 UTC
What if branchtransfers.tobranch is not the logged in user's library?
Comment 10 Nick Clemens 2020-03-10 12:33:57 UTC
(In reply to Jonathan Druart from comment #9)
> What if branchtransfers.tobranch is not the logged in user's library?

Then $transferToDo should be true and the code should not be called
Comment 11 Jonathan Druart 2020-03-10 12:46:18 UTC
(In reply to Nick Clemens from comment #10)
> (In reply to Jonathan Druart from comment #9)
> > What if branchtransfers.tobranch is not the logged in user's library?
> 
> Then $transferToDo should be true and the code should not be called

circ/branchtransfers.pl
102     ModReserveAffect( $item, $borrowernumber );
Comment 12 Nick Clemens 2020-03-17 15:25:26 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Nick Clemens from comment #10)
> > (In reply to Jonathan Druart from comment #9)
> > > What if branchtransfers.tobranch is not the logged in user's library?
> > 
> > Then $transferToDo should be true and the code should not be called
> 
> circ/branchtransfers.pl
> 102     ModReserveAffect( $item, $borrowernumber );

see bug 24839 - that code is intended to allow the transfer of a waiting hold - so cancelling when setting to waiting doesn't affect that workflow (that workflow may be problematic on its own) - the hold is set waiting, active transfers closed, then a new transfer generated
Comment 13 Jonathan Druart 2020-04-14 09:49:15 UTC
Created attachment 102914 [details] [review]
Bug 24620: Unit tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2020-04-14 09:49:18 UTC
Created attachment 102915 [details] [review]
Bug 24620: Close existing transfers when setting item to waiting

This patch adds a clause in ModReserveAffect to check if there
are existing transfers and close them when setting a hold to waiting

To test:
 1 - Set AutomaticItemReturn to Do
 2 - Checkin an item from Library B at Library A
 3 - Confirm item is in transfer (check the details page)
 4 - Place a item level hold for pickup at library A
 5 - Checkin the item at Library A
 6 - Confirm the hold
 7 - View the details page
 8 - Note the item is in transit and waiting
 9 - Apply patch
10 - Delete hold and repeat
11 - Confirm that transfer is closed when hold marked waiting

Signed-off-by: Sally <sally.healey@cheshirewestandchester.gov.uk>

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2020-04-14 09:49:21 UTC
Created attachment 102916 [details] [review]
Bug 24620: Fix test, remove replaced code, use dt_from_string

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Martin Renvoize 2020-04-14 16:23:45 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Joy Nelson 2020-05-05 22:24:09 UTC
doesn't apply to 19.11.x not backported
Comment 18 Nick Clemens 2020-05-06 15:15:03 UTC
Created attachment 104437 [details] [review]
Bug 24620: [19.11.x] Close existing transfer when hold set to waiting (squashed)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 24620: Close existing transfers when setting item to waiting

This patch adds a clause in ModReserveAffect to check if there
are existing transfers and close them when setting a hold to waiting

To test:
 1 - Set AutomaticItemReturn to Do
 2 - Checkin an item from Library B at Library A
 3 - Confirm item is in transfer (check the details page)
 4 - Place a item level hold for pickup at library A
 5 - Checkin the item at Library A
 6 - Confirm the hold
 7 - View the details page
 8 - Note the item is in transit and waiting
 9 - Apply patch
10 - Delete hold and repeat
11 - Confirm that transfer is closed when hold marked waiting

Signed-off-by: Sally <sally.healey@cheshirewestandchester.gov.uk>

Signed-off-by: Stina Hallin <stina.hallin@ub.lu.se>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Bug 24620: Fix test, remove replaced code, use dt_from_string

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Joy Nelson 2020-05-07 17:33:06 UTC
Thanks for the rebase - backported to 19.11.x for 19.11.06
Comment 20 Lucas Gass 2020-05-14 16:11:38 UTC
missing dependencies for 19.05.x no backport