Merge lp:~rpadovani/launchpad/unhide-comments into lp:launchpad

[Riccardo Padovani]

Show hidden bug comments to their owners.

Summary

Fix bug #1391394, impossible to unhide my comment

Proposed fix

Now owner of comments could see the comment also if it's hidden

Tests

I'm a bit confused about how implementing the test.
I see there is a TestBugCommentVisibility class in bugs/browser/tests/test_bugcomment.py, and there is a method called makeHiddenMessage().

But, as far as I understand, the message created here is owned by admin.

So I think I need to create a method that creates a comment with a non-admin account, then I need to set visibility of the comment to false (comment.visible = False could work?) and then test if the comment is visible to the owner and then test if it's hide to a normal user?

Could work as approach?

[Colin Watson (cjwatson)]

The fact that you're modifying something in lib/lp/coop/answersbugs/ should be a clue that you need to go and look in lib/lp/answers/ for users of the same test. In this case your change is going to make TestQuestionMessageVisibility fail. As it happens, questions already behave the right way here, and there's a specific test for that. I would suggest removing TestQuestionMessageVisibility.test_commenter_can_see_comments in favour of your coop test, and adjusting TestQuestionMessageVisibility.makeHiddenMessage to match, something like this:

    if comment_owner is None:
        comment_owner = self.factory.makePerson()
    with celebrity_logged_in('admin'):
        question = self.factory.makeQuestion()
        comment = question.addComment(comment_owner, self.comment_text)
        removeSecurityProxy(comment).message.visible = False
    return question

The other thing you need to do is to do something about the presentation. Users should see their self-hidden comments in the same way that admins do, which requires having the adminHiddenContent class set (slightly inaccurate name, but no matter). That will require changing BugComment.show_for_admin; you probably want to change BugComment.__init__ to save user_owns_comment as an instance attribute, and then you can test that in show_for_admin. As far as testing for that goes, you're probably best off editing the existing pagetest in lib/lp/bugs/stories/bugs/xx-bug-hidden-comments.txt, which is the only place that tests the presence of the adminHiddenComment class at the moment.

review: Needs Fixing

[Colin Watson (cjwatson)]

review: Approve

=== modified file 'lib/lp/answers/browser/tests/test_questionmessages.py'
--- lib/lp/answers/browser/tests/test_questionmessages.py	2012-12-11 04:08:19 +0000
+++ lib/lp/answers/browser/tests/test_questionmessages.py	2015-09-14 16:38:44 +0000
@@ -26,13 +26,14 @@
 
     layer = DatabaseFunctionalLayer
 
-    def makeHiddenMessage(self):
+    def makeHiddenMessage(self, comment_owner=None):
         """Required by the mixin."""
         administrator = getUtility(ILaunchpadCelebrities).admin.teamowner
-        self.commenter = self.factory.makePerson()
+        if comment_owner is None:
+            comment_owner = self.factory.makePerson()
         with person_logged_in(administrator):
             question = self.factory.makeQuestion()
-            comment = question.addComment(self.commenter, self.comment_text)
+            comment = question.addComment(comment_owner, self.comment_text)
             removeSecurityProxy(comment).message.visible = False
         return question
 
@@ -44,12 +45,6 @@
             no_login=no_login)
         return view
 
-    def test_commenter_can_see_comments(self):
-        # The author of the comment can see the hidden comment.
-        context = self.makeHiddenMessage()
-        view = self.getView(context=context, user=self.commenter)
-        self.assertIn(self.html_comment_text, view.contents)
-
 
 class TestHideQuestionMessageControls(
         BrowserTestCase, TestHideMessageControlMixin):
 
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2015-08-11 00:54:41 +0000
+++ lib/lp/bugs/browser/bugtask.py	2015-09-14 16:38:44 +0000
@@ -295,7 +295,8 @@
         role = PersonRoles(user)
         strip_invisible = not (role.in_admin or role.in_registry_experts)
     if strip_invisible:
-        visible_comments = [c for c in visible_comments if c.visible]
+        visible_comments = [c for c in visible_comments
+                            if c.visible or c.owner == user]
 
     return visible_comments
 
 
=== modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py'
--- lib/lp/bugs/browser/tests/test_bugcomment.py	2012-12-26 01:32:19 +0000
+++ lib/lp/bugs/browser/tests/test_bugcomment.py	2015-09-14 16:38:44 +0000
@@ -211,12 +211,12 @@
 
     layer = DatabaseFunctionalLayer
 
