keystonejs / keystone-classic

Node.js CMS and web app framework
http://v4.keystonejs.com
MIT License
14.64k stars 2.21k forks source link

Datetime field type adds 5 hours to data #4082

Open christroutner opened 7 years ago

christroutner commented 7 years ago

Expected behavior

When converting a JavaScript Datetime object to an ISOString and then uploading to a KeystoneJS app, I expect the get the same time string back from the server.

Actual behavior

The Datetime string returned by the server has 5 hours appended to the value.

Steps to reproduce the behavior

  1. I create a model with a Datetime field like so: reportGenerated: { type: Types.Datetime, default: Date.now }

  2. Below is an example of an object with JavaScript Date objects:

Image 1

  1. I use an API, similar to the API Gist by Jed or the File and Image API Guide. That API will reject JavaScript Date objects, but will accept an ISO string. Here is an example of the same object with it's Date objects converted to ISO strings using Date.toISOString() in preparation of uploading to the KeystoneJS server:

Image 2

  1. Send data to the server succeeds. However, when retrieving the same data from the server, the ISO strings have 5 hours added to them:

Image 3

tomhardman0 commented 7 years ago

It looks like it's normalising your original GMT-0800 to UTC, so the ISO string you end up with looks like it's 8 hours ahead when really it's just UTC.

intmainvoid commented 7 years ago

@christroutner - Was/Is your client in a separate timezone to your server when you tried this?

What happens if you turn the utc option on on your reportGenerated field? (ie. change

reportGenerated: { type: Types.Datetime, default: Date.now }

to

reportGenerated: { type: Types.Datetime, default: Date.now, utc: true }

I've been working on a fix to a couple of related problems (See PR: https://github.com/keystonejs/keystone/issues/4112). Maybe it will fix your issue also?

jamiebuilds commented 7 years ago

Does #4112 fix this issue?

creynders commented 7 years ago

@thejameskyle I don't think so, since it only fixes the Date field, not DateTime fields. TBH I think these kinds of bugs keep on popping up until the fields are re-written properly from scratch.

creynders commented 7 years ago

I've been struggling with this bug in v0.3.x for a day now, and can't seem to get it fixed. The problem occurs when the server's in a different time zone than the client. What it boils down to is that the code does not take into account that the time coming from the client might be a different TZ than that one on the server and only deals with the server TZ. In other words the time coming from the client is being treated as having no TZ information, and the server TZ will be used. However, when the time is re-rendered in the client, moment automatically detects the client TZ and will add/remove the missing hours.

For example: server is GMT+2 and client is GMT+5, it will treat the time coming from the client as GMT+2 when writing it to the database, but when rendering it will identify the time from the database as GMT+2 and add another 3 since the client is GMT+5. This way the time jumps 3 hours ahead each time you save the document.

AFAICT this is a problem in all of the date fields (Date, Datetime and DateArray) in both v4 and v0.3.x. Unfortunately the fix in #4112 isn't really a fix, it's a hack that normalises the date if it deviates from 0:00, with all due respect.

What would need to happen is that the time zone information from the client is taken into account. Ideally all of the code repetition in all of the date fields is extracted to a separate helper, to avoid the situations where date gets fixed, but datetime not etc.

christroutner commented 7 years ago

Sorry for taking so long to respond. Just saw the activity in this thread.

In the example I showed above, my understanding is that the server is in the same timezone as myself. I'm based in Seattle (Washington) and the server is in San Francisco (California). It's a Digital Ocean Droplet. Everything should be running the same time zone.

At any rate, I don't think timezone should matter, as everything is referencing UTC time.

In a previous software company that I worked at, all time was referenced to UTC specifically to avoid bugs like this. The time was only converted to local timezone at the very last stage, in the client UI. Everything used in APIs and databases used UTC. I believe this is best practice.

creynders commented 7 years ago

@christroutner in the v0.3.x branch there is a bug with how the datetimes are translated from front to back, even when using utc: true. I don't know if it's the same bug in v4

alexwilczewski commented 5 years ago

I have the same issue from 0.3.x I'll share my solution. I apologize, my markdown is limited and the solution is not great; it's just to get me by. Yes, I modified the files directly in node_modules. I hope this helps someone.

Emulating @creynders previous post, the frontend is not converting the date/time to the server time prior to saving.

The current implementation has 2 fields to modify a DateTime -- they are the date input and the time input. These have the name "_date" and "_time". When the form is submitted, these values are sent with the POST, converted to server time, then saved.

My solution is to convert to server time on the frontend and pass the date/time back to the server. I created 4 fields: the same 2 previous fields with different names to allow user edits, and 2 hidden fields with the original names for converted values. When the user edits either field, then the hidden fields are updated to the respective server time.

Here are the code snippets: fields/types/datetime/DatetimeFields.js

getInitialState: function() {
    var dateValue = '';
    var timeValue = '';
    var serverDateValue = '';
    var serverTimeValue = '';
    if(this.props.value) {
        dateValue = this.moment(this.props.value).format(this.dateInputFormat);
        timeValue = this.moment(this.props.value).format(this.timeInputFormat);
        var serverDateTime = this.getServerDateTimeFromLocalDateTime(dateValue, timeValue);
        serverDateValue = serverDateTime.date;
        serverTimeValue = serverDateTime.time;
    }
    return {
        dateValue: dateValue,
        timeValue: timeValue,
        serverDateValue: serverDateValue,
        serverTimeValue: serverTimeValue
    };
},
...
handleChange: function(dateValue, timeValue) {
    this.updateServerDateTime(dateValue, timeValue);
    var value = dateValue + ' ' + timeValue;
    var datetimeFormat = this.dateInputFormat + ' ' + this.timeInputFormat;
    this.props.onChange({
        path: this.props.path,
        value: this.isValid(value) ? moment(value, datetimeFormat).toISOString() : null
    });
},

updateServerDateTime: function (dateValue, timeValue) {
    var serverDateTime = this.getServerDateTimeFromLocalDateTime(dateValue, timeValue);
    this.setState({
        serverDateValue: serverDateTime.date,
        serverTimeValue: serverDateTime.time
    });
},

getServerDateTimeFromLocalDateTime: function (dateValue, timeValue) {
    var value = dateValue + ' ' + timeValue;
    var datetimeFormat = this.dateInputFormat + ' ' + this.timeInputFormat;
-->    var momentDate = moment(value, datetimeFormat).utc();
    return {
        date: momentDate.format("YYYY-MM-DD"),
        time: momentDate.format("hh:mm:ss a")
    };
},

I marked above the line where server time conversion is done. Update this to your server time. Mine is utc, so I used the utc() flag moment offers.

alexwilczewski commented 5 years ago

Continuing off previously, I found that my server was not utc. I had to use utcOffset(...). However, I ran into further problems. If the datetime was in DST I had to use a different utcOffset(...).

var momentDate = moment(value, datetimeFormat);
var offsetValue;
if(momentDate.isDST()) {
    offsetValue = -240;
} else {
    offsetValue = -300;
}
momentDate.utcOffset(offsetValue);

What I would do to make this robust is to let the server pass up it's offset for a DST datetime and it's offset for a non-DST datetime.