Bug 35906: Remove preference and add override handling

This patch updates the bookable nature of items to allow setting at the
itemtype level and then overriding that setting at item level should you
so wish to do so.

We also now properly handle the item_level-itypes preference such that
we look at item or biblioitem level appropriately.

Signed-off-by: Esther <esther@bywatersolutions.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>

Koha/Item.pm

@@ -1917,6 +1917,20 @@ sub effective_not_for_loan_status {
     return ( defined($itype_notforloan) && !$self->notforloan ) ? $itype_notforloan : $self->notforloan;
 }
 
+=head3 effective_bookable
+
+  my $bookable = $item->effective_bookable;
+
+Returns the effective bookability of the current item, be that item or itemtype level
+
+=cut
+
+sub effective_bookable {
+    my ($self) = @_;
+
+    return defined( $self->bookable ) ? $self->bookable : $self->itemtype->bookable;
+}
+
 =head3 orders
 
   my $orders = $item->orders();

Koha/Items.pm

@@ -197,12 +197,17 @@ Returns a new resultset, containing only those items that are allowed to be book
 sub filter_by_bookable {
     my ($self) = @_;
 
-    my $params =
-        C4::Context->preference('item-level_booking') == 1
-        ? { bookable => 1 }
-        : { itype    => { -in => \'SELECT itemtype FROM itemtypes WHERE bookable = 1' } };
+    my @bookable_itemtypes = Koha::ItemTypes->search( { bookable => 1 } )->get_column('itemtype');
 
-    return $self->search($params);
+    my ( $where, $attr );
+    if ( C4::Context->preference("item-level_itypes") ) {
+        $where = [ { bookable => 1 }, { bookable => undef, itype => { -in => \@bookable_itemtypes } } ];
+    } else {
+        $where = [ { bookable => 1 }, { bookable => undef, 'biblioitem.itemtype' => { -in => \@bookable_itemtypes } } ];
+        $attr  = { join => 'biblioitem' };
+    }
+
+    return $self->search( $where, $attr );
 }
 
 =head3 move_to_biblio

api/v1/swagger/definitions/item.yaml

@@ -24,6 +24,11 @@ properties:
       - "null"
     description: Information about the acquisition source (it is not really a vendor id)
   bookable:
+    type:
+      - boolean
+      - "null"
+    description: Item level bookability override.
+  effective_bookable:
     type: boolean
     description: Allow bookings on this item.
   home_library_id:
@@ -90,7 +95,8 @@ properties:
       - string
       - "null"
     format: date-time
-    description: The date and time an item was last marked as withdrawn, NULL if not
+    description:
+      The date and time an item was last marked as withdrawn, NULL if not
       withdrawn
   callnumber:
     type:
@@ -155,14 +161,16 @@ properties:
     type:
       - string
       - "null"
-    description: Linked to the CART and PROC temporary locations feature, stores the
+    description:
+      Linked to the CART and PROC temporary locations feature, stores the
       permanent shelving location
   checked_out_date:
     type:
       - string
       - "null"
     format: date
