Hi Kyle,

as holds are quite complex - could you please flesh out your test plan some more? Just testing "everything" is not likely to get done by testers. I wouldn't feel confident doing that.

I see changes to 4 C4 modules, heavy changes to Reserves.pm, new routines and no unit tests.

This change has a big risk of breaking something at the very core of Koha. I would prefer this being broken up into smaller testable pieces with unit tests.

Test Plan:
 1) Apply patch
 2) Pick an item, place a hold on it for 5 patrons
 3) Test the ability to reorder the patrons for the hold
 4) Test the ability to set 'lowest priority'
 5) Test the ability to suspend a hold
 6) Fill the hold for the first patron
 7) Verify it's waiting status
 8) Check it out to the patron
 9) Return the item
10) Add another hold to the item via the OPAC
11) Cancel a hold via the OPAC
12) Cancel a hold via the staff interface

For changes of this scope there really needs to be a unit test that will check for regressions.

(In reply to comment #6)
> Created attachment 14863 [details] [review] [review]
> Bug 9394 - Use reserve_id where possible
> 
> This patch switches from using a combination of
> biblionumber/borrowernumber to using reserve_id where possible.
> 
> Test Plan:
> 1) Apply patch
> 2) Run t/db_dependent/Holds.t

I tried running the test, but it spat out quite a few errors/fails. I'm not sure if this is a problem with the test or my database though...

Here are the test results:

t/db_dependent/Holds.t .. 1/18 # Creating biblio instance for testing.
# Creating item instance for testing.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`...`.`items`, CONSTRAINT `items_ibfk_2` FOREIGN KEY (`homebranch`) REFERENCES `branches` (`branchcode`) ON UPDATE CASCADE) at .../lib//C4/Items.pm line 2105.
t/db_dependent/Holds.t .. 2/18
#   Failed test 'Test GetReservesFromItemnumber()'
#   at t/db_dependent/Holds.t line 68.

#   Failed test 'Test CancelReserve()'
#   at t/db_dependent/Holds.t line 80.
Use of uninitialized value in string eq at t/db_dependent/Holds.t line 92.

#   Failed test 'Test GetReserve(), priority changed correctly'
#   at t/db_dependent/Holds.t line 92.

#   Failed test 'Test GetReserve(), suspend hold'
#   at t/db_dependent/Holds.t line 93.
Use of uninitialized value in string eq at t/db_dependent/Holds.t line 94.

#   Failed test 'Test GetReserve(), suspend until date'
#   at t/db_dependent/Holds.t line 94.
Use of uninitialized value in string eq at t/db_dependent/Holds.t line 102.

#   Failed test 'Test ToggleSuspend(), with date'
#   at t/db_dependent/Holds.t line 102.
DBD::mysql::st execute failed: Column 'borrowernumber' cannot be null at .../lib//C4/Reserves.pm line 198.
Use of uninitialized value in string eq at t/db_dependent/Holds.t line 126.
Use of uninitialized value $itemnumber in string eq at t/db_dependent/Holds.t line 126.
Use of uninitialized value in string eq at t/db_dependent/Holds.t line 130.
Use of uninitialized value in string eq at t/db_dependent/Holds.t line 130.
# Deleting holds.
# Deleting item testing instance.
DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 at .../lib//C4/Items.pm line 2295.
# Deleting biblio testing instance.
# Looks like you failed 6 tests of 18.
t/db_dependent/Holds.t .. Dubious, test returned 6 (wstat 1536, 0x600)
Failed 6/18 subtests

Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 1536 Tests: 18 Failed: 6)
  Failed tests:  3, 6-9, 11
  Non-zero exit status: 6
Files=1, Tests=18,  2 wallclock secs ( 0.05 usr  0.01 sys +  0.82 cusr  0.15 csys =  1.03 CPU)
Result: FAIL

Holds.t is db dependent and requires the use of the default sample data ( or at least a branch with the branchcode CPL ).

Did you run the test on a system using the sample data?

