http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16026|0 |1 is obsolete| | --- Comment #3 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 16092 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16092&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Improvements when calling |Improvements for calling |GetReservesFromItemnumber |GetReservesFromItemnumber -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Koha Team Lyon 3 <koha@univ-lyon3.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@univ-lyon3.fr --- Comment #4 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #7 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #9 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 19192 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19192&action=edit 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.. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #16092|0 |1 is obsolete| | --- Comment #11 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19193 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19193&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19192|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19194 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19194&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19194|0 |1 is obsolete| | --- Comment #14 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 19941 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19941&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19941|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19941|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19941|0 |1 is obsolete| | --- Comment #18 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 19944 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19944&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #20 from Kyle M Hall <kyle@bywatersolutions.com> --- Question, would it not be better to use the KohaDates TT plugin, rather than using format_date in the source code? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #22 from Kyle M Hall <kyle@bywatersolutions.com> --- 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?

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #24 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|9761, 9367 | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #26 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 21481 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=21481&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #28 from Galen Charlton <gmcharlt@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #30 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19944|0 |1 is obsolete| | --- Comment #32 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 22309 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22309&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 I'm just a bot <gitbot@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- When did the bot|2013-09-29 00:00:00 |2013-10-30 last check this| | --- Comment #34 from I'm just a bot <gitbot@bugs.koha-community.org> --- Patch applied cleanly, go forth and signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19943|Bug 9788: Improvements when |[SIGNED_OFF] Bug 9788: description|calling |Improvements when calling |GetReservesFromItemnumber |GetReservesFromItemnumber -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #36 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 22610 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22610&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22610|0 |1 is obsolete| | --- Comment #37 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 22610 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22610 [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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #38 from M. Tompsett <mtompset@hotmail.com> --- 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$ -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #40 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #21479|0 |1 is obsolete| | --- Comment #43 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22641 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22641&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19943|0 |1 is obsolete| | Attachment #22309|0 |1 is obsolete| | Attachment #22640|0 |1 is obsolete| | Attachment #22641|0 |1 is obsolete| | Attachment #22642|0 |1 is obsolete| | --- Comment #45 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22643 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22643&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #47 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22645 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22645&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|Small patch |Medium patch QA Contact| |jonathan.druart@biblibre.co | |m --- Comment #49 from Jonathan Druart <jonathan.druart@biblibre.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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22644|0 |1 is obsolete| | --- Comment #51 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 22686 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22686&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22646|0 |1 is obsolete| | --- Comment #53 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 22688 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22688&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22689|0 |1 is obsolete| | --- Comment #55 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 22693 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22693&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |9823 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22686|0 |1 is obsolete| | --- Comment #58 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 23932 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23932&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22688|0 |1 is obsolete| | --- Comment #60 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 23934 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23934&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #62 from M. 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 --- Comment #64 from Galen Charlton <gmcharlt@gmail.com> --- I expect to push this some time on Thursday. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #66 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master, along with a follow-up updating the Bootstrap them. Thanks, Marcel! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9788 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolyn.somers@biblibre.co | |m --- Comment #67 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Patches pushed to 3.14.x, will be in 3.14.5 -- You are receiving this mail because: You are watching all bug changes.