Merge lp:~spiv/launchpad/bmp-inline-diffs into lp:launchpad

[Andrew Bennetts]

[r=danilo][bug=813349] Add inline diff expanders to revision summaries in merge proposal comments.

This still needs some polish, but it's close enough to be worth a review. It's guarded by a feature flag so it ought to be safe to land as is.

It adds inline dynamic diffs to branch index and merge proposal pages. It fetches them from loggerhead (proxied via the lpnet webapp).

It's guarded by a feature flag, the jslint is clean, and it has some tests (although it could use more). I don't think this does anything outrageously wrong, but many of the tools used here are new to me so I could be wrong! So let me know what you think!

[Martin Pool (mbp)]

[tweak] Could you please change the css so you get a pointing-hand cursor (link cursor) on the expander?

[Martin Pool (mbp)]

http://people.canonical.com/~andrew/Inline-diff-screenshot.png

[Martin Pool (mbp)]

[tweak] The flag ought to be called foo.enabled

I think this is not going to work for private branches because of <https://bugs.launchpad.net/launchpad/+bug/806713>. For an initial closed beta we can probably just live with it being broken. As an interim step, I guess we would have the option of disabling it for private branches. Eventually we'd need to fix that.

[Данило Шеган (danilo)]

I'm adding more API to the expander to allow one to linkify the "toggler" with an easy parameter to setUp in my follow-up branch for expander-anim. expander-anim should also already accept failed parameter to receive method (so it tries to re-load stuff when it fails, just use receive(node, true) in the error handler).

[Gavin Panella (allenap)]

Hi,

There are conflicts here with Danilo's branch I think. Could you take a look?

Gavin.

[Andrew Bennetts (spiv)]

Conflicts have been resolved.

[Martin Pool (mbp)]

Specifically, the thing with the flag is that the name in the templates is not the same as the name in the flags.py documentation.

[Andrew Bennetts (spiv)]

Regarding the CSS, as far as I'm aware my patch is doing everything it's supposed to. It looks to me like a bug in the expander infrastructure rather than this patch. I've pinged Danilo on IRC about it.

I've fixed the feature flag name: I actually had it correct in the TAL, but misnamed in flags.py. Fixed now, I think.

[Данило Шеган (danilo)]

Andrew, just a few minor comments:

 - getDiffUrl could probably be dropped and replaced with bmpGetDiffUrl use (i.e. bmpGetDiffUrl(revno, revno-1))
 - perhaps you could accept undefined (and use Y.Lang.isUndefined or Y.Lang.isValue [which is false for nulls and undefineds]) as the end_revno in bmpGetDiffUrl when you just want one revision
 - with the changes to the expander in the meantime, it'd probably be better to instanciate the expander directly instead of using createByCSS. That will allow you to call expander.setUp(true), when the content inside the "toggle" node will be turned into a proper link (and you shouldn't wrap it inside <a> in TAL, or it'll end up being <a> inside an <a>)
 - alternatively to the above, you can just add 'href="#"' to get the pointing hand cursor to appear if you feel like the link should stay in TAL or you prefer createByCSS API
 - generally, tables should not be used for non-tabular-data formatting, but if existing CSS classes you use don't work if you simply replace <td>s with <div>s, just keep it as is
 - you may also need to change test set-up (especially in HTML) because of the recent changes to the test infrastructure; try looking at other existing tests for reference and make sure the test passes when run as "bin/test -t test_branchrevisionexpander.html"

I consider most of the above minor issues, so here's a conditional approval on fixing those listed above.

review: Approve

[Данило Шеган (danilo)]

FWIW, expander-anim is landing as soon as we get out of RC mode.

