launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

Date.now and the ISO 8601 support in Date.parse are relatively new, and we probably want to fall back if they don't work. This will fail at least in IE8, and quite possibly on the Lucid buildbots which use an old WebKit. We probably don't care about IE8 at this point, but buildbot will be a problem.

Also, your bzr whoami is wrong; consider correcting it and recommitting.

And there are a few comments inline.

Diff comments:

> === modified file 'lib/lp/code/javascript/branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-05-29 16:30:42 +0000
> +++ lib/lp/code/javascript/branchmergeproposal.inlinecomments.js	2014-07-03 03:05:24 +0000
> @@ -127,6 +127,37 @@
>          'tr[id^="diff-line"]');
>  };
>  
> +
> +namespace.approximatedate = function(date) {
> +    // Display approximate time an event happened when it happened less than 1
> +    // day ago.
> +    var now = Date.now();
> +    var timedelta = now - Date.parse(date);
> +    days = timedelta / 86400000
> +    hours = timedelta / 3600000
> +    minutes = timedelta / 60000

These all end up as global variables.

> +    if (timedelta > '86400000') {

Why's this a string?

> +        return 'on ' + Y.Date.format(new Date(date), {format: '%Y-%m-%d'});
> +    } else {
> +        if (days >= 1) {
> +            amount = days
> +            unit = "day"
> +        } else if (hours >= 1) {
> +            amount = hours
> +            unit = "hour"
> +        } else if (minutes >= 1) {
> +            amount = minutes
> +            unit = "minute"
> +        } else {
> +            return "a moment ago"
> +        }
> +        if (parseInt(amount) > 1) {

Why might this be a string?

> +            unit = unit + 's'
> +        }
> +        return parseInt(amount) + ' ' + unit + ' ago'
> +    }
> +};
> +
>  namespace.create_row = function(comment) {
>      // Find or create the container for this line's comments.
>      var comments_tr = Y.one('#comments-diff-line-' + comment.line_number);
> @@ -184,9 +215,8 @@
>          // XXX cprov 20140226: the date should be formatted according to
>          // the user locale/timezone similar to fmt:displaydate.
>          // We should leverage Y.Intl features.
> -        var fmt_date = 'on ' + Y.Date.format(
> -            new Date(comment.date), {format: '%Y-%m-%d'});
> -        headerspan.one('span').set('text', fmt_date);
> +        var date = namespace.approximatedate(comment.date);
> +        headerspan.one('span').set('text', date);
>          newrow.one('.boardCommentDetails').append(headerspan);
>          newrow.one('.boardCommentBody div').append('<span></span>');
>          newrow.one('.boardCommentBody').one('span').set('text', comment.text);
> 
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-05-26 12:44:57 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.js	2014-07-03 03:05:24 +0000
> @@ -70,25 +70,44 @@
>                  "ws.op=getInlineComments&previewdiff_id=1",
>                  mockio.requests[0].config.data);
>              mockio.last_request = mockio.requests[0];
> +            var now = Date.now()
>              published_comments = [
>                  {'line_number': '2',
>                   'person': person_obj,
>                   'text': 'This is preloaded.',
> -                 'date': '2012-08-12T10:00:00.00001+00:00'}
> +                 'date': '2012-08-12T10:00:00.00001+00:00'},
> +                {'line_number': '3',
> +                 'person': person_obj,
> +                 'text': 'This is great.',
> +                 'date': new Date(now - 12600000).toISOString(), 
> +                },
> +                {'line_number': '3',
> +                 'person': person_obj,
> +                 'text': 'This is bad stuff.',
> +                 'date': new Date(now - 65000).toISOString(), 
> +                },
> +                {'line_number': '3',
> +                 'person': person_obj,
> +                 'text': 'This is good stuff.',
> +                 'date': new Date(now - 150).toISOString(), 
> +                },
>              ];
>              mockio.success({
>                  responseText: Y.JSON.stringify(published_comments),
>                  responseHeaders: {'Content-Type': 'application/json'}});
>  
>              // Published comment is displayed.
> -            var comments = Y.one('#diff-line-2').next().one('div');
> +            var first_comments = Y.one('#diff-line-2').next().one('div');
>              // XXX cprov 20140226: test disabled due to ES4 lack of
>              // ISO8601 support. Although the vast majority of production
>              // clients run ES5.
>              //Y.Assert.areEqual(
>              //    'Foo Bar (name16) wrote on 2012-08-12',
>              //    header.get('text'));
> -            var first = comments.one('div:first-child')
> +            var first = first_comments.one('div:first-child')
> +            Y.Assert.areEqual(
> +                'Foo Bar (name16) wrote on 2012-08-12:',
> +                first.one('.boardCommentDetails').get('text'));
>              Y.Assert.areEqual(
>                  'This is preloaded.',
>                  first.one('.boardCommentBody').get('text'));
> @@ -103,12 +122,26 @@
>                  'Boing!', second.one('.boardCommentBody').get('text'));
>  
>              // Draft comment for line 3 is displayed.
> -            var third = Y.one('#diff-line-3').next();
> +            var second_comments = Y.one('#diff-line-3').next().one('div');
> +            var third = second_comments.one('div:first-child')
> +            console.log(third);
>              Y.Assert.areEqual(
> -                'Unsaved comment',
> +                'Foo Bar (name16) wrote 3 hours ago:',
>                  third.one('.boardCommentDetails').get('text'));
> -            Y.Assert.areEqual(
> -                'Zoing!', third.one('.boardCommentBody').get('text'));
> +            var fourth = third.next()
> +            Y.Assert.areEqual(
> +                'Foo Bar (name16) wrote 1 minute ago:',
> +                fourth.one('.boardCommentDetails').get('text'));
> +            var fifth = fourth.next()
> +            Y.Assert.areEqual(
> +                'Foo Bar (name16) wrote a moment ago:',
> +                fifth.one('.boardCommentDetails').get('text'));
> +            var sixth = fifth.next()
> +            Y.Assert.areEqual(
> +                'Unsaved comment',
> +                sixth.one('.boardCommentDetails').get('text'));
> +            Y.Assert.areEqual(
> +                'Zoing!', sixth.one('.boardCommentBody').get('text'));
>          },
>  
>          test_draft_handler: function() {
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/ic-approx-time/+merge/225420
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.