From b4e58b810f17b7c9acb212fffb6ec7ad2e88b869 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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 17a5ffc11f0..287a9d7c021 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: @@ -422,6 +423,7 @@ type: string enum: - +strings + - effective_bookable collectionFormat: csv - $ref: "../swagger.yaml#/parameters/match" - $ref: "../swagger.yaml#/parameters/order_by" @@ -786,7 +788,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 6987cacbd05..1cdd4089a6e 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 "set_public_note" ) { # i.e., itemnotes parameter passed from for $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 "set_bookable" && $bookable ne $item_data_hashref->{'bookable'} ) { + undef($bookable) if $bookable eq ""; $item->bookable($bookable); } elsif ( $op eq "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 39b3b5679f5..688984cd242 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/itemtypes.tt @@ -257,17 +257,15 @@ [% END %] If checked, items will be automatically checked in once they've reached their due date. This feature requires the misc/cronjobs/automatic_checkin.pl cronjob. Ask your system administrator to schedule it. - [% IF Koha.Preference('item-level_booking') == 0 %] -
  • - - [% IF itemtype.bookable %] - - [% ELSE %] - - [% END %] - If checked, all items of this type will be enabled for future bookings. -
    • - [% END %] +
  • + + [% IF itemtype.bookable %] + + [% ELSE %] + + [% END %] + If checked, all items of this type will be enabled for future bookings unless overridden at the item level. +
  • 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 68b312a415b..080e8fa4948 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. - '
    NOTE: This system preference requires the misc/cronjobs/automatic_checkin.pl 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 fb6ff3e917b..aaecea0af03 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/moredetail.tt @@ -355,21 +355,28 @@ - + [% IF ITEM_DAT.bookable == 1 %] + - [% ELSE %] + [% ELSIF ITEM_DAT.bookable == 0 %] + - [% END %] + [% ELSE %] + + + + [% END %] [% 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 %] + Item type bookable: [% IF ITEM_DAT.effective_itemtype.bookable == 1 %]Yes[% ELSE %]No[% END %]
    • 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.43.2