(In reply to comment #8)
> Holds.t is db dependent and requires the use of the default sample data ( or
> at least a branch with the branchcode CPL ).
> 
> Did you run the test on a system using the sample data?

I figured that might be the issue. 

Do you mean the sample data that comes pre-packaged with an install? No. I used some local sample data. 

Is there an SQL dump that I could use with master to provide this data, or would it be better just to create a branch with the branchcode CPL?

I noticed a 3.6 Koha database SQL dump in the Wiki somewhere. Is that the sample one, which would just need to be updated using updatedatabase.pl?

> Do you mean the sample data that comes pre-packaged with an install? No. I
> used some local sample data. 

Yes, that's what I meant. However, all you really need to do is create a branch with the branchcode CPL. That's all it really needs.

Please don't limit testing to running the test suite, but also check the test plan in comment 3 and beyond. This is quite a big change.

Something went wrong !
Applying: Bug 9394 - Use reserve_id where possible
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Biblio.pm
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Auto-merging circ/circulation.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 9394 - Use reserve_id where possible
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 9394 - Use reserve_id where possible

Bug 9394 - Use reserve_id where possible
Apply? [yn]
Patch left in /tmp/Bug-9394---Use-reserveid-where-possible-R9Io0H.patch

We tried to test but patch uses syspref "MaxHoldsPerRecord" witch is introduced by Bug 7710.

Hi Kyle, this is a bit confusing - it seems 7710 depends on 9394 and vice versa?

(In reply to comment #17)
> Hi Kyle, this is a bit confusing - it seems 7710 depends on 9394 and vice
> versa?

According to the dependency tree, 7710 depends on 9394, which is correct. 7710 originally included all the code in the patch for 9394, but was split into two patches for ease of testing. It looks like a couple lines that should have been moved to 7710 were still here in 9394. This has now been corrected.

Interested in Bug 7710, but this is needed, so

Small test plan,
2) Run t/db_dependent/Holds.t ... All tests successful.

Big test plan,
 1) Apply patch
 2) Pick an item, place a hold on it for 5 patrons
Ok
 3) Test the ability to reorder the patrons for the hold
Not working (pre patch yes)
 4) Test the ability to set 'lowest priority'
Not working (pre patch yes)
 5) Test the ability to suspend a hold
Not working (pre patch yes)
 6) Fill the hold for the first patron
(what's this?)
 7) Verify it's waiting status
Not working (run build_holds_queue.pl, no holds showing, pre patch yes)
 8) Check it out to the patron
Ok
 9) Return the item
Ok
10) Add another hold to the item via the OPAC
Ok
11) Cancel a hold via the OPAC
Ok
12) Cancel a hold via the staff interface
Not working (pre patch yes)

Pre & post patch there are log messages (not related with this patch)
request.pl: Use of uninitialized value in string eq at /home/bgkriegel/kohaclone/reserve/request.pl line 521.
Any idea?

Kyle, is there anything else I need to touch to test this?

No koha-qa errors :-)

That looks good to me!

> Kyle, is there anything else I need to touch to test this?
> 
> No koha-qa errors :-)

Followup adds the lines from 7710 that needed to be in this patch set instead.

Sandbox setup by unknown with database 2 and bug 9394 on Thu May 23 09:50:10 2013

Something went wrong !
Applying: Bug 9394 - Use reserve_id where possible
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Reserves.pm
Auto-merging circ/circulation.pl
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt
Auto-merging reserve/request.pl
CONFLICT (content): Merge conflict in reserve/request.pl
Auto-merging serials/routing-preview.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 9394 - Use reserve_id where possible
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 9394 - Use reserve_id where possible

Rebased and signed off patch
Apply? [yn]
Patch left in /tmp/Rebased-and-signed-off-patch-FnVHt7.patch

For me applies on a today's fresh master.
I re-set to signed-off.

QA comment:

It's a great patch Kyle, well done!

I found some problems while testing your patch, I will provide a followup.

I tested some use cases but not all. It would be good if this patch is QAed by someone else from the QA team (in order to have another pair of eyes on this patch).

Having this patch into master asap would permit to have a lot of testers. I think it is better than blocking this patch any longer.

Marked as Passed QA

*** Bug 5696 has been marked as a duplicate of this bug. ***

