Bug 21944 - Fix waiting holds at wrong location bug
Summary: Fix waiting holds at wrong location bug
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 21346
Blocks: 23070
  Show dependency treegraph
 
Reported: 2018-12-04 16:28 UTC by Christopher Brannon
Modified: 2020-03-18 17:05 UTC (History)
14 users (show)

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


Attachments
Bug 21944: Reroute waiting hold from wrong branch (2.01 KB, patch)
2018-12-04 16:43 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (1.91 KB, patch)
2018-12-05 00:07 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (1.86 KB, patch)
2018-12-13 00:10 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (2.00 KB, patch)
2019-02-04 20:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (2.00 KB, patch)
2019-02-04 20:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21944: Reroute waiting hold from wrong branch (2.19 KB, patch)
2019-02-04 20:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (1.84 KB, patch)
2019-09-05 15:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21944: Reroute waiting hold from wrong branch (2.17 KB, patch)
2019-09-05 15:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (1.90 KB, patch)
2019-10-15 16:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21944: Reroute waiting hold from wrong branch (2.23 KB, patch)
2019-10-15 16:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (1.97 KB, patch)
2019-12-03 16:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21944: Reroute waiting hold from wrong branch (2.30 KB, patch)
2019-12-03 16:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21944: Improve efficiency of code (4.00 KB, patch)
2019-12-03 16:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (2.02 KB, patch)
2020-02-06 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21944: Reroute waiting hold from wrong branch (2.35 KB, patch)
2020-02-06 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21944: Improve efficiency of code (4.04 KB, patch)
2020-02-06 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21944: Test for Circulation.pm (2.09 KB, patch)
2020-02-18 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21944: Reroute waiting hold from wrong branch (2.42 KB, patch)
2020-02-18 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21944: Improve efficiency of code (4.11 KB, patch)
2020-02-18 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2018-12-04 16:28:21 UTC
To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
Comment 1 Christopher Brannon 2018-12-04 16:43:01 UTC
Created attachment 82848 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.
Comment 2 Christopher Brannon 2018-12-04 16:56:57 UTC
Working on a unit test update.
Comment 3 Christopher Brannon 2018-12-05 00:07:21 UTC
Created attachment 82852 [details] [review]
Bug 21944: Test for Circulation.pm
Comment 4 Christopher Brannon 2018-12-13 00:10:44 UTC
Created attachment 83144 [details] [review]
Bug 21944: Test for Circulation.pm
Comment 5 Christopher Brannon 2019-01-25 19:10:57 UTC
Comment on attachment 83144 [details] [review]
Bug 21944: Test for Circulation.pm

