Bug 17737 - Move GetReservesFromItemnumber to Koha::Item->holds
Summary: Move GetReservesFromItemnumber to Koha::Item->holds
Status: CLOSED 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:
Keywords:
Depends on: 17736
Blocks: 17728 17738 17829
  Show dependency treegraph
 
Reported: 2016-12-07 14:47 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today (15.41 KB, patch)
2016-12-07 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber (2.05 KB, patch)
2016-12-07 16:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today (15.07 KB, patch)
2017-01-09 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber (2.05 KB, patch)
2017-01-09 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: Rename holds_placed_before_today with current_holds (7.01 KB, patch)
2017-01-09 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: [QA Follow-up] Remove unused reservedfor variable (1.12 KB, patch)
2017-02-03 10:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today (15.17 KB, patch)
2017-02-03 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber (2.15 KB, patch)
2017-02-03 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17737: Rename holds_placed_before_today with current_holds (7.10 KB, patch)
2017-02-03 12:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17737: [QA Follow-up] Remove unused reservedfor variable (1.12 KB, patch)
2017-02-03 12:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today (15.17 KB, patch)
2017-04-12 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber (2.11 KB, patch)
2017-04-12 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: Rename holds_placed_before_today with current_holds (7.06 KB, patch)
2017-04-12 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17737: [QA Follow-up] Remove unused reservedfor variable (1.08 KB, patch)
2017-04-12 16:42 UTC, Jonathan Druart
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:20 UTC

    
Comment 1 Jonathan Druart 2016-12-07 16:24:56 UTC
Created attachment 58018 [details] [review]
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today

On the same way of Koha::Biblio->get_holds,
Koha::Biblio->get_holds_placed_before_today and Koha::Patron->get_holds,
this new subroutin will permit to retrieve the holds placed on a
specific item.
Note that at the moment we do not need a Koha::Item->get_holds method:
we do not want to display future holds placed in the future.

Test plan:
I would suggest to test this patch with patches from bug 17736 and bug 17738,
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:25:00 UTC
Created attachment 58019 [details] [review]
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber

At this point, there should not be any occurrences of
GetReservesFromItemnumber anymore.
Comment 3 Marcel de Rooy 2017-01-06 12:58:04 UTC
Please see 17736 and use something like current_holds ?
Comment 4 Jonathan Druart 2017-01-09 08:13:33 UTC
Created attachment 58673 [details] [review]
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today

On the same way of Koha::Biblio->get_holds,
Koha::Biblio->get_holds_placed_before_today and Koha::Patron->get_holds,
this new subroutin will permit to retrieve the holds placed on a
specific item.
Note that at the moment we do not need a Koha::Item->get_holds method:
we do not want to display future holds placed in the future.

Test plan:
I would suggest to test this patch with patches from bug 17736 and bug 17738,
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-01-09 08:13:37 UTC
Created attachment 58674 [details] [review]
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber

At this point, there should not be any occurrences of
GetReservesFromItemnumber anymore.
Comment 6 Jonathan Druart 2017-01-09 08:13:41 UTC
Created attachment 58675 [details] [review]
Bug 17737: Rename holds_placed_before_today with current_holds
Comment 7 Marcel de Rooy 2017-02-03 08:20:59 UTC
Running Reserves.t

ok 51 - Reserve in waiting status cant be canceled
Use of uninitialized value $tagno in concatenation (.) or string at /usr/share/perl5/MARC/Field.pm line 83.
Tag "" is not a valid tag. at t/db_dependent/Reserves.t line 525.
# Looks like your test exited with 255 just after 51.
Comment 8 Jonathan Druart 2017-02-03 09:20:45 UTC
(In reply to Marcel de Rooy from comment #7)
> Running Reserves.t
> 
> ok 51 - Reserve in waiting status cant be canceled
> Use of uninitialized value $tagno in concatenation (.) or string at
> /usr/share/perl5/MARC/Field.pm line 83.
> Tag "" is not a valid tag. at t/db_dependent/Reserves.t line 525.
> # Looks like your test exited with 255 just after 51.

Not related with these patches, see bug 18045.
Comment 9 Marcel de Rooy 2017-02-03 10:51:58 UTC
Created attachment 59872 [details] [review]
Bug 17737: [QA Follow-up] Remove unused reservedfor variable

The changes in this patch set obsolete this variable.
Remove confusing comment about reserve via host record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-02-03 11:09:39 UTC
Minor inconsistency on the interface: if I set OPACShowHoldQueueDetails and place a future hold, opac-details says Available but also 1 (priority 1).
Should be resolved somewhere else though.
Comment 11 Marcel de Rooy 2017-02-03 11:23:50 UTC
+my $item = Koha::Items->find( $itemnumber );
+$holds = $item->holds_placed_before_today;
+my $dtf = Koha::Database->new->schema->storage->datetime_parser;
+my $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } );
+is( $future_holds->count, 0, 'holds_placed_before_today does not return a future next available hold');
+# 9788b: holds_placed_before_today does not return future item level hold

