Bugzilla – Attachment 172814 Details for
Bug 38175
Improve bookings behavior with new status field
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 38175: (follow-up) Refactor staff interface additions to vanilla JS; adjust modules, controllers for coding guidelines and consistency
Bug-38175-follow-up-Refactor-staff-interface-addit.patch (text/plain), 19.62 KB, created by
Paul Derscheid
on 2024-10-16 12:19:06 UTC
(
hide
)
Description:
Bug 38175: (follow-up) Refactor staff interface additions to vanilla JS; adjust modules, controllers for coding guidelines and consistency
Filename:
MIME Type:
Creator:
Paul Derscheid
Created:
2024-10-16 12:19:06 UTC
Size:
19.62 KB
patch
obsolete
>From f36ae87ca41b49a735ae16cb77594a1029d20155 Mon Sep 17 00:00:00 2001 >From: Paul Derscheid <paul.derscheid@lmscloud.de> >Date: Wed, 16 Oct 2024 14:13:42 +0200 >Subject: [PATCH] Bug 38175: (follow-up) Refactor staff interface additions to > vanilla JS; adjust modules, controllers for coding guidelines and consistency >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >In the spirit of the current movement for more modern JS, the refactor tries to limit jQuery usage. > >Test plan: > 1) Create a booking on a bookable item > 2) Cancel it and see that it's simply deleted > 3) Apply the patch and run ârestart_allâ. > 4) Repeat the same cancel operation and see that it's still there, albeit with a different appearance. > 5) Try the filters in the table >--- > Koha/Booking.pm | 35 +++-- > Koha/Bookings.pm | 6 +- > Koha/REST/V1/Bookings.pm | 9 +- > .../prog/en/modules/bookings/list.tt | 123 +++++++++++------- > .../prog/js/cancel_booking_modal.js | 112 +++++++++++----- > .../prog/js/modals/place_booking.js | 6 +- > t/db_dependent/Koha/Booking.t | 6 +- > 7 files changed, 190 insertions(+), 107 deletions(-) > >diff --git a/Koha/Booking.pm b/Koha/Booking.pm >index 5e03754ff3..c15775325e 100644 >--- a/Koha/Booking.pm >+++ b/Koha/Booking.pm >@@ -27,6 +27,8 @@ use Koha::Libraries; > > use C4::Letters; > >+use List::Util qw(any); >+ > use base qw(Koha::Object); > > =head1 NAME >@@ -259,22 +261,26 @@ sub delete { > > =head3 edit > >-This method adds possibility to edit a booking >+This method allows patching a booking > > =cut > > sub edit { > my ( $self, $params ) = @_; > >- if ( $params->{status} ) { >- if ( $params->{status} eq 'cancelled' ) { >- $self->cancel( { send_letter => 1 } ); >- } >- $self->_set_status( $params->{status} ); >+ my $new_status = $params->{'status'}; >+ unless ($new_status) { >+ return $self->store; > } >- $self->store(); > >- return $self; >+ $self->_set_status($new_status); >+ >+ my $status = $self->status; >+ if ( $status eq 'cancelled' ) { >+ $self->cancel( { send_letter => 1 } ); >+ } >+ >+ return $self->store; > } > > =head3 cancel >@@ -290,7 +296,7 @@ sub cancel { > my $patron = $self->patron; > my $pickup_library = $self->pickup_library; > >- if ( $params->{send_letter} ) { >+ if ( $params->{'send_letter'} ) { > my $letter = C4::Letters::GetPreparedLetter( > module => 'bookings', > letter_code => 'BOOKING_CANCELLATION', >@@ -313,7 +319,9 @@ sub cancel { > } > > >-=head3 set_status >+=head2 Internal methods >+ >+=head3 _set_status > > This method changes the status of a booking > >@@ -322,16 +330,15 @@ This method changes the status of a booking > sub _set_status { > my ( $self, $new_status ) = @_; > >- my @valid_statuses = qw(pending completed cancelled); >- unless ( grep { $_ eq $new_status } @valid_statuses ) { >+ my @valid_statuses = qw(new completed cancelled); >+ my $is_valid = any { $new_status eq $_ } @valid_statuses; >+ unless ($is_valid) { > die "Invalid status: $new_status"; > } > > $self->status($new_status); > } > >-=head2 Internal methods >- > =head3 _type > > =cut >diff --git a/Koha/Bookings.pm b/Koha/Bookings.pm >index 77008e69b4..d63e063647 100644 >--- a/Koha/Bookings.pm >+++ b/Koha/Bookings.pm >@@ -36,7 +36,7 @@ Koha::Bookings - Koha Booking object set class > > $bookings->filter_by_active; > >-Will return the bookings that have not ended and without "CANCELLED" status. >+Will return the bookings that have not ended, were cancelled or are completed. > > =cut > >@@ -44,8 +44,8 @@ sub filter_by_active { > my ($self) = @_; > return $self->search( > { >- end_date => { '>=' => \'NOW()' }, >- status => { '!=' => 'cancelled' } >+ end_date => { '>=' => \'NOW()' }, >+ status => { q{!=} => [ -and => qw(cancelled completed) ] } > } > ); > } >diff --git a/Koha/REST/V1/Bookings.pm b/Koha/REST/V1/Bookings.pm >index 69badb404a..87e0161973 100644 >--- a/Koha/REST/V1/Bookings.pm >+++ b/Koha/REST/V1/Bookings.pm >@@ -150,20 +150,19 @@ sub delete { > > =head3 edit > >-Controller function that editing an existing booking >+Controller function that handles editing an existing booking > > =cut > > sub edit { >- my $c = shift->openapi->valid_input or return; >- my $body = $c->req->json; >+ my $c = shift->openapi->valid_input or return; > >- my $booking = Koha::Bookings->find( $c->param('booking_id') ); >+ my $booking = $c->objects->find_rs( Koha::Bookings->new, $c->param('booking_id') ); > return $c->render_resource_not_found("Booking") > unless $booking; > > return try { >- $booking->edit($body); >+ $booking->edit( $c->req->json ); > return $c->render( > status => 200, > openapi => $c->objects->to_api($booking), >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/bookings/list.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/bookings/list.tt >index e7a1526788..6619b20e00 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/bookings/list.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/bookings/list.tt >@@ -50,9 +50,9 @@ > <h1>Bookings for [% INCLUDE 'biblio-title-head.inc' %]</h1> > <div class="page-section" id="bookings-timeline"></div> > <div class="page-section"> >- <fieldset class="action filters" style="cursor:pointer; display: flex; flex-direction: column;"> >- <a id="expired_filter" class="filtered"><i class="fa fa-bars"></i> Show expired</a> >- <a id="cancelled_filter" class="filtered"><i class="fa fa-bars"></i> Show cancelled</a> >+ <fieldset class="action filters d-flex gap-2" style="cursor: pointer;"> >+ <a id="expired_filter" data-filter="expired"><i class="fa fa-bars"></i> Show expired</a> >+ <a id="cancelled_filter" data-filter="cancelled"><i class="fa fa-bars"></i> Show cancelled</a> > </fieldset> > > <table id="bookings_table"></table> >@@ -118,6 +118,11 @@ > > var bookingsSet = new vis.DataSet(); > for (booking of bookings[0]){ >+ const isActive = ["new", "pending", "active"].includes(booking.status); >+ const patronContent = $patron_to_html(booking.patron, { >+ display_cardnumber: true, >+ url: false >+ }); > bookingsSet.add({ > id: booking.booking_id, > booking: booking.booking_id, >@@ -125,18 +130,14 @@ > pickup_library: booking.pickup_library_id, > start: dayjs(booking.start_date).toDate(), > end: dayjs(booking.end_date).toDate(), >- content: $patron_to_html(booking.patron, { >- display_cardnumber: true, >- url: false >- }), >+ content: !isActive ? `<s>${patronContent}</s>` : patronContent, > [% IF CAN_user_circulate_manage_bookings %] >- editable: booking.status != 'cancelled' ? { remove: true, updateTime: true } : false, >+ editable: booking.status !== "cancelled" ? { remove: true, updateTime: true } : false, > [% ELSE %] > editable: false, > [% END %] > type: 'range', >- group: booking.item_id ? booking.item_id : 0, >- className: booking.status, >+ group: booking.item_id ?? 0, > }); > } > >@@ -225,24 +226,23 @@ > } > ); > >- let filter_expired = true; >- let filter_cancelled = false; >- let additional_filters = { >- end_date: function(){ >- if ( filter_expired ) { >+ const filterStates = { expired: true, cancelled: false }; >+ const additional_filters = { >+ end_date: () => { >+ if (filterStates.expired) { > let today = new Date(); >- return { ">=": today.toISOString() } >- } else { >- return; >+ return { ">=": today.toISOString() }; > } > }, >- status: function(){ >- if ( filter_cancelled ) { >- return { "=": 'cancelled' } >- } else { >- return; >+ status: () => { >+ const defaults = ["new", "pending", "active"]; >+ if (filterStates.cancelled) { >+ const filtered = [...defaults, "cancelled"]; >+ return { "-in": filtered }; > } >- } >+ >+ return { "-in": defaults }; >+ }, > }; > > var bookings_table_url = "/api/v1/biblios/[% biblionumber | uri %]/bookings"; >@@ -268,15 +268,23 @@ > orderable: false, > visible: false, > render: function (data, type, row, meta) { >- let is_expired = dayjs(row.end_date).isBefore(new Date()); >- let is_cancelled = row.status == 'cancelled' ? 1 : 0; >+ const is_expired = dayjs(row.end_date).isBefore(new Date()); > if (is_expired) { >- return '<span class="badge rounded-pill bg-secondary">' + _("Expired") + '</span>'; >+ return `<span class="badge rounded-pill bg-secondary"> >+ ${_("Expired")} >+ </span>`; > } >+ >+ const is_cancelled = row.status === "cancelled"; > if (is_cancelled) { >- return '<span class="badge rounded-pill bg-secondary">' + _("Cancelled") + '</span>'; >+ return `<span class="badge rounded-pill bg-secondary"> >+ ${_("Cancelled")} >+ </span>`; > } >- return '<span class="badge rounded-pill bg-success">' + _("Active") + '</span>'; >+ >+ return `<span class="badge rounded-pill bg-success"> >+ ${_("New")} >+ </span>`; > } > }, > { >@@ -340,9 +348,9 @@ > "orderable": false, > "render": function(data, type, row, meta) { > let result = ""; >- let is_cancelled = row.status == 'cancelled' ? 1 : 0; >+ let is_cancelled = row.status === "cancelled"; > [% IF CAN_user_circulate_manage_bookings %] >- if( !is_cancelled ){ >+ if (!is_cancelled) { > result += '<button type="button" class="btn btn-default btn-xs edit-action" data-bs-toggle="modal" data-bs-target="#placeBookingModal" data-booking="'+row.booking_id+'" data-biblionumber="[% biblionumber | uri %]" data-itemnumber="'+row.item_id+'" data-patron="'+row.patron_id+'" data-pickup_library="'+row.pickup_library_id+'" data-start_date="'+row.start_date+'" data-end_date="'+row.end_date+'"><i class="fa fa-pencil" aria-hidden="true"></i> '+_("Edit")+'</button>'; > result += '<button type="button" class="btn btn-default btn-xs cancel-action" data-bs-toggle="modal" data-bs-target="#cancelBookingModal" data-booking="'+row.booking_id+'"><i class="fa fa-trash" aria-hidden="true"></i> '+_("Cancel")+'</button>'; > } >@@ -352,25 +360,46 @@ > }] > }, [], 0, additional_filters); > >- function setupFilter(filterId, activeText, inactiveText, filterVariable) { >- $(filterId).on("click", function() { >- if ($(this).hasClass('filtered')) { >- filterVariable = false; >- $(this).html('<i class="fa fa-filter"></i> ' + inactiveText); >- } else { >- filterVariable = true; >- $(this).html('<i class="fa fa-bars"></i> ' + activeText); >- } >+ document >+ .getElementById("expired_filter") >+ .addEventListener("click", (e) => >+ handleFilter( >+ e, >+ { active: _("Hide expired"), inactive: _("Show expired") }, >+ filterStates, >+ ), >+ ); > >- bookings_table.DataTable().ajax.reload(() => { >- bookings_table.DataTable().column("status:name").visible(!filterVariable, false); >- }); >- $(this).toggleClass('filtered'); >+ document >+ .getElementById("cancelled_filter") >+ .addEventListener("click", (e) => >+ handleFilter( >+ e, >+ { active: _("Hide cancelled"), inactive: _("Show cancelled") }, >+ filterStates, >+ ), >+ ); >+ >+ function handleFilter(e, text, filterStates) { >+ const target = e.target; >+ const { filter } = target.dataset; >+ if (target.classList.contains("filtered")) { >+ filterStates[filter] = false; >+ target.innerHTML = `<i class="fa fa-bars"></i> ${text.inactive}`; >+ } else { >+ filterStates[filter] = true; >+ target.innerHTML = `<i class="fa fa-filter"></i> ${text.active}`; >+ } >+ >+ bookings_table.DataTable().ajax.reload(() => { >+ bookings_table >+ .DataTable() >+ .column("status:name") >+ .visible(filterStates[filter], false); > }); >- } > >- setupFilter("#expired_filter", _("Show expired"), _("Hide expired"), filter_expired); >- setupFilter("#cancelled_filter", _("Show cancelled"), _("Hide cancelled"), filter_cancelled); >+ target.classList.toggle("filtered"); >+ } > > }); > </script> >diff --git a/koha-tmpl/intranet-tmpl/prog/js/cancel_booking_modal.js b/koha-tmpl/intranet-tmpl/prog/js/cancel_booking_modal.js >index bee9abf8d3..0c77696fbc 100644 >--- a/koha-tmpl/intranet-tmpl/prog/js/cancel_booking_modal.js >+++ b/koha-tmpl/intranet-tmpl/prog/js/cancel_booking_modal.js >@@ -1,36 +1,80 @@ >-$('#cancelBookingModal').on('show.bs.modal', function(e) { >- var button = $(e.relatedTarget); >- var booking = button.data('booking'); >- $('#cancel_booking_id').val(booking); >-}); >- >-$("#cancelBookingForm").on('submit', function(e) { >- e.preventDefault(); >- >- var booking_id = $('#cancel_booking_id').val(); >- var url = '/api/v1/bookings/'+booking_id; >- >- var cancelling = $.ajax({ >- 'method': "PATCH", >- 'url': url, >- 'data': JSON.stringify({"status": "cancelled"}), >- 'contentType': "application/json" >- }); >- >- >- cancelling.done(function(data) { >- cancel_success = 1; >- if (bookings_table) { >- bookings_table.api().ajax.reload(); >+(() => { >+ document >+ .getElementById("cancelBookingModal") >+ ?.addEventListener("show.bs.modal", handleShowBsModal); >+ document >+ .getElementById("cancelBookingForm") >+ ?.addEventListener("submit", handleSubmit); >+ >+ async function handleSubmit(e) { >+ e.preventDefault(); >+ >+ const bookingIdInput = document.getElementById("cancel_booking_id"); >+ if (!bookingIdInput) { >+ return; >+ } >+ >+ const bookingId = bookingIdInput.value; >+ if (!bookingId) { >+ return; >+ } >+ >+ let [error, response] = await catchError( >+ fetch(`/api/v1/bookings/${bookingId}`, { >+ method: "PATCH", >+ body: JSON.stringify({ status: "cancelled" }), >+ headers: { >+ "Content-Type": "application/json", >+ }, >+ }) >+ ); >+ if (error || !response.ok) { >+ const alertContainer = document.getElementById( >+ "cancel_booking_result" >+ ); >+ alertContainer.outerHTML = ` >+ <div id="booking_result" class="alert alert-danger"> >+ ${__("Failure")} >+ </div> >+ `; >+ >+ return; > } >- if (typeof timeline !== 'undefined') { >- timeline.itemsData.remove(Number(booking_id)); >+ >+ cancel_success = true; >+ bookings_table?.api().ajax.reload(); >+ timeline?.itemsData.remove(Number(booking_id)); >+ >+ $("#cancelBookingModal").modal("hide"); >+ >+ const bookingsCount = document.querySelector(".bookings_count"); >+ if (!bookingsCount) { >+ return; > } >- $('.bookings_count').html(parseInt($('.bookings_count').html(), 10)-1); >- $('#cancelBookingModal').modal('hide'); >- }); >- >- cancelling.fail(function(data) { >- $('#cancel_booking_result').replaceWith('<div id="booking_result" class="alert alert-danger">'+__("Failure")+'</div>'); >- }); >-}); >+ >+ bookingsCount.innerHTML = parseInt(bookingsCount.innerHTML, 10) - 1; >+ } >+ >+ function handleShowBsModal(e) { >+ const button = e.relatedTarget; >+ if (!button) { >+ return; >+ } >+ >+ const booking = button.dataset.booking; >+ if (!booking) { >+ return; >+ } >+ >+ const bookingIdInput = document.getElementById("cancel_booking_id"); >+ if (!bookingIdInput) { >+ return; >+ } >+ >+ bookingIdInput.value = booking; >+ } >+ >+ function catchError(promise) { >+ return promise.then(data => [undefined, data]).catch(error => [error]); >+ } >+})(); >diff --git a/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js b/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js >index 4a168d107b..90b35e06a8 100644 >--- a/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js >+++ b/koha-tmpl/intranet-tmpl/prog/js/modals/place_booking.js >@@ -301,7 +301,11 @@ $("#placeBookingModal").on("show.bs.modal", function (e) { > > // Fetch list of existing bookings > let bookingsFetch = $.ajax({ >- url: "/api/v1/bookings?biblio_id=" + biblionumber + "&_per_page=-1", >+ url: >+ "/api/v1/bookings?biblio_id=" + >+ biblionumber + >+ "&_per_page=-1" + >+ '&q={"status":{"-in":["new","pending","active"]}}', > dataType: "json", > type: "GET", > }); >diff --git a/t/db_dependent/Koha/Booking.t b/t/db_dependent/Koha/Booking.t >index 5727e6d6e8..b9669c9b68 100755 >--- a/t/db_dependent/Koha/Booking.t >+++ b/t/db_dependent/Koha/Booking.t >@@ -481,16 +481,16 @@ subtest 'set_status() tests' => sub { > )->store; > > my $booking_with_old_status = Koha::Bookings->find( $booking->booking_id ); >- $booking_with_old_status->set_status('completed'); >+ $booking_with_old_status->_set_status('completed'); > is( $booking_with_old_status->unblessed->{status}, 'completed', 'Booking status is now "completed"' ); > >- $booking_with_old_status->set_status('cancelled'); >+ $booking_with_old_status->_set_status('cancelled'); > is( $booking_with_old_status->unblessed->{status}, 'cancelled', 'Booking status is now "cancelled"' ); > > subtest 'unauthorized status' => sub { > plan tests => 2; > >- eval { $booking_with_old_status->set_status('blah'); }; >+ eval { $booking_with_old_status->_set_status('blah'); }; > > if ($@) { > like( >-- >2.47.0
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=172814">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 38175
:
172770
|
172771
|
172772
|
172773
|
172814
|
172815
|
172816
|
172817
|
172880
|
172881
|
172882
|
172883
|
172884
|
173172
|
173173
|
173174
|
173175
|
173176
|
173177
|
173178
|
173201
|
173202
|
173203
|
173346
|
173915