[Andrew Bennetts (spiv)]

 - I dropped getDiffUrl (and renamed bmpGetDiffUrl to bmp_get_diff_url to be more consistent with the rest of the file)
 - I added more JS tests for bmp_get_diff_url (and fixed a bug that uncovered! — diff ranges starting with 0 other than 0..1 were wrong)
 - bmp_get_diff_url now accepts either a single revno, or a pair of revnos describing a range
 - I'm still using createByCSS, as that seems to be more concise and convenient than the alternative
 - I added href="#', which fixes the cursor, thanks!
 - The table is precisely how the existing merge proposal diff is formatted. If that should change, then that's an issue for a separate patch I think. I'm just reusing what we already do.
 - I tested with bin/test as suggested, and the test appears to run just fine. Hooray!

Thanks for the review. I've addressed all the points, so I think this is ready to land. Sweet!

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2011-07-04 15:48:18 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2011-07-20 03:39:50 +0000
@@ -38,6 +38,7 @@
 from lazr.lifecycle.event import ObjectModifiedEvent
 from lazr.restful.interface import copy_field
 from lazr.restful.interfaces import (
+    IJSONRequestCache,
     IWebServiceClientRequest,
     )
 import simplejson
@@ -596,6 +597,16 @@
     label = "Proposal to merge branch"
     schema = ClaimButton
 
+    def initialize(self):
+        super(BranchMergeProposalView, self).initialize()
+        cache = IJSONRequestCache(self.request)
+        cache.objects.update({
+            'branch_diff_link':
+                'https://%s/+loggerhead/%s/diff/' %
+                (config.launchpad.code_domain,
+                 self.context.source_branch.unique_name)
+            })
+
     @action('Claim', name='claim')
     def claim_action(self, action, data):
         """Claim this proposal."""

=== added file 'lib/lp/code/javascript/branch.revisionexpander.js'
--- lib/lp/code/javascript/branch.revisionexpander.js	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/branch.revisionexpander.js	2011-07-20 03:39:50 +0000
@@ -0,0 +1,103 @@
+/* Copyright 2011 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Expandable branch revisions.
+ *
+ * @module lp.code.branch.revisionexpander
+ * @requires node, lp.client.plugins
+ */
+
+YUI.add('lp.code.branch.revisionexpander', function(Y) {
+
+var namespace = Y.namespace('lp.code.branch.revisionexpander');
+
+/*
+ * Take a single revno, or a pair of revnos specifying a revision range, and
+ * construct a URL to get the diff of those revnos using
+ * LP.cache.branch_diff_link.
+ */
+function bmp_get_diff_url(start_revno, end_revno) {
+    var diff_url;
+    if (Y.Lang.isUndefined(end_revno)) {
+        /* No end_revno passed, so only after a single revision diff. */
+        return LP.cache.branch_diff_link + start_revno;
+    }
+    diff_url = LP.cache.branch_diff_link + end_revno;
+    if (start_revno !== 0) {
+       diff_url += '/' + start_revno;
+    } else if (start_revno === 0 && end_revno !== 1) {
+       diff_url += '/null:';
+    }
+    return diff_url;
+}
+
+function difftext_to_node(difftext) {
+    var node = Y.Node.create('<table class="diff"></table>');
+    var difflines = difftext.split('\n');
+    var i;
+    /* Remove the empty final row caused by a trailing newline
+     * (if it is empty) */
+    if (difflines.length > 0 && difflines[difflines.length-1] === '') {
+        difflines.pop();
+    }
+
+    for (i=0; i < difflines.length; i++) {
+        var line = difflines[i];
+        var line_node = Y.Node.create('<td/>');
+        line_node.set('text', line + '\n');
+        /* Colour the unified diff */
+        var header_pat = /^(===|---|\+\+\+) /;
+        var chunk_pat = /^@@ /;
+        if (line.match(header_pat)) {
+            line_node.addClass('diff-header');
+        } else if (line.match(chunk_pat)) {
+            line_node.addClass('diff-chunk');
+        } else {
+            switch (line[0]) {
+                case '+':
+                    line_node.addClass('diff-added');
+                    break;
+                case '-':
+                    line_node.addClass('diff-removed');
+                    break;
+            }
+        }
+        line_node.addClass('text');
+        var row = Y.Node.create('<tr></tr>');
+        row.appendChild(line_node);
+        node.appendChild(row);
+    }
+    return node;
+}
+
+function revision_expander_config(expander){
+   return {
+        on: {
+            success: function nodify_result(diff) {
+                expander.receive(difftext_to_node(diff));
+            },
+            failure: function(trid, response, args) {
+                expander.receive(Y.Node.create('<pre><i>Error</i></pre>'));
+            }
+        }
+   };
+}
+
+function bmp_diff_loader(expander, lp_client) {
+    if (lp_client === undefined) {
+        lp_client = new Y.lp.client.Launchpad();
+    }
+    var rev_no_range = expander.icon_node.get(
+        'id').replace('expandable-', '').split('-');
+    var start_revno = rev_no_range[0]-1;
+    var end_revno = rev_no_range[1];
+
+    lp_client.get(bmp_get_diff_url(start_revno, end_revno),
+        revision_expander_config(expander));
+}
+
+namespace.bmp_diff_loader = bmp_diff_loader;
+namespace.difftext_to_node = difftext_to_node;
+namespace.bmp_get_diff_url = bmp_get_diff_url;
+
+}, "0.1", {"requires": ["node", "lp.app.widgets.expander"]});

