Bug 13478 - Force item holds only for specific biblios
Summary: Force item holds only for specific biblios
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13479
  Show dependency treegraph
 
Reported: 2014-12-18 10:58 UTC by Marcel de Rooy
Modified: 2017-01-03 15:46 UTC (History)
5 users (show)

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


Attachments
Bug 13478: Force item holds only for specific biblios (14.79 KB, patch)
2014-12-18 12:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13478: Database revision for table biblio_force_item_holds (2.24 KB, patch)
2015-01-05 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13478: Force item holds only for specific biblios (12.92 KB, patch)
2015-01-05 10:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13478: Database revision for table biblio_force_item_holds (2.21 KB, patch)
2015-02-26 15:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13478: Force item holds only for specific biblios (12.93 KB, patch)
2015-02-27 21:06 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 13478: Database revision for table biblio_force_item_holds (2.27 KB, patch)
2015-02-27 21:06 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 13478: Force item holds only for specific biblios (13.04 KB, patch)
2015-03-05 11:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13478: Database revision (new biblio column and pref description) (2.47 KB, patch)
2015-03-05 11:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED QA] Bug 13478: Force item holds only for specific biblios (13.06 KB, patch)
2015-03-13 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13478: Database revision (new biblio column and pref description) (2.50 KB, patch)
2015-03-13 14:42 UTC, Kyle M Hall
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 2014-12-18 10:58:04 UTC

    
Comment 1 Marcel de Rooy 2014-12-18 12:37:14 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2014-12-18 12:38:21 UTC
I will open a new report to add the option of adding to the list.
Comment 3 Marcel de Rooy 2015-01-05 10:48:59 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2015-01-05 10:49:03 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-02-26 15:30:54 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens (kidclamp) 2015-02-26 20:16:02 UTC
All seemed to work as expected except for the test in step 2 (output below):

t/db_dependent/Reserves.t .. 1/57 Subroutine C4::Context::userenv redefined at t/db_dependent/Reserves.t line 42.
t/db_dependent/Reserves.t .. 29/57 C4::Context->userenv not defined! at /usr/share/koha/lib/C4/Items.pm line 2282
Undefined subroutine &C4::Reserves::CheckBiblioForceItemHolds called at t/db_dependent/Reserves.t line 512.
# Looks like you planned 57 tests but ran 53.
# Looks like your test exited with 255 just after 53.
t/db_dependent/Reserves.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/57 subtests

