Bug 9394 - Use reserve_id where possible
Summary: Use reserve_id where possible
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
: 5696 (view as bug list)
Depends on:
Blocks: 5696 7710 10247 10949
  Show dependency treegraph
 
Reported: 2013-01-15 15:13 UTC by Kyle M Hall
Modified: 2015-06-16 14:23 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Large patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9394 - Use reserve_id where possible (43.83 KB, patch)
2013-01-15 15:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (52.15 KB, patch)
2013-01-25 14:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (49.27 KB, patch)
2013-01-25 15:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (50.28 KB, patch)
2013-02-27 18:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (50.28 KB, patch)
2013-02-27 19:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (50.31 KB, patch)
2013-03-21 16:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (49.98 KB, patch)
2013-04-15 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible - Followup (7.00 KB, patch)
2013-05-17 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Rebased and signed off patch (56.90 KB, patch)
2013-05-22 18:36 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 9394 - Use reserve_id where possible (56.94 KB, patch)
2013-05-29 13:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9394: QA Followup (4.11 KB, patch)
2013-05-29 13:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9394: QA Followup (4.07 KB, patch)
2013-05-30 07:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9394: QA Followup (4.11 KB, patch)
2013-06-03 11:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9394: Use reserve_id where possible (56.99 KB, patch)
2013-07-23 22:21 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: QA Followup (4.17 KB, patch)
2013-07-23 22:22 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: (follow-up) modernize test cases (2.62 KB, patch)
2013-07-23 22:22 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: (follow-up) silence spurious warns (1.82 KB, patch)
2013-07-23 22:22 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: (follow-up) restore ability to cancel holds from the patron summary (1.97 KB, patch)
2013-07-23 22:22 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: (follow-up) fix query column alias (1.33 KB, patch)
2013-07-23 22:22 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: (follow-up) stylistic tidying (1.33 KB, patch)
2013-07-23 22:22 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 9394: Use reserve_id where possible (57.03 KB, patch)
2013-07-24 04:44 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9394: QA Followup (4.23 KB, patch)
2013-07-24 04:47 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9394: (follow-up) modernize test cases (2.68 KB, patch)
2013-07-24 04:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9394: (follow-up) silence spurious warns (1.87 KB, patch)
2013-07-24 04:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9394: (follow-up) restore ability to cancel holds from the patron summary (2.02 KB, patch)
2013-07-24 04:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9394: (follow-up) fix query column alias (1.38 KB, patch)
2013-07-24 04:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9394: (follow-up) stylistic tidying (1.38 KB, patch)
2013-07-24 05:01 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-01-15 15:13:45 UTC
This patch switches from using a combination of biblionumber/borrowernumber to using reserve_id where possible.
Comment 1 Kyle M Hall 2013-01-15 15:15:14 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2013-01-24 22:29:54 UTC
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.
Comment 3 Kyle M Hall 2013-01-25 12:27:18 UTC
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
Comment 4 Jared Camins-Esakov 2013-01-25 13:20:11 UTC
For changes of this scope there really needs to be a unit test that will check for regressions.
Comment 5 Kyle M Hall 2013-01-25 14:51:56 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-01-25 15:16:49 UTC Comment hidden (obsolete)
Comment 7 David Cook 2013-02-13 23:49:34 UTC
(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
Comment 8 Kyle M Hall 2013-02-14 13:08:47 UTC
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?
Comment 9 David Cook 2013-02-15 00:57:55 UTC
(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?
Comment 10 Kyle M Hall 2013-02-15 14:26:04 UTC
> 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.
Comment 11 Kyle M Hall 2013-02-27 18:22:18 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-02-27 19:15:13 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-02-28 10:42:00 UTC
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.
Comment 14 cedric.vita 2013-03-20 10:43:04 UTC
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
Comment 15 Kyle M Hall 2013-03-21 16:24:43 UTC Comment hidden (obsolete)
Comment 16 Fridolin Somers 2013-03-22 14:25:10 UTC
We tried to test but patch uses syspref "MaxHoldsPerRecord" witch is introduced by Bug 7710.
Comment 17 Katrin Fischer 2013-04-12 20:41:35 UTC
Hi Kyle, this is a bit confusing - it seems 7710 depends on 9394 and vice versa?
Comment 18 Kyle M Hall 2013-04-15 11:56:39 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-04-15 12:00:43 UTC
(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.
Comment 20 Bernardo Gonzalez Kriegel 2013-04-30 18:35:11 UTC
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 :-)
Comment 21 Kyle M Hall 2013-05-14 20:50:47 UTC
That looks good to me!

> Kyle, is there anything else I need to touch to test this?
> 
> No koha-qa errors :-)
Comment 22 Kyle M Hall 2013-05-17 18:27:19 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-05-17 18:31:00 UTC
Followup adds the lines from 7710 that needed to be in this patch set instead.
Comment 24 Maxime Pelletier 2013-05-22 18:36:31 UTC Comment hidden (obsolete)
Comment 25 cedric.vita 2013-05-23 07:54:58 UTC
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
Comment 26 Fridolin Somers 2013-05-23 12:54:25 UTC
For me applies on a today's fresh master.
I re-set to signed-off.
Comment 27 Jonathan Druart 2013-05-29 13:03:04 UTC
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
Comment 28 Jonathan Druart 2013-05-29 13:03:53 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-05-29 13:04:00 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2013-05-29 13:06:09 UTC
*** Bug 5696 has been marked as a duplicate of this bug. ***
Comment 31 Maxime Pelletier 2013-05-29 21:31:30 UTC
(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 ?
Comment 32 Jonathan Druart 2013-05-30 07:18:37 UTC
(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!
Comment 33 Jonathan Druart 2013-05-30 07:19:29 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2013-06-03 11:53:44 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2013-06-03 12:02:12 UTC
Thanks Kyle.

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

(see comment 27)
Comment 36 Galen Charlton 2013-06-10 16:31:03 UTC
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.
Comment 37 Kyle M Hall 2013-06-10 16:37:17 UTC
(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.
Comment 38 Galen Charlton 2013-06-10 16:49:52 UTC
(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.
Comment 39 Galen Charlton 2013-07-09 15:39:00 UTC
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.
Comment 40 Galen Charlton 2013-07-23 22:21:54 UTC Comment hidden (obsolete)
Comment 41 Galen Charlton 2013-07-23 22:22:02 UTC Comment hidden (obsolete)
Comment 42 Galen Charlton 2013-07-23 22:22:08 UTC Comment hidden (obsolete)
Comment 43 Galen Charlton 2013-07-23 22:22:17 UTC Comment hidden (obsolete)
Comment 44 Galen Charlton 2013-07-23 22:22:25 UTC Comment hidden (obsolete)
Comment 45 Galen Charlton 2013-07-23 22:22:31 UTC Comment hidden (obsolete)
Comment 46 Galen Charlton 2013-07-23 22:22:37 UTC Comment hidden (obsolete)
Comment 47 Galen Charlton 2013-07-23 22:23:25 UTC
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.
Comment 48 Chris Cormack 2013-07-24 04:44:54 UTC
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>
Comment 49 Chris Cormack 2013-07-24 04:47:58 UTC
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>
Comment 50 Chris Cormack 2013-07-24 04:50:10 UTC
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>
Comment 51 Chris Cormack 2013-07-24 04:51:20 UTC
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>
Comment 52 Chris Cormack 2013-07-24 04:57:08 UTC
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>
Comment 53 Chris Cormack 2013-07-24 04:58:16 UTC
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>
Comment 54 Chris Cormack 2013-07-24 05:01:39 UTC
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>
Comment 55 Galen Charlton 2013-07-24 05:05:43 UTC
Pushed to master.  Thanks, Kyle!