=== added file 'lib/lp/code/javascript/tests/test_branchrevisionexpander.html'
--- lib/lp/code/javascript/tests/test_branchrevisionexpander.html	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchrevisionexpander.html	2011-07-20 03:39:50 +0000
@@ -0,0 +1,35 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
+
+<html>
+  <head>
+  <title>Test page for branch revisionexpander JavaScript unit tests</title>
+
+  <!-- YUI 3.0 Setup -->
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/yui/yui/yui.js"></script>
+  <script type="text/javascript" src="../../../../canonical/launchpad/icing/lazr/build/lazr.js"></script>
+  <script type="text/javascript" src="../../../app/javascript/client.js"></script>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssreset/reset.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssfonts/fonts.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/yui/cssbase/base.css"/>
+  <link rel="stylesheet" href="../../../../canonical/launchpad/javascript/test.css" />
+  <link rel="stylesheet" href="../../../../canonical/launchpad/icing/lazr/build/lazr-sam.css" />
+
+  <!-- expected variable -->
+  <script type="text/javascript">
+    var LP = {
+        cache: {},
+        links: {}
+    };
+  </script>
+
+  <!-- The module under test -->
+  <script type="text/javascript" src="../branch.revisionexpander.js"></script>
+
+  <!-- The test suite -->
+  <script type="text/javascript" src="test_branchrevisionexpander.js"></script>
+  </head>
+
+<body class="yui3-skin-sam">
+</body>
+</html>
+

