launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/hidden-comment-count-error into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #818456 in Launchpad itself: "Hidden comments expander fails with out-of-order emails ("-1 comments hidden")"
  https://bugs.launchpad.net/launchpad/+bug/818456
  Bug #867593 in Launchpad itself: "Displayed number of comments hidden is sometimes +1 to the actual value"
  https://bugs.launchpad.net/launchpad/+bug/867593

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/hidden-comment-count-error/+merge/114219

Summary
=======
There are two problems with the current hidden comment/activity collapse
display; it can claim there are more hidden comments are hidden then there
are, and it can claim negative comments. Both of these have to do with the
arrangement of comment indices.

This fixes up some math in the figuring of hidden comments and adds tests to
the display of the collapsed comments.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
The original calculation was broken by design. When a break in visible
comments was detected, it determined the number missing was the difference
between the previous visible comment's index and the current one. But:

1) indices can occasionally be in reverse order, owing to mail messages.
2) The number of items between two items isn't the difference of their
indices. It's one less than that difference. (There is one number between 1
and 3, not 2).

To correct these, we can just get one less than the absolute difference
between the two visible comments.

Tests
=====
bin/test -vvct TestCommentCollapseVisibility

QA
==
Go play with hiding some comments and make sure the numbers are correct on
page load without all comments showing.

LoC
===
This is part of the already resourced disclore arc. Plus, after this we can
remove some feature flags which reduces LoC.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/tests/test_bugtask.py
  lib/lp/bugs/browser/bugtask.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/hidden-comment-count-error/+merge/114219
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/hidden-comment-count-error into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-07-04 11:07:57 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-07-10 15:47:27 +0000
@@ -945,9 +945,14 @@
                     continue
                 if prev_comment.index + 1 != comment.index:
                     # There is a gap here, record it.
+
+                    # The number of items between two items is one less than
+                    # their difference. There is one number between 1 and 3,
+                    # not 2 (their difference).
+                    num_hidden = abs(comment.index - prev_comment.index) - 1
                     separator = {
                         'date': prev_comment.datecreated,
-                        'num_hidden': comment.index - prev_comment.index,
+                        'num_hidden': num_hidden,
                         }
                     events.insert(index, separator)
                     index += 1

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-07-04 11:07:57 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-07-10 15:47:27 +0000
@@ -1757,6 +1757,68 @@
             BugActivityItem(bug.activity[-1]).change_details)
 
 
+class TestCommentCollapseVisibility(TestCaseWithFactory):
+    """Test for the conditions around display of collapsed/hidden comments."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def makeBugWithComments(self, num_comments):
+        """Create and return a bug with a lot of comments and activity."""
+        bug = self.factory.makeBug()
+        with person_logged_in(bug.owner):
+            for i in range(num_comments):
+                msg = self.factory.makeMessage(
+                    owner=bug.owner, content="Message %i." % i)
+                bug.linkMessage(msg, user=bug.owner)
+        return bug
+
+    def test_comments_hidden_message_truncation_only(self):
+        bug = self.makeBugWithComments(20)
+        url = canonical_url(bug.default_bugtask)
+        browser = self.getUserBrowser(url=url)
+        contents = browser.contents
+        self.assertTrue("10 comments hidden" in contents)
+        self.assertEqual(1, contents.count('comments hidden'))
+
+    def test_comments_hidden_message_truncation_and_hidden(self):
+        bug = self.makeBugWithComments(20)
+        url = canonical_url(bug.default_bugtask)
+
+        #Hide a comment
+        comments = list(bug.messages)
+        removeSecurityProxy(comments[-5]).visible = False
+
+        browser = self.getUserBrowser(url=url)
+        contents = browser.contents
+        self.assertTrue("10 comments hidden" in browser.contents)
+        self.assertTrue("1 comments hidden" in browser.contents)
+        self.assertEqual(2, contents.count('comments hidden'))
+
+    def test_comments_hidden_message_truncation_and_hidden_out_of_order(self):
+        bug = self.makeBugWithComments(20)
+        url = canonical_url(bug.default_bugtask)
+
+        #Hide a comment
+        comments = list(bug.messages)
+        hidden_comment = comments[-5]
+        removeSecurityProxy(hidden_comment).visible = False
+
+        #Mess with ordering. This requires a transaction since the view will
+        #re-fetch the comments.
+        last_comment = comments[-1]
+        removeSecurityProxy(hidden_comment).datecreated += timedelta(1)
+        removeSecurityProxy(last_comment).datecreated += timedelta(2)
+        transaction.commit()
+
+        browser = self.getUserBrowser(url=url)
+        contents = browser.contents
+        with file('/home/jc/wtf.html', 'w') as log:
+            log.write(contents)
+        self.assertTrue("10 comments hidden" in browser.contents)
+        self.assertTrue("1 comments hidden" in browser.contents)
+        self.assertEqual(2, contents.count('comments hidden'))
+
+
 class TestBugTaskBatchedCommentsAndActivityView(TestCaseWithFactory):
     """Tests for the BugTaskBatchedCommentsAndActivityView class."""
 
@@ -1875,6 +1937,7 @@
             unbatched_view.activity_and_comments[4:],
             batched_view.activity_and_comments)
 
+
 no_target_specified = object()