Description
Kelly McElligott
2019-11-04 14:53:09 UTC
Created attachment 95016 [details] [review] Bug 23964: Unit tests 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 Created attachment 95021 [details] [review] Bug 23964: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> Code scans well, tests pass and qa script passes. This looks logically correct to me and works as described so I'm signing off. 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? Created attachment 95111 [details] [review] Bug 23964: (follow-up) Add comments and improve readability (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. Created attachment 95249 [details] [review] Bug 23964: Use is instead of isnt in tests 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->priority == 1 here? 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. 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. Created attachment 95544 [details] [review] Bug 23964: (follow-up) more test clarification (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 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. 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> 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> 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> 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> Created attachment 95598 [details] [review] Bug 23964: (follow-up) more test clarification Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> Looking here 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> 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> 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 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> 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> 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> Nice work! Pushed to master for 19.11.00 Created attachment 95780 [details] [review] Bug 23964: (follow-up) We should check for only active holds when determining to set waiting 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> Nice work! Pushed to master for 19.11.00 |