-    def makeHiddenMessage(self):
+    def makeHiddenMessage(self, comment_owner=None):
         """Required by the mixin."""
         with celebrity_logged_in('admin'):
             bug = self.factory.makeBug()
             comment = self.factory.makeBugComment(
-                    bug=bug, body=self.comment_text)
+                    bug=bug, body=self.comment_text, owner=comment_owner)
             comment.visible = False
         return bug
 
 
=== modified file 'lib/lp/bugs/stories/bugs/xx-bug-hidden-comments.txt'
--- lib/lp/bugs/stories/bugs/xx-bug-hidden-comments.txt	2011-03-10 20:32:56 +0000
+++ lib/lp/bugs/stories/bugs/xx-bug-hidden-comments.txt	2015-09-14 16:38:44 +0000
@@ -37,10 +37,11 @@
 For ordinary users, the newly created message no longer appears
 in the list once visible has been set False.
 
-    >>> user_browser.open(
+    >>> test_browser = setupBrowser(auth="Basic [e-mail]:test")
+    >>> test_browser.open(
     ...     'http://bugs.launchpad.dev'
     ...     '/jokosher/+bug/11')
-    >>> main_content = find_main_content(user_browser.contents)
+    >>> main_content = find_main_content(test_browser.contents)
     >>> last_comment = main_content('div', 'boardCommentBody')[-1]
     >>> last_comment_text = extract_text(last_comment.div)
     >>> print last_comment_text
@@ -50,11 +51,11 @@
 Ordinary users also cannot reach the message via direct link.
 
     >>> comments = find_tags_by_class(
-    ...     user_browser.contents, 'boardComment')
+    ...     test_browser.contents, 'boardComment')
     >>> for comment in comments:
     ...     number_node = comment.find(None, 'bug-comment-index')
     ...     latest_index = extract_text(number_node)
-    >>> user_browser.open(
+    >>> test_browser.open(
     ...     'http://bugs.launchpad.dev'
     ...     '/jokosher/+bug/11/comments/%d' % (int(latest_index[1:]) + 1))
     Traceback (most recent call last):
@@ -94,3 +95,37 @@
     >>> last_comment_text = extract_text(last_comment.div)
     >>> print last_comment.parent['class']
     boardComment adminHiddenComment
+
+Also for the owner of comment the message is still visible in the bug page.
+
+    >>> user_browser.open(
+    ...     'http://bugs.launchpad.dev'
+    ...     '/jokosher/+bug/11')
+    >>> main_content = find_main_content(user_browser.contents)
+    >>> last_comment = main_content('div', 'boardCommentBody')[-1]
+    >>> last_comment_text = extract_text(last_comment.div)
+    >>> print last_comment_text
+    This comment will not be visible when the test completes.
+
+Owner of the comment will see the hidden message highlighted with an
+'adminHiddenComment' style.
+
+    >>> print last_comment.parent['class']
+    boardComment adminHiddenComment
+
+Owner of the comment can also reach the message via direct link, and it is
+highlighted with the 'adminHiddenComment style there too.
+
+    >>> user_browser.open(
+    ...     'http://bugs.launchpad.dev'
+    ...     '/jokosher/+bug/11/comments/%d' % (int(latest_index[1:]) + 1))
+    >>> contents = extract_text(user_browser.contents)
+    >>> print contents
+    Comment #7 : Bug #11 ...
+    This comment will not be visible when the test completes.
+    ...
+    >>> main_content = find_main_content(user_browser.contents)
+    >>> last_comment = main_content('div', 'boardCommentBody')[-1]
+    >>> last_comment_text = extract_text(last_comment.div)
+    >>> print last_comment.parent['class']
+    boardComment adminHiddenComment
 
=== modified file 'lib/lp/coop/answersbugs/visibility.py'
--- lib/lp/coop/answersbugs/visibility.py	2012-12-11 04:08:19 +0000
+++ lib/lp/coop/answersbugs/visibility.py	2015-09-14 16:38:44 +0000
@@ -58,6 +58,12 @@
         view = self.getView(context=context)
         self.assertNotIn(self.html_comment_text, view.contents)
 
+    def test_comment_owner_can_see_hidden_comment(self):
+        owner = self.factory.makePerson()
+        context = self.makeHiddenMessage(comment_owner=owner)
+        view = self.getView(context=context, user=owner)
+        self.assertIn(self.html_comment_text, view.contents)
+
 
 class TestHideMessageControlMixin: