Hello, For the moment, in the "Bookings" code and functionality, when a booking is cancelled via the action button or graphically on the timeline, it is completely deleted. Given the recent enhancement adding the status column to the bookings table, it would probably be preferable to change the status and keep the booking in the table. Later, having another table like holds and old_holds would be preferable, but it's a first step towards a more logical change.
Created attachment 172770 [details] [review] Bug 38175: Improve Bookings feature with status With the integration of the new status column for bookings, it would be preferable to keep the booking in database and simply change its status to 'cancel' when you cancel it via the action button or the timeline. So I've added partial updating via the API with a new PATCH method to partially edit a booking. It is currently active for status changes. Graphically, this translates into the disappearance of the action buttons if the booking has already been canceled, although it remains visible in the table and timeline (the style is a proposal for the moment, intended simply to graphically differentiate a canceled booking from others). I've also added a filter to the filter_by_active method to exclude cancelled bookings. 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 Sponsored by: BibLibre wq
Created attachment 172771 [details] [review] Bug 38175: Add tests Sponsored by: BibLibre
Created attachment 172772 [details] [review] Bug 38175: Improve Bookings feature with status With the integration of the new status column for bookings, it would be preferable to keep the booking in database and simply change its status to 'cancel' when you cancel it via the action button or the timeline. So I've added partial updating via the API with a new PATCH method to partially edit a booking. It is currently active for status changes. Graphically, this translates into the disappearance of the action buttons if the booking has already been canceled, although it remains visible in the table and timeline (the style is a proposal for the moment, intended simply to graphically differentiate a canceled booking from others). I've also added a filter to the filter_by_active method to exclude cancelled bookings. 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 Sponsored by: BibLibre
Created attachment 172773 [details] [review] Bug 38175: Add tests Sponsored by: BibLibre
I know Paul is looking here, but I'm also lined up for a QA asap once he's tested and worked on it a bit too :) Thanks Thubaud :)
Created attachment 172814 [details] [review] Bug 38175: (follow-up) Refactor staff interface additions to vanilla JS; adjust modules, controllers for coding guidelines and consistency 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
Created attachment 172815 [details] [review] Bug 38175: Improve Bookings feature with status With the integration of the new status column for bookings, it would be preferable to keep the booking in database and simply change its status to 'cancel' when you cancel it via the action button or the timeline. So I've added partial updating via the API with a new PATCH method to partially edit a booking. It is currently active for status changes. Graphically, this translates into the disappearance of the action buttons if the booking has already been canceled, although it remains visible in the table and timeline (the style is a proposal for the moment, intended simply to graphically differentiate a canceled booking from others). I've also added a filter to the filter_by_active method to exclude cancelled bookings. 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 Sponsored by: BibLibre Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 172816 [details] [review] Bug 38175: Add tests Sponsored by: BibLibre Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Created attachment 172817 [details] [review] Bug 38175: (follow-up) Refactor staff interface additions to vanilla JS; adjust modules, controllers for coding guidelines and consistency 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 Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Working on this one, just lining up the ducks for an easy push for Katrin.. I'm going to rebase it against the new dependencies I've added in.
Created attachment 172880 [details] [review] Bug 38175: Improve Bookings feature with status With the integration of the new status column for bookings, it would be preferable to keep the booking in database and simply change its status to 'cancel' when you cancel it via the action button or the timeline. So I've added partial updating via the API with a new PATCH method to partially edit a booking. It is currently active for status changes. Graphically, this translates into the disappearance of the action buttons if the booking has already been canceled, although it remains visible in the table and timeline (the style is a proposal for the moment, intended simply to graphically differentiate a canceled booking from others). I've also added a filter to the filter_by_active method to exclude cancelled bookings. 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 Sponsored by: BibLibre Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 172881 [details] [review] Bug 38175: Add tests Sponsored by: BibLibre Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 172882 [details] [review] Bug 38175: (follow-up) Refactor staff interface additions to vanilla JS; adjust modules, controllers for coding guidelines and consistency 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 Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 172883 [details] [review] Bug 38175: (QA follow-up) Remove superflous method If all we're doing is calling SUPER, then we don't need to wrap ;P Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 172884 [details] [review] 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>
Found a missing unit test and also a bug in clash detection.. working on the follow-up now.
Created attachment 173172 [details] [review] Bug 38175: Improve Bookings feature with status With the integration of the new status column for bookings, it would be preferable to keep the booking in database and simply change its status to 'cancel' when you cancel it via the action button or the timeline. So I've added partial updating via the API with a new PATCH method to partially edit a booking. It is currently active for status changes. Graphically, this translates into the disappearance of the action buttons if the booking has already been canceled, although it remains visible in the table and timeline (the style is a proposal for the moment, intended simply to graphically differentiate a canceled booking from others). I've also added a filter to the filter_by_active method to exclude cancelled bookings. 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 Sponsored by: BibLibre Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173173 [details] [review] Bug 38175: Add tests Sponsored by: BibLibre Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173174 [details] [review] Bug 38175: (follow-up) Refactor staff interface additions to vanilla JS; adjust modules, controllers for coding guidelines and consistency 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 Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173175 [details] [review] Bug 38175: (QA follow-up) Remove superflous method If all we're doing is calling SUPER, then we don't need to wrap ;P Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173176 [details] [review] 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>
Created attachment 173177 [details] [review] Bug 38175: (QA follow-up) Ensure we handle cancelled in clash detection We need to ensure we handle the new status tracking cancelled/completed in our clash detection code too. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173178 [details] [review] Bug 38175: (QA follow-up) Unit test for filter_by_active change We now filter out 'cancelled' and 'completed' bookings in the filter_by_active method. We need tests for that. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173201 [details] [review] Bug 38175: Unit tests for assign_item_for_booking This patch adds more comprehensive tests to the item autoassignment for 'Any item' handling in bookings. We check for both random assignment, which was missing before now but always intended, and that we take cancelled status into account. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173202 [details] [review] Bug 38175: Handle satus in item auto-assignment for bookings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173203 [details] [review] Bug 38175: Handle status in item auto-assignment for bookings Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
*** Bug 36248 has been marked as a duplicate of this bug. ***
Pushed for 24.11! Well done everyone, thank you!
Created attachment 173346 [details] [review] Bug 38175: (QA follow-up) Add cancelled handling to find_booking We need to also exclude cancelled bookings from the find_booking routine used at checkout. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173915 [details] [review] Bug 38175: (follow-up) Quiet warnings in tests We weren't passing the required data into the FKConstraint exception, this patch passes biblio_id and value as required.
Please re-set to PQA when you have a follow-up - it helps so I don't miss things.
(In reply to Katrin Fischer from comment #32) > Please re-set to PQA when you have a follow-up - it helps so I don't miss > things. Looks like the follow-ups were already pushed including "Quiet warnings in tests".
Enhancement will not be included in 24.05.x