Merge lp:~bac/launchpad/bug-639703-pg-bugs into lp:launchpad

Screenshot at http://people.canonical.com/~bac/pg-no-buggy.png

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

Very happy to see this change, thanks Brad.

I'd like to suggest some alternative text:

  Sorry, you can't report a bug for the $ProjectGroup project group. None of the
  projects within the group use Launchpad for bug tracking. Please check the
  $ProjectGroup website for details of where they track bug reports.

I have suggested this change because I think the most important thing about the message is that you can't report the bug you wanted to. Only then should we explain why. I think we should then give the person a next step ... ideally we should link to their external bug tracker, or as a second-best we should link to the project's website. If we can't do either, I think the above text is a nod towards being helpful.

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

Thanks for the text changes Matthew. On IRC we agreed on:

            You cannot report a bug for <tal:name replace="name"/> as none of
            the projects within the group use Launchpad for bug tracking.
            Please check the individual projects for details
            of where bugs are reported.

So, the code is good now. Thanks for adding the XXX and filing the bug.

UI-wise I find the page too empty and inconsistent with similar pages we have on project groups and projects. Please have a look here:
http://people.canonical.com/~henninge/screenshots/unconfigured_blueprints_pg.png
http://people.canonical.com/~henninge/screenshots/unconfigured_bugtracker.png

The page will at least need a page heading and bread crumbs. The text should most likely be <strong> like on the other pages to indicate that it needs attention. Add a help link for bonus points. ;-)

Thanks

approved code
needs-fixing ui*

Hi Henning,

Thanks for the UI review. I have added the page heading and breadcrumbs:

http://people.canonical.com/~bac/pg-bugs-no-projects.png
http://people.canonical.com/~bac/pg-bugs-mit-projects.png

I have not yet added a help link but will do so, pending Curtis' UI mentoring review.

Thanks Brad and Henning. This is a very nice fix and I am very pleased we fixed some 3.0-ism too. As Henning pointed, we want a consistent experience:

The page should show the owner the same help link as is shown on projects:
    (i) Getting started with bug tracking in Launchpad.
    <https://help.launchpad.net/Bugs>

There is a secondary issue that help does not explain exactly what bug tracking features project groups provide. Actually, Lp has a poor understanding of what PGs provide. I think PG documentation is a secondary issue. We will discuss PGs over the next few months and decide their fate.

All good now, thanks for improving this! ;-)

Looks like this is all reviewed, so abstaining on behalf of Launchpad code reviewers.