|
Description
Paul Derscheid
2024-08-08 15:45:56 UTC
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. Missing DBIC schema changes patch in main. (In reply to Jonathan Druart from comment #21) > Missing DBIC schema changes patch in main. Argh. I'll fix in a few min. (In reply to Katrin Fischer from comment #22) > (In reply to Jonathan Druart from comment #21) > > Missing DBIC schema changes patch in main. > > Argh. I'll fix in a few min. DBIC schema update pushed. Something is wrong:
dbrev:
+ ADD COLUMN `status` ENUM('new', 'cancelled', 'completed') NOT NULL DEFAULT 'new' COMMENT 'current status of the booking' $after;
kohastructure:
1222 `status` enum('created', 'cancelled') NOT NULL DEFAULT 'created' COMMENT 'current status of the booking',
Currently I have this in DB:
| status | enum('new','cancelled','completed') | NO | | new | |
And the tests are failing:
# Failed test 'list() tests'
# at t/db_dependent/api/v1/bookings.t line 145.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Data truncated for column 'status' at row 1 at /kohadevbox/koha/t/lib/TestBuilder.pm line 385
# Looks like your test exited with 2 just after 1.
Created attachment 171653 [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 171654 [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> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 171655 [details] [review] Bug 37601: (QA follow-up) Adjust kohastructure.sql (In reply to Paul Derscheid from comment #27) > Created attachment 171655 [details] [review] [review] > Bug 37601: (QA follow-up) Adjust kohastructure.sql Pushed to main. Confirmed that it fixes the failure. |