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.
Created attachment 58019 [details] [review] Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber At this point, there should not be any occurrences of GetReservesFromItemnumber anymore.
Please see 17736 and use something like current_holds ?
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.
Created attachment 58674 [details] [review] Bug 17737: Remove C4::Reserves::GetReservesFromItemnumber At this point, there should not be any occurrences of GetReservesFromItemnumber anymore.
Created attachment 58675 [details] [review] Bug 17737: Rename holds_placed_before_today with current_holds
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.
(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.
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>
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.
+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 ?
Seems the same for two other occurrences.
Need feedback on comment 11 before signoff.
(In reply to Marcel de Rooy from comment #13) > Need feedback on comment 11 before signoff. No longer, just misread the search call.
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>
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>
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>
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>
Success, Martin. You can do it :)
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
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.
(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.
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>
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>
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>
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>
Just a rebase.
Pushed to master for 17.05, thanks Jonathan, Marcel!
This won't get ported back to 16.11.x as it is an enhancement.