Bug 23964

Summary: An item level hold when placed is set to Waiting, if ReservesNeedReturn is set to Automatic
Product: Koha Reporter: Kelly McElligott <kelly>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: gmcharlt, jonathan.druart, m.de.rooy, martin.renvoize
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00
Attachments: Bug 23964: Unit tests
Bug 23964: ReservesNeedReturn shoudl only apply to available items
Bug 23964: Unit tests
Bug 23964: ReservesNeedReturn should only apply to available items
Bug 23964: (follow-up) Add comments and improve readability
Bug 23964: Use is instead of isnt in tests
Bug 23964: (follow-up) more test clarification
Bug 23964: Unit tests
Bug 23964: ReservesNeedReturn should only apply to available items
Bug 23964: (follow-up) Add comments and improve readability
Bug 23964: Use is instead of isnt in tests
Bug 23964: (follow-up) more test clarification
Bug 23964: 3 more tests to cover found and match the description
Bug 23964: Unit tests
Bug 23964: ReservesNeedReturn should only apply to available items
Bug 23964: (follow-up) Add comments and improve readability
Bug 23964: Use is instead of isnt in tests
Bug 23964: (follow-up) more test clarification
Bug 23964: 3 more tests to cover found and match the description
Bug 23964: (follow-up) We should check for only active holds when determining to set waiting
Bug 23964: (follow-up) We should check for only active holds when determining to set waiting

Description Kelly McElligott 2019-11-04 14:53:09 UTC
If a library has the system preference, ReservesNeedReturn, set to Automatically; when an item level hold is placed- regardless if the item is checked out, the hold goes right to Waiting.

This should not go to Waiting when the Item is checked out. It should be an item level hold until the item has been checked in.
Comment 1 Nick Clemens 2019-11-04 16:09:58 UTC
Created attachment 95016 [details] [review]
Bug 23964: Unit tests
Comment 2 Nick Clemens 2019-11-04 16:10:00 UTC
Created attachment 95017 [details] [review]
Bug 23964: ReservesNeedReturn shoudl only apply to available items

To test:
 1 - Checkout an item to a patron
 2 - Make sure 'ReservesNeedReturn' is set to 'Automatically'
 3 - Place an item level hold on the checked out item
 4 - Note the hold is marked waiting
 5 - Delete the hold
 6 - Apply patch
 7 - Place a new hold
 8 - Hold is not marked waiting
 9 - Please a hold on a different item
10 - Note it is marked waiting
11 - Test when item is damaged and hold not allowed on damaged items
12 - Test when item is in transit
13 - Test when item has another hold
14 - Only in the case where none of the above are true should the hold be marked waiting
Comment 3 Martin Renvoize 2019-11-04 17:09:20 UTC
Created attachment 95021 [details] [review]
Bug 23964: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2019-11-04 17:09:24 UTC
Created attachment 95022 [details] [review]
Bug 23964: ReservesNeedReturn should only apply to available items

To test:
 1 - Checkout an item to a patron
 2 - Make sure 'ReservesNeedReturn' is set to 'Automatically'
 3 - Place an item level hold on the checked out item
 4 - Note the hold is marked waiting
 5 - Delete the hold
 6 - Apply patch
 7 - Place a new hold
 8 - Hold is not marked waiting
 9 - Please a hold on a different item
10 - Note it is marked waiting
11 - Test when item is damaged and hold not allowed on damaged items
12 - Test when item is in transit
13 - Test when item has another hold
14 - Only in the case where none of the above are true should the hold be marked waiting

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-11-04 17:10:26 UTC
Code scans well, tests pass and qa script passes.  This looks logically correct to me and works as described so I'm signing off.
Comment 6 Jonathan Druart 2019-11-06 14:27:05 UTC
Nick, can you explain a bit please?

Also I would love to see this line a bit more readable, on several lines with comments.

