Created attachment 170144 [details] [review] Bug 37592: Add created_at, updated_at fields to bookings table
It would be helpful for some institutions to track when bookings were created and updated, for example when communicating with patrons.
Created attachment 170161 [details] [review] Bug 37592: Add created_at, updated_at fields to bookings table Signed-off-by: Owen Leonard <oleonard@myacpl.org>
I like this.. but lets make sure we have a 100% consensus on the API guidelines and have it cloned to Database guidelines before we push it: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#REST1.3.4.1_date.2Fdatetime.2Ftimestamp_fields Currently the guidelines says: * Where a field contains a 'date' it should be consistently named thing_date as opposed to date_thing and it should always return a full datetime.
Good call!
Created attachment 170763 [details] [review] Bug 37592: (follow-up) Change created_at, updated_at to created_on, updated_on As per a discussion in the community chat, this change is more in line with the existing schema.
Created attachment 171254 [details] [review] Bug 37592: (follow-up) Change created_at, updated_at to created_on, updated_on As per a discussion in the community chat, this change is more in line with the existing schema. Signed-off-by: LEBSimonsen <simonsen@bz-sh.de>
Created attachment 171376 [details] [review] Bug 37592: Add created_at, updated_at fields to bookings table Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171377 [details] [review] Bug 37592: (follow-up) Change created_at, updated_at to created_on, updated_on As per a discussion in the community chat, this change is more in line with the existing schema. Signed-off-by: LEBSimonsen <simonsen@bz-sh.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 171378 [details] [review] Bug 37592: (QA follow-up) Add API mapping and definition Sticking to API guidelines, this adds the creation_date and modification_date fields to the api definitions and the required to_api_mappings for those fields to be properly populated. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I don't want to block this, but forgive me this question: Independent of the general discussion on naming things, should we not try to avoid API mappings for new columns? I think if we use creation_date in the API we might also name the column that. What do you think? sub to_api_mapping { - return {}; + return { + created_on => "creation_date", + updated_on => "modification_date" + }; } Also a question for the release notes :) Signed-off-by: LEBSimonsen <simonsen@bz-sh.de> This will show as LEBSimonsen in the release notes - would you like to update to the name or another term?
I'll gladly refactor it, if it's pushed immediately afterwards as these are literally just name changes.
(In reply to Paul Derscheid from comment #12) > I'll gladly refactor it, if it's pushed immediately afterwards as these are > literally just name changes. Once the patch has reached my queue and it comes back again, it automatically goes on top of the enhancements pile. Check the dashboard :)
Created attachment 171524 [details] [review] Bug 37592: (QA follow-up) Change created_on, updated_on to creation_date, modification_date It makes sense not to introduce mapping code if there's no reason for it. Accordingly the the columns are now of type DATETIME instead.
Pushed for 24.11! Well done everyone, thank you!
Created attachment 171645 [details] [review] Bug 37592: (QA follow-up) Fetch database fields for api return Creation and Modification times are maintained by the database, but on add/update we were not fetching the updated fields from the database for the api response. This patch corrects that and also updates the api schema to reflect that these are readOnly fields. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Follow-up pushed as RM Assistant; This gets the tests passing again and fixes a flaw in our logic.
Thanks for the assist Martin!
I am currently in the process of making sure that an updated installation will have the same database structure as a new installation. Here something seems to have gone wrong: +++ b/Koha/Schema/Result/Booking.pm @@ -87,7 +87,7 @@ the end date of the booking data_type: 'timestamp' datetime_undef_if_invalid: 1 default_value: current_timestamp - is_nullable: 0 + is_nullable: 1 the timestamp for when a booking was created @@ -96,7 +96,7 @@ the timestamp for when a booking was created data_type: 'timestamp' datetime_undef_if_invalid: 1 default_value: current_timestamp - is_nullable: 0 + is_nullable: 1 the timestamp for when a booking has been updated @@ -157,14 +157,14 @@ __PACKAGE__->add_columns( data_type => "timestamp", datetime_undef_if_invalid => 1, default_value => \"current_timestamp", - is_nullable => 0, + is_nullable => 1, }, "modification_date", { data_type => "timestamp", datetime_undef_if_invalid => 1, default_value => \"current_timestamp", - is_nullable => 0, + is_nullable => 1, }, "status", { @@ -257,8 +257,8 @@ __PACKAGE__->belongs_to( );
Please also adjust the COMMENTs
Created attachment 174968 [details] [review] Bug 37592: (QA follow-up) Update db_rev to align DBIx::Class with database schema for creation_date, modification_date - DBIx::Class appears to determine column nullability based on database metadata. When columns are added via `ALTER TABLE` without explicitly specifying `NOT NULL`, the metadata may indicate `IS_NULLABLE = "YES"`, causing DBIx::Class to generate `is_nullable => 1` in the schema files. This behavior might not account for the implicit `NOT NULL` enforcement of `DEFAULT CURRENT_TIMESTAMP`. - Adding `NOT NULL` explicitly in the `ALTER TABLE` statements ensures the database metadata reflects the intended constraints, potentially resolving this issue. - Additionally, comments in the atomic update and `kohastructure.sql` are aligned for consistency and clarity.
I believe we need to update kohastructure.sql as well: diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index f3dd950328d..e829cacc62b 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1217,6 +1217,8 @@ CREATE TABLE `bookings` ( `pickup_library_id` varchar(10) NOT NULL COMMENT 'Identifier for booking pickup library', `start_date` datetime DEFAULT NULL COMMENT 'the start date of the booking', `end_date` datetime DEFAULT NULL COMMENT 'the end date of the booking', + `created_at` timestamp DEFAULT current_timestamp() COMMENT 'the timestamp for when a booking was created', + `updated_at` timestamp DEFAULT current_timestamp() ON UPDATE current_timestamp() COMMENT 'the timestamp for when a booking has been updated',
Created attachment 174969 [details] [review] Bug 37592: (QA follow-up) Ensure consistent NOT NULL placement and make implicit behavior explicit - Explicitly set `NOT NULL` constraints for `creation_date` and `modification_date` in `kohastructure.sql` to clarify the implicit behavior of `DEFAULT CURRENT_TIMESTAMP`. - Adjusted the `modification_date` column definition in the atomic update file to place `NOT NULL` before `ON UPDATE` for consistency.
playing the following commands: git checkout v24.05.00 reset_all git checkout main updatedatabase perl misc/devel/update_dbix_class_files.pl --koha-conf $KOHA_CONF git commit -a -m"test diff" dbic git diff "test diff" commit contains a diff that should not be there (datetime vs timestamp): From f75fa56e49d62b7444926d06d351a74d33810bc9 Mon Sep 17 00:00:00 2001 From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Date: Mon, 25 Nov 2024 13:49:37 +0100 Subject: [PATCH 1/1] test diff --- Koha/Schema/Result/Booking.pm | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/Koha/Schema/Result/Booking.pm b/Koha/Schema/Result/Booking.pm index 2231dbf19a3..6f8ec07b818 100644 --- a/Koha/Schema/Result/Booking.pm +++ b/Koha/Schema/Result/Booking.pm @@ -84,21 +84,21 @@ the end date of the booking =head2 creation_date - data_type: 'timestamp' + data_type: 'datetime' datetime_undef_if_invalid: 1 - default_value: current_timestamp + default_value: 'current_timestamp()' is_nullable: 0 -the timestamp for when a booking was created +the datetime for when a bookings was created =head2 modification_date - data_type: 'timestamp' + data_type: 'datetime' datetime_undef_if_invalid: 1 - default_value: current_timestamp + default_value: 'current_timestamp()' is_nullable: 0 -the timestamp for when a booking has been updated +the datetime for when a booking has been updated =head2 status @@ -154,16 +154,16 @@ __PACKAGE__->add_columns( }, "creation_date", { - data_type => "timestamp", + data_type => "datetime", datetime_undef_if_invalid => 1, - default_value => \"current_timestamp", + default_value => "current_timestamp()", is_nullable => 0, }, "modification_date", { - data_type => "timestamp", + data_type => "datetime", datetime_undef_if_invalid => 1, - default_value => \"current_timestamp", + default_value => "current_timestamp()", is_nullable => 0, }, "status", @@ -257,8 +257,8 @@ __PACKAGE__->belongs_to( ); -# Created by DBIx::Class::Schema::Loader v0.07051 @ 2024-10-24 16:23:05 -# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:kTR2kwiwY2PnjU1E0P+CMQ +# Created by DBIx::Class::Schema::Loader v0.07051 @ 2024-11-25 12:47:59 +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:QU3grfGiob0SxIHPVbY6ZA # You can replace this text with custom code or comments, and it will be preserved on regeneration -- 2.34.1
Created attachment 174970 [details] [review] Bug 37592: (Follow-up) Fix timestamp vs datatime in db_rev
Created attachment 174971 [details] [review] Bug 37592: (Follow-up) Fix timestamp vs datetime in db_rev
Created attachment 174972 [details] [review] Bug 37592: (follow-up) Fix timestamp vs datetime in db_rev Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174973 [details] [review] Bug 37592: (follow-up) Fix comments in db_rev Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Picked the last 2 follow-up patches "Fix timestamp vs. datetime.." and "Fix comments..." for main.
(In reply to Katrin Fischer from comment #29) > Picked the last 2 follow-up patches "Fix timestamp vs. datetime.." and "Fix > comments..." for main. Should I do the same for 24.05?
Leaving final answer to Martin, but I think this feature is not in 24.05 yet and is part of a big tree, so I would not include in 24.05.
(In reply to Katrin Fischer from comment #31) > Leaving final answer to Martin, but I think this feature is not in 24.05 yet > and is part of a big tree, so I would not include in 24.05. Thanks Katrin! Not backporting to 24.05 unless requested, or advised by Martin :)