Bugzilla – Attachment 194803 Details for
Bug 14962
Temp Shelving Location / On Display Module
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14962: (QA follow-up) Refuse to add items to a display if they are already on display
fa4f4c5.patch (text/plain), 14.99 KB, created by
Jake Deery
on 2026-03-06 16:46:19 UTC
(
hide
)
Description:
Bug 14962: (QA follow-up) Refuse to add items to a display if they are already on display
Filename:
MIME Type:
Creator:
Jake Deery
Created:
2026-03-06 16:46:19 UTC
Size:
14.99 KB
patch
obsolete
>From fa4f4c58b5bdc35e89c7a49e25027899d490b050 Mon Sep 17 00:00:00 2001 >From: Jake Deery <jake.deery@openfifth.co.uk> >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("<br>")); >@@ -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) >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14962
:
190216
|
190217
|
190218
|
190219
|
190220
|
190221
|
190222
|
190223
|
190224
|
190225
|
190226
|
190227
|
190228
|
190229
|
190230
|
190231
|
190232
|
190571
|
190572
|
193719
|
193720
|
193721
|
193722
|
193723
|
193724
|
193725
|
193726
|
193727
|
193728
|
193729
|
193730
|
193731
|
193732
|
193733
|
193734
|
193735
|
193736
|
193737
|
193738
|
193739
|
193740
|
193741
|
193742
|
193743
|
193744
|
193745
|
193746
|
193747
|
193748
|
193749
|
193750
|
193751
|
193752
|
193753
|
193754
|
193755
|
194285
|
194286
|
194287
|
194288
|
194289
|
194290
|
194291
|
194292
|
194293
|
194294
|
194295
|
194296
|
194297
|
194298
|
194299
|
194300
|
194301
|
194302
|
194303
|
194304
|
194305
|
194306
|
194307
|
194308
|
194309
|
194310
|
194311
|
194312
|
194313
|
194314
|
194315
|
194316
|
194317
|
194318
|
194320
|
194321
|
194322
|
194786
|
194795
|
194796
|
194797
|
194798
|
194799
|
194800
|
194801
|
194802
|
194803
|
194804
|
194805
|
194806
|
194807
|
194808
|
194809
|
194813
|
194814
|
194815
|
194816
|
194817
|
194818
|
194819
|
194820
|
194821
|
194822
|
194823
|
194824
|
194825
|
194826
|
194827
|
194828
|
194829
|
194830
|
194831
|
194832
|
194833
|
194834
|
194835
|
194836
|
194837
|
194838
|
194839
|
194840
|
194841
|
194842
|
194843
|
194844
|
194845
|
194846
|
194847
|
194848
|
194849
|
194850
|
194851
|
194852
|
194853
|
194854
|
194855
|
194856
|
194857
|
194858
|
194859
|
194860
|
194861
|
194862
|
194863
|
194864