-    description: Defines if item is checked out (NULL for not checked out, and checkout
+    description:
+      Defines if item is checked out (NULL for not checked out, and checkout
       date for checked out)
   call_number_source:
     type:

api/v1/swagger/paths/biblios.yaml

@@ -480,6 +480,7 @@
             - item_group_item.item_group.description
             - serial_item.serial
             - return_claims
+            - effective_bookable
         collectionFormat: csv
       - $ref: "../swagger.yaml#/parameters/match"
       - $ref: "../swagger.yaml#/parameters/order_by"

api/v1/swagger/paths/items.yaml

@@ -22,6 +22,7 @@
           enum:
             - +strings
             - biblio
+            - effective_bookable
         collectionFormat: csv
       - $ref: "../swagger.yaml#/parameters/match"
       - $ref: "../swagger.yaml#/parameters/order_by"
@@ -88,6 +89,7 @@
           type: string
           enum:
             - +strings
+            - effective_bookable
         collectionFormat: csv
     consumes:
       - application/json
@@ -157,7 +159,7 @@
             * linked_analytics: The item has linked analytic records
             * not_same_branch: The item is blocked by independent branches
         schema:
-              $ref: "../swagger.yaml#/definitions/error"
+          $ref: "../swagger.yaml#/definitions/error"
       "500":
         description: |
           Internal server error. Possible `error_code` attribute values:

catalogue/updateitem.pl

@@ -39,7 +39,7 @@ my $itemnotes_nonpublic=$cgi->param('itemnotes_nonpublic');
 my $withdrawn=$cgi->param('withdrawn');
 my $damaged=$cgi->param('damaged');
 my $exclude_from_local_holds_priority = $cgi->param('exclude_from_local_holds_priority');
-my $bookable = $cgi->param('bookable');
+my $bookable = $cgi->param('bookable') // q{};
 
 my $confirm=$cgi->param('confirm');
 my $dbh = C4::Context->dbh;
@@ -77,6 +77,7 @@ elsif ( $op eq "cud-set_public_note" ) { # i.e., itemnotes parameter passed from
     $item->exclude_from_local_holds_priority($exclude_from_local_holds_priority);
     $messages = "updated_exclude_from_local_holds_priority=$exclude_from_local_holds_priority&";
 } elsif ( $op eq "cud-set_bookable" && $bookable ne $item_data_hashref->{'bookable'} ) {
+    undef($bookable) if $bookable eq "";
     $item->bookable($bookable);
 } elsif ( $op eq "cud-set_damaged" && $damaged ne $item_data_hashref->{'damaged'}) {
     $item->damaged($damaged);

installer/data/mysql/atomicupdate/Bug_35906_add-column-bookable-itemtype.pl

@@ -7,15 +7,29 @@ return {
         my ($args) = @_;
         my ( $dbh, $out ) = @$args{qw(dbh out)};
 
-        $dbh->do(q{
-            INSERT IGNORE INTO systempreferences (`variable`, `value`, `options`, `explanation`, `type`) VALUES  ('item-level_booking', 1, '', 'enables item type level for future booking', 'YesNo');
-        });
-
-        $dbh->do(q{
+        $dbh->do(
+            q{
             ALTER TABLE itemtypes ADD IF NOT EXISTS bookable INT(1) DEFAULT 0
-        });
+        }
+        );
 
-        say $out "Added new system preference 'item-level_booking'";
         say $out "Added column 'itemtypes.bookable'";
+
+        $dbh->do(
+            q{
+            ALTER TABLE items MODIFY COLUMN bookable tinyint(1) DEFAULT NULL COMMENT 'nullable boolean value defining whether this this item is available for bookings or not'
+        }
+        );
+
+        say $out "Updated column 'items.bookable' allow nullable";
+
+        $dbh->do(
+            q{
+            ALTER TABLE deleteditems MODIFY COLUMN bookable tinyint(1) DEFAULT NULL COMMENT 'nullable boolean value defining whether this this item is available for bookings or not'
+        }
+        );
+
+        say $out "Updated column 'deleteditems.bookable' allow nullable";
+
     },
 };

installer/data/mysql/kohastructure.sql

@@ -2755,7 +2755,7 @@ CREATE TABLE `deleteditems` (
   `biblionumber` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key from biblio table used to link this item to the right bib record',
   `biblioitemnumber` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key from the biblioitems table to link to item to additional information',
   `barcode` varchar(20) DEFAULT NULL COMMENT 'item barcode (MARC21 952$p)',
-  `bookable` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'boolean value defining whether this item is available for bookings or not',
+  `bookable` tinyint(1) DEFAULT NULL COMMENT 'nullable boolean value defining whether this this item is available for bookings or not',
   `dateaccessioned` date DEFAULT NULL COMMENT 'date the item was acquired or added to Koha (MARC21 952$d)',
   `booksellerid` longtext DEFAULT NULL COMMENT 'where the item was purchased (MARC21 952$e)',
   `homebranch` varchar(10) DEFAULT NULL COMMENT 'foreign key from the branches table for the library that owns this item (MARC21 952$a)',
@@ -4014,7 +4014,7 @@ CREATE TABLE `items` (
   `biblionumber` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key from biblio table used to link this item to the right bib record',
   `biblioitemnumber` int(11) NOT NULL DEFAULT 0 COMMENT 'foreign key from the biblioitems table to link to item to additional information',
   `barcode` varchar(20) DEFAULT NULL COMMENT 'item barcode (MARC21 952$p)',
-  `bookable` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'boolean value defining whether this item is available for bookings or not',
+  `bookable` tinyint(1) DEFAULT NULL COMMENT 'nullable boolean value defining whether this this item is available for bookings or not',
   `dateaccessioned` date DEFAULT NULL COMMENT 'date the item was acquired or added to Koha (MARC21 952$d)',
   `booksellerid` longtext DEFAULT NULL COMMENT 'where the item was purchased (MARC21 952$e)',
   `homebranch` varchar(10) DEFAULT NULL COMMENT 'foreign key from the branches table for the library that owns this item (MARC21 952$a)',

installer/data/mysql/mandatory/sysprefs.sql

@@ -355,7 +355,6 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
 ('IssueLostItem','alert','Defines what should be done when an attempt is made to issue an item that has been marked as lost.','alert|confirm|nothing','Choice'),
 ('IssuingInProcess','0',NULL,'If ON, disables fines if the patron is issuing item that accumulate debt','YesNo'),
 ('item-level_itypes','1','','If ON, enables Item-level Itemtype / Issuing Rules','YesNo'),
-('item-level_booking','1','','If ON, enables Item-level for future booking feature','YesNo'),
 ('itemBarcodeFallbackSearch','0',NULL,'If set, uses scanned item barcodes as a catalogue search if not found as barcodes','YesNo'),
 ('itemBarcodeInputFilter','','whitespace|T-prefix|cuecat|libsuite8|EAN13','If set, allows specification of a item barcode input filter','Choice'),
 ('itemcallnumber','',NULL,'The MARC field/subfield that is used to calculate the itemcallnumber (Dewey would be 082ab or 092ab; LOC would be 050ab or 090ab) could be 852hi from an item record','free'),

koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt

@@ -258,17 +258,15 @@
                         [% END %]
                         <span class="hint">If checked, items will be automatically checked in once they've reached their due date. This feature requires the <code>misc/cronjobs/automatic_checkin.pl</code> cronjob. Ask your system administrator to schedule it.</span>
                 </li>
-                [% IF Koha.Preference('item-level_booking') == 0 %]
-                    <li>
-                        <label for="bookable">Bookable: </label>
-                            [% IF itemtype.bookable %]
-                                <input type="checkbox" id="bookable" name="bookable" checked="checked" value="1" />
-                            [% ELSE %]
-                                <input type="checkbox" id="bookable" name="bookable" value="1" />
-                            [% END %]
-                            <span class="hint">If checked, all items of this type will be enabled for future bookings.</span>
-                    </li>
-                [% END %]
+                <li>
+                    <label for="bookable">Bookable: </label>
+                        [% IF itemtype.bookable %]
+                            <input type="checkbox" id="bookable" name="bookable" checked="checked" value="1" />
+                        [% ELSE %]
+                            <input type="checkbox" id="bookable" name="bookable" value="1" />
+                        [% END %]
+                        <span class="hint">If checked, all items of this type will be enabled for future bookings unless overridden at the item level.</span>
+                </li>
                 <li>
                     <label for="rentalcharge">Rental charge: </label>
                     <input type="text" id="rentalcharge" name="rentalcharge" size="10" value="[% itemtype.rentalcharge | $Price on_editing => 1 %]" min="0" />

koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref

@@ -1089,13 +1089,6 @@ Circulation:
                   1: "Do"
                   0: "Don't"
             - place a hold when ordering from a suggestion.
-        -
-            - Use the type of the
-            - pref: item-level_booking
-              choices:
-                  1: item
-                  0: itemtype
-            - level to defined witch items can be bookable in future.
     Patron restrictions:
         -
             - pref: PatronRestrictionTypes

koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt

@@ -353,21 +353,28 @@
                                                     <input type="hidden" name="biblionumber" value="[% ITEM_DAT.biblionumber | html %]" />
                                                     <input type="hidden" name="biblioitemnumber" value="[% ITEM_DAT.biblioitemnumber | html %]" />
                                                     <input type="hidden" name="itemnumber" value="[% ITEM_DAT.itemnumber | html %]" />
-                                                    <select name="bookable" [% IF ITEM_DAT.effective_itemtype.bookable == 1 && Koha.Preference('item-level_booking') == 0 %]disabled[% END %]>
-                                                    [% IF ITEM_DAT.bookable == 1 || ITEM_DAT.effective_itemtype.bookable == 1 %]
+                                                    <select name="bookable">
+                                                    [% IF ITEM_DAT.bookable == 1 %]
+                                                        <option value="">Follow item type</option>
                                                         <option value="1" selected="selected">Yes</option>
                                                         <option value="0">No</option>
-                                                        [% ELSE %]
+                                                    [% ELSIF ITEM_DAT.bookable == 0 %]
+                                                        <option value="">Follow item type</option>
                                                         <option value="1">Yes</option>
                                                         <option value="0" selected="selected">No</option>
-                                                        [% END %]
+                                                    [% ELSE %]
+                                                        <option value="" selected="selected">Follow item type</option>
+                                                        <option value="1">Yes</option>
+                                                        <option value="0">No</option>
+                                                    [% END %]
                                                     </select>
                                                     <input type="hidden" name="op" value="cud-set_bookable" />
                                                     <input type="submit" name="submit" class="btn btn-primary btn-xs" value="Update" />
                                                 </form>
                                             [% ELSE %]
-                                                [% IF ITEM_DAT.bookable == 1 %] Yes [% ELSE %] No [% END %]
+                                                [% IF ITEM_DAT.bookable == 1 %] Yes [% ELSIF ITEM_DAT.bookable == 0 %] No [% ELSE %] Follow item type [% END %]
                                             [% END %]
+                                            <span class="hint"> Item type bookable: [% IF ITEM_DAT.effective_itemtype.bookable == 1 %]Yes[% ELSE %]No[% END %]</span>
                                         </li>
 
                                     </ol> <!-- /.bibliodetails -->

t/db_dependent/Koha/Items.t

@@ -2176,41 +2176,37 @@ subtest 'filter_by_for_hold' => sub {
 };
 
 subtest 'filter_by_bookable' => sub {
-    plan tests => 3;
+    plan tests => 4;
 
     $schema->storage->txn_begin;
 
-    my $biblio = $builder->build_sample_biblio;
+    t::lib::Mocks::mock_preference('item-level_itypes', 0);
+
+    my $bookable_item_type = $builder->build_object( { class => 'Koha::ItemTypes', value => { bookable => 1 } } );
+    my $non_bookable_item_type = $builder->build_object( { class => 'Koha::ItemTypes', value => { bookable => 0 } } );
+    my $biblio = $builder->build_sample_biblio({ itemtype => $bookable_item_type->itemtype });
 
     # bookable items
-    my $bookable_item1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 1 } );
+    my $bookable_item1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, itype => $bookable_item_type->itemtype, bookable => 1 } );
 
     # not bookable items
