Bugzilla – Attachment 172884 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: (QA follow-up) DRY out PATCH handling
Bug-38175-QA-follow-up-DRY-out-PATCH-handling.patch (text/plain), 29.92 KB, created by
Martin Renvoize (ashimema)
on 2024-10-17 14:10:10 UTC
(
hide
)
Description:
Bug 38175: (QA follow-up) DRY out PATCH handling
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2024-10-17 14:10:10 UTC
Size:
29.92 KB
patch
obsolete
>From af89fbe7281f88b0143161b6ca3783c77bc9a7fe Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Date: Thu, 17 Oct 2024 12:02:10 +0100 >Subject: [PATCH] Bug 38175: (QA follow-up) DRY out PATCH handling > >We were repeating some code for the PATCH endpoint instead of using the >existing update handling code. This lead to two issues: > >1) We weren't catching status updates on standard updates, only the > patch. >2) We were limiting the PATCH endpoint to just status updates when it > could happily be used for other fields too. > >This patch removes the introduction of the 'edit' method from both the >REST controller and the corresponding object and moves the logic into >the store method of the object where it's easily testable. > >We also DRY out the notice sending code a little for re-use. > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > Koha/Booking.pm | 163 +++------ > Koha/Object.pm | 10 + > Koha/REST/V1/Bookings.pm | 24 -- > api/v1/swagger/definitions/booking_patch.yaml | 45 +++ > api/v1/swagger/paths/bookings.yaml | 25 +- > api/v1/swagger/swagger.yaml | 2 + > t/db_dependent/Koha/Booking.t | 311 +++++------------- > t/db_dependent/api/v1/bookings.t | 5 +- > 8 files changed, 206 insertions(+), 379 deletions(-) > create mode 100644 api/v1/swagger/definitions/booking_patch.yaml > >diff --git a/Koha/Booking.pm b/Koha/Booking.pm >index c0c1b67f8ba..e32b0f50358 100644 >--- a/Koha/Booking.pm >+++ b/Koha/Booking.pm >@@ -143,7 +143,6 @@ sub store { > booking_id => $self->in_storage ? $self->booking_id : undef > } > ); >- > # FIXME: We should be able to combine the above two functions into one > > # Assign item at booking time >@@ -151,57 +150,26 @@ sub store { > $self->_assign_item_for_booking; > } > >- my $is_modification = $self->in_storage; >- my $old_booking = $self->get_from_storage; >- >- if ( $self = $self->SUPER::store ) { >- my $patron = $self->patron; >- my $pickup_library = $self->pickup_library; >- my $branch = C4::Context->userenv->{'branch'}; >- >- if ( $is_modification >- and any { $old_booking->$_ ne $self->$_ } qw(pickup_library_id start_date end_date) ) >+ if ( !$self->in_storage ) { >+ $self->SUPER::store; >+ $self->discard_changes; >+ $self->_send_notice( { notice => 'BOOKING_CONFIRMATION' } ); >+ } else { >+ my %updated_columns = $self->_result->get_dirty_columns; >+ return $self->SUPER::store unless %updated_columns; >+ >+ my $old_booking = $self->get_from_storage; >+ $self->SUPER::store; >+ >+ if ( exists( $updated_columns{status} ) && $updated_columns{status} eq 'cancelled' ) { >+ $self->_send_notice( >+ { notice => 'BOOKING_CANCELLATION', objects => { old_booking => $old_booking } } ); >+ } elsif ( exists( $updated_columns{pickup_library_id} ) >+ or exists( $updated_columns{start_date} ) >+ or exists( $updated_columns{end_date} ) ) > { >- my $letter = C4::Letters::GetPreparedLetter( >- module => 'bookings', >- letter_code => 'BOOKING_MODIFICATION', >- message_transport_type => 'email', >- branchcode => $branch, >- lang => $patron->lang, >- objects => { >- old_booking => $old_booking, >- booking => $self >- }, >- ); >- >- if ($letter) { >- C4::Letters::EnqueueLetter( >- { >- letter => $letter, >- borrowernumber => $patron->borrowernumber, >- message_transport_type => 'email', >- } >- ); >- } >- } elsif ( !$is_modification ) { >- my $letter = C4::Letters::GetPreparedLetter( >- module => 'bookings', >- letter_code => 'BOOKING_CONFIRMATION', >- message_transport_type => 'email', >- branchcode => $branch, >- lang => $patron->lang, >- objects => { booking => $self }, >- ); >- >- if ($letter) { >- C4::Letters::EnqueueLetter( >- { >- letter => $letter, >- borrowernumber => $patron->borrowernumber, >- message_transport_type => 'email', >- } >- ); >- } >+ $self->_send_notice( >+ { notice => 'BOOKING_MODIFICATION', objects => { old_booking => $old_booking } } ); > } > } > } >@@ -298,85 +266,44 @@ sub to_api_mapping { > return {}; > } > >-=head3 edit >- >-This method allows patching a booking >- >-=cut >- >-sub edit { >- my ( $self, $params ) = @_; >- >- my $new_status = $params->{'status'}; >- unless ($new_status) { >- return $self->store; >- } >- >- $self->_set_status($new_status); >- >- my $status = $self->status; >- if ( $status eq 'cancelled' ) { >- $self->cancel( { send_letter => 1 } ); >- } >+=head2 Internal methods > >- return $self->store; >-} >+=head3 _send_notice > >-=head3 cancel >+ $self->_send_notice(); > >-This method adds possibility of cancelling a booking (kept in table but flagged with 'cancelled' status) >-Also adds param to send a letter to the borrower affected by the cancellation >+Sends appropriate notice to patron. > > =cut > >-sub cancel { >+sub _send_notice { > my ( $self, $params ) = @_; > >- my $branch = C4::Context->userenv->{'branch'}; >- my $patron = $self->patron; >- my $pickup_library = $self->pickup_library; >- >- if ( $params->{'send_letter'} ) { >- my $letter = C4::Letters::GetPreparedLetter( >- module => 'bookings', >- letter_code => 'BOOKING_CANCELLATION', >- message_transport_type => 'email', >- branchcode => $branch, >- lang => $patron->lang, >- objects => { booking => $self } >- ); >+ my $notice = $params->{notice}; >+ my $objects = $params->{objects} // {}; >+ $objects->{booking} = $self; > >- if ($letter) { >- C4::Letters::EnqueueLetter( >- { >- letter => $letter, >- borrowernumber => $patron->borrowernumber, >- message_transport_type => 'email', >- } >- ); >- } >- } >-} >- >- >-=head2 Internal methods >- >-=head3 _set_status >+ my $branch = C4::Context->userenv->{'branch'}; >+ my $patron = $self->patron; > >-This method changes the status of a booking >- >-=cut >- >-sub _set_status { >- my ( $self, $new_status ) = @_; >+ my $letter = C4::Letters::GetPreparedLetter( >+ module => 'bookings', >+ letter_code => $notice, >+ message_transport_type => 'email', >+ branchcode => $branch, >+ lang => $patron->lang, >+ objects => $objects >+ ); > >- my @valid_statuses = qw(new completed cancelled); >- my $is_valid = any { $new_status eq $_ } @valid_statuses; >- unless ($is_valid) { >- die "Invalid status: $new_status"; >+ if ($letter) { >+ C4::Letters::EnqueueLetter( >+ { >+ letter => $letter, >+ borrowernumber => $patron->borrowernumber, >+ message_transport_type => 'email', >+ } >+ ); > } >- >- $self->status($new_status); > } > > =head3 _type >diff --git a/Koha/Object.pm b/Koha/Object.pm >index ad34bbc9480..d9c3d528cbb 100644 >--- a/Koha/Object.pm >+++ b/Koha/Object.pm >@@ -202,6 +202,16 @@ sub store { > property => $property =~ /(\w+\.\w+)$/ ? $1 : $property, # results in table.column without quotes or backtics > ); > } >+ elsif ( $_->{msg} =~ /Data truncated for column \W?(?<property>\w+)/ ) { # The optional \W in the regex might be a quote or backtick >+ my $property = $+{property}; >+ my $type = $columns_info->{$property}->{data_type}; >+ Koha::Exceptions::Object::BadValue->throw( >+ type => 'enum', >+ property => $property =~ /(\w+\.\w+)$/ >+ ? $1 >+ : $property, # results in table.column without quotes or backtics >+ ) if $type eq 'enum'; >+ } > } > # Catch-all for foreign key breakages. It will help find other use cases > $_->rethrow(); >diff --git a/Koha/REST/V1/Bookings.pm b/Koha/REST/V1/Bookings.pm >index 87e0161973d..096c58961b3 100644 >--- a/Koha/REST/V1/Bookings.pm >+++ b/Koha/REST/V1/Bookings.pm >@@ -148,28 +148,4 @@ sub delete { > }; > } > >-=head3 edit >- >-Controller function that handles editing an existing booking >- >-=cut >- >-sub edit { >- my $c = shift->openapi->valid_input or return; >- >- 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( $c->req->json ); >- return $c->render( >- status => 200, >- openapi => $c->objects->to_api($booking), >- ); >- } catch { >- $c->unhandled_exception($_); >- }; >-} >- > 1; >diff --git a/api/v1/swagger/definitions/booking_patch.yaml b/api/v1/swagger/definitions/booking_patch.yaml >new file mode 100644 >index 00000000000..f8fd060653d >--- /dev/null >+++ b/api/v1/swagger/definitions/booking_patch.yaml >@@ -0,0 +1,45 @@ >+--- >+additionalProperties: false >+properties: >+ biblio_id: >+ description: Internal identifier for the parent bibliographic record >+ type: integer >+ booking_id: >+ description: Internal booking identifier >+ readOnly: true >+ type: integer >+ creation_date: >+ description: Creation date and time of this booking >+ readOnly: true >+ format: date-time >+ type: string >+ end_date: >+ description: Start date and time of this booking >+ format: date-time >+ type: string >+ item_id: >+ description: Internal item identifier >+ type: >+ - integer >+ - 'null' >+ modification_date: >+ description: Modification date and time of this booking >+ readOnly: true >+ format: date-time >+ type: string >+ patron_id: >+ description: Internal patron identifier >+ type: integer >+ pickup_library_id: >+ description: Internal pickup_library identifier >+ type: string >+ start_date: >+ description: Start date and time of this booking >+ format: date-time >+ type: string >+ status: >+ description: Status of the booking >+ type: >+ - string >+ - "null" >+type: object >diff --git a/api/v1/swagger/paths/bookings.yaml b/api/v1/swagger/paths/bookings.yaml >index 047329ab5f2..4c8817c6f14 100644 >--- a/api/v1/swagger/paths/bookings.yaml >+++ b/api/v1/swagger/paths/bookings.yaml >@@ -208,17 +208,19 @@ > operationId: updateBooking > parameters: > - $ref: "../swagger.yaml#/parameters/booking_id_pp" >- - description: A booking object >+ - description: A complete booking object to replace the current one > in: body > name: body > required: true > schema: > $ref: ../swagger.yaml#/definitions/booking >+ consumes: >+ - application/json > produces: > - application/json > responses: > 200: >- description: A booking >+ description: Updated booking > schema: > $ref: ../swagger.yaml#/definitions/booking > 400: >@@ -253,24 +255,15 @@ > circulate: manage_bookings > x-mojo-to: Bookings#update > patch: >- x-mojo-to: Bookings#edit > operationId: editBooking >- tags: >- - bookings >- summary: Edit booking > parameters: > - $ref: "../swagger.yaml#/parameters/booking_id_pp" >- - name: body >+ - description: A partial booking object containing fields to modify > in: body >- description: A JSON object containing fields to modify >+ name: body > required: true > schema: >- type: object >- properties: >- status: >- description: Set booking status >- type: string >- additionalProperties: false >+ $ref: ../swagger.yaml#/definitions/booking_patch > consumes: > - application/json > produces: >@@ -304,6 +297,10 @@ > description: Under maintenance > schema: > $ref: ../swagger.yaml#/definitions/error >+ summary: Update booking >+ tags: >+ - bookings > x-koha-authorization: > permissions: > circulate: manage_bookings >+ x-mojo-to: Bookings#update >diff --git a/api/v1/swagger/swagger.yaml b/api/v1/swagger/swagger.yaml >index dd72361380e..e682c7665f3 100644 >--- a/api/v1/swagger/swagger.yaml >+++ b/api/v1/swagger/swagger.yaml >@@ -20,6 +20,8 @@ definitions: > $ref: ./definitions/basket.yaml > booking: > $ref: ./definitions/booking.yaml >+ booking_patch: >+ $ref: ./definitions/booking_patch.yaml > bundle_link: > $ref: ./definitions/bundle_link.yaml > cash_register: >diff --git a/t/db_dependent/Koha/Booking.t b/t/db_dependent/Koha/Booking.t >index bbb4426e0fe..1c4ec2debca 100755 >--- a/t/db_dependent/Koha/Booking.t >+++ b/t/db_dependent/Koha/Booking.t >@@ -20,7 +20,7 @@ > use Modern::Perl; > use utf8; > >-use Test::More tests => 6; >+use Test::More tests => 2; > > use Test::Exception; > >@@ -123,7 +123,7 @@ subtest 'Relation accessor tests' => sub { > }; > > subtest 'store() tests' => sub { >- plan tests => 15; >+ plan tests => 16; > $schema->storage->txn_begin; > > my $patron = $builder->build_object( { class => "Koha::Patrons" } ); >@@ -323,15 +323,72 @@ subtest 'store() tests' => sub { > # รข Any (1) |--| > }; > >- subtest 'modification notice trigger' => sub { >- plan tests => 3; >+ subtest 'confirmation notice trigger' => sub { >+ plan tests => 2; > > my $original_notices_count = Koha::Notice::Messages->search( >+ { >+ letter_code => 'BOOKING_CONFIRMATION', >+ borrowernumber => $patron->borrowernumber, >+ } >+ )->count; >+ >+ # Reuse previous booking to produce a clash >+ eval { $booking = Koha::Booking->new( $booking->unblessed )->store }; >+ >+ my $post_notices_count = Koha::Notice::Messages->search( >+ { >+ letter_code => 'BOOKING_CONFIRMATION', >+ borrowernumber => $patron->borrowernumber, >+ } >+ )->count; >+ is( >+ $post_notices_count, >+ $original_notices_count, >+ 'Koha::Booking->store should not have enqueued a BOOKING_CONFIRMATION email if booking creation fails' >+ ); >+ >+ $start_1 = dt_from_string->add( months => 1 )->truncate( to => 'day' ); >+ $end_1 = $start_1->clone()->add( days => 1 ); >+ >+ $booking = Koha::Booking->new( >+ { >+ patron_id => $patron->borrowernumber, >+ biblio_id => $biblio->biblionumber, >+ pickup_library_id => $item_2->homebranch, >+ start_date => $start_1->datetime(q{ }), >+ end_date => $end_1->datetime(q{ }), >+ } >+ )->store; >+ >+ $post_notices_count = Koha::Notice::Messages->search( >+ { >+ letter_code => 'BOOKING_CONFIRMATION', >+ borrowernumber => $patron->borrowernumber, >+ } >+ )->count; >+ is( >+ $post_notices_count, >+ $original_notices_count + 1, >+ 'Koha::Booking->store should have enqueued a BOOKING_CONFIRMATION email for a new booking' >+ ); >+ }; >+ >+ subtest 'modification/cancellation notice triggers' => sub { >+ plan tests => 5; >+ >+ my $original_modification_notices_count = Koha::Notice::Messages->search( > { > letter_code => 'BOOKING_MODIFICATION', > borrowernumber => $patron->borrowernumber, > } > )->count; >+ my $original_cancellation_notices_count = Koha::Notice::Messages->search( >+ { >+ letter_code => 'BOOKING_CANCELLATION', >+ borrowernumber => $patron->borrowernumber, >+ } >+ )->count; > > $start_1 = dt_from_string->add( months => 1 )->truncate( to => 'day' ); > $end_1 = $start_1->clone()->add( days => 1 ); >@@ -347,15 +404,15 @@ subtest 'store() tests' => sub { > } > )->store; > >- my $post_notices_count = Koha::Notice::Messages->search( >+ my $post_modification_notices_count = Koha::Notice::Messages->search( > { > letter_code => 'BOOKING_MODIFICATION', > borrowernumber => $patron->borrowernumber, > } > )->count; > is( >- $post_notices_count, >- $original_notices_count, >+ $post_modification_notices_count, >+ $original_modification_notices_count, > 'Koha::Booking->store should not have enqueued a BOOKING_MODIFICATION email for a new booking' > ); > >@@ -367,15 +424,15 @@ subtest 'store() tests' => sub { > } > ); > >- $post_notices_count = Koha::Notice::Messages->search( >+ $post_modification_notices_count = Koha::Notice::Messages->search( > { > letter_code => 'BOOKING_MODIFICATION', > borrowernumber => $patron->borrowernumber, > } > )->count; > is( >- $post_notices_count, >- $original_notices_count, >+ $post_modification_notices_count, >+ $original_modification_notices_count, > 'Koha::Booking->store should not have enqueued a BOOKING_MODIFICATION email for a booking with modified item_id' > ); > >@@ -386,15 +443,15 @@ subtest 'store() tests' => sub { > ); > > # start_date, end_date and pickup_library_id should behave identical >- $post_notices_count = Koha::Notice::Messages->search( >+ $post_modification_notices_count = Koha::Notice::Messages->search( > { > letter_code => 'BOOKING_MODIFICATION', > borrowernumber => $patron->borrowernumber, > } > )->count; > is( >- $post_notices_count, >- $original_notices_count + 1, >+ $post_modification_notices_count, >+ $original_modification_notices_count + 1, > 'Koha::Booking->store should have enqueued a BOOKING_MODIFICATION email for a booking with modified start_date' > ); > >@@ -403,241 +460,55 @@ subtest 'store() tests' => sub { > end_date => $end_1->clone()->add( days => 1 )->datetime(q{ }), > } > ); >- }; >- >- subtest 'confirmation notice trigger' => sub { >- plan tests => 2; > >- my $original_notices_count = Koha::Notice::Messages->search( >+ $booking->update( > { >- letter_code => 'BOOKING_CONFIRMATION', >- borrowernumber => $patron->borrowernumber, >+ status => 'completed', > } >- )->count; >- >- # Reuse previous booking to produce a clash >- eval { $booking = Koha::Booking->new( $booking->unblessed )->store }; >+ ); > >- my $post_notices_count = Koha::Notice::Messages->search( >+ my $post_cancellation_notices_count = Koha::Notice::Messages->search( > { >- letter_code => 'BOOKING_CONFIRMATION', >+ letter_code => 'BOOKING_CANCELLATION', > borrowernumber => $patron->borrowernumber, > } > )->count; > is( >- $post_notices_count, >- $original_notices_count, >- 'Koha::Booking->store should not have enqueued a BOOKING_CONFIRMATION email if booking creation fails' >+ $post_cancellation_notices_count, >+ $original_cancellation_notices_count, >+ 'Koha::Booking->store should NOT have enqueued a BOOKING_CANCELLATION email for a booking status change that is not a "cancellation"' > ); > >- $start_1 = dt_from_string->add( months => 1 )->truncate( to => 'day' ); >- $end_1 = $start_1->clone()->add( days => 1 ); >- >- $booking = Koha::Booking->new( >- { >- patron_id => $patron->borrowernumber, >- biblio_id => $biblio->biblionumber, >- pickup_library_id => $item_2->homebranch, >- start_date => $start_1->datetime(q{ }), >- end_date => $end_1->datetime(q{ }), >- } >- )->store; >- >- $post_notices_count = Koha::Notice::Messages->search( >+ $booking->update( > { >- letter_code => 'BOOKING_CONFIRMATION', >- borrowernumber => $patron->borrowernumber, >+ status => 'cancelled', > } >- )->count; >- is( >- $post_notices_count, >- $original_notices_count + 1, >- 'Koha::Booking->store should have enqueued a BOOKING_CONFIRMATION email for a new booking' > ); >- }; >- >- $schema->storage->txn_rollback; >-}; >- >-subtest 'delete() tests' => sub { >- plan tests => 2; >- >- $schema->storage->txn_begin; >- >- my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >- t::lib::Mocks::mock_userenv( { patron => $patron } ); >- my $biblio = $builder->build_sample_biblio; >- my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); >- my $start_0 = dt_from_string->subtract( days => 2 )->truncate( to => 'day' ); >- my $end_0 = $start_0->clone->add( days => 6 ); >- my $original_notices_count = Koha::Notice::Messages->search( >- { >- letter_code => 'BOOKING_CANCELLATION', >- borrowernumber => $patron->borrowernumber, >- } >- )->count; >- >- $item_1->bookable(1)->store; >- >- my $booking = Koha::Booking->new( >- { >- patron_id => $patron->borrowernumber, >- biblio_id => $biblio->biblionumber, >- item_id => $item_1->itemnumber, >- pickup_library_id => $item_1->homebranch, >- start_date => $start_0, >- end_date => $end_0 >- } >- )->store; >- >- my $deleted = $booking->delete; >- is( >- ref($deleted), 'Koha::Booking', >- 'Koha::Booking->delete should return the Koha::Booking object if the booking has been correctly deleted' >- ); >- is( >- Koha::Bookings->search( { booking_id => $booking->booking_id } )->count, 0, >- 'Koha::Booking->delete should have deleted the booking' >- ); >- >- $schema->storage->txn_rollback; >-}; >- >-subtest 'edit() tests' => sub { >- plan tests => 1; >- >- $schema->storage->txn_begin; >- >- my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >- my $biblio = $builder->build_sample_biblio; >- my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); >- my $start_0 = dt_from_string->subtract( days => 2 )->truncate( to => 'day' ); >- my $end_0 = $start_0->clone->add( days => 6 ); >- >- $item_1->bookable(1)->store; >- >- my $booking = Koha::Booking->new( >- { >- patron_id => $patron->borrowernumber, >- biblio_id => $biblio->biblionumber, >- item_id => $item_1->itemnumber, >- pickup_library_id => $item_1->homebranch, >- start_date => $start_0, >- end_date => $end_0, >- } >- )->store; >- >- my $booking_to_edit = Koha::Bookings->find( $booking->booking_id ); >- $booking_to_edit->edit( { status => 'completed' } ); > >- is( >- $booking_to_edit->unblessed->{status}, 'completed', >- 'Koha::Booking->edit should edit booking with passed params' >- ); >- >- $schema->storage->txn_rollback; >-}; >- >-subtest 'cancel() tests' => sub { >- plan tests => 1; >- >- $schema->storage->txn_begin; >- >- my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >- my $biblio = $builder->build_sample_biblio; >- my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); >- my $start_0 = dt_from_string->subtract( days => 2 )->truncate( to => 'day' ); >- my $end_0 = $start_0->clone->add( days => 6 ); >- my $original_notices_count = Koha::Notice::Messages->search( >- { >- letter_code => 'BOOKING_CANCELLATION', >- borrowernumber => $patron->borrowernumber, >- } >- )->count; >- >- $item_1->bookable(1)->store; >- >- my $booking = Koha::Booking->new( >- { >- patron_id => $patron->borrowernumber, >- biblio_id => $biblio->biblionumber, >- item_id => $item_1->itemnumber, >- pickup_library_id => $item_1->homebranch, >- start_date => $start_0, >- end_date => $end_0, >- } >- )->store; >- >- my $booking_to_cancel = Koha::Bookings->find( $booking->booking_id ); >- $booking_to_cancel->cancel( { send_letter => 1 } ); >- >- subtest 'notice trigger' => sub { >- plan tests => 1; >- >- my $post_notices_count = Koha::Notice::Messages->search( >+ $post_cancellation_notices_count = Koha::Notice::Messages->search( > { > letter_code => 'BOOKING_CANCELLATION', > borrowernumber => $patron->borrowernumber, > } > )->count; > is( >- $post_notices_count, >- $original_notices_count + 1, >- 'Koha::Booking->cancel should have enqueued a BOOKING_CANCELLATION email' >+ $post_cancellation_notices_count, >+ $original_cancellation_notices_count + 1, >+ 'Koha::Booking->store should have enqueued a BOOKING_CANCELLATION email for a booking status change that is a "cancellation"' > ); > }; > >- $schema->storage->txn_rollback; >-}; >- >-subtest 'set_status() tests' => sub { >- plan tests => 3; >- >- $schema->storage->txn_begin; >- >- my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); >- my $biblio = $builder->build_sample_biblio; >- my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); >- my $start_0 = dt_from_string->subtract( days => 2 )->truncate( to => 'day' ); >- my $end_0 = $start_0->clone->add( days => 6 ); >- >- $item_1->bookable(1)->store; >- >- my $booking = Koha::Booking->new( >- { >- patron_id => $patron->borrowernumber, >- biblio_id => $biblio->biblionumber, >- item_id => $item_1->itemnumber, >- pickup_library_id => $item_1->homebranch, >- start_date => $start_0, >- end_date => $end_0, >- status => 'new', >- } >- )->store; >- >- my $booking_with_old_status = Koha::Bookings->find( $booking->booking_id ); >- $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'); >- is( $booking_with_old_status->unblessed->{status}, 'cancelled', 'Booking status is now "cancelled"' ); >- >- subtest 'unauthorized status' => sub { >+ subtest 'status change exception' => sub { > plan tests => 2; > >- eval { $booking_with_old_status->_set_status('blah'); }; >- >- if ($@) { >- like( >- $@, qr/Invalid status: blah/, >- 'An error is raised for unauthorized status' >- ); >- } else { >- fail('Expected an error but none was raised'); >- } >+ $booking->discard_changes; >+ my $status = $booking->status; >+ throws_ok { $booking->update( { status => 'blah' } ) } 'Koha::Exceptions::Object::BadValue', >+ 'Throws exception when passed booking status would fail enum constraint'; > > # Status unchanged >- is( $booking_with_old_status->unblessed->{status}, 'cancelled', 'Booking status is still "cancelled"' ); >+ $booking->discard_changes; >+ is( $booking->status, $status, 'Booking status is unchanged' ); > }; > > $schema->storage->txn_rollback; >diff --git a/t/db_dependent/api/v1/bookings.t b/t/db_dependent/api/v1/bookings.t >index 44329537020..349e488c961 100755 >--- a/t/db_dependent/api/v1/bookings.t >+++ b/t/db_dependent/api/v1/bookings.t >@@ -477,9 +477,8 @@ subtest 'patch() tests' => sub { > my $booking_id = $builder->build_object( { class => 'Koha::Bookings' } )->id; > > # Unauthorized attempt to partial update via PATCH >- $t->patch_ok( >- "//$unauth_userid:$password@/api/v1/bookings/$booking_id" => json => { status => 'cancelled' } >- )->status_is(403); >+ $t->patch_ok( "//$unauth_userid:$password@/api/v1/bookings/$booking_id" => json => { status => 'cancelled' } ) >+ ->status_is(403); > > my $biblio = $builder->build_sample_biblio; > my $item = $builder->build_sample_item( { bookable => 1, biblionumber => $biblio->id } ); >-- >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=172884">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