launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #17060
Re: [Merge] lp:~cjohnston/launchpad/ic-approx-time into lp:launchpad
Review: Needs Fixing code
Thanks for the fixes. Just a few more trivialities and this will be good to land!
Diff comments:
> === added file 'lib/lp/app/javascript/date.js'
> --- lib/lp/app/javascript/date.js 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/javascript/date.js 2014-07-03 21:16:30 +0000
> @@ -0,0 +1,34 @@
> +/* Copyright 2014 Canonical Ltd. This software is licensed under the
> + * GNU Affero General Public License version 3 (see the file LICENSE). */
> +
> +YUI.add('lp.app.date', function(Y) {
> +
> +var namespace = Y.namespace('lp.app.date');
> +
> +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 (days > 1) {
> + return 'on ' + Y.Date.format(new Date(date), {format: '%Y-%m-%d'});
> + } else {
> + if (hours >= 1) {
> + amount = hours
> + unit = "hour"
You need to pre-declare amount and unit, or they will be globals.
> + } else if (minutes >= 1) {
> + amount = minutes
> + unit = "minute"
> + } else {
> + return "a moment ago"
> + }
> + if (Math.floor(amount) > 1) {
> + unit = unit + 's'
> + }
> + return Math.floor(amount) + ' ' + unit + ' ago'
> + }
> +};
> +}, "0.1", {'requires': ['datatype-date', 'lp.app.date']});
It depends on itself?
>
> === added file 'lib/lp/app/javascript/tests/test_date.html'
> --- lib/lp/app/javascript/tests/test_date.html 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/javascript/tests/test_date.html 2014-07-03 21:16:30 +0000
> @@ -0,0 +1,37 @@
> +<!DOCTYPE html>
> +<!--
> +Copyright 2014 Canonical Ltd. This software is licensed under the
> +GNU Affero General Public License version 3 (see the file LICENSE).
> +-->
> +<html>
> + <head>
> + <title>Test lp-names</title>
> + <!-- YUI and test setup -->
> + <script type="text/javascript"
> + src="../../../../../build/js/yui/yui/yui.js">
> + </script>
> + <link rel="stylesheet"
> + href="../../../../../build/js/yui/console/assets/console-core.css" />
> + <link rel="stylesheet"
> + href="../../../../../build/js/yui/test-console/assets/skins/sam/test-console.css" />
> + <link rel="stylesheet"
> + href="../../../../../build/js/yui/test/assets/skins/sam/test.css" />
> +
> + <script type="text/javascript"
> + src="../../../../../build/js/lp/app/testing/testrunner.js"></script>
> + <link rel="stylesheet" href="../../../app/javascript/testing/test.css" />
> +
> + <!-- The module under test. -->
> + <script type="text/javascript" src="../date.js"></script>
> +
> + <!-- The test suite. -->
> + <script type="text/javascript" src="test_date.js"></script>
> +
> + </head>
> + <body class="yui3-skin-sam">
> + <ul id="suites">
> + <li>date.test</li>
> + </ul>
> +
> + </body>
> +</html>
>
> === added file 'lib/lp/app/javascript/tests/test_date.js'
> --- lib/lp/app/javascript/tests/test_date.js 1970-01-01 00:00:00 +0000
> +++ lib/lp/app/javascript/tests/test_date.js 2014-07-03 21:16:30 +0000
> @@ -0,0 +1,36 @@
> +YUI.add('date.test', function (Y) {
> +
> + var tests = Y.namespace('date.test');
> + tests.suite = new Y.Test.Suite("date tests");
> +
> + var now = (new Date).valueOf()
> + var hoursAgo = (new Date(now - 12600000))
> + var minuteAgo = (new Date(now - 65000))
> + var momentAgo = (new Date(now - 150))
Can you inline these *Ago? They're test-specific, and also not valid variable names -- we use_underscores for variables.
> + tests.suite.add(new Y.Test.Case({
> + name: 'test_approximatedate',
> +
> + test_return_moment_ago: function () {
> + Y.Assert.areEqual(
> + 'a moment ago', Y.lp.app.date.approximatedate(momentAgo));
> + },
> +
> + test_return_minute_ago: function () {
> + Y.Assert.areEqual(
> + '1 minute ago', Y.lp.app.date.approximatedate(minuteAgo));
> + },
> +
> + test_return_hours_ago: function () {
> + Y.Assert.areEqual(
> + '3 hours ago', Y.lp.app.date.approximatedate(hoursAgo));
> + },
> + test_return_days_ago: function () {
> + Y.Assert.areEqual(
> + 'on 2012-08-12', Y.lp.app.date.approximatedate(
> + '2012-08-12T10:00:00.00001+00:00'));
> + },
> + }));
> +
> +}, '0.1', {
> + requires: ['lp.testing.runner', 'test', 'lp', 'lp.app.date']
> +});
>
> === modified file 'lib/lp/app/templates/base-layout-macros.pt'
> --- lib/lp/app/templates/base-layout-macros.pt 2014-01-02 05:02:26 +0000
> +++ lib/lp/app/templates/base-layout-macros.pt 2014-07-03 21:16:30 +0000
> @@ -121,7 +121,7 @@
> //<![CDATA[
> LPJS.use('base', 'node', 'console', 'event',
> 'oop', 'lp', 'lp.app.foldables','lp.app.sorttable',
> - 'lp.app.inlinehelp', 'lp.app.links', 'lp.app.longpoll',
> + 'lp.app.inlinehelp', 'lp.app.links', 'lp.app.longpoll', 'lp.app.date',
Not all pages need lp.app.date, so this isn't necessary here.
> 'lp.bugs.bugtask_index', 'lp.bugs.subscribers',
> 'lp.app.ellipsis', 'lp.code.branchmergeproposal.diff',
> 'lp.views.global',
>
> === 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 21:16:30 +0000
> @@ -127,6 +127,7 @@
> 'tr[id^="diff-line"]');
> };
>
> +
Blank line.
> 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 +185,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 = Y.lp.app.date.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);
> @@ -701,4 +701,4 @@
> namespace.DiffNav = DiffNav;
>
> }, '0.1', {requires: ['datatype-date', 'event', 'io', 'node', 'widget',
> - 'lp.client', 'lp.ui.editor']});
> + 'lp.client', 'lp.ui.editor', 'lp.app.date']});
>
> === modified file 'lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html'
> --- lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-05-19 21:56:02 +0000
> +++ lib/lp/code/javascript/tests/test_branchmergeproposal.inlinecomments.html 2014-07-03 21:16:30 +0000
> @@ -32,6 +32,8 @@
> <script type="text/javascript"
> src="../../../../../build/js/lp/app/client.js"></script>
> <script type="text/javascript"
> + src="../../../../../build/js/lp/app/date.js"></script>
> + <script type="text/javascript"
> src="../../../../../build/js/lp/app/testing/mockio.js"></script>
> <script type="text/javascript"
> src="../../../../../build/js/lp/app/ui/ui.js"></script>
>
> === 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 21:16:30 +0000
> @@ -70,25 +70,44 @@
> "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': hoursAgo,
> + },
> + {'line_number': '3',
> + 'person': person_obj,
> + 'text': 'This is bad stuff.',
> + 'date': minuteAgo,
> + },
> + {'line_number': '3',
> + 'person': person_obj,
> + 'text': 'This is good stuff.',
> + 'date': momentAgo,
> + },
You don't need to test all of this here, since we now have unit tests of the underlying formatter function. I'd just test two cases, and inline the *Ago again for clarity.
> ];
> 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(
> 'This is preloaded.',
> first.one('.boardCommentBody').get('text'));
> @@ -103,12 +122,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.
References
