launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05803
[Merge] lp:~sinzui/launchpad/bug-supervisor-labels into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/bug-supervisor-labels into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/bug-supervisor-labels/+merge/84343
Fix the structural subscription label wrongly described as 'bug supervisor'.
Launchpad bug:
https://bugs.launchpad.net/bugs/262898
https://bugs.launchpad.net/bugs/503682
https://bugs.launchpad.net/bugs/562001
Pre-implementation: gmb
While at UDS-P, the launchpad team was puzzled by the repeated stories
of how Ubuntu and other project locate bugs they are interested. Users
create structural subscription, but then search for bug supervisor.
We know that series and packages do not have bug supervisors so the
search should not succeed. The search does work because of a mis-labeling
of fields in advanced search; bug_supervisor searches structural
subscriptions.
--------------------------------------------------------------------
RULES
My initial thought was to add a structural_subscriber field and
update search to fallback to bug_supervisor to support older API.
I discovered structural_subscriber already exists and it addresses
another bug that is deeper than labels -- Bug #562001.
The bug_supervisor field has a lot of problems:
1. Searching for just bug_supervisor in a bug target will return all or
no bugs. the field only makes sense in the context of a user or
team who might work on many projects.
2. Most projects do not explicitly set the field. The default bug_supervisor
is the maintainer, but that field is not searched.
This is why the field falls back to structural subscriptions.
I decided to update the advanced search for to use the structural_subscriber
field instead, and update the form label to clearly state what is being
searched.
* Update the search schema to use structural_subscriber.
* Update the view to setup the structural_subscriber
* Correct the labels in advanced bug search template.
* Do not show the field when the target is a package or project series
because a value leads to all or none results.
* Fix the title/alt text of links
QA
Bug #503682 Show package report link should say "subscriber" not "supervisor"
* Visit https://bugs.qastaging.launchpad.net/~sinzui
* Hover over the Subscribed packages link to show the tooltip.
* Verify ends with "is a subscriber"
Bug #262898 Inconsistent and incorrect labeling of advanced search page
* Subscribe to a package if you are not already.
https://bugs.qastaging.launchpad.net/ubuntu/+source/pocket-lint
and use the subscribe to bug mail link.
* Visit https://bugs.qastaging.launchpad.net/ubuntu/+bugs?advanced=1
* Verify the first fields in the People section are:
Assignee, Reporter, Commenter, Subscriber,
Package, or series subscriber
* Enter your name in the structural subscription field and search
* Verify the search returns matching bugs.
* Visit https://bugs.qastaging.launchpad.net/ubuntu/precise/+bugs?advanced=1
* Verify that the structural subscriber label is
Package subscriber
* Visit https://bugs.qastaging.launchpad.net/launchpad/+bugs?advanced=1
* Verify that the structural subscriber label is
Series subscriber
* Visit https://bugs.qastaging.launchpad.net/launchpad-project/+bugs?advanced=1
* Verify that the structural subscriber label is
Project or series subscriber
* Visit https://bugs.qastaging.launchpad.net/~sinzui/+bugs?advanced=1
* Verify that the structural subscriber label is
Project, distribution, package, or series subscriber
* Visit https://bugs.qastaging.launchpad.net/ubuntu/+source/pocket-lint+bugs?advanced=1
* Verify the structural subscriber field is not shown
* Visit https://bugs.qastaging.launchpad.net/ubuntu/precise/+source/pocket-lint+bugs?advanced=1
* Verify the structural subscriber field is not shown
* Visit https://bugs.qastaging.launchpad.net/launchpad/trunk?advanced=1
* Verify the structural subscriber field is not shown
Bug #562001 Series structural subscriptions advanced search
* Visit https://bugs.qastaging.launchpad.net/ubuntu/oneiric
* Subscribe to the series
* Visit https://bugs.qastaging.launchpad.net/ubuntu/oneiric/+bugs?advanced=1
* Enter your name in the structural subscription field and search
* Verify the result contains matches.
LINT
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/tests/test_bugtask.py
lib/lp/bugs/interfaces/bugtask.py
lib/lp/bugs/stories/bugs/xx-bugs-advanced-search-upstream-status.txt
lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt
lib/lp/bugs/templates/bugtask-macros-tableview.pt
lib/lp/registry/browser/person.py
^ Lint does not like the two stories I can clean this up after the review.
TEST
./bin/test -vvc -t uctural lp.bugs.browser.tests.test_bugtask
./bin/test -vvc -t advanced-search -t advanced-peop lp.bugs.tests.test_doc
IMPLEMENTATION
Updated the advanced search interface to`use the structural_subscriber
field instead instead of the bug_supervisor field.
lib/lp/bugs/interfaces/bugtask.py
lib/lp/bugs/stories/bugs/xx-bugs-advanced-search-upstream-status.txt
Updated the base search view to use the structural_subscriber field.
Updated the base search view to make an informative field label for the
page and use that label to determine if the widget should be shown.
Updated the template to use the new label and fixed the inconsistent labels.
Removed a error test from xx-advanced-people-filters because it is already
tested is a unittest.
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/tests/test_bugtask.py
lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt
lib/lp/bugs/templates/bugtask-macros-tableview.pt
Fix the link wording summary shown in the tooltip.
lib/lp/registry/browser/person.py
--
https://code.launchpad.net/~sinzui/launchpad/bug-supervisor-labels/+merge/84343
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/bug-supervisor-labels into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-11-28 19:20:17 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-12-02 22:29:33 +0000
@@ -2524,7 +2524,7 @@
custom_widget('assignee', PersonPickerWidget)
custom_widget('bug_reporter', PersonPickerWidget)
custom_widget('bug_commenter', PersonPickerWidget)
- custom_widget('bug_supervisor', PersonPickerWidget)
+ custom_widget('structural_subscriber', PersonPickerWidget)
custom_widget('subscriber', PersonPickerWidget)
@cachedproperty
@@ -3049,11 +3049,12 @@
IDistroSeries.providedBy(context) or
ISourcePackage.providedBy(context))
- def shouldShowSupervisorWidget(self):
- """
- Should the bug supervisor widget be shown on the advanced search page?
- """
- return True
+ def shouldShowStructuralSubscriberWidget(self):
+ """Should the structural subscriber widget be shown on the page?
+
+ Show the widget when there are subordinate structures.
+ """
+ return self.structural_subscriber_label is not None
def shouldShowNoPackageWidget(self):
"""Should the widget to filter on bugs with no package be shown?
@@ -3096,6 +3097,21 @@
"""Should the User's Teams portlet me shown in the results?"""
return False
+ @property
+ def structural_subscriber_label(self):
+ if IDistribution.providedBy(self.context):
+ return 'Package, or series subscriber'
+ elif IDistroSeries.providedBy(self.context):
+ return 'Package subscriber'
+ elif IProduct.providedBy(self.context):
+ return 'Series subscriber'
+ elif IProjectGroup.providedBy(self.context):
+ return 'Project or series subscriber'
+ elif IPerson.providedBy(self.context):
+ return 'Project, distribution, package, or series subscriber'
+ else:
+ return None
+
def getSortLink(self, colname):
"""Return a link that can be used to sort results by colname."""
form = self.request.form
@@ -3198,7 +3214,7 @@
error_message = _(
"There's no person with the name or email address '%s'.")
- for name in ('assignee', 'bug_reporter', 'bug_supervisor',
+ for name in ('assignee', 'bug_reporter', 'structural_subscriber',
'bug_commenter', 'subscriber'):
if self.getFieldError(name):
self.setFieldError(
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-11-30 16:53:01 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-12-02 22:29:33 +0000
@@ -1436,8 +1436,138 @@
self.assertTrue(notifications.pop().message.startswith(expected))
+class TestPersonBugs(TestCaseWithFactory):
+ """Test the bugs overview page for distributions."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestPersonBugs, self).setUp()
+ self.target = self.factory.makePerson()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+
+ def test_structural_subscriber_label(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertEqual(
+ 'Project, distribution, package, or series subscriber',
+ view.structural_subscriber_label)
+
+
+class TestDistributionBugs(TestCaseWithFactory):
+ """Test the bugs overview page for distributions."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistributionBugs, self).setUp()
+ self.target = self.factory.makeDistribution()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+
+ def test_structural_subscriber_label(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertEqual(
+ 'Package, or series subscriber', view.structural_subscriber_label)
+
+
+class TestDistroSeriesBugs(TestCaseWithFactory):
+ """Test the bugs overview page for distro series."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistroSeriesBugs, self).setUp()
+ self.target = self.factory.makeDistroSeries()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+
+ def test_structural_subscriber_label(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertEqual(
+ 'Package subscriber', view.structural_subscriber_label)
+
+
+class TestDistributionSourcePackageBugs(TestCaseWithFactory):
+ """Test the bugs overview page for distribution source packages."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistributionSourcePackageBugs, self).setUp()
+ self.target = self.factory.makeDistributionSourcePackage()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertFalse(view.shouldShowStructuralSubscriberWidget())
+
+
+class TestDistroSeriesSourcePackageBugs(TestCaseWithFactory):
+ """Test the bugs overview page for distro series source packages."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestDistroSeriesSourcePackageBugs, self).setUp()
+ self.target = self.factory.makeSourcePackage()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertFalse(view.shouldShowStructuralSubscriberWidget())
+
+
+class TestProductBugs(TestCaseWithFactory):
+ """Test the bugs overview page for projects."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProductBugs, self).setUp()
+ self.target = self.factory.makeProduct()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+
+ def test_structural_subscriber_label(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertEqual(
+ 'Series subscriber', view.structural_subscriber_label)
+
+
+class TestProductSeriesBugs(TestCaseWithFactory):
+ """Test the bugs overview page for project series."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestProductSeriesBugs, self).setUp()
+ self.target = self.factory.makeProductSeries()
+
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.target, name=u'+bugs', rootsite='bugs')
+ self.assertFalse(view.shouldShowStructuralSubscriberWidget())
+
+
class TestProjectGroupBugs(TestCaseWithFactory):
- """Test the bugs overview page for Project Groups."""
+ """Test the bugs overview page for project groups."""
layer = DatabaseFunctionalLayer
@@ -1534,6 +1664,17 @@
help_link = find_tag_by_id(contents, 'getting-started-help')
self.assertIs(None, help_link)
+ def test_shouldShowStructuralSubscriberWidget(self):
+ view = create_initialized_view(
+ self.projectgroup, name=u'+bugs', rootsite='bugs')
+ self.assertTrue(view.shouldShowStructuralSubscriberWidget())
+
+ def test_structural_subscriber_label(self):
+ view = create_initialized_view(
+ self.projectgroup, name=u'+bugs', rootsite='bugs')
+ self.assertEqual(
+ 'Project or series subscriber', view.structural_subscriber_label)
+
class TestBugActivityItem(TestCaseWithFactory):
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-11-18 04:06:16 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-12-02 22:29:33 +0000
@@ -914,7 +914,7 @@
"""
def userHasPrivileges(user):
- """Is the user a privledged one, allowed to changed details on a
+ """Is the user a priviledged one, allowed to changed details on a
bug?.
:return: A boolean.
@@ -1017,10 +1017,11 @@
required=False)
has_cve = Bool(
title=_('Show only bugs associated with a CVE'), required=False)
- bug_supervisor = Choice(
- title=_('Bug supervisor'), vocabulary='ValidPersonOrTeam',
- description=_('Show only bugs in packages this person or team '
- 'is subscribed to.'),
+ structural_subscriber = Choice(
+ title=_('Structural Subscriber'), vocabulary='ValidPersonOrTeam',
+ description=_(
+ 'Show only bugs in projects, series, distributions, and packages '
+ 'that this person or team is subscribed to.'),
required=False)
bug_commenter = Choice(
title=_('Bug commenter'), vocabulary='ValidPersonOrTeam',
=== modified file 'lib/lp/bugs/stories/bugs/xx-bugs-advanced-search-upstream-status.txt'
--- lib/lp/bugs/stories/bugs/xx-bugs-advanced-search-upstream-status.txt 2011-06-28 15:04:29 +0000
+++ lib/lp/bugs/stories/bugs/xx-bugs-advanced-search-upstream-status.txt 2011-12-02 22:29:33 +0000
@@ -159,7 +159,7 @@
... 'assignee_option': 'any',
... 'field.assignee': '',
... 'field.bug_reporter': '',
- ... 'field.bug_supervisor': '',
+ ... 'field.structural_subscriber': '',
... 'field.component-empty-marker': '1',
... 'field.omit_dupes.used': '',
... 'field.omit_dupes': 'on',
=== modified file 'lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt'
--- lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt 2011-11-24 23:22:16 +0000
+++ lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt 2011-12-02 22:29:33 +0000
@@ -1,7 +1,7 @@
= Advanced searching using person-based filters =
The advanced search form allows us to filter bugs based on the people
-related to them: assignees, commenters, bug supervisors and subscribers.
+related to them: assignees, commenters, package subscribers and subscribers.
== Widget basics ==
@@ -97,13 +97,13 @@
On the advanced search there's a field for specifying a bug commenter.
>>> anon_browser.open('http://bugs.launchpad.dev/ubuntu/+bugs?advanced=1')
- >>> anon_browser.getControl('Bug commenter') is not None
+ >>> anon_browser.getControl('Commenter') is not None
True
If an non-existent person is entered there, an error message is
displayed.
- >>> anon_browser.getControl('Bug commenter').value = 'non-existent'
+ >>> anon_browser.getControl('Commenter').value = 'non-existent'
>>> anon_browser.getControl('Search', index=0).click()
>>> for message in find_tags_by_class(anon_browser.contents, 'message'):
... print message.renderContents()
@@ -111,7 +111,7 @@
Entering an existing person shows all bugs that person has commented on.
- >>> anon_browser.getControl('Bug commenter').value = 'foo.bar@xxxxxxxxxxxxx'
+ >>> anon_browser.getControl('Commenter').value = 'foo.bar@xxxxxxxxxxxxx'
>>> anon_browser.getControl('Search', index=0).click()
>>> from lp.bugs.tests.bug import print_bugtasks
@@ -120,29 +120,23 @@
linux-source-2.6.15 Medium New
-== Searching for a bug supervisor's bugs ==
+== Searching for a package subscriber's bugs ==
-On the advanced search there's a field for specifying a bug supervisor.
+On the advanced search there's a field for specifying a project,
+distribution, package, or series subscriber.
>>> anon_browser.open('http://bugs.launchpad.dev/ubuntu/+bugs?advanced=1')
- >>> anon_browser.getControl('Bug supervisor') is not None
+ >>> anon_browser.getControl('Package, or series subscriber') is not None
True
-If an non-existent person is entered there, an error message is
-displayed.
-
- >>> anon_browser.getControl('Bug supervisor').value = 'non-existent'
- >>> anon_browser.getControl('Search', index=0).click()
- >>> for message in find_tags_by_class(anon_browser.contents, 'message'):
- ... print message.renderContents()
- There's no person with the name or email address 'non-existent'.
-
Entering an existing person shows all bugs for packages or products that the
-person is a bug supervisor for. Since we're in the ubuntu context, only bugs
-for Ubuntu packages will be returned. In Ubuntu, Foo Bar is a bug supervisor
-for mozilla-firefox and pmount, but there aren't any bugs open for pmount.
+person is a package subscriber for. Since we're in the ubuntu context, only
+bugs for Ubuntu packages will be returned. In Ubuntu, Foo Bar is a package
+subscriber for mozilla-firefox and pmount, but there aren't any bugs open for
+pmount.
- >>> anon_browser.getControl('Bug supervisor').value = 'foo.bar@xxxxxxxxxxxxx'
+ >>> anon_browser.getControl(
+ ... 'Package, or series subscriber').value = 'foo.bar@xxxxxxxxxxxxx'
>>> anon_browser.getControl('Search', index=0).click()
>>> from lp.bugs.tests.bug import print_bugtasks
@@ -157,12 +151,12 @@
>>> search_url = 'http://bugs.launchpad.dev/firefox/+bugs?advanced=1'
>>> anon_browser.open(search_url)
- >>> anon_browser.getControl('Bug subscriber') is not None
+ >>> anon_browser.getControl('Subscriber') is not None
True
If an non-existent person is entered there, an error message is displayed:
- >>> anon_browser.getControl('Bug subscriber').value = 'non-existent'
+ >>> anon_browser.getControl('Subscriber').value = 'non-existent'
>>> anon_browser.getControl('Search', index=0).click()
>>> for message in find_tags_by_class(anon_browser.contents, 'message'):
... print message.renderContents()
@@ -173,7 +167,7 @@
subscribed to any bugs. In this case, no bugs are found:
>>> subscriber = 'no-priv@xxxxxxxxxxxxx'
- >>> anon_browser.getControl('Bug subscriber').value = subscriber
+ >>> anon_browser.getControl('Subscriber').value = subscriber
>>> anon_browser.getControl('Search', index=0).click()
>>> print extract_text(find_main_content(anon_browser.contents))
Bugs : Mozilla Firefox ...
@@ -226,7 +220,7 @@
find our first bug within the results:
>>> anon_browser.open(search_url)
- >>> anon_browser.getControl('Bug subscriber').value = subscriber
+ >>> anon_browser.getControl('Subscriber').value = subscriber
>>> anon_browser.getControl('Search', index=0).click()
>>> from lp.bugs.tests.bug import extract_bugtasks
>>> for bugtask in extract_bugtasks(anon_browser.contents):
@@ -250,7 +244,7 @@
we'll find both of our bugs within the results:
>>> anon_browser.open(search_url)
- >>> anon_browser.getControl('Bug subscriber').value = subscriber
+ >>> anon_browser.getControl('Subscriber').value = subscriber
>>> anon_browser.getControl('Search', index=0).click()
>>> for bugtask in extract_bugtasks(anon_browser.contents):
... print 'Task:' + bugtask
=== modified file 'lib/lp/bugs/templates/bugtask-macros-tableview.pt'
--- lib/lp/bugs/templates/bugtask-macros-tableview.pt 2011-12-01 14:36:35 +0000
+++ lib/lp/bugs/templates/bugtask-macros-tableview.pt 2011-12-02 22:29:33 +0000
@@ -379,36 +379,11 @@
</tr>
</tal:render-if>
- <tal:render-if condition="view/shouldShowSupervisorWidget">
- <tr>
- <td>
- <label for="field.bug_supervisor">Bug supervisor:</label>
- </td>
- </tr>
- <tr>
- <td style="white-space: nowrap">
- <div class="field"
- tal:define="error
- python:view.getFieldError('bug_supervisor')">
- <div tal:attributes=
- "class python:error and 'error' or None">
- <span tal:content="structure
- view/widgets/bug_supervisor"/>
- <div tal:condition="error"
- class="message"
- tal:content="error">An error on owner widget
- </div>
- </div>
- </div>
- </td>
- </tr>
- </tal:render-if>
-
<tal:render-if condition="view/shouldShowCommenterWidget">
<tr>
<td>
<label for="field.bug_commenter">
- Bug commenter:
+ Commenter:
</label>
</td>
</tr>
@@ -435,7 +410,7 @@
<tr>
<td>
<label for="field.subscriber">
- Bug subscriber:
+ Subscriber:
</label>
</td>
</tr>
@@ -458,6 +433,32 @@
</tr>
</tal:render-if>
+ <tal:render-if condition="view/shouldShowStructuralSubscriberWidget">
+ <tr>
+ <td>
+ <label for="field.structural_subscriber"
+ tal:content="view/structural_subscriber_label" />
+ </td>
+ </tr>
+ <tr>
+ <td style="white-space: nowrap">
+ <div class="field"
+ tal:define="error
+ python:view.getFieldError('structural_subscriber')">
+ <div tal:attributes=
+ "class python:error and 'error' or None">
+ <span tal:content="structure
+ view/widgets/structural_subscriber"/>
+ <div tal:condition="error"
+ class="message"
+ tal:content="error">An error on owner widget
+ </div>
+ </div>
+ </div>
+ </td>
+ </tr>
+ </tal:render-if>
+
</table>
</td>
<td> </td>
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2011-11-25 12:34:41 +0000
+++ lib/lp/registry/browser/person.py 2011-12-02 22:29:33 +0000
@@ -631,7 +631,7 @@
def softwarebugs(self):
text = 'Subscribed packages'
summary = (
- 'A summary report for packages where %s is a bug supervisor.'
+ 'A summary report for packages where %s is a subscriber.'
% self.context.displayname)
return Link('+packagebugs', text, site='bugs', summary=summary)
