launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Review: Needs Fixing code
Hello Bac,
thanks for making Launchpad a bit more user-friendly! ;-) I still need to send
you on another round-trip, though. Please find my comments below.

Henning

Am 04.10.2010 15:44, schrieb Brad Crittenden:
> === modified file 'lib/lp/bugs/browser/bugtask.py'
> --- lib/lp/bugs/browser/bugtask.py	2010-09-28 14:50:19 +0000
> +++ lib/lp/bugs/browser/bugtask.py	2010-10-04 12:55:59 +0000
> @@ -168,6 +168,7 @@
>      ObjectImageDisplayAPI,
>      PersonFormatterAPI,
>      )
> +
>  from canonical.lazr.interfaces import IObjectPrivacy
>  from canonical.lazr.utils import smartquote
>  from canonical.widgets.bug import BugTagsWidget
> @@ -2987,8 +2988,17 @@
>              return False
>
>      @property
> +    def should_show_bug_information(self):
> +        """Return False if a project group that does not use Launchpad."""
> +        if not self._projectContext():
> +            return True
> +        involvement = getMultiAdapter((self.context, self.request),
> +                                      name="+get-involved")
> +        return involvement.official_malone

This seems odd, to create a view just to get a property that is really a
property of the context. The "official_malone" property should be defined in
ProjectGroup and could be implemented like this: ;-)

    @cachedproperty
    def offcial_malone(self):
        return any([product.offcial_malone for product in self.products])

Unless I misunderstood something, I see this misuse of a view as a real
blocker for landing this. This earned you the "need-fixing" ;-)

> +
> +    @property
>      def form_has_errors(self):
> -        """Return True of the form has errors, otherwise False."""
> +        """Return True if the form has errors, otherwise False."""

Good catch! ;)

>          return len(self.errors) > 0
>
>      def validateVocabulariesAdvancedForm(self):
>
> === modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
> --- lib/lp/bugs/browser/tests/test_bugtask.py	2010-09-23 14:51:48 +0000
> +++ lib/lp/bugs/browser/tests/test_bugtask.py	2010-10-04 12:55:59 +0000

[...]

> @@ -418,6 +424,88 @@
>              view.form_fields['assignee'].field.vocabularyName)
>
>
> +class TestProjectGroupBugs(TestCaseWithFactory):
> +    """Test the bugs overview page for Project Groups."""
> +
> +    layer = LaunchpadFunctionalLayer
> +
> +    def setUp(self):
> +        super(TestProjectGroupBugs, self).setUp()
> +        self.owner = self.factory.makePerson(name='bob')
> +        self.projectgroup = self.factory.makeProject(name='container',
> +                                                     owner=self.owner)
> +
> +    def makeSubordinateProduct(self, tracks_bugs_in_lp):
> +        """Create a new product and add it to the project group."""
> +        product = self.factory.makeProduct(official_malone=tracks_bugs_in_lp)
> +        with person_logged_in(product.owner):
> +            product.project = self.projectgroup
> +        expected = {True: 'LAUNCHPAD',
> +                    False: 'UNKNOWN',
> +                    }
> +        self.assertEqual(
> +            expected[tracks_bugs_in_lp], product.bug_tracking_usage.name)

This looks like a forgotten sanity check? If you wish to keep it in here you
should give it its own tests and mention in the comment their sanity nature.

> +
> +    def test_empty_project_group(self):

This one and the next three tests belong in the model tests for
ProjectGroup.official_malone.

