On 6.3.2014 13:01, Misnyovszki Adam wrote:
On Tue, 25 Feb 2014 18:05:28 +0100
Petr Vobornik <pvobo...@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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to