[Freeipa-devel] [PATCH] 546 webui: Datetime parsing and formatting

[Petr Vobornik]

On 6.3.2014 13:01, Misnyovszki Adam wrote:
> On Tue, 25 Feb 2014 18:05:28 +0100
> Petr Vobornik <pvoborni at redhat.com> wrote:
>
>> prerequisite for patch 547, 548
>> depends on tbabej's datetime patch
>>
>> this patch implements:
>> - output_formatter in field. It should be used in par with formatter.
>> Formatter serves for datasource->widget conversion, output_formatter
>> for widget->datasource format conversion.
>> - datetime module which parses/format strings in subset of ISO 8601
>> and LDAP generalized time format to Date.
>> - utc formatter replaced with new datetime formatter
>> - datetime_validator introduced
>> - new datetime field, extension of text field, which by default uses
>> datetime formatter and validator
>>
>> Dojo was regenerated to include dojo/string module
>>
>> https://fedorahosted.org/freeipa/ticket/4194
>
> Hi,
> these are the results of my review:
> - if incorrect number specified for any of the parts(ie 2013-01-01
>    25:00:00), then it counts forward(result: 2013-01-02 01:00:00), does
>    it supposed to work this way? at least some warning should be given
>    to the user, that the date is incorrect(imho)

It's standard behavior of JavaScript Date object's setUTCFullYear 
method. I did not find better methods which would not require pulling 
third-party lib or do real evaluation of the dates.

In the end it's not that bad.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/setUTCFullYear

> - couldn't test non utc datetime input(no test cases in the ui yet),
>    but other tests(integration and ui) passed which are connected to
>    this issue

Non UTC are not supported therefore it's disabled. But there are unit 
tests in test/utils_tests.js

> - validity fields accept non existing timeframe(ie start: 2013-01-01
>    00:00:00Z, end: 2012-01-01 00:00:00Z)

I don't think it's checked even on a server. Maybe it should be.

> - validity fields only accept UTC time, it's good
>
> so besides that timeframe issue(which the api should handle i think),
> it's an ACK
>
> Be careful, it should be pushed to master with pvoborni's 531-541 and
> 546-548, wait for the review of those!
>
> Greets:
> Adam


-- 
Petr Vobornik