If we give GetReservesFromItemnumber an additional return value (waitingdate), we can remove some code from several scripts. If we pass the alldates parameter in these cases, Koha takes into account future hold requests and provides more correct information in some cases. This report builds further on changes for bug 9761 and bug 9367. All calls to this function have been checked.
Will still add some additional information for testing.
Created attachment 16026 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber
Created attachment 16092 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive.
I try to test this patch but I can't understand "Enable ConfirmFutureHolds by inserting a number of days, say 2." I can't find ConfirmFutureHolds in the syspref.
(In reply to comment #4) > I try to test this patch but I can't understand "Enable ConfirmFutureHolds > by inserting a number of days, say 2." I can't find ConfirmFutureHolds in > the syspref. Did you apply patch 9761 (listed as dependency) ? Note that the other patch is in master now.
I recieved this error when I tried to confirm the hold: The 'days' parameter (undef) to DateTime::Duration::new was an 'undef', which is not one of the allowed types: scalar at /usr/lib/perl5/DateTime/Duration.pm line 31. DateTime::Duration::new(undef, 'days', undef) called at /usr/lib/perl5/DateTime.pm line 1629 DateTime::add('DateTime=HASH(0x42cf260)', 'days', undef) called at /home/koha/kohaclone/C4/Letters.pm line 614 C4::Letters::_parseletter('HASH(0x42ced20)', 'reserves', 'HASH(0x42d43b0)') called at /home/koha/kohaclone/C4/Letters.pm line 555 C4::Letters::_substitute_tables('HASH(0x42ced20)', 'HASH(0x42d0ca0)') called at /home/koha/kohaclone/C4/Letters.pm line 503 C4::Letters::GetPreparedLetter('letter_code', 'HOLD_PRINT', 'tables', 'HASH(0x42d0ca0)', 'substitute', 'HASH(0x42d0130)', 'branchcode', 'CPL', 'module', ...) called at /home/koha/kohaclone/C4/Reserves.pm line 1907 C4::Reserves::_koha_notify_reserve(3, 19, 1) called at /home/koha/kohaclone/C4/Reserves.pm line 1294 C4::Reserves::ModReserveAffect(3, 19, undef) called at /home/koha/kohaclone/circ/returns.pl line 145
Thanks for testing Kyle! I would really like to see this patch joining patch 9761. Since I do not have this error, I checked where you crashed in C4/Letters: 613 my $dt = dt_from_string(); 614 $dt->add( days => C4::Context->preference('ReservesMaxPickUpDelay') ); L613 gets back the date of today. L614 adds a few days. Since the crash is on L614, it seems that your pref ReservesMaxPickupDelay is undefined (perhaps deleted in some other course of development..) ? Could you check that please? We can add a test with || 0 or something, but it should normally not be needed. Hope that fixes it..
Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. I will send a small followup to at least partially improve this..
Created attachment 19192 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref..
OK. I tested the following now: With patch 1 only (and deleted maxpickupdelay): Crash just like Kyle mentioned. With patch 2 applied too (still undef maxpickupdelay): No crash! So my assumption seems to be correct about the deleted pref :) Don't forget to restore the maxpickupdelay value if you should want to test this too..
Created attachment 19193 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19194 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19940 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19941 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Rebased both patches (for changes from bug 9394). Tested them again.
Still see something. Will submit again.
Created attachment 19943 [details] [review] [SIGNED_OFF] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 19944 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Ready for QA again. Note that they depend on bug 9761.
Question, would it not be better to use the KohaDates TT plugin, rather than using format_date in the source code?
(In reply to Kyle M Hall from comment #20) > Question, would it not be better to use the KohaDates TT plugin, rather than > using format_date in the source code? This patch does not focus on transition to KohaDates; since format_date from C4::Dates is used several times in the context of this change, I probably copied it. If we change this, we should update them all. But isn't that outside the scope of this report?
Agreed, it would be better to file a separate bug report later to transition all the calls at once. (In reply to M. de Rooy from comment #21) > (In reply to Kyle M Hall from comment #20) > > Question, would it not be better to use the KohaDates TT plugin, rather than > > using format_date in the source code? > > This patch does not focus on transition to KohaDates; since format_date from > C4::Dates is used several times in the context of this change, I probably > copied it. If we change this, we should update them all. But isn't that > outside the scope of this report?
This patch will be in conflict with bug 8367.
Just a note for QA: I am thinking of expanding the unit tests in Holds.t for the change in GetReservesFromItemnumber on this report. I will probably add an additional patch in the next few days.
Created attachment 21479 [details] [review] Bug 9788: Followup removing the alldates parameter Before bug 9788 the alldates parameter of GetReservesFromItemnumber was actually not used in the codebase. The first patch of bug 9788 did change that and passed true by default. But a closer look revealed that we do not really need it. The parameter is removed by this patch; the SQL statement is slightly adjusted: if reservedate<=now or a waitingdate is filled for the requested itemnumber, GetReservesFromItemnumber will return the reserve. This includes so-called future waits: a future hold that has been confirmed ahead of time with pref ConfirmFutureHolds > 0 days. Note that future item-level holds are not really interesting to return; this just corresponds to original behavior. Future next-available holds are not in view at all; they do not contain an item number. Test plan: Actually, the test plan of the first patch is valid. But for completeness I repeat it here: [1] Enable future holds and set ConfirmFutureHolds to 2 days. [2] Place a future next-available hold for 2 days ahead. [3] Check item status on catalogue detail. Available? That is fine. [4] Confirm the future hold by checking it in. ('future wait') [5] Look at item status again on catalogue detail. Must be Waiting now. [6] Switch to OPAC and login as another opac user. Goto Place a hold. [7] Check item status with item level hold info. Is it waiting? [8] Try to place hold in staff, check item level status again. Waiting? [9] Make a transfer for the item. Switch branch. Check hold status on Transfers to receive.
Created attachment 21481 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail.
With the patch for 9761 pushed, this bug should be considered as just a minor bugfix. I have changed the severity accordingly. I added two patches now: One patch removes an actually superfluous alldates parameter from GetReservesFromItemnumber. All calls have been changed likewise. Note that behavior changes very slightly (only when using ConfirmFutureHolds) and goes back more to original behavior since future item level holds are not reported now (as it was). The second patch adds three tests for this routine to Reserves.t. Running Holds.t is recommended too. The report was in Signed off state as from June 21 (so three months passed). I will set it back to Needs Signoff, since the additional two patches need a signoff. Hopefully, this gets some attention soon though.. The extra patches should help convince QA/RM :)
(In reply to Jonathan Druart from comment #23) > This patch will be in conflict with bug 8367. Can you expand on what sort of conflict? Just a merge conflict that would be resolvable with more or less effort, or conflicting notions of the desired functionality or implementation thereof?
Patch applied cleanly, go forth and signoff
(In reply to Galen Charlton from comment #28) > (In reply to Jonathan Druart from comment #23) > > This patch will be in conflict with bug 8367. > > Can you expand on what sort of conflict? Just a merge conflict that would > be resolvable with more or less effort, or conflicting notions of the > desired functionality or implementation thereof? When I wrote this note, bug 8367 was signed off. Now, it does not apply :) In bug 8367 I change the list of returned values for the GetReservesFromItemnumber routine (I return directly the reserve hashref). Patches from this report add a returned value too (waitingdate). So I thought it would be good to base them onto bug 8267's patches. The first argument was: patches are really (reaally) boring to rebase. It was certainly not relevant, so please don't take into account this note.
Created attachment 21750 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail.
Created attachment 22309 [details] [review] [SIGNED_OFF] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Rebased for different parameter passing to output_pref.
This report still needs some help! There are four patches (two have a signoff). Two patches need a signoff: the unit test and the alldates followup. For testing you need to run two unit tests (see commit message). And follow the test plan in the alldates followup. You should be able to complete them in well 10 minutes?
Created attachment 22610 [details] [review] [SIGNED OFF] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail. Holds.t required adding CPL branch to function. Reserves.t does not fail for the three tests in question. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment on attachment 22610 [details] [review] [SIGNED OFF] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber FAIL t/db_dependent/Reserves.t OK pod OK forbidden patterns FAIL valid "my" variable @results masks earlier declaration in same scope OK critic Oops! Caught this while looking at the other patch, not signed off patch. I really should run this all the time. Sorry, I should not have signed off.
I did not run any code that would trigger all the change points in the "Followup removing the alldates parameter" patch. I did, however, do a file comparison between master and the modified code to confirm that the code was identical. That would mean the last four diffs are fully accounted for, because the left side of the diffs are due to another part of the four patches attached to this bug. Since I also ran the prove on the attached test, I know the SQL function is working, and hence the first portion of this specific patch ("Followup removing the alldates parameter") is functional. It seems that initially, the code was NOT called with a parameter passed for $alldates, and so the unless statement was always run. This is why it was moved up into the SQL, which is working. I suppose ordering by priority is considered part of the improvements too. And with the addition of the waitingdate into the SQL statement there is an improvement by including a confirmed future hold. Though, this is the part that makes me feel uncomfortable signing this off, even though it probably is okay. This series of patches have removed the alldates parameter and just put the date restriction into the function. The perldoc has been appropriately updated as well. mtompset@ubuntu:~/kohaclone$ git reset --hard origin/master HEAD is now at 5d7aa7e Bug 10725: Add filters on the pending orders table mtompset@ubuntu:~/kohaclone$ grep -n GetReservesFromItemnumber `find . -name "*.p[lm]" | grep -v blib` `find . -name "*.t"` ./C4/Reserves.pm:98: &GetReservesFromItemnumber ./C4/Reserves.pm:351:=head2 GetReservesFromItemnumber ./C4/Reserves.pm:353: ( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); ./C4/Reserves.pm:359:sub GetReservesFromItemnumber { ./circ/transferstoreceive.pl:102: my @checkreserv = GetReservesFromItemnumber($num->{'itemnumber'} ); ./opac/opac-reserve.pl:434: my ($reservedate,$reservedfor,$expectedAt) = GetReservesFromItemnumber($itemNum); ./reserve/request.pl:331: my ($reservedate,$reservedfor,$expectedAt,$reserve_id) = GetReservesFromItemnumber($itemnumber); ./catalogue/detail.pl:223: my ($reservedate,$reservedfor,$expectedAt) = GetReservesFromItemnumber($item->{itemnumber}); ./t/db_dependent/Holds.t:79:my ( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); ./t/db_dependent/Holds.t:80:ok($reserve_id, "Test GetReservesFromItemnumber()"); ./t/db_dependent/Holds.t:95:( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); mtompset@ubuntu:~/kohaclone$ git bz apply 9788 Bug 9788 - Improvements for calling GetReservesFromItemnumber 19943 - [SIGNED_OFF] Bug 9788: Improvements when calling GetReservesFromItemnumber 21479 - Bug 9788: Followup removing the alldates parameter 22309 - [SIGNED_OFF] Bug 9788: Followup for expirationdate in Letter.pm 22610 - [SIGNED OFF] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Apply? [(y)es, (n)o, (i)nteractive] i Applying: Bug 9788: Improvements when calling GetReservesFromItemnumber Applying: Bug 9788: Followup for expirationdate in Letter.pm Applying: Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Applying: Bug 9788: Followup removing the alldates parameter mtompset@ubuntu:~/kohaclone$ grep -n GetReservesFromItemnumber `find . -name "*.p[lm]" | grep -v blib` `find . -name "*.t"` ./C4/Reserves.pm:98: &GetReservesFromItemnumber ./C4/Reserves.pm:351:=head2 GetReservesFromItemnumber ./C4/Reserves.pm:353: ( $reservedate, $borrowernumber, $branchcode, $reserve_id, $waitingdate ) = GetReservesFromItemnumber($itemnumber); ./C4/Reserves.pm:361:sub GetReservesFromItemnumber { ./circ/transferstoreceive.pl:102: my @checkreserv = GetReservesFromItemnumber($num->{'itemnumber'}); ./opac/opac-reserve.pl:434: my ($reservedate,$reservedfor,$expectedAt,undef,$wait) = GetReservesFromItemnumber($itemNum); ./reserve/request.pl:331: my ($reservedate,$reservedfor,$expectedAt,$reserve_id,$wait) = GetReservesFromItemnumber($itemnumber); ./catalogue/detail.pl:223: my ($reservedate,$reservedfor,$expectedAt,undef,$wait) = GetReservesFromItemnumber($item->{itemnumber}); ./t/db_dependent/Reserves.t:269:# Tests for bug 9788: Does GetReservesFromItemnumber return a future wait? ./t/db_dependent/Reserves.t:270:# 9788a: GetReservesFromItemnumber does not return future next available hold ./t/db_dependent/Reserves.t:280:my @results= GetReservesFromItemnumber($itemnumber); ./t/db_dependent/Reserves.t:281:is(defined $results[3]?1:0, 0, 'GetReservesFromItemnumber does not return a future next available hold'); ./t/db_dependent/Reserves.t:282:# 9788b: GetReservesFromItemnumber does not return future item level hold ./t/db_dependent/Reserves.t:287:my @results= GetReservesFromItemnumber($itemnumber); ./t/db_dependent/Reserves.t:288:is(defined $results[3]?1:0, 0, 'GetReservesFromItemnumber does not return a future item level hold'); ./t/db_dependent/Reserves.t:289:# 9788c: GetReservesFromItemnumber returns future wait (confirmed future hold) ./t/db_dependent/Reserves.t:291:my @results= GetReservesFromItemnumber($itemnumber); ./t/db_dependent/Reserves.t:292:is(defined $results[3]?1:0, 1, 'GetReservesFromItemnumber returns a future wait (confirmed future hold)'); ./t/db_dependent/Holds.t:79:my ( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); ./t/db_dependent/Holds.t:80:ok($reserve_id, "Test GetReservesFromItemnumber()"); ./t/db_dependent/Holds.t:95:( $reservedate, $borrowernumber, $branchcode, $reserve_id ) = GetReservesFromItemnumber($itemnumber); mtompset@ubuntu:~/kohaclone$ perldoc C4/Reserves.pm | grep GetReservesFromItemnumber ( $reservedate, $borrowernumber, $branchcode, $reserve_id, $waitingdate ) = GetReservesFromItemnumber($itemnumber); mtompset@ubuntu:~/kohaclone$
Created attachment 22615 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail.
(In reply to M. Tompsett from comment #37) > FAIL t/db_dependent/Reserves.t > FAIL valid > "my" variable @results masks earlier declaration in same > scope > Oops! Caught this while looking at the other patch, not signed off patch. I > really should run this all the time. Sorry, I should not have signed off. Thanks for testing. Easy fix. Deleted two my's.
(In reply to M. Tompsett from comment #38) > It seems that initially, the code was NOT called with a parameter passed for > $alldates, and so the unless statement was always run. This is why it was > moved up into the SQL, which is working. I suppose ordering by priority is > considered part of the improvements too. And with the addition of the > waitingdate into the SQL statement there is an improvement by including a > confirmed future hold. Though, this is the part that makes me feel > uncomfortable signing this off, even though it probably is okay. Thanks for your comments. Note that ordering by priority is a logical step. It actually solves the following very small bug: If you have two item-level holds, the first is on date A and the second is on date A-1, the second hold will have priority 1, but catalogue/detail will show the first hold with priority 2 (because priority was not used but just record order). This bug does not really hurt, it is just wrong presentation. The priority sort fixes it. On the future hold: Could you further clarify? What makes you feel uncomfortable about that part? The idea behind this is: With ConfirmFutureHolds enabled, you confirmed a hold for tomorrow. Currently, the routine checks <= NOW and thus ignores the fact that the book is already waiting for borrower A (with highest priority) and could say that it is still available (when no holds are returned) but it is no longer. Makes that sense for you?
Created attachment 22640 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22641 [details] [review] Bug 9788: Followup removing the alldates parameter Before bug 9788 the alldates parameter of GetReservesFromItemnumber was actually not used in the codebase. The first patch of bug 9788 did change that and passed true by default. But a closer look revealed that we do not really need it. The parameter is removed by this patch; the SQL statement is slightly adjusted: if reservedate<=now or a waitingdate is filled for the requested itemnumber, GetReservesFromItemnumber will return the reserve. This includes so-called future waits: a future hold that has been confirmed ahead of time with pref ConfirmFutureHolds > 0 days. Note that future item-level holds are not really interesting to return; this just corresponds to original behavior. Future next-available holds are not in view at all; they do not contain an item number. Test plan: Actually, the test plan of the first patch is valid. But for completeness I repeat it here: [1] Enable future holds and set ConfirmFutureHolds to 2 days. [2] Place a future next-available hold for 2 days ahead. [3] Check item status on catalogue detail. Available? That is fine. [4] Confirm the future hold by checking it in. ('future wait') [5] Look at item status again on catalogue detail. Must be Waiting now. [6] Switch to OPAC and login as another opac user. Goto Place a hold. [7] Check item status with item level hold info. Is it waiting? [8] Try to place hold in staff, check item level status again. Waiting? [9] Make a transfer for the item. Switch branch. Check hold status on Transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22642 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22643 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22644 [details] [review] Bug 9788: Followup removing the alldates parameter Before bug 9788 the alldates parameter of GetReservesFromItemnumber was actually not used in the codebase. The first patch of bug 9788 did change that and passed true by default. But a closer look revealed that we do not really need it. The parameter is removed by this patch; the SQL statement is slightly adjusted: if reservedate<=now or a waitingdate is filled for the requested itemnumber, GetReservesFromItemnumber will return the reserve. This includes so-called future waits: a future hold that has been confirmed ahead of time with pref ConfirmFutureHolds > 0 days. Note that future item-level holds are not really interesting to return; this just corresponds to original behavior. Future next-available holds are not in view at all; they do not contain an item number. Test plan: Actually, the test plan of the first patch is valid. But for completeness I repeat it here: [1] Enable future holds and set ConfirmFutureHolds to 2 days. [2] Place a future next-available hold for 2 days ahead. [3] Check item status on catalogue detail. Available? That is fine. [4] Confirm the future hold by checking it in. ('future wait') [5] Look at item status again on catalogue detail. Must be Waiting now. [6] Switch to OPAC and login as another opac user. Goto Place a hold. [7] Check item status with item level hold info. Is it waiting? [8] Try to place hold in staff, check item level status again. Waiting? [9] Make a transfer for the item. Switch branch. Check hold status on Transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22645 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 22646 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA comment: Looks good. 3 remarks: 1/ format_date should be removed (use the TT plugin) - not introduced by these patches 2/ CURRENT_DATE is a mysqlism 3/ I would prefer GetReservesFromItemnumber returns only a reserve hashref (what I did for bug 8367). I will provide a followup for 1 and 2. Marked as Passed QA.
Created attachment 22685 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22686 [details] [review] Bug 9788: Followup removing the alldates parameter Before bug 9788 the alldates parameter of GetReservesFromItemnumber was actually not used in the codebase. The first patch of bug 9788 did change that and passed true by default. But a closer look revealed that we do not really need it. The parameter is removed by this patch; the SQL statement is slightly adjusted: if reservedate<=now or a waitingdate is filled for the requested itemnumber, GetReservesFromItemnumber will return the reserve. This includes so-called future waits: a future hold that has been confirmed ahead of time with pref ConfirmFutureHolds > 0 days. Note that future item-level holds are not really interesting to return; this just corresponds to original behavior. Future next-available holds are not in view at all; they do not contain an item number. Test plan: Actually, the test plan of the first patch is valid. But for completeness I repeat it here: [1] Enable future holds and set ConfirmFutureHolds to 2 days. [2] Place a future next-available hold for 2 days ahead. [3] Check item status on catalogue detail. Available? That is fine. [4] Confirm the future hold by checking it in. ('future wait') [5] Look at item status again on catalogue detail. Must be Waiting now. [6] Switch to OPAC and login as another opac user. Goto Place a hold. [7] Check item status with item level hold info. Is it waiting? [8] Try to place hold in staff, check item level status again. Waiting? [9] Make a transfer for the item. Switch branch. Check hold status on Transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22687 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22688 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22689 [details] [review] Bug 9788: QA followup 1/ CURRENT_DATE is a mysql and should be replaced with CAST(now() AS date). 2/ The date formatting should be done in the template (using the TT plugin). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 22693 [details] [review] Bug 9788: QA followup 1/ CURRENT_DATE is a mysql and should be replaced with CAST(now() AS date). 2/ The date formatting should be done in the template (using the TT plugin). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #49) > 1/ format_date should be removed (use the TT plugin) - not introduced by > these patches > 2/ CURRENT_DATE is a mysqlism > 3/ I would prefer GetReservesFromItemnumber returns only a reserve hashref > (what I did for bug 8367). > > I will provide a followup for 1 and 2. Thanks, Jonathan. I would suggest to open a new report for your third point.
Created attachment 23931 [details] [review] Bug 9788: Improvements when calling GetReservesFromItemnumber Description: GetReservesFromItemnumber also returns waiting date. Simplifies some code. Improves item status display on catalogue detail, when placing a hold at opac-reserve and in staff, and on transfers to receive form. This patch builds on work from reports 9367 and 9761. Test plan: Place a future next-av. hold (enable future holds prefs), say 2 days ahead. Check item status on catalogue detail. Nothing to see. Enable ConfirmFutureHolds by inserting a number of days, say 2. Confirm earlier hold by checking it in. Look at item status again on detail. Switch to other opac user. Try to place a hold again. Check item status with item level hold info. Try to place hold in staff, check item level status. Make a transfer for that item. Switch branch. Look at transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 23932 [details] [review] Bug 9788: Followup removing the alldates parameter Before bug 9788 the alldates parameter of GetReservesFromItemnumber was actually not used in the codebase. The first patch of bug 9788 did change that and passed true by default. But a closer look revealed that we do not really need it. The parameter is removed by this patch; the SQL statement is slightly adjusted: if reservedate<=now or a waitingdate is filled for the requested itemnumber, GetReservesFromItemnumber will return the reserve. This includes so-called future waits: a future hold that has been confirmed ahead of time with pref ConfirmFutureHolds > 0 days. Note that future item-level holds are not really interesting to return; this just corresponds to original behavior. Future next-available holds are not in view at all; they do not contain an item number. Test plan: Actually, the test plan of the first patch is valid. But for completeness I repeat it here: [1] Enable future holds and set ConfirmFutureHolds to 2 days. [2] Place a future next-available hold for 2 days ahead. [3] Check item status on catalogue detail. Available? That is fine. [4] Confirm the future hold by checking it in. ('future wait') [5] Look at item status again on catalogue detail. Must be Waiting now. [6] Switch to OPAC and login as another opac user. Goto Place a hold. [7] Check item status with item level hold info. Is it waiting? [8] Try to place hold in staff, check item level status again. Waiting? [9] Make a transfer for the item. Switch branch. Check hold status on Transfers to receive. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 23933 [details] [review] Bug 9788: Followup for expirationdate in Letter.pm Pasting comment from the Bugzilla report: Looking bit longer at this code, it is kind of strange to find it there in the first place. Adding maxpickupdelay in Letters.pm should not be there, but it is.. Also this date is not used normally in the default HOLD Available for Pickup notice (that we are generating in this case). And if it would be undef, the expiration date should imo be empty instead of today+0. (before adding maxreservespickupdelay, you should test the allowexpire pref first) So it is an (invisible) bug on its own. Test plan: See former patch. Kyle just discovered this bug, apparently by deleting the maxpickupdelay pref.. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 23934 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 23935 [details] [review] Bug 9788: QA followup 1/ CURRENT_DATE is a mysql and should be replaced with CAST(now() AS date). 2/ The date formatting should be done in the template (using the TT plugin). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
One of the patches needed a rebase for changes in Reserves.t. Galen: This patch waits for some months already in PQA and was written long before that. Could you explain why this patch cannot be pushed? What should be improved?
Created attachment 23936 [details] [review] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber Adds three tests to Reserves.t for GetReservesFromItemnumber. We test if this routine does not return a future next available hold, a future item level hold. And if it does return a future wait (that is: a confirmed future hold, using ConfirmFutureHolds). Note that Holds.t does also contains some basic tests for this routine, but the additional tests seem to better located in the direct context of tests for bug 9761 for ConfirmFutureHolds. Test plan: Run both t/db_dependent/Holds.t and t/db_dependent/Reserves.t. Verify if both tests do not fail. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
I expect to push this some time on Thursday.
(In reply to Galen Charlton from comment #64) > I expect to push this some time on Thursday. Good to hear. Thanks.
Pushed to master, along with a follow-up updating the Bootstrap them. Thanks, Marcel!
Patches pushed to 3.14.x, will be in 3.14.5