launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/bmp-show-diffstat into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bmp-show-diffstat/+merge/260921

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bmp-show-diffstat into lp:launchpad.

=== 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 10:20:58 +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."""
@@ -64,9 +64,15 @@
         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(
+                '%s (+%d/-%d)' % (path, added, removed)
+                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,
@@ -84,8 +90,8 @@
         else:
             return (
                 '<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>%(file_text)s' % args)
 
 
 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 10:20:58 +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,11 @@
                 '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))
 
@@ -147,10 +155,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 +173,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 10:20:58 +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 no-priv@xxxxxxxxxxxxx:test        
+    ... Authorization: Basic no-priv@xxxxxxxxxxxxx: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 10:20:58 +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,