launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

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.

Diff comments:

> === 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 13:20:38 +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]

This would be more readable if wrapped just before the "if".

>  
>      return visible_comments
>  
> 
> === 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 13:20:38 +0000
> @@ -58,6 +58,12 @@
>          view = self.getView(context=context)
>          self.assertNotIn(self.html_comment_text, view.contents)
>  
> +    def test_comment_owner_could_see_hidden_comment(self):

"can" rather than "could", please, to match other nearby tests and because neither the simple past nor the conditional makes sense here.

> +        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:
>  


-- 
https://code.launchpad.net/~rpadovani/launchpad/unhide-comments/+merge/270950
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.