=== added file 'lib/lp/code/javascript/tests/test_branchrevisionexpander.js'
--- lib/lp/code/javascript/tests/test_branchrevisionexpander.js	1970-01-01 00:00:00 +0000
+++ lib/lp/code/javascript/tests/test_branchrevisionexpander.js	2011-07-20 03:39:50 +0000
@@ -0,0 +1,151 @@
+/* Copyright 2010 Canonical Ltd.  This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Tests for lp.code.branch.revisionexpander.
+ *
+ */
+
+YUI({
+    base: '../../../../canonical/launchpad/icing/yui/',
+    filter: 'raw', combine: false
+    }).use('test', 'console', 'node-event-simulate', 'lp.client',
+           'lp.code.branch.revisionexpander', function(Y) {
+
+    var module = Y.lp.code.branch.revisionexpander;
+    var suite = new Y.Test.Suite("branch.revisionexpander Tests");
+
+    var MockClient = function() {};
+    MockClient.prototype = {
+        'calls': [],
+        'get': function(uri, config) {
+            this.calls.push({'uri': uri});
+            config.on.success(samplediff);
+        }
+    };
+
+    var samplediff = (
+        "=== modified file 'README'\n" +
+        "--- README 2011-01-20 23:05:06 +0000\n" +
+        "+++ README 2011-06-30 10:47:28 +0000\n" +
+        "@@ -1,3 +1,4 @@\n" +
+        "+Green sheep!\n" +
+        " =========\n" +
+        " testtools\n" +
+        " =========\n" +
+        "            \n");
+
+    suite.add(new Y.Test.Case({
+        name: 'Test difftext_to_node',
+
+        /*
+         * Unified diffs are rendered to a table, one row per line
+         */
+        test_difftext_to_node_outputs_table: function() {
+            var node = module.difftext_to_node(samplediff);
+            Y.Assert.areEqual('TABLE', node.get('tagName'));
+            Y.Assert.isTrue(node.hasClass('diff'));
+            /* samplediff has 9 lines, so the table will have 9 rows
+             * (trailing newlines don't result in a final row containing an
+             * empty string) */
+            Y.Assert.areEqual(9, node.get('children').size());
+        },
+
+        /*
+         * Diffs are not interpreted as HTML.
+         */
+        test_difftext_to_node_escaping: function() {
+            var node = module.difftext_to_node("<p>hello</p>");
+            var td = node.one('td');
+            Y.Assert.isNull(td.one('p'));
+        }
+        }));
+
+    suite.add(new Y.Test.Case({
+        name: 'Tests for bmp_diff_loader and bmp_get_diff_url',
+
+        setUp: function() {
+            LP.cache.branch_diff_link = 'fake-link-base/';
+            },
+
+        tearDown: function() {
+            delete LP.cache.branch_diff_link;
+            },
+
+        /*
+         * bmp_diff_loader fetches from the URI specified by the div id and
+         * renders a diff.
+         */
+        test_bmp_diff_loader_fetches_from_diff_uri: function() {
+            var FakeExpander = function() {};
+            FakeExpander.prototype = {
+                'icon_node':
+                    Y.Node.create('<div id="expandable-23-45"></div>'),
+                'receive': function (node) {
+                    this.received_node = node;
+                }
+            };
+            var mock_client = new MockClient();
+            var fake_expander = new FakeExpander();
+            module.bmp_diff_loader(fake_expander, mock_client);
+            Y.Assert.areEqual(
+                'fake-link-base/45/22', mock_client.calls[0].uri);
+            Y.Assert.areEqual(
+                'TABLE', fake_expander.received_node.get('tagName'));
+            },
+
+        /*
+         * bmp_get_diff_url(revno) gets the URL for a diff of just that revision
+         */
+        test_bmp_get_diff_url_one_arg: function() {
+            Y.Assert.areEqual(
+                'fake-link-base/1234',
+                module.bmp_get_diff_url(1234));
+            },
+
+        /*
+         * bmp_get_diff_url(start_revno, end_revno) gets the URL for a diff of
+         * the given revision range.
+         */
+        test_bmp_get_diff_url_two_args: function() {
+            Y.Assert.areEqual(
+                'fake-link-base/33/22',
+                module.bmp_get_diff_url(22, 33));
+            },
+
+        /*
+         * bmp_get_diff_url(0, 1) just returns URL_BASE/1, rather than
+         * URL_BASE/1/0 which Loggerhead will reject.
+         */
+        test_bmp_get_diff_url_of_0_to_1: function() {
+            Y.Assert.areEqual(
+                'fake-link-base/1',
+                module.bmp_get_diff_url(0, 1));
+            },
+
+        /*
+         * bmp_get_diff_url(0, 2) just returns URL_BASE/1, rather than
+         * URL_BASE/1/0 which Loggerhead will reject.
+         */
+        test_bmp_get_diff_url_of_0_to_2: function() {
+            Y.Assert.areEqual(
+                'fake-link-base/2/null:',
+                module.bmp_get_diff_url(0, 2));
+            }
+        }));
+
+    var handle_complete = function(data) {
+        window.status = '::::' + JSON.stringify(data);
+        };
+    Y.Test.Runner.on('complete', handle_complete);
+    Y.Test.Runner.add(suite);
+
+    var console = new Y.Console({newestOnTop: false});
+    console.render('#log');
+
+    // Start the test runner on Y.after to ensure all setup has had a
+    // chance to complete.
+    Y.after('domready', function() {
+        Y.Test.Runner.run();
+    });
+});
+