(In reply to comment #29)
> Created attachment 18466 [details] [review] [review]
> Bug 9394: QA Followup
> 
> * C4::Reserves::GetReserveId
> This subroutine did not have a unit test.

This new test fails for me. You pass an itemnumber to GetReserveId after an AddReserve with no itemnumber, so GetReserveId returns the wrong of the 2 existing reserves at that point. We could either remove the "itemnumber => $itemnumber," line or add modify the AddReserve above it. What do you think ?

(In reply to comment #31)
> (In reply to comment #29)
> > Created attachment 18466 [details] [review] [review] [review]
> > Bug 9394: QA Followup
> > 
> > * C4::Reserves::GetReserveId
> > This subroutine did not have a unit test.
> 
> This new test fails for me. You pass an itemnumber to GetReserveId after an
> AddReserve with no itemnumber, so GetReserveId returns the wrong of the 2
> existing reserves at that point. We could either remove the "itemnumber =>
> $itemnumber," line or add modify the AddReserve above it. What do you think ?

Ho yes, of course! 50% chance, the test passes on my instance...
I will resubmit the patch and switch back the status to Needs Signoff!

Thanks Kyle.

Since the last patch (qa followup) is signed off, I mark the 2 patches as Passed QA.

(see comment 27)

I've started reviewing this.  One thing that jumped out -- the QA follow-up (re?)introduces a change in the behavior of AlterPriority that should get a test case.  I'll deal with that with a follow-up patch myself unless I find other reason to bounce back to QA.

(In reply to comment #36)
> I've started reviewing this.  One thing that jumped out -- the QA follow-up
> (re?)introduces a change in the behavior of AlterPriority that should get a
> test case.  I'll deal with that with a follow-up patch myself unless I find
> other reason to bounce back to QA.

Good catch! I'm re-evaluating it right now and I don't think it's going to affect anything. Koha does not expose canceled reserves iirc, so a scenario where a canceled reserve has it's priority altered shouldn't come up. That may be why Jonathan added the check.

(In reply to comment #37)
> Good catch! I'm re-evaluating it right now and I don't think it's going to
> affect anything. Koha does not expose canceled reserves iirc, so a scenario
> where a canceled reserve has it's priority altered shouldn't come up. That
> may be why Jonathan added the check.

Indeed; I understand the purpose of the check, I'm just trying to encourage folks to write unit tests for most any change to core routines, even obvious ones.

Update during testing; I discover that the ability to cancel holds from the patron summary (i.e., members/moremember.pl) was broken.  A follow-up along the lines of Jonathan's update to circulation.tt fixes it.

This is about ready to go.  I've attached the patches along with several follows; setting status to signed-off for a final QA pass, particularly of the follow-ups.

Created attachment 19902 [details] [review]
Bug 9394: Use reserve_id where possible

This patch switches from using a combination of
biblionumber/borrowernumber to using reserve_id where possible.

Test Plan:
1) Apply patch
2) Run t/db_dependent/Holds.t

Signed-off-by: Maxime Pelletier <maxime.pelletier@libeo.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 19904 [details] [review]
Bug 9394: QA Followup

* C4::Reserves::_FixPriority
  - The previous code checked the cancellationdate. If think you never pass
in it with bad parameters, but in order to be sure I added the check on
this value.
  - The reservedates array was never used.

* circ/circulation.tt
There was a bug: it was not possible to remove an hold from the
circulation page. Passing reserve_id fixes the issue.

* C4::Reserves::GetReserveId
This subroutine did not have a unit test.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 19905 [details] [review]
Bug 9394: (follow-up) modernize test cases

- wrap in a transaction
- create the patron records needed for the test

To test:

[1] Run prove -v t/db_dependent/Holds.t
[2] Verify that all tests have passed.
[3] Verify that the additional patron records created
    by the test no longer exist in the database.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 19906 [details] [review]
Bug 9394: (follow-up) silence spurious warns

One just needed parentheses, but the one on line 219 reflects
a slightly deeper problem -- it's running patron checks when
a patron hasn't actually been supplied.

To test:

[1] From the staff interface, bring up a biblio that has no
    holds on it, then go to the holds tab.
[2] Verify that the Apache log has no new warnings containing:

request.pl: Use of uninitialized value in string eq at /home/gmc/koha/reserve/request.pl line 219

or

request.pl: Use of uninitialized value in string eq at /home/gmc/koha/reserve/request.pl line 488

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 19907 [details] [review]
Bug 9394: (follow-up) restore ability to cancel holds from the patron summary

To test:

- Place a hold request for a patron
- From the patron details page, navigate to the holds table,
  then cancel (delete) it.
- Verify that the hold request is gone.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 19908 [details] [review]
Bug 9394: (follow-up) fix query column alias

A change-and-replace went a tick too far.  This patch
adjusts the column alias in the query run in MergeHolds()
to reflect that the value being returned is the number of
hold requests, not an ID.

To test:

[1] This patch should have no visible changes to behavior.  To
    verify, pick to bib records that have hold requests on them,
    then merge them together.  Verify that the merged bib
    contains sll of the hold requests on it.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 19909 [details] [review]
Bug 9394: (follow-up) stylistic tidying

- fix identation in one line
- remove a commented-out warn

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Pushed to master.  Thanks, Kyle!