Merge lp:~cjwatson/launchpad/bmp-show-diffstat into lp:launchpad

[Colin Watson]

When linking to a diff, include a per-file diffstat under an expander.

One of the items on the Launchpad stakeholder list at the moment is "List files that are being modified at top of an MP for convenience". This seems like a good idea as long as it's behind an expander so that it doesn't make the page get too long, and Launchpad already has all the information it needs to render this.

[William Grant (wgrant)]

review: Needs Fixing (code)

[Colin Watson (cjwatson)]

Good point. I've pushed an attempted conversion; how's this?

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/browser/diff.py'
--- lib/lp/code/browser/diff.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/browser/diff.py	2015-06-03 11:21:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Display classes relating to diff objects of one sort or another."""
@@ -15,6 +15,7 @@
 from lp.services.browser_helpers import get_plural_text
 from lp.services.librarian.browser import FileNavigationMixin
 from lp.services.webapp import Navigation
+from lp.services.webapp.escaping import structured
 from lp.services.webapp.publisher import canonical_url
 
 
@@ -64,13 +65,18 @@
         diffstat = diff.diffstat
         if diffstat is not None:
             file_count = len(diffstat)
-            file_text = get_plural_text(
-                file_count, _(' %d file modified'), _(' %d files modified'))
-            file_text = file_text % file_count
+            basic_file_text = get_plural_text(
+                file_count, _('%d file modified'), _('%d files modified'))
+            basic_file_text = basic_file_text % file_count
+            diffstat_text = '<br/>'.join(
+                structured('%s (+%s/-%s)', path, added, removed).escapedtext
+                for path, (added, removed) in sorted(diffstat.items()))
+            file_text = (
+                '<div class="collapsible"><span>%s</span><div>%s</div></div>' %
+                (basic_file_text, diffstat_text))
 
         args = {
             'line_count': _('%s lines') % diff.diff_lines_count,
-            'file_text': file_text,
             'conflict_text': conflict_text,
             'count_text': count_text,
             'url': self.url(view_name),
@@ -78,14 +84,14 @@
         # Under normal circumstances, there will be an associated file,
         # however if the diff is empty, then there is no alias to link to.
         if args['url'] is None:
-            return (
+            return structured(
                 '<span class="empty-diff">'
-                '%(line_count)s</span>' % args)
+                '%(line_count)s</span>', **args).escapedtext
         else:
-            return (
+            return structured(
                 '<a href="%(url)s" class="diff-link">'
-                '%(line_count)s%(count_text)s%(file_text)s%(conflict_text)s'
-                '</a>' % args)
+                '%(line_count)s%(count_text)s%(conflict_text)s'
+                '</a>', **args).escapedtext + file_text
 
 
 class PreviewDiffFormatterAPI(DiffFormatterAPI):

=== modified file 'lib/lp/code/browser/tests/test_tales.py'
--- lib/lp/code/browser/tests/test_tales.py	2014-07-09 02:42:47 +0000
+++ lib/lp/code/browser/tests/test_tales.py	2015-06-03 11:21:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the tales formatters."""
@@ -110,7 +110,11 @@
                 'file2': (3, 0)})
         self.assertEqual(
             '<a href="%s/+files/preview.diff" class="diff-link">'
-            '10 lines (+4/-0) 2 files modified</a>'
+            '10 lines (+4/-0)</a>'
+            '<div class="collapsible">'
+            '<span>2 files modified</span>'
+            '<div>file1 (+1/-0)<br/>file2 (+3/-0)</div>'
+            '</div>'
             % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
@@ -121,7 +125,26 @@
                 'file': (3, 0)})
         self.assertEqual(
             '<a href="%s/+files/preview.diff" class="diff-link">'
-            '10 lines (+4/-0) 1 file modified</a>'
+            '10 lines (+4/-0)</a>'
+            '<div class="collapsible">'
+            '<span>1 file modified</span>'
+            '<div>file (+3/-0)</div>'
+            '</div>'
+            % canonical_url(preview),
+            test_tales('preview/fmt:link', preview=preview))
+
+    def test_fmt_lines_escapes_file_name(self):
+        # File names that include HTML metacharacters are escaped.
+        preview = self._createPreviewDiff(
+            10, added=4, removed=0, diffstat={
+                'file<name>': (3, 0)})
+        self.assertEqual(
+            '<a href="%s/+files/preview.diff" class="diff-link">'
+            '10 lines (+4/-0)</a>'
+            '<div class="collapsible">'
+            '<span>1 file modified</span>'
+            '<div>file&lt;name&gt; (+3/-0)</div>'
+            '</div>'
             % canonical_url(preview),
             test_tales('preview/fmt:link', preview=preview))
 
