The goal is to add a status column to the bookings table to track a bookings state Two existing ideas are: - confirmed (the default state for a freshly created booking, open for suggestions on a better name for this one) - cancelled For a follow-up bug: - active (booking is currently in the boundary of start and end date) - expired (bookings end date is in the past) - pending (bookings start date is in the future) should be calculated at runtime and zipped into the api response so that the datatable ordering and searching still works as expected. Additional opinions would be appreciated!
Created attachment 170260 [details] [review] Bug 37601: Add status field to bookings table
This is a great start, I can see it being really useful in the future of bookings. For example, I can see there being a process for opac bookings that would allow for booking request followed by confirmation by a librarian (certainly not everyone's workflow need, but some might want that.. this gives us the option to use status's to denote that flow) I like 'cancelled' to allow for historic booking storage.. right now we just delete.. we can certainly introduce that state at a later if we opt for a workflow that cares about cancelled booking data staying around. I can see there also being 'collected'/'onloan' for a booking having been picked up.. right now we just leave the booking hanging around once it's collected but there's also a bug that converts the booking into a checkout by deleting the row.. we might want to change that to a status. Anyway.. I really like the direction of travel here.
Created attachment 170767 [details] [review] Bug 37601: (follow-up) Change updated_at to updated_on in accordance with Bug 37592
Created attachment 171241 [details] [review] Bug 37601: (follow-up) Change updated_at to updated_on in accordance with Bug 37592 Signed-off-by: LEBSimonsen <simonsen@bz-sh.de>
Created attachment 171380 [details] [review] Bug 37601: Add status field to bookings table Signed-off-by: LEBSimonsen <simonsen@bz-sh.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171381 [details] [review] Bug 37601: (follow-up) Change updated_at to updated_on in accordance with Bug 37592 Signed-off-by: LEBSimonsen <simonsen@bz-sh.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171382 [details] [review] Bug 37601: (QA follow-up) Add status to api definition Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
1) I wonder about only having 2 status: * created * cancelled Cancelled makes perfect sense to me. Created seems a bit unusual compared to other status. Maybe it would be a little nicer as 'new' to match a bit closer how we handle article requests? (suggestions use "ASEKD", which also seems not ideal) 2) You say below that at the moment we just delete the filled bookings. Is there a plan to keep them later on? I wonder about statistics about successfully filled booking requests especially, which could be interesting to libraries. (question, not blocker)
I gladly use 'new' if that's more in line with the conventions. The plan for me is just to add stati on demand, meaning another bug needs an additional status, we push it onto the enum. Concerning your second question: We can of course also add a 'deleted' status to the enum, then we don't need another column.
On the other hand, maybe it shouldn't be a status. If all now to be combined tables have a column for that, it's better to follow the convention then.
I think the status would probably be "complete" or similar. And the clean-up would be done separately, if we went down the route of keeping them around at first.
Then we can add 'completed' and introduce the clean-up in a follow-up bug.
Talked to Martin and he is OK with switching to "new" - after reviewing the code base it's a bit of the usual mess...
Created attachment 171526 [details] [review] Bug 37601: (QA follow-up) Change updated_on to modification_date in accordance with Bug 37592
Created attachment 171527 [details] [review] Bug 37601: (QA follow-up) Rename status 'created' as 'new' to be more consistent with other modules
Created attachment 171528 [details] [review] Bug 37601: (QA follow-up) Add status 'completed' This will probably be useful when transforming a booking into a checkout.
Pushed for 24.11! Well done everyone, thank you!
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'AFTER `end_date`' at line 2 at /kohadevbox/koha/C4/Installer.pm line 742
This one is on me, fix tested and pushed. Sorry for breaking your patch, Paul!
No worries.