|
Description
Jonathan Druart
2016-12-07 14:47:48 UTC
Created attachment 58020 [details] [review] Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds This patch replace the different calls to GetReservesFromBorrowernumber with a calls to Koha::Patron->get_holds. In some places we need to get a restricted set of holds, that's why we process a search on this holds returned by ->get_holds (on the found status for instance). The changes are quite trivial and reading the diff should be enough to catch bugs. Test plan: I would suggest to test this patch with patches from bug 17736 and bug 17737, to place different kind of holds (biblio and item level, future and past). Then do a whole workflow to detect bug, view a record, delete record, order, place a hold on an item which has been ordered, etc. The hold's informations should always be the same without or without these patches. Created attachment 58021 [details] [review] Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber At this point, there should not be any occurrences of GetReservesFromBorrowernumber anymore. Wanted to test on top of 17737 but 17737 does not apply: sha1 information lacking or useless (opac/opac-reserve.pl) Created attachment 62109 [details] [review] Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds This patch replace the different calls to GetReservesFromBorrowernumber with a calls to Koha::Patron->get_holds. In some places we need to get a restricted set of holds, that's why we process a search on this holds returned by ->get_holds (on the found status for instance). The changes are quite trivial and reading the diff should be enough to catch bugs. Test plan: I would suggest to test this patch with patches from bug 17736 and bug 17737, to place different kind of holds (biblio and item level, future and past). Then do a whole workflow to detect bug, view a record, delete record, order, place a hold on an item which has been ordered, etc. The hold's informations should always be the same without or without these patches. Created attachment 62110 [details] [review] Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber At this point, there should not be any occurrences of GetReservesFromBorrowernumber anymore. (In reply to Marc Véron from comment #3) > Wanted to test on top of 17737 but 17737 does not apply: > sha1 information lacking or useless (opac/opac-reserve.pl) Both have been rebased. Tested both patches together on top of 17737. No problems found. However, QA tool complains with first patch: FAIL opac/opac-reserve.pl FAIL valid "my" variable $patron masks earlier declaration in same scope (In reply to Marc Véron from comment #7) > Tested both patches together on top of 17737. No problems found. > > However, QA tool complains with first patch: > FAIL opac/opac-reserve.pl > FAIL valid > "my" variable $patron masks earlier declaration in same scope I can not find out why QA tool complains, code seems OK to me. Maybe QAers can find out? - Singing off. Signing... :-) Created attachment 62188 [details] [review] Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds This patch replace the different calls to GetReservesFromBorrowernumber with a calls to Koha::Patron->get_holds. In some places we need to get a restricted set of holds, that's why we process a search on this holds returned by ->get_holds (on the found status for instance). The changes are quite trivial and reading the diff should be enough to catch bugs. Test plan: I would suggest to test this patch with patches from bug 17736 and bug 17737, to place different kind of holds (biblio and item level, future and past). Then do a whole workflow to detect bug, view a record, delete record, order, place a hold on an item which has been ordered, etc. The hold's informations should always be the same without or without these patches. Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 62189 [details] [review] Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber At this point, there should not be any occurrences of GetReservesFromBorrowernumber anymore. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 62208 [details] [review] Bug 17738: Remove warning about redeclaration of $patron Created attachment 62219 [details] [review] Bug 17738: Remove warning about redeclaration of $patron Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 64330 [details] [review] Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds This patch replace the different calls to GetReservesFromBorrowernumber with a calls to Koha::Patron->get_holds. In some places we need to get a restricted set of holds, that's why we process a search on this holds returned by ->get_holds (on the found status for instance). The changes are quite trivial and reading the diff should be enough to catch bugs. Test plan: I would suggest to test this patch with patches from bug 17736 and bug 17737, to place different kind of holds (biblio and item level, future and past). Then do a whole workflow to detect bug, view a record, delete record, order, place a hold on an item which has been ordered, etc. The hold's informations should always be the same without or without these patches. Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 64331 [details] [review] Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber At this point, there should not be any occurrences of GetReservesFromBorrowernumber anymore. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 64332 [details] [review] Bug 17738: Remove warning about redeclaration of $patron Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 64749 [details] [review] Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds This patch replace the different calls to GetReservesFromBorrowernumber with a calls to Koha::Patron->get_holds. In some places we need to get a restricted set of holds, that's why we process a search on this holds returned by ->get_holds (on the found status for instance). The changes are quite trivial and reading the diff should be enough to catch bugs. Test plan: I would suggest to test this patch with patches from bug 17736 and bug 17737, to place different kind of holds (biblio and item level, future and past). Then do a whole workflow to detect bug, view a record, delete record, order, place a hold on an item which has been ordered, etc. The hold's informations should always be the same without or without these patches. Tested both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64750 [details] [review] Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber At this point, there should not be any occurrences of GetReservesFromBorrowernumber anymore. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64751 [details] [review] Bug 17738: Remove warning about redeclaration of $patron Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 64752 [details] [review] Bug 17738: [QA Follow-up] Remove second find of same patron We can just use the $patron from line 77 here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Pushed to master for 17.11, thanks to everybody involved! Enhancement not pushed to 17.05.x |