Bug 31150 - _on or _date for DB date columns?
Summary: _on or _date for DB date columns?
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-13 07:25 UTC by Jonathan Druart
Modified: 2022-10-14 07:12 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-07-13 07:25:19 UTC
We need to decide a naming for the dates column.
We have _on and _date:

% git grep -c '_date`' installer/data/mysql/kohastructure.sql
installer/data/mysql/kohastructure.sql:14

% git grep -c '_on`' installer/data/mysql/kohastructure.sql 
installer/data/mysql/kohastructure.sql:31

I've tried to find previous discussion but haven't found much.
Comment 1 Jonathan Druart 2022-07-13 07:27:14 UTC
IRC log, 20200723

<ashimema> remind me of our policy on db column name consistency
<Joubu> There_isNoCon_sistency!
<caroline> lol!
<caroline> maybe this? https://wiki.koha-community.org/wiki/DBIC_naming_conventions
<Joubu> for a date, should end with _on 
<Joubu> or _date, apparently
<caroline> if above link is not good, let me know and I'll add a warning saying this is not accurate
<caroline> I don't know for you, but I feel like no column should ever be called just "id", but somethingid, so that it's the same in all tables
<ashimema> I wrote that guideline I think.
<ashimema> oh.. maybe not.. maybe I wrote the corresponding api one 
<ashimema> I prefer _on.. but don't really like inconsistency within a table
<ashimema> but I also don't want to update other fields in the table.
Comment 2 Martin Renvoize 2022-07-13 07:34:03 UTC
Interesting.. I think we've since settled on _date to stay in line with the also recent discussions around api field names as described here: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.3.4:_mapping

We should update the db fields page you linked to link between the two definitions and make sure they're consistent.
Comment 3 Jonathan Druart 2022-07-13 07:35:11 UTC
At 20200129 dev meeting we discussed about _date for the REST API (does it cover timestamp and datetime as well, etc.)

https://meetings.koha-community.org/2020/development_irc_meeting_29_january_2020.2020-01-29-14.00.log.txt
Comment 4 Martin Renvoize 2022-07-13 07:36:45 UTC
As for existing inconsistencies.. I'd love to fix those but I always get met with the "This will break reports" argument so in general I've ended up leaving the DB inconsistent and kept the old field names.

I did have some partners complaining only a day ago that it's really annoying that our API field names dont match our database field names.. but I do think the API is much more consistent and better named in general...
Comment 5 Jonathan Druart 2022-07-13 07:37:03 UTC
(In reply to Martin Renvoize from comment #2)
> Interesting.. I think we've since settled on _date to stay in line with the
> also recent discussions around api field names as described here:
> https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.3.4:
> _mapping
> 
> We should update the db fields page you linked to link between the two
> definitions and make sure they're consistent.

We never decided either. Hence this bug report.
See previous comment. If we move to "*_date", does it include datetime and timestamp, etc. It's not that trivial. And in my mind we decided on "_on", or at least I picked that because it had the more occurrences in the codebase.

There is no record of a community decision (at least I didn't find one).
Comment 6 Martin Renvoize 2022-07-13 08:59:23 UTC
Hmm, we dropped the 'DRAFT' part from the API Guidelines wiki page in the evening of 5th Februrary 2020, and I presume I only did that after we voted and agreed to it in a meeting.. I'll go hunting.

https://wiki.koha-community.org/w/index.php?title=Coding_Guidelines_-_API&action=historysubmit&diff=24914&oldid=24910
Comment 8 Jonathan Druart 2022-07-13 10:10:06 UTC
Yes, that's for the API, not the DB :-)
Comment 9 Fridolin Somers 2022-09-28 22:18:04 UTC
I'd say the "xxx_date" may be confusing because it is a data format.
Using "xxx_on" is more specific to say "xxx field as been modified on this date".
Plus have "created_on", "modified_on", "deleted_on".
SQL Comments can always explain more.

We will have to discuss some existing occurrences like :
reserves.cancellationdate
branchtransfers.date*

"_on" +1
Comment 10 Martin Renvoize 2022-09-29 08:05:18 UTC
DOI we really want to have contradictory guidelines for this.. it's already annoying having API fields named differently in the database.. here your basically suggesting enforcing that.. DB fields must not match API field.. that really doesn't make sense to me.  I honestly don't remember the discussion around _on vas _date.. but I do remember we all agreed on one and put it in a guideline to prevent a debate every time we came to adding another API..
Comment 11 Fridolin Somers 2022-10-01 05:03:29 UTC
Ahhh sorry I had not understood there is an existing guide line :

https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.3.4.1_date.2Fdatetime.2Ftimestamp_fields

I'm OK to stick to it and apply the same on DB fields.

Maybe keep the items.xxx_on as a legacy ;)
Comment 12 Jonathan Druart 2022-10-14 07:12:10 UTC
(In reply to Martin Renvoize from comment #10)
> DOI we really want to have contradictory guidelines for this.. it's already
> annoying having API fields named differently in the database.. here your
> basically suggesting enforcing that.. DB fields must not match API field..
> that really doesn't make sense to me.  I honestly don't remember the
> discussion around _on vas _date.. but I do remember we all agreed on one and
> put it in a guideline to prevent a debate every time we came to adding
> another API..

We never agreed on a convention for DB column names, hence this bug report.
The guideline is for the REST API.

""" Where a field contains a 'date' it should be consistently named *_date as opposed to date_* and it should always return a full datetime. """

As stated before, the problem is that a _date suffix is confusing for other datatypes IMO.