Code review comment for lp:~bac/launchpad/bug-639703-pg-bugs

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