Bug 36100 - Regression in bookings edit
Summary: Regression in bookings edit
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 35468
Blocks:
  Show dependency treegraph
 
Reported: 2024-02-15 10:14 UTC by Martin Renvoize
Modified: 2024-04-26 20:33 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04


Attachments
Bug 36100: Remove readOnly from bookings definition. (924 bytes, patch)
2024-02-15 10:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36100: Remove readOnly from bookings definition. (1.98 KB, patch)
2024-02-15 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36100: Update unit tests for add (1.85 KB, patch)
2024-02-15 12:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36100: Remove readOnly from bookings definition. (2.03 KB, patch)
2024-02-15 22:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 36100: Update unit tests for add (1.89 KB, patch)
2024-02-15 22:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 36100: Remove readOnly from bookings definition. (2.07 KB, patch)
2024-03-01 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36100: Update unit tests for add (1.93 KB, patch)
2024-03-01 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36100: (QA follow-up) Move 400 to 409 (2.10 KB, patch)
2024-03-01 07:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36100: Remove readOnly from bookings definition. (2.12 KB, patch)
2024-03-01 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36100: Update unit tests for add (1.99 KB, patch)
2024-03-01 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36100: (QA follow-up) Move 400 to 409 (2.15 KB, patch)
2024-03-01 08:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2024-02-15 10:14:08 UTC
I somehow did the oposite of what I meant to.. likely a rebase issue in bug 35468.. the readOnly should be removed, not added for the bookings spec, else it blocks edits.
Comment 1 Martin Renvoize 2024-02-15 10:16:12 UTC
Created attachment 162192 [details] [review]
Bug 36100: Remove readOnly from bookings definition.

Whilst readOnly feels correct here, swagger doesn't work that way at our
version. It must be removed to restore the ability to edit exising
bookings.
Comment 2 Martin Renvoize 2024-02-15 10:17:02 UTC
Test plan is to create a booking and then attempt to edit it.. without the patch you'll be faced with an error.. with the patch the edit should succeed.
Comment 3 Martin Renvoize 2024-02-15 12:34:33 UTC
Created attachment 162202 [details] [review]
Bug 36100: Remove readOnly from bookings definition.

Whilst readOnly feels correct here, swagger doesn't work that way at our
version. It must be removed to restore the ability to edit exising
bookings.
Comment 4 Martin Renvoize 2024-02-15 12:34:35 UTC
Created attachment 162203 [details] [review]
Bug 36100: Update unit tests for add

This patch updates the unit tests for booking add to confirm that
without readOnly we still return an error should a user attempt to
submit a booking with a booking_id that would clash.
Comment 5 David Nind 2024-02-15 22:50:55 UTC
Created attachment 162229 [details] [review]
Bug 36100: Remove readOnly from bookings definition.

Whilst readOnly feels correct here, swagger doesn't work that way at our
version. It must be removed to restore the ability to edit exising
bookings.

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-02-15 22:50:57 UTC
Created attachment 162230 [details] [review]
Bug 36100: Update unit tests for add

This patch updates the unit tests for booking add to confirm that
without readOnly we still return an error should a user attempt to
submit a booking with a booking_id that would clash.

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2024-02-15 22:57:01 UTC
Testing notes (using KTD):

1. Enable an item to be booked:
   1.1 Go to a record with item(s).
   1.2 Go to the items tab.
   1.3 Update an item so that it is bookable (Priority section, update Bookable to 'Yes'

2. From the buttons above the title, select 'Place booking':
   2.1 Choose a patron
   2.2 Select a period (click a start and end date)
   2.3 Submit

3. Attempt to edit a booking from the Bookings tab:
   3.1 Select 'Edit' for an item.
   3.2 Delete the current period.
   3.4 Add a new period.
   3.5 Submit.
   3.6 Get a 'Failure' message.

4. Apply the patch.

5. Repeat step 3 - should now be able to successfully edit the booking.
Comment 8 Julian Maurice 2024-02-22 14:18:25 UTC
Editing a booking works if I just remove booking_id from the PUT JSON payload (booking ID is in the URL already so it's not needed in the body):

diff --git a/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js b/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
index 946be01e3cd..74cc90e49a6 100644
--- a/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
+++ b/koha-tmpl/intranet-tmpl/prog/js/place_booking_modal.js
@@ -504,3 +504,2 @@ $("#placeBookingForm").on('submit', function(e) {
             'data': JSON.stringify({
-                "booking_id": booking_id,
                 "start_date": start_date,

Is there anything else that is fixed by removing readOnly ? Does removing booking_id from the body like I did cause issues ?
Comment 9 Martin Renvoize 2024-02-29 09:44:37 UTC
I'm happy with that alternative approach too to be honest.. I'm not sure we're at all consistent in our schema to date regarding readOnly.
Comment 10 Marcel de Rooy 2024-03-01 07:59:48 UTC
Created attachment 162633 [details] [review]
Bug 36100: Remove readOnly from bookings definition.

Whilst readOnly feels correct here, swagger doesn't work that way at our
version. It must be removed to restore the ability to edit exising
bookings.

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Marcel de Rooy 2024-03-01 07:59:50 UTC
Created attachment 162634 [details] [review]
Bug 36100: Update unit tests for add

This patch updates the unit tests for booking add to confirm that
without readOnly we still return an error should a user attempt to
submit a booking with a booking_id that would clash.

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 Marcel de Rooy 2024-03-01 07:59:53 UTC
Created attachment 162635 [details] [review]
Bug 36100: (QA follow-up) Move 400 to 409

Duplicate exceptions tend to generate a 409 in our REST API.
Comment 13 Marcel de Rooy 2024-03-01 08:00:52 UTC
Created attachment 162636 [details] [review]
Bug 36100: Remove readOnly from bookings definition.

Whilst readOnly feels correct here, swagger doesn't work that way at our
version. It must be removed to restore the ability to edit exising
bookings.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-03-01 08:00:54 UTC
Created attachment 162637 [details] [review]
Bug 36100: Update unit tests for add

This patch updates the unit tests for booking add to confirm that
without readOnly we still return an error should a user attempt to
submit a booking with a booking_id that would clash.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2024-03-01 08:00:56 UTC
Created attachment 162638 [details] [review]
Bug 36100: (QA follow-up) Move 400 to 409

Duplicate exceptions tend to generate a 409 in our REST API.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Katrin Fischer 2024-03-07 15:17:29 UTC
Will push this patch, but something to investigate more later:

Editing the linked patron and the booking dates works with this patch, but changing the item assigned from the barcode to 'any item' does still result in failure.

==> /var/log/koha/kohadev/plack-api-error.log <==
[2024/03/07 15:16:41] [ERROR] PUT /api/v1/bookings/10: unhandled exception (Mojo::Exception)<<Can't call method "itemnumber" on an undefined value at /kohadevbox/koha/Koha/Biblio.pm line 321.>>
Comment 17 Katrin Fischer 2024-03-07 15:38:06 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Lucas Gass 2024-03-19 19:35:02 UTC
Missing 23.05.x dependencies, no backport.