Bug 30447 - pendingreserves.pl is checking too many transfers
Summary: pendingreserves.pl is checking too many transfers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 31183 31920
  Show dependency treegraph
 
Reported: 2022-04-04 12:09 UTC by Nick Clemens
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
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


Attachments
Bug 30447: Check if transfers arrived or cancelled (2.04 KB, patch)
2022-04-04 12:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30447: Check if transfers arrived or cancelled (2.09 KB, patch)
2022-04-04 13:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 30447: Check if transfers arrived or cancelled (2.09 KB, patch)
2022-06-16 16:25 UTC, Michal Urban
Details | Diff | Splinter Review
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived (5.69 KB, patch)
2022-06-16 16:25 UTC, Michal Urban
Details | Diff | Splinter Review
Added test to ensure that Koha::Holds->get_items_that_can_fill returns items with datecancelled and datearrived (5.69 KB, patch)
2022-06-16 17:05 UTC, Michal Urban
Details | Diff | Splinter 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. (3.49 KB, patch)
2022-06-16 17:05 UTC, Michal Urban
Details | Diff | Splinter Review
Continued from previous patch: (2.20 KB, patch)
2022-06-16 17:25 UTC, Michal Urban
Details | Diff | Splinter Review
Bug 30447: Check if transfers arrived or cancelled (4.31 KB, patch)
2022-06-16 18:25 UTC, Michal Urban
Details | Diff | Splinter Review
Bug 30447: Check if transfers arrived or cancelled (2.09 KB, patch)
2022-06-16 18:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30447: Unit tests (5.61 KB, patch)
2022-06-16 18:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30447: Check if transfers arrived or cancelled (2.16 KB, patch)
2022-07-14 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30447: Unit tests (5.68 KB, patch)
2022-07-14 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.