Bug 17738 - Move GetReservesFromBorrowernumber to Koha::Patron->holds
Summary: Move GetReservesFromBorrowernumber to Koha::Patron->holds
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 17737 17740
Blocks: 17728 17829
  Show dependency treegraph
 
Reported: 2016-12-07 14:47 UTC by Jonathan Druart
Modified: 2017-07-10 10:12 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds (13.79 KB, patch)
2016-12-07 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber (1.75 KB, patch)
2016-12-07 16:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds (13.82 KB, patch)
2017-04-12 16:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber (1.75 KB, patch)
2017-04-12 16:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds (14.00 KB, patch)
2017-04-15 07:02 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber (1.88 KB, patch)
2017-04-15 07:03 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17738: Remove warning about redeclaration of $patron (672 bytes, patch)
2017-04-17 12:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17738: Remove warning about redeclaration of $patron (806 bytes, patch)
2017-04-17 15:46 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds (14.06 KB, patch)
2017-06-15 14:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber (1.93 KB, patch)
2017-06-15 14:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17738: Remove warning about redeclaration of $patron (863 bytes, patch)
2017-06-15 14:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17738: Replace GetReservesFromBorrowernumber with Koha::Patron->get_holds (14.15 KB, patch)
2017-06-30 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17738: Remove C4::Reserves::GetReservesFromBorrowernumber (2.03 KB, patch)
2017-06-30 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17738: Remove warning about redeclaration of $patron (960 bytes, patch)
2017-06-30 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17738: [QA Follow-up] Remove second find of same patron (868 bytes, patch)
2017-06-30 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review

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