[11:01] <kidclamp> your tests look lright - except that you call ModReserveAffect
[11:02] <kidclamp> the test shoudl be to do AddReturn on the waiting hold as if it is at another branch
[11:02] <kidclamp> the tests are thenjust checks to verify the situation is as you expect it
Comment 6 Christopher Brannon 2019-02-04 20:01:26 UTC
(In reply to Christopher Brannon from comment #5)
> Comment on attachment 83144 [details] [review] [review]
> Bug 21944: Test for Circulation.pm
> 
> [11:01] <kidclamp> your tests look lright - except that you call
> ModReserveAffect
> [11:02] <kidclamp> the test shoudl be to do AddReturn on the waiting hold as
> if it is at another branch
> [11:02] <kidclamp> the tests are thenjust checks to verify the situation is
> as you expect it

Nick,
The question is, how do I test the branch destination?  Essentially the test needs to check in an item that is waiting at a branch other than the branch it is waiting at, and see if the item is put back into transit and the destination is set for the pickup branch.  I am not sure how to do this.
Comment 7 Nick Clemens 2019-02-04 20:48:40 UTC
Created attachment 84752 [details] [review]
Bug 21944: Test for Circulation.pm
Comment 8 Nick Clemens 2019-02-04 20:48:57 UTC
Created attachment 84753 [details] [review]
Bug 21944: Test for Circulation.pm
Comment 9 Nick Clemens 2019-02-04 20:49:00 UTC
Created attachment 84754 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.
Comment 10 Nick Clemens 2019-02-04 20:53:07 UTC
(In reply to Christopher Brannon from comment #6)
> 
> Nick,
> The question is, how do I test the branch destination?  Essentially the test
> needs to check in an item that is waiting at a branch other than the branch
> it is waiting at, and see if the item is put back into transit and the
> destination is set for the pickup branch.  I am not sure how to do this.

Your tests need to cover the change you make. They should illustrate the expected behavior of the subroutine. In this case: When AddReturn is called from branch B on an item that is a waiting hold at branch A then the hold should now be marked as 'not waiting' and the message returned should reflect the hold's new status

You should (ideally) write your tests first to show how the current code is problematic, then change the subroutine to match the expectation of the tests.

In this case you need to check that the hold is not waiting and the priority is set - also that the message returned indicates these values

I added new tests and also switched the code to use 'RevertWaitingStatus' since that method does exactly what we want

Check these out and let me know if they make sense
Comment 11 Magnus Enger 2019-09-05 13:10:21 UTC
Any chance we could get this moving again?
Comment 12 Christopher Brannon 2019-09-05 14:43:44 UTC
(In reply to Magnus Enger from comment #11)
> Any chance we could get this moving again?

I tried.  My patch worked, but I couldn't figure out how to write the test.  It's been a while, so correct me if I am wrong, but Nick's test may work, but his patch doesn't, when I look at the actual results.

I gave up because the testing was too complicated, and I couldn't wrap my head around it.
Comment 13 Nick Clemens 2019-09-05 15:23:22 UTC
Created attachment 92609 [details] [review]
Bug 21944: Test for Circulation.pm
Comment 14 Nick Clemens 2019-09-05 15:23:27 UTC
Created attachment 92610 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.
Comment 15 Nick Clemens 2019-09-05 15:24:15 UTC
Patches rebased and retested, they work for me, can you test and explain the problem Christopher?
Comment 16 Magnus Enger 2019-09-06 11:35:22 UTC
The patch works as advertised for me, but I get a bunch of errors when I try to run the tests: 

$ sudo koha-shell -c "prove t/db_dependent/Circulation.t" kohadev
t/db_dependent/Circulation.t .. 1/45 
        #   Failed test 'GetLatestAutoRenewDate should return -5 days if no_auto_renewal_after = 5 and date_due is 10 days before'
        #   at t/db_dependent/Circulation.t line 862.
        #          got: '2019-09-01T11:32:00'
        #     expected: '2019-09-01T11:32:00'

        #   Failed test 'GetLatestAutoRenewDate should return +5 days if no_auto_renewal_after = 15 and date_due is 10 days before'
        #   at t/db_dependent/Circulation.t line 869.
        #          got: '2019-09-11T11:32:00'
        #     expected: '2019-09-11T11:32:00'

        #   Failed test 'GetLatestAutoRenewDate should return +2 days if no_auto_renewal_after_hard_limit is defined and not no_auto_renewal_after'
        #   at t/db_dependent/Circulation.t line 876.
        #          got: '2019-09-08T00:00:00'
        #     expected: '2019-09-08T00:00:00'

        #   Failed test 'GetLatestAutoRenewDate should return +2 days if no_auto_renewal_after_hard_limit is < no_auto_renewal_after'
        #   at t/db_dependent/Circulation.t line 882.
        #          got: '2019-09-08T00:00:00'
        #     expected: '2019-09-08T00:00:00'
        # Looks like you failed 4 tests of 5.

    #   Failed test 'GetLatestAutoRenewDate'
    #   at t/db_dependent/Circulation.t line 887.
    # Looks like you failed 1 test of 71.
t/db_dependent/Circulation.t .. 15/45 
#   Failed test 'CanBookBeRenewed tests'
#   at t/db_dependent/Circulation.t line 998.
t/db_dependent/Circulation.t .. 18/45 
    #   Failed test 'There should not be any errors or alerts (impossible) (error: EXPIRED) (question: ISSUED_TO_ANOTHER issued_firstname issued_borrowernumber issued_surname issued_cardnumber)'
    #   at t/db_dependent/Circulation.t line 1326.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'There should not be any errors or alerts (impossible) (error: EXPIRED) (question: issued_surname issued_borrowernumber issued_cardnumber issued_firstname ISSUED_TO_ANOTHER)'
    #   at t/db_dependent/Circulation.t line 1331.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'There should not be any errors or alerts (impossible) (error: EXPIRED) (question: issued_firstname ISSUED_TO_ANOTHER issued_cardnumber issued_borrowernumber issued_surname)'
    #   at t/db_dependent/Circulation.t line 1335.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'There should not be any errors or alerts (impossible) (error: EXPIRED) (question: ISSUED_TO_ANOTHER issued_firstname issued_borrowernumber issued_surname issued_cardnumber)'
    #   at t/db_dependent/Circulation.t line 1349.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'There should not be any errors or alerts (impossible) (error: EXPIRED) (question: issued_firstname ISSUED_TO_ANOTHER issued_cardnumber issued_surname issued_borrowernumber)'
    #   at t/db_dependent/Circulation.t line 1363.
    #          got: '1'
    #     expected: '0'
    # Looks like you failed 5 tests of 24.
t/db_dependent/Circulation.t .. 22/45 
#   Failed test 'CanBookBeIssued & AllowReturnToBranch'
#   at t/db_dependent/Circulation.t line 1379.
t/db_dependent/Circulation.t .. 23/45 
    #   Failed test 'No key for error and alert (error: EXPIRED) (question: USERBLOCKEDOVERDUE)'
    #   at t/db_dependent/Circulation.t line 1489.
    #          got: '1'
    #     expected: '0'
    # Looks like you failed 1 test of 8.
t/db_dependent/Circulation.t .. 24/45 
#   Failed test 'CanBookBeIssued + Koha::Patron->is_debarred|has_overdues'
#   at t/db_dependent/Circulation.t line 1508.
t/db_dependent/Circulation.t .. 25/45 
    #   Failed test 'No error or alert should be raised (error: EXPIRED) (question: BIBLIO_ALREADY_ISSUED)'
    #   at t/db_dependent/Circulation.t line 1674.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'No BIBLIO_ALREADY_ISSUED flag should be set if AllowMultipleIssuesOnABiblio=1 (error: EXPIRED)'
    #   at t/db_dependent/Circulation.t line 1679.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'No BIBLIO_ALREADY_ISSUED flag should be set if it is a subscription (error: EXPIRED)'
    #   at t/db_dependent/Circulation.t line 1686.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'No BIBLIO_ALREADY_ISSUED flag should be set if it is a subscription (error: EXPIRED)'
    #   at t/db_dependent/Circulation.t line 1690.
    #          got: '1'
    #     expected: '0'
    # Looks like you failed 4 tests of 5.
t/db_dependent/Circulation.t .. 27/45 
#   Failed test 'CanBookBeIssued + AllowMultipleIssuesOnABiblio'
#   at t/db_dependent/Circulation.t line 1691.
t/db_dependent/Circulation.t .. 38/45 
    #   Failed test 'Item can be issued to this patron'
    #   at t/db_dependent/Circulation.t line 2980.
    #     Structures begin differing at:
    #          $got->{EXPIRED} = '1'
    #     $expected->{EXPIRED} = Does not exist
    # Looks like you failed 1 test of 2.

#   Failed test 'CanBookBeIssued | item-level_itypes=biblio'
#   at t/db_dependent/Circulation.t line 2981.

        #   Failed test 'Item can be issued to this patron'
        #   at t/db_dependent/Circulation.t line 3025.
        #     Structures begin differing at:
        #          $got->{EXPIRED} = '1'
        #     $expected->{EXPIRED} = Does not exist

        #   Failed test 'Item can not be issued, not for loan at item type level'
        #   at t/db_dependent/Circulation.t line 3034.
        #     Structures begin differing at:
        #          $got->{EXPIRED} = '1'
        #     $expected->{EXPIRED} = Does not exist

        #   Failed test 'Item can not be issued, not for loan at item type level'
        #   at t/db_dependent/Circulation.t line 3042.
        #     Structures begin differing at:
        #          $got->{EXPIRED} = '1'
        #     $expected->{EXPIRED} = Does not exist
        # Looks like you failed 3 tests of 6.

    #   Failed test 'item-level_itypes = 1'
    #   at t/db_dependent/Circulation.t line 3047.

        #   Failed test 'Item can be issued to this patron'
        #   at t/db_dependent/Circulation.t line 3067.
        #     Structures begin differing at:
        #          $got->{EXPIRED} = '1'
        #     $expected->{EXPIRED} = Does not exist

        #   Failed test 'Item can not be issued, not for loan at item type level'
        #   at t/db_dependent/Circulation.t line 3076.
        #     Structures begin differing at:
        #          $got->{EXPIRED} = '1'
        #     $expected->{EXPIRED} = Does not exist

        #   Failed test 'Item can not be issued, not for loan at item type level'
        #   at t/db_dependent/Circulation.t line 3084.
        #     Structures begin differing at:
        #          $got->{EXPIRED} = '1'
        #     $expected->{EXPIRED} = Does not exist
        # Looks like you failed 3 tests of 6.

    #   Failed test 'item-level_itypes = 0'
    #   at t/db_dependent/Circulation.t line 3089.
    # Looks like you failed 2 tests of 2.
t/db_dependent/Circulation.t .. 40/45 
#   Failed test 'CanBookBeIssued | notforloan'
#   at t/db_dependent/Circulation.t line 3092.
t/db_dependent/Circulation.t .. 45/45 # Looks like you failed 6 tests of 45.
t/db_dependent/Circulation.t .. Dubious, test returned 6 (wstat 1536, 0x600)
Failed 6/45 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 1536 Tests: 45 Failed: 6)
  Failed tests:  15, 22, 24, 27, 39-40
  Non-zero exit status: 6
Files=1, Tests=45, 37 wallclock secs ( 0.07 usr  0.04 sys + 26.04 cusr  4.54 csys = 30.69 CPU)
Result: FAIL
Comment 17 Nick Clemens 2019-09-06 12:26:44 UTC
(In reply to Magnus Enger from comment #16)
> The patch works as advertised for me, but I get a bunch of errors when I try
> to run the tests: 

I cannot recreate these failures, 

I do note that I get some failures if I have a timezone set in koha-conf.xml - if I remove that all pass, but that is true without my patch as well
Comment 18 Magnus Enger 2019-09-06 12:47:54 UTC
The first set of failures looks weird: 

        #          got: '2019-09-08T00:00:00'
        #     expected: '2019-09-08T00:00:00'

Are they not identical?
Comment 19 Magnus Enger 2019-09-06 12:52:23 UTC
...and it looks like I get the same errors from running this:
$ sudo koha-shell -c "prove t/db_dependent/Circulation.t" kohadev
on current master (without the patches on this bug) as I get from running it *with* the patches. So those failures are probably not related to these patches.
Comment 20 Martin Renvoize 2019-10-15 16:15:27 UTC
Created attachment 94220 [details] [review]
Bug 21944: Test for Circulation.pm

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2019-10-15 16:15:31 UTC
Created attachment 94221 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-10-15 16:15:54 UTC
Seems to work as described, Signing off
Comment 23 Jonathan Druart 2019-10-24 09:58:11 UTC
(In reply to Magnus Enger from comment #19)
> ...and it looks like I get the same errors from running this:
> $ sudo koha-shell -c "prove t/db_dependent/Circulation.t" kohadev
> on current master (without the patches on this bug) as I get from running it
> *with* the patches. So those failures are probably not related to these
> patches.

Yes, see bug 23234. I get those ones as well.
Comment 24 Marcel de Rooy 2019-11-01 07:48:53 UTC
QAing
Comment 25 Marcel de Rooy 2019-11-01 08:19:47 UTC
    not ok 19 - AddReturn must have debarred the patron
    #   Failed test 'AddReturn must have debarred the patron'
    #   at t/db_dependent/Circulation.t line 86.
    #          got: ''
    #     expected: '1'
    # AddReturn returned message $VAR1 = {
    #           'WasReturned' => 1
    #         };
    not ok 20 - Test at line 1922
    #   Failed test 'Test at line 1922'
    #   at t/db_dependent/Circulation.t line 90.
    #          got: '0'
    #     expected: '1'
    not ok 21 - Test at line 1922
    #   Failed test 'Test at line 1922'
    #   at t/db_dependent/Circulation.t line 93.
    #          got: undef
    #     expected: '2019-11-15'
    # Looks like you failed 3 tests of 21.
not ok 29 - AddReturn + suspension_chargeperiod
#   Failed test 'AddReturn + suspension_chargeperiod'
#   at t/db_dependent/Circulation.t line 1931.
# Looks like you failed 1 test of 45.

First time I ran Circulation.t, I got this. But I do not manage to reproduce it another time ;) And it seems unrelated. No holds stuff there.
Comment 26 Marcel de Rooy 2019-11-01 08:36:54 UTC
    ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->itemnumber, undef, $lookahead ) unless ( $item->withdrawn );
    # if a hold is found and is waiting at another branch, change the priority back to 1 and trigger the hold (this will trigger a transfer and update the hold status properly)
    if ( $resfound eq "Waiting" and $branch ne $resrec->{branchcode} ) {
        C4::Reserves::RevertWaitingStatus( { itemnumber => $item->itemnumber } );
        #If the hold is reverted we need to refetch for the return values
        ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->itemnumber, undef, $lookahead ) unless ( $item->withdrawn );
    }
    ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->itemnumber, undef, $lookahead ) unless ( $item->withdrawn );

