launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/fix-index-download into lp:launchpad with lp:~abentley/launchpad/attachment-timeout as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #924926 in Launchpad itself: "comment download redirect on main merge proposal page"
  https://bugs.launchpad.net/launchpad/+bug/924926

For more details, see:
https://code.launchpad.net/~abentley/launchpad/fix-index-download/+merge/91160

= Summary =
Fix bug #924926: comment download redirect on main merge proposal page

== Proposed fix ==
Render +index using a subclass of CodeReviewCommentView

== Pre-implementation notes ==
None

== Implementation details ==
Attempted to avoid subclassing by moving the redirect code to a method and using "attribute" to select that method in the zcml, but it did not work.

== Tests ==
bin/test -t test_excessive_conversation_comments_no_redirect -t test_excessive_comments_redirect_to_download

== Demo and Q/A ==
Create a comment that is more than 10,000 characters long.  Go to its merge proposal page.  The page should show normally.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/codereviewcomment.py
  lib/lp/code/browser/configure.zcml
  lib/lp/code/browser/tests/test_branchmergeproposal.py
-- 
https://code.launchpad.net/~abentley/launchpad/fix-index-download/+merge/91160
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/fix-index-download into lp:launchpad.

=== modified file 'lib/lp/code/browser/codereviewcomment.py'
--- lib/lp/code/browser/codereviewcomment.py	2012-02-01 20:03:27 +0000
+++ lib/lp/code/browser/codereviewcomment.py	2012-02-01 20:03:27 +0000
@@ -188,16 +188,19 @@
         return download_body(
             CodeReviewDisplayComment(self.context), self.request)
 
+    # Should the comment be shown in full?
+    full_comment = True
+    # Show comment expanders?
+    show_expanders = False
+
+
+class CodeReviewCommentIndexView(CodeReviewCommentView):
+
     def __call__(self):
         """View redirects to +download if comment is too long to render."""
         if self.comment.too_long_to_render:
             return self.request.response.redirect(self.comment.download_url)
-        return super(CodeReviewCommentView, self).__call__()
-
-    # Should the comment be shown in full?
-    full_comment = True
-    # Show comment expanders?
-    show_expanders = False
+        return super(CodeReviewCommentIndexView, self).__call__()
 
 
 class CodeReviewCommentSummary(CodeReviewCommentView):

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2012-02-01 20:03:27 +0000
+++ lib/lp/code/browser/configure.zcml	2012-02-01 20:03:27 +0000
@@ -645,9 +645,6 @@
         class="lp.code.browser.codereviewcomment.CodeReviewCommentView"
         permission="zope.Public">
         <browser:page
-            name="+index"
-            template="../templates/codereviewcomment-index.pt"/>
-        <browser:page
             name="+fragment"
             template="../templates/codereviewcomment-fragment.pt"/>
         <browser:page
@@ -655,6 +652,13 @@
             attribute="download"
         />
     </browser:pages>
+    <browser:page
+        for="lp.code.interfaces.codereviewcomment.ICodeReviewComment"
+        name="+index"
+        template="../templates/codereviewcomment-index.pt"
+        class="lp.code.browser.codereviewcomment.CodeReviewCommentIndexView"
+        permission="zope.Public"
+        />
     <browser:pages
         for="lp.code.browser.codereviewcomment.ICodeReviewDisplayComment"
         layer="lp.code.publisher.CodeLayer"

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-02-01 20:03:27 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2012-02-01 20:03:27 +0000
@@ -1120,15 +1120,19 @@
         browser = self.getViewBrowser(comment.branch_merge_proposal)
         self.assertIn('x y' * 100, browser.contents)
 
-    def test_long_conversation_comments_truncated(self):
-        """Long comments in a conversation should be truncated."""
-        comment = self.factory.makeCodeReviewComment(body='x y' * 2000)
+    def has_read_more(self, comment):
         url = canonical_url(comment, force_local_path=True)
-        browser = self.getViewBrowser(comment.branch_merge_proposal)
-        self.assertNotIn('x y' * 2000, browser.contents)
         read_more = Tag(
             'Read more link', 'a', {'href': url}, text='Read more...')
-        self.assertThat(browser.contents, HTMLContains(read_more))
+        return HTMLContains(read_more)
+
+    def test_long_conversation_comments_truncated(self):
+        """Long comments in a conversation should be truncated."""
+        comment = self.factory.makeCodeReviewComment(body='x y' * 2000)
+        has_read_more = self.has_read_more(comment)
+        browser = self.getViewBrowser(comment.branch_merge_proposal)
+        self.assertNotIn('x y' * 2000, browser.contents)
+        self.assertThat(browser.contents, has_read_more)
 
     def test_short_conversation_comments_no_download(self):
         """Short comments should not have a download link."""
@@ -1150,6 +1154,15 @@
             text='Download full text')
         self.assertThat(browser.contents, HTMLContains(body))
 
+    def test_excessive_conversation_comments_no_redirect(self):
+        """An excessive comment does not force a redict on proposal page."""
+        comment = self.factory.makeCodeReviewComment(body='x' * 10001)
+        mp_url = canonical_url(comment.branch_merge_proposal)
+        has_read_more = self.has_read_more(comment)
+        browser = self.getUserBrowser(mp_url)
+        self.assertThat(browser.contents, Not(has_read_more))
+        self.assertEqual(mp_url, browser.url)
+
 
 class TestLatestProposalsForEachBranch(TestCaseWithFactory):
     """Confirm that the latest branch is returned."""