Bug 17738

Summary: Move GetReservesFromBorrowernumber to Koha::Patron->holds
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, m.de.rooy, veron
Version: unspecified   
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:
Bug Depends on: 17737, 17740    
Bug Blocks: 17728, 17829    
Attachments: Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber
Bug 17738: Remove warning about redeclaration of $patron
Bug 17738: Remove warning about redeclaration of $patron
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber
Bug 17738: Remove warning about redeclaration of $patron
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber
Bug 17738: Remove warning about redeclaration of $patron
Bug 17738: [QA Follow-up] Remove second find of same patron

Description Jonathan Druart 2016-12-07 14:47:48 UTC

    
Comment 1 Jonathan Druart 2016-12-07 16:26:40 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.
Comment 2 Jonathan Druart 2016-12-07 16:26:43 UTC
Created attachment 58021 [details] [review]
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber

At this point, there should not be any occurrences of
GetReservesFromBorrowernumber anymore.
Comment 3 Marc Véron 2017-04-12 16:25:42 UTC
Wanted to test on top of 17737 but 17737 does not apply:
sha1 information lacking or useless (opac/opac-reserve.pl)
Comment 4 Jonathan Druart 2017-04-12 16:44:07 UTC
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.
Comment 5 Jonathan Druart 2017-04-12 16:44:10 UTC
Created attachment 62110 [details] [review]
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber

At this point, there should not be any occurrences of
GetReservesFromBorrowernumber anymore.
Comment 6 Jonathan Druart 2017-04-12 16:44:27 UTC
(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.
Comment 7 Marc Véron 2017-04-12 21:02:20 UTC
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
Comment 8 Marc Véron 2017-04-15 06:59:09 UTC
(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.
Comment 9 Marc Véron 2017-04-15 07:00:00 UTC
Signing... :-)
Comment 10 Marc Véron 2017-04-15 07:02:33 UTC
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>
Comment 11 Marc Véron 2017-04-15 07:03:40 UTC
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>
Comment 12 Jonathan Druart 2017-04-17 12:01:11 UTC
Created attachment 62208 [details] [review]
Bug 17738: Remove warning about redeclaration of $patron
Comment 13 Marc Véron 2017-04-17 15:46:47 UTC
Created attachment 62219 [details] [review]
Bug 17738: Remove warning about redeclaration of $patron

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 14 Kyle M Hall 2017-06-15 14:42:16 UTC
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>
Comment 15 Kyle M Hall 2017-06-15 14:42:40 UTC
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>
Comment 16 Kyle M Hall 2017-06-15 14:43:04 UTC
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>
Comment 17 Marcel de Rooy 2017-06-30 10:08:10 UTC
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>
Comment 18 Marcel de Rooy 2017-06-30 10:08:14 UTC
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>
Comment 19 Marcel de Rooy 2017-06-30 10:08:18 UTC
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>
Comment 20 Marcel de Rooy 2017-06-30 10:08:22 UTC
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>
Comment 21 Jonathan Druart 2017-07-05 17:05:18 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 22 Fridolin Somers 2017-07-10 10:12:16 UTC
Enhancement not pushed to 17.05.x