From bd171788e1127e4b435e5de1396fbf6471989fb1 Mon Sep 17 00:00:00 2001
From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Date: Mon, 26 Feb 2024 16:14:28 +0000
Subject: [PATCH] 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.
---
 Koha/Item.pm                                  | 14 +++++++
 Koha/Items.pm                                 | 15 ++++---
 api/v1/swagger/definitions/item.yaml          | 14 +++++--
 api/v1/swagger/paths/biblios.yaml             |  6 ++-
 api/v1/swagger/paths/items.yaml               |  4 +-
 catalogue/updateitem.pl                       |  3 +-
 .../Bug_35906_add-column-bookable-itemtype.pl | 28 +++++++++----
 installer/data/mysql/kohastructure.sql        |  4 +-
 installer/data/mysql/mandatory/sysprefs.sql   |  1 -
 .../prog/en/modules/admin/itemtypes.tt        | 20 +++++-----
 .../admin/preferences/circulation.pref        |  7 ----
 .../prog/en/modules/catalogue/moredetail.tt   | 17 +++++---
 t/db_dependent/Koha/Items.t                   | 40 +++++++++----------
 13 files changed, 106 insertions(+), 67 deletions(-)
 mode change 100644 => 100755 installer/data/mysql/atomicupdate/Bug_35906_add-column-bookable-itemtype.pl

diff --git a/Koha/Item.pm b/Koha/Item.pm
index 11e4fb89c44..030c956a74a 100644
--- a/Koha/Item.pm
+++ b/Koha/Item.pm
@@ -1699,6 +1699,20 @@ sub itemtype {
     return Koha::ItemTypes->find( $self->effective_itemtype );
 }
 
+=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();
diff --git a/Koha/Items.pm b/Koha/Items.pm
index aa89d87d9ac..80d5fd9d729 100644
--- a/Koha/Items.pm
+++ b/Koha/Items.pm
@@ -169,12 +169,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
diff --git a/api/v1/swagger/definitions/item.yaml b/api/v1/swagger/definitions/item.yaml
index 1d0ed7ac95f..a6e73513a12 100644
--- a/api/v1/swagger/definitions/item.yaml
+++ b/api/v1/swagger/definitions/item.yaml
@@ -25,6 +25,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:
@@ -91,7 +96,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:
@@ -151,14 +157,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:
diff --git a/api/v1/swagger/paths/biblios.yaml b/api/v1/swagger/paths/biblios.yaml
index 30702535577..516aba495e1 100644
--- a/api/v1/swagger/paths/biblios.yaml
+++ b/api/v1/swagger/paths/biblios.yaml
@@ -372,7 +372,8 @@
         collectionFormat: csv
       - name: checked_in
         in: query
-        description: By default, current checkouts are returned, when this is true then
+        description:
+          By default, current checkouts are returned, when this is true then
           checked in checkouts are returned as result.
         type: boolean
     produces:
@@ -423,6 +424,7 @@
           type: string
           enum:
             - +strings
+            - effective_bookable
         collectionFormat: csv
       - $ref: "../swagger.yaml#/parameters/match"
       - $ref: "../swagger.yaml#/parameters/order_by"
@@ -787,7 +789,7 @@
                 - integer
                 - "null"
           required:
-              - rating
+            - rating
           additionalProperties: false
     produces:
       - application/json
diff --git a/api/v1/swagger/paths/items.yaml b/api/v1/swagger/paths/items.yaml
index 3f4677a9a35..12edb8eb3af 100644
--- a/api/v1/swagger/paths/items.yaml
+++ b/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"
@@ -81,6 +82,7 @@
           type: string
           enum:
             - +strings
+            - effective_bookable
         collectionFormat: csv
     consumes:
       - application/json
@@ -150,7 +152,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:
diff --git a/catalogue/updateitem.pl b/catalogue/updateitem.pl
index 5b092763dfa..087dd98f8aa 100755
--- a/catalogue/updateitem.pl
+++ b/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);
diff --git a/installer/data/mysql/atomicupdate/Bug_35906_add-column-bookable-itemtype.pl b/installer/data/mysql/atomicupdate/Bug_35906_add-column-bookable-itemtype.pl
old mode 100644
new mode 100755
index 642505fd2fd..a289e189ae7
--- a/installer/data/mysql/atomicupdate/Bug_35906_add-column-bookable-itemtype.pl
+++ b/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";
+
     },
 };
diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql
index dd2a6107005..05d82da2d3f 100644
--- a/installer/data/mysql/kohastructure.sql
+++ b/installer/data/mysql/kohastructure.sql
@@ -2738,7 +2738,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 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)',
@@ -3995,7 +3995,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 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)',
diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql
index 9e96d47ba64..669074aa506 100644
--- a/installer/data/mysql/mandatory/sysprefs.sql
+++ b/installer/data/mysql/mandatory/sysprefs.sql
@@ -345,7 +345,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'),
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
index d93cee3b87f..707237449ac 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt
+++ b/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" />
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
index 487281dc92c..bae185a1f5c 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ -1008,13 +1008,6 @@ Circulation:
                   0: "Don't allow"
             - holds to be automatically filled after being automatically checked in.
             - '<br><strong>NOTE:</strong> This system preference requires the <code>misc/cronjobs/automatic_checkin.pl</code> cronjob. Ask your system administrator to schedule it.'
-        -
-            - 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
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt
index f6a45d0f2a4..8e9755b0394 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt
@@ -360,21 +360,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 -->
diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t
index 8dc564023ed..6f1d82836fe 100755
--- a/t/db_dependent/Koha/Items.t
+++ b/t/db_dependent/Koha/Items.t
@@ -2002,41 +2002,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 );
-
-    # 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_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" );
 
-    # 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;
-
 };
-- 
2.30.2