@@ -147,10 +170,16 @@
             (self.factory.getUniqueString(), (2, 3)) for x in range(23))
         preview = self._createStalePreviewDiff(
             500, 89, 340, diffstat=diffstat)
+        expected_diffstat = '<br/>'.join(
+            '%s (+2/-3)' % path for path in sorted(diffstat))
         self.assertEqual(
             '<a href="%s/+files/preview.diff" class="diff-link">'
-            '500 lines (+89/-340) 23 files modified</a>'
-            % canonical_url(preview),
+            '500 lines (+89/-340)</a>'
+            '<div class="collapsible">'
+            '<span>23 files modified</span>'
+            '<div>%s</div>'
+            '</div>'
+            % (canonical_url(preview), expected_diffstat),
             test_tales('preview/fmt:link', preview=preview))
 
     def test_fmt_stale_non_empty_diff_with_conflicts(self):
@@ -159,10 +188,16 @@
             (self.factory.getUniqueString(), (2, 3)) for x in range(23))
         preview = self._createStalePreviewDiff(
             500, 89, 340, u'conflicts', diffstat=diffstat)
+        expected_diffstat = '<br/>'.join(
+            '%s (+2/-3)' % path for path in sorted(diffstat))
         self.assertEqual(
             '<a href="%s/+files/preview.diff" class="diff-link">'
-            '500 lines (+89/-340) 23 files modified (has conflicts)</a>'
-            % canonical_url(preview),
+            '500 lines (+89/-340) (has conflicts)</a>'
+            '<div class="collapsible">'
+            '<span>23 files modified</span>'
+            '<div>%s</div>'
+            '</div>'
+            % (canonical_url(preview), expected_diffstat),
             test_tales('preview/fmt:link', preview=preview))
 
 

=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-05-28 08:35:20 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt	2015-06-03 11:21:01 +0000
@@ -13,7 +13,6 @@
     ...     BranchMergeProposalStatus,
     ...     BranchSubscriptionNotificationLevel,
     ...     CodeReviewNotificationLevel)
-    ...     BranchSubscriptionNotificationLevel, CodeReviewNotificationLevel)
     >>> from lp.code.interfaces.branchlookup import IBranchLookup
     >>> from lp.code.tests.helpers import (
     ...     make_merge_proposal_without_reviewers)
@@ -638,7 +637,7 @@
     >>> from lazr.uri import URI
     >>> print http(r"""
     ... GET %s HTTP/1.1
-    ... Authorization: Basic [e-mail]:test        
+    ... Authorization: Basic [e-mail]:test
     ... """ % URI(link['href']).path)
     HTTP/1.1 303 See Other
     ...
@@ -667,7 +666,8 @@
 Preview diff generation status
 ------------------------------
 
-    >>> update = find_tag_by_id(nopriv_browser.contents, 'diff-pending-update')
+    >>> update = find_tag_by_id(
+    ...     nopriv_browser.contents, 'diff-pending-update')
     >>> print extract_text(update)
     Updating diff...
     An updated diff will be available in a few minutes.  Reload to see the
@@ -713,7 +713,10 @@
     >>> print_tag_with_id(nopriv_browser.contents, 'landing-targets')
     Ready for review for merging into lp://dev/fooix
       Eric the Viking: Pending (code) requested ... ago
-      Diff: 47 lines (+7/-0) 2 files modified                api
+      Diff: 47 lines (+7/-13) 2 files modified
+      file1 (+3/-8)
+      file2 (+4/-5)
+      api
 
 
 Merge proposal details shown on the bug page
@@ -734,4 +737,7 @@
     lp://dev/~fred/fooix/proposed
       Ready for review for merging into lp://dev/fooix
         Eric the Viking: Pending (code) requested ... ago
-        Diff: 47 lines (+7/-0) 2 files modified              api
+        Diff: 47 lines (+7/-13) 2 files modified
+        file1 (+3/-8)
+        file2 (+4/-5)
+	api

=== modified file 'lib/lp/code/tests/helpers.py'
--- lib/lp/code/tests/helpers.py	2015-04-21 16:53:45 +0000
+++ lib/lp/code/tests/helpers.py	2015-06-03 11:21:01 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper functions for code testing live here."""
@@ -117,7 +117,7 @@
     naked_bmp.target_branch.last_scanned_id = preview.target_revision_id
     preview.diff_lines_count = 47
     preview.added_lines_count = 7
-    preview.remvoed_lines_count = 13
+    preview.removed_lines_count = 13
     preview.diffstat = {'file1': (3, 8), 'file2': (4, 5)}
     return {
         'eric': eric, 'fooix': fooix, 'trunk': trunk, 'feature': feature,