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.

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

Section "Binding of date and time values to DateTimePicker" contains misleading text #2543

Closed
boghyon opened this issue Jun 11, 2019 · 7 comments

Comments

@boghyon
Copy link
Contributor

boghyon commented Jun 11, 2019

URL (minimal example if possible)

https://openui5nightly.hana.ondemand.com/#/topic/e1ddc69c01474faf830a522db8c9238a

Steps to reproduce the problem

  1. Open the above topic page

  2. Scroll down to the section Binding of date and time values to DateTimePicker

  3. It says the following:

    Note that the UTC: true in the formatOptions is needed to convert the local date values chosen by the user to UTC ones before sending them to the backend.

This is slightly incorrect. The value chosen by the user is always converted to UTC before it's sent to the backend, no matter whether formatOptions: { UTC: true } is set or not.

Internally, datajs uses getTime() which is always in UTC.

var formatDateTimeOffsetJSON = function (value) {
return "\/Date("+value.getTime()+")\/";
};

getTime() always uses UTC for time representation. (From MDN)

What is the expected result?

  • Improved text

What happens instead?

  • The quoted text gives the impression that formatOptions: { UTC: true } is always required whenever 'sap.ui.model.odata.type.DateTime' is used, even though it depends on the use case/ user preference.
@stephania87 stephania87 self-assigned this Jun 21, 2019
@stephania87
Copy link

Forwarded to development via #1970273628
BR

@stopcoder
Copy link
Member

HI @boghyon,

the type sap.ui.model.odata.type.DateTime is designed for entity property with type Edm.DateTime. Edm.DateTime is a string representation of a date and it doesn't contain timezone information. Therefore it's recommended to always use UTC to eliminate ambiguity caused by time zone difference.

The mentioned code from datajs is used to convert a date object in the model before setting it to the backend. But if a property has Edm.DateTime type, this function isn't used.

Best regards,
Jiawei

@boghyon
Copy link
Contributor Author

boghyon commented Jun 26, 2019

Hi @stopcoder,

But if a property has Edm.DateTime type, this function isn't used.

Maybe I'm misunderstanding or overlooking something but that formatDateTimeOffsetJSON does get called with Edm.DateTime property as well.

Here is a minimal sample: https://jsbin.com/pulunib/edit?js,output

  1. Open this sample page. The service provides ReleaseDate property with Edm.DateTime type.
  2. In the datajs source within the devtool, set the breakpoint in formatDateTimeOffsetJSON (currently there is no datajs-dbg.js mapped but when pretty-printed, setting breakpoint is still possible. Just look for "\/Date(".. Or just bootstrap the demo with data-sap-ui-debug="true").
  3. Change the date-time value from the picker (on change, the sample invokes submitChanges()).

We can see that the debugger stops in that function, meaning the values are always sent as UTC (getTime()) no matter which settings were applied.

Could you verify if it's true?

As far as I understood, the UTC: true format option is for display only, not for converting it to UTC before it's sent.


it's recommended to always use UTC

In that case, users would have to add/subtract their local time offset every time they enter date-time values because the app tells them to do so. It's hard to believe that this the UX that UI5-apps should provide.

@stopcoder
Copy link
Member

Hi @boghyon,

thank you for making the example and I now fully understand what the issue is. My last comment is related to sap.ui.model.type.DateTime and it has nothing to do with sap.ui.model.odata.type.DateTime.

I always thought that once a property is set to type Edm.DateTime, the backend accepts the value for this property only when it follows the Edm.DateTime format like datetime'2000-12-12T12:00' but it's obviously not the case since the OData V2 demo service simply delivers the property with Edm.DateTime type a Date in JSON-Notation \/Date(694224000000)\/.

You are right that the documentation should be adapted. The UTC: true isn't needed for defining the formatOptions for sap.ui.model.odata.type.DateTime because UI5 passes the Javascript Date instance to datajs which converts the Javascript Date instance to the JSON-Notation by using Date.prototype.getTime which already uses UTC.

I will forward the internal incident to the responsible colleague.

Best regards,
Jiawei

@boghyon
Copy link
Contributor Author

boghyon commented Jul 17, 2019

The Note below the DateTimePicker sample is also a bit confusing.

Note

(...) Currently, all dates that are API properties in the DatePicker, TimePicker, DateTimePicker, PlanningCalendar and Calendar controls use local time. For example, if a user chooses 19.02.2018 as a date from the DatePicker, the app developer calls the getDateValue() method. In this case they will get 19.02.2018 00:00:00 local time. The disadvantage here is that by default this value will be sent to the backend in UTC, which may change the date by +/- one day.

It's confusing because that Note comes right after the DateTimePicker section whereas DatePicker is taken as an example. DateTimePicker intends to send date and time to the backend.
DatePicker (which intends to send date only) without displayFormat: 'Date' constraint takes the risk described in the Note.

IMHO, it would be better if that Note could be moved below the DatePicker section instead of DateTimePicker.


Another issue in the same topic: #2591

@boghyon
Copy link
Contributor Author

boghyon commented Jul 18, 2019

As the quoted text has been removed by SAP/openui5-docs@b9063cf, this issue can be closed.

@stopcoder
Copy link
Member

Hi @boghyon,

I forget to post an update to this as you already found that we removed the text in question.

Therefore I would like to close this issue report.

Best regards,
Jiawei

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants