Bug 37592 - Add a record of creation and modification to bookings
Summary: Add a record of creation and modification to bookings
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Paul Derscheid
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 29002
Blocks: 37601
  Show dependency treegraph
 
Reported: 2024-08-07 13:49 UTC by Paul Derscheid
Modified: 2024-12-16 23:18 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds `created_at` and `updated_at` fields to the bookings table, providing institutions with the ability to track the creation and modification timestamps of bookings. **Sponsored by** *Büchereizentrale Schleswig-Holstein*
Version(s) released in:
24.11.00
Circulation function: bookings


Attachments
Bug 37592: Add created_at, updated_at fields to bookings table (4.45 KB, patch)
2024-08-07 15:58 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37592: Add created_at, updated_at fields to bookings table (4.50 KB, patch)
2024-08-08 14:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 37592: (follow-up) Change created_at, updated_at to created_on, updated_on (5.96 KB, patch)
2024-08-27 11:13 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37592: (follow-up) Change created_at, updated_at to created_on, updated_on (6.01 KB, patch)
2024-09-10 15:31 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 37592: Add created_at, updated_at fields to bookings table (4.56 KB, patch)
2024-09-12 10:52 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37592: (follow-up) Change created_at, updated_at to created_on, updated_on (6.08 KB, patch)
2024-09-12 10:52 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37592: (QA follow-up) Add API mapping and definition (1.77 KB, patch)
2024-09-12 10:52 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37592: (QA follow-up) Change created_on, updated_on to creation_date, modification_date (5.39 KB, patch)
2024-09-16 10:11 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37592: (QA follow-up) Fetch database fields for api return (2.09 KB, patch)
2024-09-18 04:01 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37592: (QA follow-up) Update db_rev to align DBIx::Class with database schema for creation_date, modification_date (2.33 KB, patch)
2024-11-25 11:32 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37592: (QA follow-up) Ensure consistent NOT NULL placement and make implicit behavior explicit (2.99 KB, patch)
2024-11-25 12:01 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37592: (Follow-up) Fix timestamp vs datatime in db_rev (1.54 KB, patch)
2024-11-25 13:09 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37592: (Follow-up) Fix timestamp vs datetime in db_rev (1.54 KB, patch)
2024-11-25 13:10 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37592: (follow-up) Fix timestamp vs datetime in db_rev (1.61 KB, patch)
2024-11-25 13:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37592: (follow-up) Fix comments in db_rev (1.61 KB, patch)
2024-11-25 13:23 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Derscheid 2024-08-07 13:49:31 UTC

    
Comment 1 Paul Derscheid 2024-08-07 15:58:02 UTC
Created attachment 170144 [details] [review]
Bug 37592: Add created_at, updated_at fields to bookings table
Comment 2 Paul Derscheid 2024-08-07 16:03:39 UTC
It would be helpful for some institutions to track when bookings were created and updated, for example when communicating with patrons.
Comment 3 Owen Leonard 2024-08-08 14:19:31 UTC
Created attachment 170161 [details] [review]
Bug 37592: Add created_at, updated_at fields to bookings table

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Martin Renvoize (ashimema) 2024-08-15 15:19:29 UTC
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.
Comment 5 Paul Derscheid 2024-08-15 15:20:15 UTC
Good call!
Comment 6 Paul Derscheid 2024-08-27 11:13:45 UTC
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.
Comment 7 PTFS Europe Sandboxes 2024-09-10 15:31:28 UTC
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>
Comment 8 Martin Renvoize (ashimema) 2024-09-12 10:52:07 UTC
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>
Comment 9 Martin Renvoize (ashimema) 2024-09-12 10:52:10 UTC
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>
Comment 10 Martin Renvoize (ashimema) 2024-09-12 10:52:13 UTC
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>
Comment 11 Katrin Fischer 2024-09-16 08:47:12 UTC
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?
Comment 12 Paul Derscheid 2024-09-16 08:52:18 UTC
I'll gladly refactor it, if it's pushed immediately afterwards as these are literally just name changes.
Comment 13 Katrin Fischer 2024-09-16 08:53:31 UTC
(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 :)
Comment 14 Paul Derscheid 2024-09-16 10:11:58 UTC
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.
Comment 15 Katrin Fischer 2024-09-16 11:49:04 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 16 Martin Renvoize (ashimema) 2024-09-18 04:01:24 UTC
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>
Comment 17 Martin Renvoize (ashimema) 2024-09-18 04:02:33 UTC
Follow-up pushed as RM Assistant; This gets the tests passing again and fixes a flaw in our logic.
Comment 18 Paul Derscheid 2024-09-18 07:36:35 UTC
Thanks for the assist Martin!
Comment 19 Katrin Fischer 2024-11-22 17:24:14 UTC
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(
 );
Comment 20 Jonathan Druart 2024-11-25 10:22:25 UTC
Please also adjust the COMMENTs
Comment 21 Paul Derscheid 2024-11-25 11:32:16 UTC
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.
Comment 22 Katrin Fischer 2024-11-25 12:01:01 UTC
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',
Comment 23 Paul Derscheid 2024-11-25 12:01:18 UTC
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.
Comment 24 Jonathan Druart 2024-11-25 12:52:13 UTC
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
Comment 25 Martin Renvoize (ashimema) 2024-11-25 13:09:43 UTC
Created attachment 174970 [details] [review]
Bug 37592: (Follow-up) Fix timestamp vs datatime in db_rev
Comment 26 Martin Renvoize (ashimema) 2024-11-25 13:10:05 UTC
Created attachment 174971 [details] [review]
Bug 37592: (Follow-up) Fix timestamp vs datetime in db_rev
Comment 27 Jonathan Druart 2024-11-25 13:23:38 UTC
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>
Comment 28 Jonathan Druart 2024-11-25 13:23:41 UTC
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>
Comment 29 Katrin Fischer 2024-11-25 13:27:21 UTC
Picked the last 2 follow-up patches "Fix timestamp vs. datetime.." and "Fix comments..." for main.
Comment 30 Wainui Witika-Park 2024-12-16 02:50:30 UTC
(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?
Comment 31 Katrin Fischer 2024-12-16 13:48:31 UTC
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.
Comment 32 Wainui Witika-Park 2024-12-16 23:18:59 UTC
(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 :)