From fa4f4c58b5bdc35e89c7a49e25027899d490b050 Mon Sep 17 00:00:00 2001 From: Jake Deery Date: Wed, 4 Mar 2026 08:36:48 +0000 Subject: [PATCH] Bug 14962: (QA follow-up) Refuse to add items to a display if they are already on display This patch makes it impossible to add an item to a display that is already active, unless the item has a start date set. This allows librarians to plan future displays, but prevents them from inadventantly adding items to displays if they are already displayed elsewhere. This patch also fixes a few minor design oversights around creating DisplayItems (namely, server-side checking and correcting for missing dates). Sponsored-by: ByWater Solutions --- Koha/BackgroundJob/BatchAddDisplayItems.pm | 4 ++ Koha/Display.pm | 37 ++++++++++++------- Koha/DisplayItem.pm | 30 ++++++++------- Koha/Item.pm | 31 ++++++++++------ api/v1/swagger/definitions/item.yaml | 5 +++ api/v1/swagger/paths/items.yaml | 3 ++ .../prog/js/fetch/item-api-client.js | 8 ++++ .../components/Display/DisplaysResource.vue | 30 ++++++++------- 8 files changed, 95 insertions(+), 53 deletions(-) diff --git a/Koha/BackgroundJob/BatchAddDisplayItems.pm b/Koha/BackgroundJob/BatchAddDisplayItems.pm index 065defcd1ba..c3fd212ea11 100644 --- a/Koha/BackgroundJob/BatchAddDisplayItems.pm +++ b/Koha/BackgroundJob/BatchAddDisplayItems.pm @@ -64,6 +64,7 @@ sub process { my $display_id = $args->{display_id}; my @barcodes = @{ $args->{barcodes} }; + my $date_added = $args->{date_added}; my $date_remove = $args->{date_remove}; my $report = { @@ -140,6 +141,7 @@ sub process { display_id => $display_id, itemnumber => $itemnumber, biblionumber => $item->biblionumber, + date_added => $date_added, date_remove => $date_remove, } )->store; @@ -190,6 +192,7 @@ sub enqueue { my @barcodes = @{ $args->{barcodes} }; my $display_id = $args->{display_id}; + my $date_added = $args->{date_added}; my $date_remove = $args->{date_remove}; $self->SUPER::enqueue( @@ -198,6 +201,7 @@ sub enqueue { job_args => { display_id => $display_id, barcodes => \@barcodes, + date_added => $date_added, date_remove => $date_remove, }, job_queue => 'long_tasks', diff --git a/Koha/Display.pm b/Koha/Display.pm index 231ca61774a..05c08215e7a 100644 --- a/Koha/Display.pm +++ b/Koha/Display.pm @@ -24,6 +24,7 @@ use Carp; use C4::Context; use C4::Log qw( logaction ); use Koha::Database; +use Koha::DateUtils qw( dt_from_string ); use Koha::Displays; use Koha::DisplayItems; use Koha::Library; @@ -73,6 +74,7 @@ sub display_items { display_id => $self->display_id, itemnumber => $new_item->{itemnumber}, biblionumber => $new_item->{biblionumber}, + date_added => $new_item->{date_added}, date_remove => $new_item->{date_remove}, } )->store; @@ -141,28 +143,38 @@ Overloaded store method to add action logging. sub store { my ($self) = @_; + my $action = $self->in_storage ? 'update' : 'create'; - my $action = $self->in_storage ? 'update' : 'create'; - my $original = $self->in_storage ? Koha::Displays->find( $self->display_id ) : undef; - - my $enabled_before = $original ? $original->enabled : undef; - my $enabled_after = $self->enabled; + if ( $action eq 'create' ) { + if ( !$self->start_date ) { + my $dt = dt_from_string(); + $self->start_date( $dt->ymd ); + } - my $result = $self->SUPER::store; + if ( !$self->end_date && $self->display_days ) { + my $dt = dt_from_string(); + $dt->add( days => $self->display_days ); + $self->end_date( $dt->ymd ); + } - if ( C4::Context->preference("DisplayItemsLog") ) { - if ( $action eq 'create' ) { - logaction( "DISPLAYS", "CREATE", $self->display_id, undef, undef, $self ); - } else { - logaction( "DISPLAYS", "MODIFY", $self->display_id, $self, undef, $original ); + logaction( "DISPLAYS", "CREATE", $self->display_id, undef, undef, $self ) + if ( C4::Context->preference("DisplayItemsLog") ); + } else { + if ( C4::Context->preference("DisplayItemsLog") ) { + my $original = $self->in_storage ? Koha::Displays->find( $self->display_id ) : undef; + my $enabled_before = $original ? $original->enabled : undef; + my $enabled_after = $self->enabled; if ( defined $enabled_before && defined $enabled_after && $enabled_before != $enabled_after ) { my $enable_action = $enabled_after ? "ENABLE" : "DISABLE"; logaction( "DISPLAYS", $enable_action, $self->display_id, undef, undef, $self ); } + + logaction( "DISPLAYS", "MODIFY", $self->display_id, $self, undef, $original ); } } + my $result = $self->SUPER::store; return $result; } @@ -177,11 +189,10 @@ Overloaded delete method to add action logging. sub delete { my ($self) = @_; - my $result = $self->SUPER::delete; - logaction( "DISPLAYS", "DELETE", $self->display_id, undef, undef, $self ) if C4::Context->preference("DisplayItemsLog"); + my $result = $self->SUPER::delete; return $result; } diff --git a/Koha/DisplayItem.pm b/Koha/DisplayItem.pm index 7416cb6879e..b4f1a9ee5d2 100644 --- a/Koha/DisplayItem.pm +++ b/Koha/DisplayItem.pm @@ -95,24 +95,30 @@ Overloaded store method to add action logging. sub store { my ($self) = @_; - my $action = $self->in_storage ? 'update' : 'create'; - if ( $action eq 'create' && !$self->date_remove ) { + if ( $action eq 'create' ) { my $display = $self->display; - if ( $display && $display->display_days ) { + my $item = $self->item; + + return if ( $item->active_display && !$self->date_added ); + + if ( !$self->date_added ) { + my $dt = dt_from_string(); + $self->date_added( $dt->ymd ); + } + + if ( !$self->date_remove && $display && $display->display_days ) { my $dt = dt_from_string(); $dt->add( days => $display->display_days ); $self->date_remove( $dt->ymd ); } - } - my $result = $self->SUPER::store; - - if ( C4::Context->preference("DisplayItemsLog") && $action eq 'create' ) { - logaction( "DISPLAYS", "ADD_ITEM", $self->display_id, "Item " . $self->itemnumber, undef, $self ); + logaction( "DISPLAYS", "ADD_ITEM", $self->display_id, "Item " . $self->itemnumber, undef, $self ) + if C4::Context->preference("DisplayItemsLog"); } + my $result = $self->SUPER::store; return $result; } @@ -127,14 +133,10 @@ Overloaded delete method to add action logging. sub delete { my ($self) = @_; - my $display_id = $self->display_id; - my $itemnumber = $self->itemnumber; - - my $result = $self->SUPER::delete; - - logaction( "DISPLAYS", "REMOVE_ITEM", $display_id, "Item " . $itemnumber, undef, $self ) + logaction( "DISPLAYS", "REMOVE_ITEM", $self->display_id, "Item " . $self->itemnumber, undef, $self ) if C4::Context->preference("DisplayItemsLog"); + my $result = $self->SUPER::delete; return $result; } diff --git a/Koha/Item.pm b/Koha/Item.pm index e26b4ad5466..8adda9084f0 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -420,7 +420,7 @@ sub effective_itemtype { my ($self) = @_; if ( my $display = $self->active_display ) { - return $display->get_column('display_itype') || $self->_result()->effective_itemtype(); + return $display->display_itype || $self->_result()->effective_itemtype(); } return $self->_result()->effective_itemtype(); @@ -806,7 +806,7 @@ sub active_display_item { my $dtf = Koha::Database->new->schema->storage->datetime_parser; my $today = dt_from_string(); - my $display_item = Koha::DisplayItems->search( + my $display_items = Koha::DisplayItems->search( { itemnumber => $self->itemnumber, 'display.enabled' => 1, @@ -826,9 +826,13 @@ sub active_display_item { ] }, { join => 'display' } - )->next; + ); - return $display_item; + if ( my $display_item = $display_items->next ) { + return $display_item; + } + + return; } =head3 request_transfer @@ -2300,10 +2304,10 @@ sub effective_homebranch { my ($self) = @_; if ( my $display = $self->active_display ) { - return $display->get_column('display_branch') || $self->homebranch; + return $display->display_branch || $self->homebranch; } - return $self->get_column('homebranch'); + return $self->homebranch; } =head3 effective_holding_library @@ -2337,10 +2341,10 @@ sub effective_holdingbranch { my ($self) = @_; if ( my $display = $self->active_display ) { - return $display->get_column('display_holding_branch') || $self->holdingbranch; + return $display->display_holding_branch || $self->holdingbranch; } - return $self->get_column('holdingbranch'); + return $self->holdingbranch; } =head3 active_display @@ -2363,8 +2367,9 @@ sub active_display { return unless C4::Context->preference('UseDisplayModule'); - my $display_item_rs = $self->_result->display_items( + my $display_item = Koha::DisplayItems->search( { + itemnumber => $self->itemnumber, 'display.enabled' => 1, '-or' => [ 'display.start_date' => { '<=', \'CURDATE()' }, @@ -2385,10 +2390,12 @@ sub active_display { join => 'display', order_by => { -desc => 'date_added' } } - ); + )->next; + + return unless $display_item; - if ( my $display_item = $display_item_rs->first ) { - return $display_item->display; + if ( my $display = $display_item->display ) { + return $display; } return; diff --git a/api/v1/swagger/definitions/item.yaml b/api/v1/swagger/definitions/item.yaml index 4de4e102171..f5332e836c4 100644 --- a/api/v1/swagger/definitions/item.yaml +++ b/api/v1/swagger/definitions/item.yaml @@ -24,6 +24,11 @@ properties: - string - "null" description: Information about the acquisition source (it is not really a vendor id) + active_display: + type: + - object + - "null" + description: The active display, if one is available bookable: type: - boolean diff --git a/api/v1/swagger/paths/items.yaml b/api/v1/swagger/paths/items.yaml index e99a6e0e55f..e63935ba696 100644 --- a/api/v1/swagger/paths/items.yaml +++ b/api/v1/swagger/paths/items.yaml @@ -21,6 +21,7 @@ type: string enum: - +strings + - active_display - biblio - effective_bookable collectionFormat: csv @@ -89,6 +90,8 @@ type: string enum: - +strings + - active_display + - biblio - effective_bookable collectionFormat: csv consumes: diff --git a/koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js b/koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js index 2d87736dc0e..12f6f757ac1 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js +++ b/koha-tmpl/intranet-tmpl/prog/js/fetch/item-api-client.js @@ -17,6 +17,10 @@ export class ItemAPIClient { get: id => this.httpClient.get({ endpoint: "items/" + id, + headers: { + "x-koha-embed": + "+strings,active_display,biblio,effective_bookable", + }, }), getByExternalId: external_id => this.httpClient.get({ @@ -26,6 +30,10 @@ export class ItemAPIClient { _match: "starts_with", external_id: external_id, }), + headers: { + "x-koha-embed": + "+strings,active_display,biblio,effective_bookable", + }, }), }; } diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Display/DisplaysResource.vue b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Display/DisplaysResource.vue index b40183d8b56..a8025ee6cfe 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/vue/components/Display/DisplaysResource.vue +++ b/koha-tmpl/intranet-tmpl/prog/js/vue/components/Display/DisplaysResource.vue @@ -422,10 +422,22 @@ export default { item == undefined || item.item_id === undefined || item.external_id !== display_item.barcode - ) + ) { errors.push( $__("The barcode entered does not match an item") ); + } + + if ( + item.active_display && + display_item.date_added == undefined + ) { + errors.push( + $__( + "The item entered is already on another active display. Set a date added for this item, or remove the item from the other display." + ) + ); + } } baseResource.setWarning(errors.join("
")); @@ -467,19 +479,9 @@ export default { await display.display_items.push(display_item); } - if (display.start_date == null) - display.start_date = epoch.toISOString().substr(0, 10); - if (display.end_date == null && display.display_days != undefined) { - let calculated_date = epoch; - calculated_date.setDate( - epoch.getDate() + Number(display.display_days) - ); - - display.end_date = calculated_date.toISOString().substr(0, 10); - } - if (display.display_days == "") display.display_days = null; - if (display.public_note == "") display.public_note = null; - if (display.staff_note == "") display.staff_note = null; + Object.entries(display).forEach(([key, value]) => { + if (value == "") display[key] = null; + }); if (displayId) { baseResource.apiClient.update(display, displayId).then( -- 2.50.1 (Apple Git-155)