On Oct 4, 2010, at 22:39 , Henning Eggers wrote:

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

Thanks for the review Henning.

> Henning
> 
> Am 04.10.2010 15:44, schrieb Brad Crittenden:
>> === modified file 'lib/lp/bugs/browser/bugtask.py'
>> 
>>     @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" ;-)

It does look odd, Henning.  For a lot of pillars, including ProjectGroup, the official usage flags are set in PillarView.  The pattern you see here is repeated elsewhere.

On IRC we discussed landing this branch and later looking at moving the settings to the appropriate models.  The bug for that work is bug 655036.
> 
>> @@ -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.

Yes, it serves no purpose and is removed.

> 
>> +
>> +    def test_empty_project_group(self):
> 
> This one and the next three tests belong in the model tests for
> ProjectGroup.official_malone.

Deferred.

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

Fixedc/launchpad/bug-639703-pg-bugs/+merge/37463
> You are the owner of lp:~bac/launchpad/bug-639703-pg-bugs.