launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

approximatedate should probably live in lib/lp/app/javascript somewhere, and it needs unit tests of its own. Also, a few more inline comments.

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 13:14:02 +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 = (new Date).valueOf();
> +    var timedelta = now - Date.parse(date);
> +    var days = timedelta / 86400000
> +    var hours = timedelta / 3600000
> +    var minutes = timedelta / 60000
> +    if (timedelta > 86400000) {

if (days >= 1)?

> +        return 'on ' + Y.Date.format(new Date(date), {format: '%Y-%m-%d'});

This will use the browser's local timezone. That's probably OK for now, but we should eventually use the user's configured timezone on the server.

> +    } else {
> +        if (days >= 1) {

This condition won't ever match; it's the same as the "on YYYY-MM-DD" case.

> +            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) {

Math.floor here again.

> +            unit = unit + 's'
> +        }
> +        return Math.floor(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 13:14:02 +0000
> @@ -30,6 +30,18 @@
>                  "lp.code.branchmergeproposal.inlinecomments module");
>          },
>  
> +        getDate: function(date) {
> +            // Convert date and time in millseconds to comma-delimited date
> +            var getYear = date.getFullYear()

This will be off by one around the end of the year.

> +            var getMonth = date.getUTCMonth()
> +            var getDate = date.getUTCDate()
> +            var getHours = date.getUTCHours()
> +            var getMinutes = date.getUTCMinutes()
> +            var getSeconds = date.getUTCSeconds()
> +            return (new Date(Date.UTC(
> +                getYear, getMonth, getDate, getHours, getMinutes, getSeconds)))

I don't quite understand what this method is trying to do. Apart from the getFullYear bug it looks like a no-op to me -- it takes a Date (which is always in the browser timezone), gets the UTC bits out of it, uses Date.UTC to turn it into a numeric timestamp that should be identical to getTime(), then passes it back into the Date constructor to get back an object that should be identical to the input Date.

> +        },
> +
>          test_population: function () {
>              // Mimics a person object as stored in LP.cache (unwrap).
>              var person_obj = {
> @@ -70,25 +82,47 @@
>                  "ws.op=getInlineComments&previewdiff_id=1",
>                  mockio.requests[0].config.data);
>              mockio.last_request = mockio.requests[0];
> +            var now = (new Date).valueOf()
> +            var hoursAgo = (new Date(now - 12600000))
> +            var minuteAgo = (new Date(now - 65000))
> +            var momentAgo = (new Date(now - 150))
>              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': this.getDate(hoursAgo),
> +                },
> +                {'line_number': '3',
> +                 'person': person_obj,
> +                 'text': 'This is bad stuff.',
> +                 'date': this.getDate(minuteAgo),
> +                },
> +                {'line_number': '3',
> +                 'person': person_obj,
> +                 'text': 'This is good stuff.',
> +                 'date': this.getDate(momentAgo),
> +                },
>              ];
>              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'));

Have you tested this on Lucid, like our buildbots? I suspect it'll have the same problem as Celso's XXXed block a few lines earlier.

>              Y.Assert.areEqual(
>                  'This is preloaded.',
>                  first.one('.boardCommentBody').get('text'));
> @@ -103,12 +137,25 @@
>                  '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')
>              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.