In short what do we see here:
call CheckReserves
if(  )
    call CheckReserves
endif
call CheckReserves
Three times in a row ? Please explain why you also add the last call. Rebase problem?
Comment 27 Kyle M Hall 2019-12-03 16:03:52 UTC
Created attachment 95950 [details] [review]
Bug 21944: Test for Circulation.pm

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2019-12-03 16:04:03 UTC
Created attachment 95951 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2019-12-03 16:04:06 UTC
Created attachment 95952 [details] [review]
Bug 21944: Improve efficiency of code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Jonathan Druart 2019-12-05 11:30:46 UTC
Christopher, could you add your stamp on Kyle's follow-up?
Comment 31 Christopher Brannon 2019-12-12 15:32:19 UTC
(In reply to Jonathan Druart from comment #30)
> Christopher, could you add your stamp on Kyle's follow-up?

I tested master without the patch, and the hold is routed properly.  Something has changed.  I don't know if it is Owen's work on the modal, but it seems to be working properly without the patch.  Can anyone confirm?
Comment 32 Nick Clemens 2019-12-13 20:06:25 UTC
The unit tests still fail without the patches, possibly we are handling something in the templates to work around this, but we should still correct the underlying issues.

Would you be willing to test with the patches Christopher to ensure they do not break anything?
Comment 33 Lisette Scheer 2020-01-15 21:24:05 UTC
With the patches appled, at step 4 in the test plan when I confirm the transfer, I get this: 

Can't call method "biblio" on an undefined value at /kohadevbox/koha/circ/returns.pl line 158 


It does seem to set the item back to a record level hold but the expiration date persists and it doesn't set it into transit, just sets the current location to be the branch it was scanned into most recently. 

Lisette
Comment 34 Benjamin Daeuber 2020-02-04 15:42:44 UTC
(In reply to Christopher Brannon from comment #31)
> (In reply to Jonathan Druart from comment #30)
> > Christopher, could you add your stamp on Kyle's follow-up?
> 
> I tested master without the patch, and the hold is routed properly. 
> Something has changed.  I don't know if it is Owen's work on the modal, but
> it seems to be working properly without the patch.  Can anyone confirm?

The problem is still occurring on master for me. However, I am getting the same error as Lisette when testing the patch.
Comment 35 Nick Clemens 2020-02-05 13:23:50 UTC
The problem still exists on master in my testing, these patches fix the problem

I cannot recreate the error encountered by Ben or Lisette, these patches do not touch returns.pl
Comment 36 Martin Renvoize 2020-02-06 12:34:01 UTC
Created attachment 98528 [details] [review]
Bug 21944: Test for Circulation.pm

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2020-02-06 12:34:05 UTC
Created attachment 98529 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2020-02-06 12:34:09 UTC
Created attachment 98530 [details] [review]
Bug 21944: Improve efficiency of code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2020-02-06 12:34:34 UTC
Back to signed off.. I can't replicate the issues either..
Comment 40 Jonathan Druart 2020-02-18 10:39:12 UTC
Created attachment 99165 [details] [review]
Bug 21944: Test for Circulation.pm

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2020-02-18 10:39:16 UTC
Created attachment 99166 [details] [review]
Bug 21944: Reroute waiting hold from wrong branch

To Test:
1) Place a hold on an item.
2) Check in the item to trigger the hold.  Item is now listed as
waiting.
3) Set branch to a different library.
4) Check in the item to trigger the hold.  Notice that nothing but a
local use is recorded.
5) Apply the patch.
6) Repeat steps 1-4.  Notice that the hold is triggered around routed to
its original pickup location.  A transfer is set and the hold status
is changed from waiting to correctly showing the item as in transit.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2020-02-18 10:39:20 UTC
Created attachment 99167 [details] [review]
Bug 21944: Improve efficiency of code

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Martin Renvoize 2020-02-19 11:34:15 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 44 Joy Nelson 2020-03-13 00:16:24 UTC
Pushed to 19.11.x branch for 19.11.04
Comment 45 Lucas Gass 2020-03-18 17:05:39 UTC
this patchset does not cleanly apply to 19.05.x, no backport 

if this is wanted/needed please rebase