Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prefer DATETIME to DATE in the MySQL Database #45

Closed
jniles opened this issue Jan 27, 2016 · 3 comments
Closed

Prefer DATETIME to DATE in the MySQL Database #45

jniles opened this issue Jan 27, 2016 · 3 comments

Comments

@jniles
Copy link
Contributor

jniles commented Jan 27, 2016

As part of the bhima 2.X release cycle, I propose we migrate as many tables as possible to using DATETIME column types instead of DATE for storing date datapoints. Under this direction, we will design our table columns following these guidelines:

  • DATE should never be used. It does not store any time information, and might result in incorrect dates conversions between JS and SQL.
  • DATETIME should be used for every column that needs to store a JS Date() object. The SQL datatype does not convert records from their original timezone to UTC (like a TIMESTAMP does), preferring to store them in the time zone of the server.
  • TIMESTAMP should be used for every column that needs to know the create/update time of the record. This should be seen as information about the MySQL row, not the entity the row represents. TIMESTAMPs may help with data migrations and analytical tools later on.

It is worth reading the page on MySQL dates [1]. Also useful is this [2] Stack Overflow discussion (see top two answers).

[1] http://dev.mysql.com/doc/refman/5.7/en/datetime.html.
[2] http://stackoverflow.com/questions/409286/should-i-use-field-datetime-or-timestamp.
@jniles jniles added the 2.X label Jan 27, 2016
@sfount
Copy link
Contributor

sfount commented Jan 27, 2016

Agreed 👍.

@DedrickEnc @jniles do you think it's worthwhile budgeting ~half a day for
someone to specifically write/ perform tests (E2E/Integration/Unit) on
DATETIME objects vs. DATES and showing that we can have some guarantees
about dates never being misinterpreted between the client/ server/
database? This would mean that 2.X can have this as a guarantee. Unless
this has already been shown within BHIMA.

Excellent suggestion backed with references.

On Wednesday, January 27, 2016, Jonathan Niles notifications@github.com
wrote:

As part of the bhima 2.X release cycle, I propose we migrate as many
tables as possible to using DATETIME column types instead of DATE for
storing date datapoints. Under this direction, we will design our table
columns following these guidelines:

  • DATE should never be used. It does not store any time information,
    and might result in incorrect dates conversions between JS and SQL.
  • DATETIME should be used for every column that needs to store a JS
    Date() object. The SQL datatype does not convert records from their
    original timezone to UTC (like a TIMESTAMP does), preferring to store them
    in the time zone of the server.
  • TIMESTAMP should be used for every column that needs to know the
    create/update time of the record. This should be seen as information about
    the MySQL row, not the entity the row represents. TIMESTAMPs may help with
    data migrations and analytical tools later on.

It is worth reading the page on MySQL dates [1]. Also useful is this [2]
Stack Overflow discussion (see top two answers).

[1] http://dev.mysql.com/doc/refman/5.7/en/datetime.html.
[2]
http://stackoverflow.com/questions/409286/should-i-use-field-datetime-or-timestamp
.

—
Reply to this email directly or view it on GitHub
#45.

@jniles
Copy link
Contributor Author

jniles commented Jan 27, 2016

We definitely need integration tests to support this proposition. I'm working on a few, updating @mbayopanda's employee API tests (which register a date of birth), as part of #24. That is probably not enough though.

Another interesting experiment would be to see how large DATEs are compared to DATETIMEs. While I would still recommend using DATETIMEs over DATEs, it would be valuable information to have.

@sfount sfount added this to the 2.X milestone Jan 27, 2016
jniles referenced this issue in jniles/bhima Jan 27, 2016
@jniles
TEST(db): all integration tests pass STRICT MODE
ac88bdc 
This commit ensures that all integration tests pass during SQL strict
mode.  In particular, the following changes have been implemented:

 1. Travis calls SET GLOBAL sql_mode = 'STRICT_ALL_TABLES' to ensure
 that it is running in SQL strict mode.

 2. The following tables now use DATETIME instead of DATE (see #45) to
 store date entities:
   a. `patient` (dob, registration_date)
   b. `employee` (dob, date_embauche)
   c. `exchange_rate` (date)

 3. A dependency on `chai-datetime` has been added to ensure dates are
 properly tested during integration tests.  An example of this can be
 found in the Employees API tests.

 4. Many controller refactors to pre-process dates or fix bugs revealed
 by failing tests.

 5. Error handling has been moved into the `interceptors` file.  Errors
 in the application are now functions, to be called as `new
 req.codes.ERROR()`.  The syntax is liable to change in a future commit.

 6. A series of database errors have been added to ensure that all
 errors are informative and handled uniformly.

 7. Removed unused `errors.js` controller file.

 8. Misc typos and style fixes.
jniles referenced this issue in jniles/bhima Jan 27, 2016
@jniles
TEST(db): all integration tests pass STRICT MODE
7c88917 
This commit ensures that all integration tests pass during SQL strict
mode.  In particular, the following changes have been implemented:

 1. Travis calls SET GLOBAL sql_mode = 'STRICT_ALL_TABLES' to ensure
 that it is running in SQL strict mode.

 2. The following tables now use DATETIME instead of DATE (see #45) to
 store date entities:
   a. `patient` (dob, registration_date)
   b. `employee` (dob, date_embauche)
   c. `exchange_rate` (date)

 3. A dependency on `chai-datetime` has been added to ensure dates are
 properly tested during integration tests.  An example of this can be
 found in the Employees API tests.

 4. Many controller refactors to pre-process dates or fix bugs revealed
 by failing tests.

 5. Error handling has been moved into the `interceptors` file.  Errors
 in the application are now functions, to be called as `new
 req.codes.ERROR()`.  The syntax is liable to change in a future commit.

 6. A series of database errors have been added to ensure that all
 errors are informative and handled uniformly.

 7. Removed unused `errors.js` controller file.

 8. Misc typos and style fixes.
@sfount sfount removed this from the 2.X milestone Mar 23, 2016
@jniles
Copy link
Contributor Author

jniles commented Mar 29, 2016

Most, if not all, of our dates have been migrated. Closing this.

@jniles jniles closed this as completed Mar 29, 2016
jniles referenced this issue in jniles/bhima Jan 4, 2017
@jniles @sfount
feature(locations): add register again checkbox (#45)
45cad8c 
* feature(locations): add register again checkbox

This commit adds a "Register another Location" checkbox to the bottom of
the location modal.  This allows a user to register multiple locations
with the modal before closing it.

Closes #38.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jniles @sfount @DedrickEnc