It also seems that we are doing almost identical conditions in other places in this module (IsAvailableForItemLevelRequest). Does it means it should be refactored?
Comment 7 Nick Clemens 2019-11-06 14:59:27 UTC
Created attachment 95111 [details] [review]
Bug 23964: (follow-up) Add comments and improve readability
Comment 8 Nick Clemens 2019-11-06 15:00:56 UTC
(In reply to Jonathan Druart from comment #6)
ents. 
> It also seems that we are doing almost identical conditions in other places
> in this module (IsAvailableForItemLevelRequest). Does it means it should be
> refactored?

That routine is more concerned whether this hold is allowed or not, the code here is to test if the hold can be set immediately to waiting.
Comment 9 Jonathan Druart 2019-11-11 08:43:00 UTC
Created attachment 95249 [details] [review]
Bug 23964: Use is instead of isnt in tests
Comment 10 Jonathan Druart 2019-11-11 08:45:26 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2019-11-19 08:17:22 UTC
725     my $hold_1 = place_item_hold( $patron, $item, $library, $priority );
726     $hold = place_item_hold( $patron_2, $item, $library, $priority );
727     is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item already on hold priority must be set to 1' );

What about $hold_1->priority? Should not we have $hold_1->priority == 1 instead of $hold->priority == 1?

737     isnt( $hold->priority, 0, 'If ReservesNeedReturns is 0 but item in transit the hold must not be set to waiting' );

Test's description does not match the test.
Comment 12 Nick Clemens 2019-11-19 11:58:17 UTC
Created attachment 95544 [details] [review]
Bug 23964: (follow-up) more test clarification
Comment 13 Nick Clemens 2019-11-19 12:00:09 UTC
(In reply to Jonathan Druart from comment #11)
> 725     my $hold_1 = place_item_hold( $patron, $item, $library, $priority );
> 726     $hold = place_item_hold( $patron_2, $item, $library, $priority );
> 727     is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item
> already on hold priority must be set to 1' );
> 
> What about $hold_1->priority? Should not we have $hold_1->priority == 1
> instead of $hold->priority == 1?

The first hold should get set to waiting, the next hold on the item doesn't because there is a hold already on the item

> 737     isnt( $hold->priority, 0, 'If ReservesNeedReturns is 0 but item in
> transit the hold must not be set to waiting' );
> 
> Test's description does not match the test.

I do not understand, I place a transfer before this test, the transfer means the hold is not waiting. Changed to 'is' '1' in any case
Comment 14 Jonathan Druart 2019-11-20 09:05:39 UTC
Comment on attachment 95544 [details] [review]
Bug 23964: (follow-up) more test clarification

Review of attachment 95544 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/guarantor_search.tt
@@ +18,4 @@
>                  "dt_address":
>                      "[% INCLUDE escape_address data=data %]",
>                  "dt_action":
> +                    "<a href=\"#\" class=\"btn btn-default btn-xs select_user\" data-borrowernumber=\"[% data.borrowernumber | html %]\">Select</a><input type=\"hidden\" id=\"borrower_data[% data.borrowernumber | html %]\" name=\"borrower_data[% data.borrowernumber | html %]\" value=\"[% data | html | $To %]\" />"

Not related to this patch. Will amend.
Comment 15 Jonathan Druart 2019-11-20 09:06:17 UTC
Created attachment 95594 [details] [review]
Bug 23964: Unit tests

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2019-11-20 09:06:22 UTC
Created attachment 95595 [details] [review]
Bug 23964: ReservesNeedReturn should only apply to available items

To test:
 1 - Checkout an item to a patron
 2 - Make sure 'ReservesNeedReturn' is set to 'Automatically'
 3 - Place an item level hold on the checked out item
 4 - Note the hold is marked waiting
 5 - Delete the hold
 6 - Apply patch
 7 - Place a new hold
 8 - Hold is not marked waiting
 9 - Please a hold on a different item
10 - Note it is marked waiting
11 - Test when item is damaged and hold not allowed on damaged items
12 - Test when item is in transit
13 - Test when item has another hold
14 - Only in the case where none of the above are true should the hold be marked waiting

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2019-11-20 09:06:27 UTC
Created attachment 95596 [details] [review]
Bug 23964: (follow-up) Add comments and improve readability

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2019-11-20 09:06:32 UTC
Created attachment 95597 [details] [review]
Bug 23964: Use is instead of isnt in tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2019-11-20 09:06:37 UTC
Created attachment 95598 [details] [review]
Bug 23964: (follow-up) more test clarification

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2019-11-20 09:06:41 UTC
Created attachment 95599 [details] [review]
Bug 23964: 3 more tests to cover found and match the description

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Marcel de Rooy 2019-11-21 06:37:32 UTC
Looking here
Comment 22 Marcel de Rooy 2019-11-21 06:55:13 UTC
Created attachment 95639 [details] [review]
Bug 23964: Unit tests

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

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2019-11-21 06:55:18 UTC
Created attachment 95640 [details] [review]
Bug 23964: ReservesNeedReturn should only apply to available items

To test:
 1 - Checkout an item to a patron
 2 - Make sure 'ReservesNeedReturn' is set to 'Automatically'
 3 - Place an item level hold on the checked out item
 4 - Note the hold is marked waiting
 5 - Delete the hold
 6 - Apply patch
 7 - Place a new hold
 8 - Hold is not marked waiting
 9 - Please a hold on a different item
10 - Note it is marked waiting
11 - Test when item is damaged and hold not allowed on damaged items
12 - Test when item is in transit
13 - Test when item has another hold
14 - Only in the case where none of the above are true should the hold be marked waiting

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

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2019-11-21 06:55:22 UTC
Created attachment 95641 [details] [review]
Bug 23964: (follow-up) Add comments and improve readability

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Corrected typo: Similaryly
Comment 25 Marcel de Rooy 2019-11-21 06:55:27 UTC
Created attachment 95642 [details] [review]
Bug 23964: Use is instead of isnt in tests

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2019-11-21 06:55:31 UTC
Created attachment 95643 [details] [review]
Bug 23964: (follow-up) more test clarification

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2019-11-21 06:55:35 UTC
Created attachment 95644 [details] [review]
Bug 23964: 3 more tests to cover found and match the description

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Martin Renvoize 2019-11-21 11:36:43 UTC
Nice work!

Pushed to master for 19.11.00
Comment 29 Nick Clemens 2019-11-25 11:33:26 UTC
Created attachment 95780 [details] [review]
Bug 23964: (follow-up) We should check for only active holds when determining to set waiting
Comment 30 Jonathan Druart 2019-11-25 11:40:57 UTC
Created attachment 95781 [details] [review]
Bug 23964: (follow-up) We should check for only active holds when determining to set waiting

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Martin Renvoize 2019-11-25 16:38:38 UTC
Nice work!

Pushed to master for 19.11.00