Hm. You look at future_holds and then say something about the results of holds_placed_before_today as saved in holds ?
Comment 12 Marcel de Rooy 2017-02-03 11:24:39 UTC
Seems the same for two other occurrences.
Comment 13 Marcel de Rooy 2017-02-03 11:25:27 UTC
Need feedback on comment 11 before signoff.
Comment 14 Marcel de Rooy 2017-02-03 11:33:31 UTC
(In reply to Marcel de Rooy from comment #13)
> Need feedback on comment 11 before signoff.

No longer, just misread the search call.
Comment 15 Marcel de Rooy 2017-02-03 12:10:53 UTC
Created attachment 59876 [details] [review]
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today

On the same way of Koha::Biblio->get_holds,
Koha::Biblio->get_holds_placed_before_today and Koha::Patron->get_holds,
this new subroutin will permit to retrieve the holds placed on a
specific item.
Note that at the moment we do not need a Koha::Item->get_holds method:
we do not want to display future holds placed in the future.

Test plan:
I would suggest to test this patch with patches from bug 17736 and bug 17738,
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.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2017-02-03 12:10:57 UTC
Created attachment 59877 [details] [review]
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber

At this point, there should not be any occurrences of
GetReservesFromItemnumber anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-02-03 12:11:01 UTC
Created attachment 59878 [details] [review]
Bug 17737: Rename holds_placed_before_today with current_holds

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-02-03 12:11:06 UTC
Created attachment 59879 [details] [review]
Bug 17737: [QA Follow-up] Remove unused reservedfor variable

The changes in this patch set obsolete this variable.
Remove confusing comment about reserve via host record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2017-03-24 07:47:44 UTC
Success, Martin. You can do it :)
Comment 20 Marcel de Rooy 2017-04-07 07:38:09 UTC
Taking the liberty to pass QA on this one now. Retested. QA tools do not complain. Tests pass. Looks still good to me, no need to wait any longer.

Passed QA
Comment 21 Martin Renvoize 2017-04-07 07:46:18 UTC
This also gets the thumbs up from me, it was in my queue to properly QA but I'd been pulled into other things part way through. As Marcel says, it passes all the tests and the code looks good to me. As far through my manual checks and testing it had all passed with flying colours too.
Comment 22 Marcel de Rooy 2017-04-07 08:26:59 UTC
(In reply to Martin Renvoize from comment #21)
> This also gets the thumbs up from me, it was in my queue to properly QA but
> I'd been pulled into other things part way through. As Marcel says, it
> passes all the tests and the code looks good to me. As far through my manual
> checks and testing it had all passed with flying colours too.

Thanks, Martin.
Comment 23 Jonathan Druart 2017-04-12 16:42:21 UTC
Created attachment 62105 [details] [review]
Bug 17737: Replace GetReservesFromItemnumber with Koha::Item->get_holds_placed_before_today

On the same way of Koha::Biblio->get_holds,
Koha::Biblio->get_holds_placed_before_today and Koha::Patron->get_holds,
this new subroutin will permit to retrieve the holds placed on a
specific item.
Note that at the moment we do not need a Koha::Item->get_holds method:
we do not want to display future holds placed in the future.

Test plan:
I would suggest to test this patch with patches from bug 17736 and bug 17738,
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.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Jonathan Druart 2017-04-12 16:42:25 UTC
Created attachment 62106 [details] [review]
Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber

At this point, there should not be any occurrences of
GetReservesFromItemnumber anymore.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Jonathan Druart 2017-04-12 16:42:29 UTC
Created attachment 62107 [details] [review]
Bug 17737: Rename holds_placed_before_today with current_holds

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Jonathan Druart 2017-04-12 16:42:34 UTC
Created attachment 62108 [details] [review]
Bug 17737: [QA Follow-up] Remove unused reservedfor variable

The changes in this patch set obsolete this variable.
Remove confusing comment about reserve via host record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Jonathan Druart 2017-04-12 16:42:58 UTC
Just a rebase.
Comment 28 Kyle M Hall 2017-04-13 12:50:01 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 29 Katrin Fischer 2017-04-16 12:23:08 UTC
This won't get ported back to 16.11.x as it is an enhancement.