=== modified file 'lib/lp/code/templates/branch-macros.pt'
--- lib/lp/code/templates/branch-macros.pt	2011-07-06 14:54:46 +0000
+++ lib/lp/code/templates/branch-macros.pt	2011-07-20 03:39:50 +0000
@@ -202,6 +202,22 @@
                   condition="revisions | nothing">
       <metal:landing-target use-macro="branch/@@+macros/revision-text"/>
     </tal:revision>
+    <tal:ajax-revision-diffs
+        condition="request/features/code.ajax_revision_diffs.enabled">
+      <tal:diff-expander condition="show_diff_expander | nothing">
+	<div class="revision-group-diff" tal:condition="revisions | nothing">
+	  <a href="#" class="unseen expander-icon"
+	     tal:define="start_revno python:revisions[0].sequence;
+			 last_revno python:revisions[-1].sequence"
+	     tal:attributes="id string:expandable-${start_revno}-${last_revno}"
+	  >Changes added by revision <tal:revno replace="start_revno">1</tal:revno>
+	    <tal:plural condition="python: start_revno!=last_revno">
+	      to revision <tal:revno replace="last_revno">2</tal:revno>
+	    </tal:plural></a>
+	<div class="unseen expander-content">Loading diff</div>
+	</div>
+      </tal:diff-expander>
+    </tal:ajax-revision-diffs>
   </dl>
 
 </metal:branch-revisions>

=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2011-02-23 22:32:15 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2011-07-20 03:39:50 +0000
@@ -200,7 +200,8 @@
   conf = <tal:status-config replace="view/status_config" />
   <!--
   LPS.use('io-base', 'lp.code.branchmergeproposal.reviewcomment',
-          'lp.code.branchmergeproposal.status', 'lp.app.comment', function(Y) {
+          'lp.code.branchmergeproposal.status', 'lp.app.comment', 
+          'lp.app.widgets.expander', 'lp.code.branch.revisionexpander', function(Y) {
 
     Y.on('load', function() {
         var logged_in = LP.links['me'] !== undefined;
@@ -219,6 +220,15 @@
         (new Y.lp.code.branchmergeproposal.reviewcomment.NumberToggle()
             ).render();
       }, window);
+
+    Y.on('domready', function() {
+        Y.lp.app.widgets.expander.createByCSS(
+            '.revision-group-diff',
+            '.expander-icon',
+            '.expander-content',
+            false,
+            Y.lp.code.branch.revisionexpander.bmp_diff_loader);
+    });
   });
   -->
 <tal:script replace="structure string:&lt;/script&gt;" />

=== modified file 'lib/lp/code/templates/codereviewnewrevisions-footer.pt'
--- lib/lp/code/templates/codereviewnewrevisions-footer.pt	2010-10-06 17:07:11 +0000
+++ lib/lp/code/templates/codereviewnewrevisions-footer.pt	2011-07-20 03:39:50 +0000
@@ -4,7 +4,8 @@
    omit-tag="">
 
   <tal:revisions define="branch context/branch;
-                         revisions context/revisions">
+                         revisions context/revisions;
+                         show_diff_expander python:True;">
     <metal:landing-target use-macro="branch/@@+macros/branch-revisions"/>
   </tal:revisions>
   <tal:diff condition="context/diff" replace="structure context/diff/text/fmt:diff" />

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-07-19 14:42:52 +0000
+++ lib/lp/services/features/flags.py	2011-07-20 03:39:50 +0000
@@ -44,6 +44,10 @@
      'boolean',
      ('Enables the display of bugtracker components.'),
      ''),
+    ('code.ajax_revision_diffs.enabled',
+     'boolean',
+     ("Offer expandable inline diffs for branch revisions."),
+     ''),
     ('code.branchmergequeue',
      'boolean',
      'Enables merge queue pages and lists them on branch pages.',