Test Summary Report
-------------------
t/db_dependent/Reserves.t (Wstat: 65280 Tests: 53 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 57 tests but ran 53.
Files=1, Tests=53,  1 wallclock secs ( 0.02 usr  0.01 sys +  0.72 cusr  0.04 csys =  0.79 CPU)
Result: FAIL

Could be something I am missing but passed on master
Comment 7 Marcel de Rooy 2015-02-27 08:50:52 UTC
(In reply to Nick Clemens from comment #6)
Thanks for testing !

> t/db_dependent/Reserves.t .. 1/57 Subroutine C4::Context::userenv redefined
> at t/db_dependent/Reserves.t line 42.
This warning is as expected (just as it is now).

> t/db_dependent/Reserves.t .. 29/57 C4::Context->userenv not defined! at
> /usr/share/koha/lib/C4/Items.pm line 2282
I have this result: 
ok 30 - patron not notified a second time (bug 11445)
C4::Context->userenv not defined! at /usr/share/koha/testclone/C4/Items.pm line 2282.
This warning is also to be expected (we could call this a separate bug in master). Test number 31 calls  DelItemCheck($dbh, $bibnum, $itemnumber) and this routine goes to userenv on line 2287 in Items.pm.
In conclusion: Not related to this report.

> Undefined subroutine &C4::Reserves::CheckBiblioForceItemHolds called at
> t/db_dependent/Reserves.t line 512.
This is actually very strange. (I do not see this warning; for me the test passes.) Note that this routine CheckBiblioForceItemHolds is created by the first patch. Is your perl5lib path pointing at the right koha clone, or something like that ?
Comment 8 Nick Clemens (kidclamp) 2015-02-27 21:06:32 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens (kidclamp) 2015-02-27 21:06:39 UTC Comment hidden (obsolete)
Comment 10 Nick Clemens (kidclamp) 2015-02-27 21:07:59 UTC
Ah, fixed it, but then did a new koha-shell and didn't fix it again, d'oh

> Is your perl5lib path pointing at the right koha clone, or
> something like that ?
Comment 11 Kyle M Hall 2015-03-02 14:47:48 UTC
Would it not make much more sense to simply have a new `biblio` table field `force_item_level_holds` rather than creating a completely new table with only a single column?
Comment 12 Marcel de Rooy 2015-03-03 11:03:07 UTC
(In reply to Kyle M Hall from comment #11)
> Would it not make much more sense to simply have a new `biblio` table field
> `force_item_level_holds` rather than creating a completely new table with
> only a single column?

Not really, it is somewhat arbitrary. If many libraries would not use this feature or just mark some exceptions as such, it is much cheaper in terms of space to have an additional small table.
I would choose now for a small start: Moving the data into biblio at some point in time is just a trivial db rev with a few lines and the table is gone!

Back to Signed off
Comment 13 Jonathan Druart 2015-03-03 15:59:00 UTC
FWIW I think Kyle is right, we don't need to create a new table for a 1:1 relationship.
Comment 14 Katrin Fischer 2015-03-03 16:48:04 UTC
I talked to Kyle earlier about this and I am also in favor of not adding another table.
Comment 15 Marcel de Rooy 2015-03-05 11:17:34 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2015-03-05 11:17:41 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2015-03-05 11:23:25 UTC
In view of the overwhelming requests for a new column instead of a new small table :) I have slightly adjusted the two patches.
Back to Signed off.
If Nick could resign or one of the column requestors (Kyle, Jonathan, Katrin), that would be welcome too. In that case feel free to set back to NSO and add your SO. Thanks.

BTW I will soon submit something for the follow-up report (13479). But no need to wait for that..
Comment 18 Marcel de Rooy 2015-03-05 11:30:26 UTC
Probably resign is something different than re-sign (sign off again)..
Comment 19 Marcel de Rooy 2015-03-05 14:37:37 UTC
(In reply to M. de Rooy from comment #17)
> BTW I will soon submit something for the follow-up report (13479). But no
> need to wait for that..

If you do not like to run the update statement manually (in the test plan), apply the 13479 preliminary patch. And you can do it in the staff client.
Comment 20 Kyle M Hall 2015-03-13 14:41:36 UTC
Created attachment 36866 [details] [review]
[PASSED QA] Bug 13478: Force item holds only for specific biblios

This patch adds the option to selectively force item holds for specific
biblionumbers.
The patch adds routine C4::Reserves::CheckBiblioForceItemHolds. It is
tested in Reserves.t.
The preference OPACItemHolds gets a new choice: selectiveforce.
The description of the pref is adjusted.

Please note that this option is specifically targeted for the OPAC. Staff
users still have both possibilities.

A follow-up report (13479) will add the possibility for staff users to
enable the forced item hold on biblio level.

TEST PLAN:
[ 1] Run the unit test t/db_dependent/Reserves.t.
[ 2] Set OPACItemHolds to force.
[ 3] Add a hold in the OPAC; verify that you cannot place a next/av hold.
[ 4] Set OPACItemHolds to Allow.
[ 5] Add a hold in the OPAC; verify that you can place both hold types.
[ 6] Set OPACItemHolds to Do not allow.
[ 7] Add a hold in the OPAC; verify that you can only place next/av holds.
[ 8] Enable forced item holds for two biblionumbers:
     UPDATE biblio SET forced_item_holds=1 WHERE biblionumber=?
[ 9] Set OPACItemHolds to Selectively force.
[10] Add a hold on three biblionumbers at once (including these two).
     Verify that you have two hold types for the single biblio and that
     you have only item holds for the two other biblios.

Signed-off-by: Nick <nick@quecheelibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added the column to biblio and removed the separate table.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2015-03-13 14:42:03 UTC
Created attachment 36867 [details] [review]
[PASSED QA] Bug 13478: Database revision (new biblio column and pref description)

TEST PLAN:
[1] Run the database revision. Check if the new column is present.

Signed-off-by: Nick <nick@quecheelibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Added a new column to biblio instead of the new table.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Marcel de Rooy 2015-04-15 11:44:49 UTC
There we go