Bug 30447

Summary: pendingreserves.pl is checking too many transfers
Product: Koha Reporter: Nick Clemens <nick>
Component: CirculationAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.05, 21.11.12
Bug Depends on:    
Bug Blocks: 31183, 31920    
Attachments: Bug 30447: Check if transfers arrived or cancelled
Bug 30447: Check if transfers arrived or cancelled
Bug 30447: Check if transfers arrived or cancelled
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with either datecancelled or datearrived (inclusive). I made an error with my understanding of my boolean logic.
Continued from previous patch:
Bug 30447: Check if transfers arrived or cancelled
Bug 30447: Check if transfers arrived or cancelled
Bug 30447: Unit tests
Bug 30447: Check if transfers arrived or cancelled
Bug 30447: Unit tests

Description Nick Clemens 2022-04-04 12:09:48 UTC
Bug 26057 added a 'cancellationdate' column to branchtransfers

The code in pendingreserves.pl checks against transfers like:
'itembib.itemnumber' => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL' }

This needs to be updated to:
'itembib.itemnumber' => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL AND datecancelled IS NULL' }
Comment 1 Nick Clemens 2022-04-04 12:25:16 UTC
Created attachment 132935 [details] [review]
Bug 30447: Check if transfers arrived or cancelled

TO test:
 1 - Find a bib with a single item
 2 - Browse to Circulation->Transfer
 3 - Transfer the item to another branch
 4 - Check the item in at current branch and cancel the transfer on the dialog
 5 - Place a hold on the biblio for a patron at current branch
 6 - Browse to Circulation->Holds to pull
 7 - Item does not show
 8 - Apply patch, restart all
 9 - Browse to Circulation->Holds to pull
10 - Item shows
Comment 2 Owen Leonard 2022-04-04 13:48:28 UTC
Created attachment 132941 [details] [review]
Bug 30447: Check if transfers arrived or cancelled

TO test:
 1 - Find a bib with a single item
 2 - Browse to Circulation->Transfer
 3 - Transfer the item to another branch
 4 - Check the item in at current branch and cancel the transfer on the dialog
 5 - Place a hold on the biblio for a patron at current branch
 6 - Browse to Circulation->Holds to pull
 7 - Item does not show
 8 - Apply patch, restart all
 9 - Browse to Circulation->Holds to pull
10 - Item shows

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Jonathan Druart 2022-04-06 14:42:36 UTC
Can you provide a test please?
Comment 4 Michal Urban 2022-06-16 16:25:02 UTC
Created attachment 136184 [details] [review]
Bug 30447: Check if transfers arrived or cancelled

TO test:
 1 - Find a bib with a single item
 2 - Browse to Circulation->Transfer
 3 - Transfer the item to another branch
 4 - Check the item in at current branch and cancel the transfer on the dialog
 5 - Place a hold on the biblio for a patron at current branch
 6 - Browse to Circulation->Holds to pull
 7 - Item does not show
 8 - Apply patch, restart all
 9 - Browse to Circulation->Holds to pull
10 - Item shows

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 Michal Urban 2022-06-16 16:25:06 UTC
Created attachment 136185 [details] [review]
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived
Comment 6 Michal Urban 2022-06-16 17:05:18 UTC
Created attachment 136186 [details] [review]
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived
Comment 7 Michal Urban 2022-06-16 17:05:21 UTC
Created attachment 136187 [details] [review]
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with either datecancelled or datearrived (inclusive). I made an error with my understanding of my boolean logic.

Also cleaned up the variables in the test by removing unblessed and using
the variable base forms.
Comment 8 Michal Urban 2022-06-16 17:25:57 UTC
Created attachment 136198 [details] [review]
Continued from previous patch:

Added test for correct number of items for each call of "get_items_that_fill"
Comment 9 Michal Urban 2022-06-16 18:25:06 UTC
Created attachment 136199 [details] [review]
Bug 30447: Check if transfers arrived or cancelled

TO test:
 1 - Find a bib with a single item
 2 - Browse to Circulation->Transfer
 3 - Transfer the item to another branch
 4 - Check the item in at current branch and cancel the transfer on the dialog
 5 - Place a hold on the biblio for a patron at current branch
 6 - Browse to Circulation->Holds to pull
 7 - Item does not show
 8 - Apply patch, restart all
 9 - Browse to Circulation->Holds to pull
10 - Item shows

Test in Holds.t

Added test to ensure that Koha::Holds->get_items_that_can_fill returns
 items with datecancelled and datearrived

Also cleaned up the variables in the test by removing unblessed and using
the variable base forms.
Comment 10 Nick Clemens 2022-06-16 18:36:03 UTC
Created attachment 136200 [details] [review]
Bug 30447: Check if transfers arrived or cancelled

TO test:
 1 - Find a bib with a single item
 2 - Browse to Circulation->Transfer
 3 - Transfer the item to another branch
 4 - Check the item in at current branch and cancel the transfer on the dialog
 5 - Place a hold on the biblio for a patron at current branch
 6 - Browse to Circulation->Holds to pull
 7 - Item does not show
 8 - Apply patch, restart all
 9 - Browse to Circulation->Holds to pull
10 - Item shows

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Nick Clemens 2022-06-16 18:36:06 UTC
Created attachment 136201 [details] [review]
Bug 30447: Unit tests

Added tests to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived
Added test for correct number of items for each call of "get_items_that_fill"
Comment 12 Jonathan Druart 2022-06-17 06:48:27 UTC
Shouldn't we want both using the same code (a filter_by method in Koha::Item::Transfers)?
Comment 13 Nick Clemens 2022-06-17 11:51:30 UTC
(In reply to Jonathan Druart from comment #12)
> Shouldn't we want both using the same code (a filter_by method in
> Koha::Item::Transfers)?

Yes, that would be a good goal. I will move that patch forward when you write it ;-) 

This one fixes the bug and backports nicely for now, can it move forward?
Comment 14 Martin Renvoize 2022-07-14 09:50:51 UTC
Created attachment 137704 [details] [review]
Bug 30447: Check if transfers arrived or cancelled

TO test:
 1 - Find a bib with a single item
 2 - Browse to Circulation->Transfer
 3 - Transfer the item to another branch
 4 - Check the item in at current branch and cancel the transfer on the dialog
 5 - Place a hold on the biblio for a patron at current branch
 6 - Browse to Circulation->Holds to pull
 7 - Item does not show
 8 - Apply patch, restart all
 9 - Browse to Circulation->Holds to pull
10 - Item shows

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-07-14 09:50:55 UTC
Created attachment 137705 [details] [review]
Bug 30447: Unit tests

Added tests to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived
Added test for correct number of items for each call of "get_items_that_fill"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-07-14 09:52:13 UTC
Good catch.

I've still got a second phase of transfers cleanup I'll get back to some time soon and will make sure we add filter methods there so I don't think we need to hold back this simple to backport version.

Works as expected and has tests.

Passing QA
Comment 17 Tomás Cohen Arazi 2022-07-18 21:54:16 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 18 Lucas Gass 2022-08-23 19:36:06 UTC
Backported to 22.05.x for 22.05.05
Comment 19 Arthur Suzuki 2022-08-30 09:26:17 UTC
thx pushed to 21.11.x for 21.11.12
Comment 20 Victor Grousset/tuxayo 2022-09-14 19:23:47 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.