On Thu, 06 Mar 2014 13:26:03 +0100
Petr Vobornik <pvobo...@redhat.com> wrote:

> 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
> 
> 


ACK,
can push to master.
Adam

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

Reply via email to