Created attachment 34513 [details] [review] Bug 13478: Force item holds only for specific biblios This patch adds the option to selectively force item holds for specific biblionumbers. These numbers are saved in a separate table. 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 patch adds the possibility for staff users to add or remove a biblio from the list of books with forced item holds. TEST PLAN: [ 1] Run the database revision. [ 2] Run the unit test t/db_dependent/Reserves.t. [ 3] Set OPACItemHolds to force. [ 4] Add a hold in the OPAC; verify that you cannot place a next/av hold. [ 5] Set OPACItemHolds to Allow. [ 6] Add a hold in the OPAC; verify that you can place both hold types. [ 7] Set OPACItemHolds to Do not allow. [ 8] Add a hold in the OPAC; verify that you can only place next/av holds. [ 9] Add two biblio numbers into table biblio_force_item_holds. [10] Set OPACItemHolds to Selectively force. [11] 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.
I will open a new report to add the option of adding to the list.
Created attachment 34947 [details] [review] Bug 13478: Database revision for table biblio_force_item_holds TEST PLAN: [1] Run the database revision. Check if the new table is present.
Created attachment 34948 [details] [review] Bug 13478: Force item holds only for specific biblios This patch adds the option to selectively force item holds for specific biblionumbers. These numbers are saved in a separate table. 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 patch adds the possibility for staff users to add or remove a biblio from the list of books with forced item holds. 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] Add two biblio numbers into table biblio_force_item_holds. [ 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.
Created attachment 36193 [details] [review] Bug 13478: Database revision for table biblio_force_item_holds Easy conflict on updatedatabase.pl fixed.
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
(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 ?
Created attachment 36237 [details] [review] Bug 13478: Force item holds only for specific biblios This patch adds the option to selectively force item holds for specific biblionumbers. These numbers are saved in a separate table. 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 patch adds the possibility for staff users to add or remove a biblio from the list of books with forced item holds. 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] Add two biblio numbers into table biblio_force_item_holds. [ 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>
Created attachment 36238 [details] [review] Bug 13478: Database revision for table biblio_force_item_holds TEST PLAN: [1] Run the database revision. Check if the new table is present. Signed-off-by: Nick <nick@quecheelibrary.org>
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 ?
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?
(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
FWIW I think Kyle is right, we don't need to create a new table for a 1:1 relationship.
I talked to Kyle earlier about this and I am also in favor of not adding another table.
Created attachment 36566 [details] [review] 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.
Created attachment 36567 [details] [review] 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.
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..
Probably resign is something different than re-sign (sign off again)..
(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.
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>
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>
There we go