> +        # An empty project group does not use Launchpad for bugs.
> +        view = create_initialized_view(
> +            self.projectgroup, name=u'+bugs', rootsite='bugs')
> +        self.assertFalse(self.projectgroup.hasProducts())
> +        self.assertFalse(view.should_show_bug_information)
> +
> +    def test_project_group_with_subordinate_not_using_launchpad(self):
> +        # A project group with all subordinates not using Launchpad
> +        # will itself be marked as not using Launchpad for bugs.
> +        self.makeSubordinateProduct(False)
> +        self.assertTrue(self.projectgroup.hasProducts())
> +        view = create_initialized_view(
> +            self.projectgroup, name=u'+bugs', rootsite='bugs')
> +        self.assertFalse(view.should_show_bug_information)
> +
> +    def test_project_group_with_subordinate_using_launchpad(self):
> +        # A project group with one subordinate using Launchpad
> +        # will itself be marked as using Launchpad for bugs.
> +        self.makeSubordinateProduct(True)
> +        self.assertTrue(self.projectgroup.hasProducts())
> +        view = create_initialized_view(
> +            self.projectgroup, name=u'+bugs', rootsite='bugs')
> +        self.assertTrue(view.should_show_bug_information)
> +
> +    def test_project_group_with_mixed_subordinates(self):

You could keep this one in here to show that view.should_show_bug_information
really reflects ProjectGroup.official_malone

> +        # A project group with one or more subordinates using Launchpad
> +        # will itself be marked as using Launchpad for bugs.
> +        self.makeSubordinateProduct(False)
> +        self.makeSubordinateProduct(True)
> +        self.assertTrue(self.projectgroup.hasProducts())
> +        view = create_initialized_view(
> +            self.projectgroup, name=u'+bugs', rootsite='bugs')
> +        self.assertTrue(view.should_show_bug_information)
> +
> +    def test_project_group_has_no_portlets_if_not_using_LP(self):
> +        # A project group that has no projects using Launchpad will not have a
> +        # 'Report a bug' link.
> +        self.makeSubordinateProduct(False)
> +        view = create_initialized_view(
> +            self.projectgroup, name=u'+bugs', rootsite='bugs',
> +            current_request=True)
> +        self.assertFalse(view.should_show_bug_information)
> +        contents = view.render()
> +        report_a_bug = find_tag_by_id(contents, 'bug-portlets')
> +        self.assertIs(None, report_a_bug)
> +
> +    def test_project_group_has_portlets_link_if_using_LP(self):
> +        # A project group that has no projects using Launchpad will not have a
> +        # 'Report a bug' link.

Copy & paste error ;-) The comment needs negation ...

> +        self.makeSubordinateProduct(True)
> +        view = create_initialized_view(
> +            self.projectgroup, name=u'+bugs', rootsite='bugs',
> +            current_request=True)
> +        self.assertTrue(view.should_show_bug_information)
> +        contents = view.render()
> +        report_a_bug = find_tag_by_id(contents, 'bug-portlets')
> +        self.assertIsNot(None, report_a_bug)
> +
> +
>  def test_suite():
>      suite = unittest.TestLoader().loadTestsFromName(__name__)
>      suite.addTest(DocTestSuite(bugtask))
>
> === modified file 'lib/lp/bugs/templates/buglisting-default.pt'

This looks OK.

[...]
> === modified file 'lib/lp/testing/views.py'
> --- lib/lp/testing/views.py	2010-09-19 03:09:49 +0000
> +++ lib/lp/testing/views.py	2010-10-04 12:55:59 +0000
> @@ -85,7 +85,8 @@
>  def create_initialized_view(context, name, form=None, layer=None,
>                              server_url=None, method=None, principal=None,
>                              query_string=None, cookie=None, request=None,
> -                            path_info='/', rootsite=None):
> +                            path_info='/', rootsite=None,
> +                            current_request=False):

Thanks for improving our testing helpers. ;-)

>      """Return a view that has already been initialized."""
>      if method is None:
>          if form is None:
> @@ -94,7 +95,8 @@
>              method = 'POST'
>      view = create_view(
>          context, name, form, layer, server_url, method, principal,
> -        query_string, cookie, request, path_info, rootsite=rootsite)
> +        query_string, cookie, request, path_info, rootsite=rootsite,
> +        current_request=current_request)
>      view.initialize()
>      return view
>
>

-- 
https://code.launchpad.net/~bac/launchpad/bug-639703-pg-bugs/+merge/37463
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-639703-pg-bugs into lp:launchpad/devel.