Bug 9788 - Improvements for calling GetReservesFromItemnumber
Summary: Improvements for calling GetReservesFromItemnumber
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-11 14:26 UTC by Marcel de Rooy
Modified: 2015-06-04 23:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9788: Improvements when calling GetReservesFromItemnumber (9.06 KB, patch)
2013-03-11 14:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Improvements when calling GetReservesFromItemnumber (9.83 KB, patch)
2013-03-13 14:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (1.93 KB, patch)
2013-06-21 16:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Improvements when calling GetReservesFromItemnumber (9.84 KB, patch)
2013-06-21 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (1.94 KB, patch)
2013-06-21 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Improvements when calling GetReservesFromItemnumber (10.00 KB, patch)
2013-07-25 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (1.98 KB, patch)
2013-07-25 08:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED_OFF] Bug 9788: Improvements when calling GetReservesFromItemnumber (10.01 KB, patch)
2013-07-25 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (1.98 KB, patch)
2013-07-25 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Followup removing the alldates parameter (6.25 KB, patch)
2013-09-26 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (2.99 KB, patch)
2013-09-26 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.07 KB, patch)
2013-10-03 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED_OFF] Bug 9788: Followup for expirationdate in Letter.pm (2.02 KB, patch)
2013-10-23 12:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.20 KB, patch)
2013-10-30 17:19 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.07 KB, patch)
2013-10-31 07:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.08 KB, patch)
2013-10-31 13:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Followup removing the alldates parameter (6.26 KB, patch)
2013-10-31 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (1.98 KB, patch)
2013-10-31 13:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Improvements when calling GetReservesFromItemnumber (9.97 KB, patch)
2013-10-31 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Followup removing the alldates parameter (6.26 KB, patch)
2013-10-31 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (1.98 KB, patch)
2013-10-31 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.08 KB, patch)
2013-10-31 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9788: Improvements when calling GetReservesFromItemnumber (10.03 KB, patch)
2013-11-04 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9788: Followup removing the alldates parameter (6.33 KB, patch)
2013-11-04 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (2.05 KB, patch)
2013-11-04 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.14 KB, patch)
2013-11-04 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9788: QA followup (7.20 KB, patch)
2013-11-04 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9788: QA followup (7.24 KB, patch)
2013-11-04 11:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Improvements when calling GetReservesFromItemnumber (10.07 KB, patch)
2014-01-06 12:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Followup removing the alldates parameter (6.37 KB, patch)
2014-01-06 12:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Followup for expirationdate in Letter.pm (2.08 KB, patch)
2014-01-06 12:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.00 KB, patch)
2014-01-06 12:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: QA followup (7.26 KB, patch)
2014-01-06 12:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9788: Unit tests for changed routine GetReservesFromItemnumber (3.14 KB, patch)
2014-01-06 12:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-03-11 14:26:18 UTC
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.
Comment 1 Marcel de Rooy 2013-03-11 14:28:31 UTC
Will still add some additional information for testing.
Comment 2 Marcel de Rooy 2013-03-11 14:28:54 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-03-13 14:37:08 UTC Comment hidden (obsolete)
Comment 4 Koha Team Lyon 3 2013-03-19 13:07:24 UTC
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.
Comment 5 Marcel de Rooy 2013-03-19 15:49:16 UTC
(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.
Comment 6 Kyle M Hall 2013-06-21 12:34:14 UTC
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
Comment 7 Marcel de Rooy 2013-06-21 15:54:14 UTC
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..
Comment 8 Marcel de Rooy 2013-06-21 16:48:50 UTC
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..
Comment 9 Marcel de Rooy 2013-06-21 16:52:07 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2013-06-21 17:02:28 UTC
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..
Comment 11 Kyle M Hall 2013-06-21 17:47:36 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-06-21 17:47:48 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2013-07-25 08:56:08 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2013-07-25 08:56:15 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2013-07-25 09:00:09 UTC
Rebased both patches (for changes from bug 9394).
Tested them again.
Comment 16 Marcel de Rooy 2013-07-25 09:03:56 UTC
Still see something. Will submit again.
Comment 17 Marcel de Rooy 2013-07-25 09:52:10 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2013-07-25 09:52:43 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2013-07-25 09:53:39 UTC
Ready for QA again. Note that they depend on bug 9761.
Comment 20 Kyle M Hall 2013-08-16 16:16:24 UTC
Question, would it not be better to use the KohaDates TT plugin, rather than using format_date in the source code?
Comment 21 Marcel de Rooy 2013-08-19 06:44:39 UTC
(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?
Comment 22 Kyle M Hall 2013-08-19 12:04:28 UTC
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?
Comment 23 Jonathan Druart 2013-09-12 14:28:52 UTC
This patch will be in conflict with bug 8367.
Comment 24 Marcel de Rooy 2013-09-25 14:43:44 UTC
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.
Comment 25 Marcel de Rooy 2013-09-26 09:49:47 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2013-09-26 10:30:51 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2013-09-26 10:41:45 UTC
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 :)
Comment 28 Galen Charlton 2013-09-26 16:23:01 UTC
(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?
Comment 29 I'm just a bot 2013-09-29 06:21:06 UTC
Patch applied cleanly, go forth and signoff
Comment 30 Jonathan Druart 2013-09-30 13:40:03 UTC
(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.
Comment 31 Marcel de Rooy 2013-10-03 10:16:05 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2013-10-23 12:19:01 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2013-10-23 12:20:41 UTC
Rebased for different parameter passing to output_pref.
Comment 34 I'm just a bot 2013-10-30 07:52:39 UTC
Patch applied cleanly, go forth and signoff
Comment 35 Marcel de Rooy 2013-10-30 15:22:35 UTC
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?
Comment 36 M. Tompsett 2013-10-30 17:19:20 UTC Comment hidden (obsolete)
Comment 37 M. Tompsett 2013-10-30 18:15:38 UTC
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.
Comment 38 M. Tompsett 2013-10-30 18:20:27 UTC
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$
Comment 39 Marcel de Rooy 2013-10-31 07:45:06 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2013-10-31 07:46:24 UTC
(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.
Comment 41 Marcel de Rooy 2013-10-31 08:21:56 UTC
(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?
Comment 42 Kyle M Hall 2013-10-31 13:22:30 UTC Comment hidden (obsolete)
Comment 43 Kyle M Hall 2013-10-31 13:29:48 UTC Comment hidden (obsolete)
Comment 44 Kyle M Hall 2013-10-31 13:29:59 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2013-10-31 13:31:28 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2013-10-31 13:31:35 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2013-10-31 13:31:41 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2013-10-31 13:31:47 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2013-11-04 10:55:32 UTC
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.
Comment 50 Jonathan Druart 2013-11-04 10:58:22 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2013-11-04 10:58:30 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2013-11-04 10:58:36 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2013-11-04 10:58:42 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2013-11-04 10:58:48 UTC Comment hidden (obsolete)
Comment 55 Marcel de Rooy 2013-11-04 11:33:50 UTC Comment hidden (obsolete)
Comment 56 Marcel de Rooy 2013-11-04 11:35:45 UTC
(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.
Comment 57 Marcel de Rooy 2014-01-06 12:44:18 UTC
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>
Comment 58 Marcel de Rooy 2014-01-06 12:44:27 UTC
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>
Comment 59 Marcel de Rooy 2014-01-06 12:44:33 UTC
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>
Comment 60 Marcel de Rooy 2014-01-06 12:44:38 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2014-01-06 12:44:43 UTC
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>
Comment 62 Marcel de Rooy 2014-01-06 12:48:12 UTC
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?
Comment 63 Marcel de Rooy 2014-01-06 12:58:27 UTC
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>
Comment 64 Galen Charlton 2014-01-16 05:24:00 UTC
I expect to push this some time on Thursday.
Comment 65 Marcel de Rooy 2014-01-16 07:45:35 UTC
(In reply to Galen Charlton from comment #64)
> I expect to push this some time on Thursday.

Good to hear. Thanks.
Comment 66 Galen Charlton 2014-01-17 05:23:48 UTC
Pushed to master, along with a follow-up updating the Bootstrap them. Thanks, Marcel!
Comment 67 Fridolin SOMERS 2014-03-18 08:31:58 UTC
Patches pushed to 3.14.x, will be in 3.14.5