-    my $non_bookable_item1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 0 } );
-    my $non_bookable_item2 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 0 } );
+    my $non_bookable_item1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, itype => $non_bookable_item_type->itemtype, bookable => 0 } );
+    my $non_bookable_item2 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, itype => $non_bookable_item_type->itemtype, bookable => 0 } );
 
-    is( $biblio->items->filter_by_bookable->count, 1, "filter_by_bookable returns the correct number of items" );
+    is( $biblio->items->filter_by_bookable->count, 1, "filter_by_bookable returns the correct number of items set at item level" );
     is(
         $biblio->items->filter_by_bookable->next->itemnumber, $bookable_item1->itemnumber,
-        "the correct item is returned from filter_by_bookable"
+        "the correct item is returned from filter_by_bookable at the item level"
     );
 
-    # unset level booking on item (for itemtype)
-    t::lib::Mocks::mock_preference( 'item-level_booking', 0 );
+    $bookable_item1->bookable(undef)->store();
+    $non_bookable_item1->bookable(undef)->store();
+    $non_bookable_item2->bookable(undef)->store();
+    $biblio->get_from_storage;
+    is( $biblio->items->filter_by_bookable->count, 3, "filter_by_bookable returns the correct number of items when not set at item level and using biblio level itemtypes" );
 
-    # test with itemtype directly bookable
-    my $item_type = $builder->build_object( { class => 'Koha::ItemTypes', value => { bookable => 1 } } );
-    my $biblio2   = $builder->build_sample_biblio( { itemtype => $item_type->itemtype } );
-
-    # bookable items
-    my $bookable_item3 = $builder->build_sample_item(
-        { biblionumber => $biblio2->biblionumber, itype => $item_type->itemtype, bookable => 1 } );
-    my $bookable_item4 = $builder->build_sample_item(
-        { biblionumber => $biblio2->biblionumber, itype => $item_type->itemtype, bookable => 0 } );
-
-    # items are bookable even if bookable => 0 on item (due to itemtype bookable => 1)
-    is( $biblio2->items->filter_by_bookable->count, 2, "filter_by_bookable returns the correct number of items" );
+    t::lib::Mocks::mock_preference('item-level_itypes', 1);
+    is( $biblio->items->filter_by_bookable->count, 1, "filter_by_bookable returns the correct number of items when not set at item level and using item level itemtypes" );
 
     $schema->storage->txn_rollback;
-
 };