Merge lp:~jcsackett/launchpad/hidden-comment-count-error into lp:launchpad

[j.c.sackett]

Fixes calculation of the number of hidden comments in the collapsed comment widget.

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

[Richard Harding (rharding)]

Works for me so long as the log files goes away. Thanks for the fix.

review: Approve

=== 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 16:06:21 +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 16:06:21 +0000
@@ -1757,6 +1757,66 @@
             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
+        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 +1935,7 @@
             unbatched_view.activity_and_comments[4:],
             batched_view.activity_and_comments)
 
+
 no_target_specified = object()