launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05802
[Merge] lp:~abentley/launchpad/packagebugs into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/packagebugs into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #887214 in Launchpad itself: "+packagebugs returns an unexpected form data error with new bug listing"
https://bugs.launchpad.net/launchpad/+bug/887214
For more details, see:
https://code.launchpad.net/~abentley/launchpad/packagebugs/+merge/84329
= Summary =
Fix bug #887214: +packagebugs returns an unexpected form data error with new bug listing
== Proposed fix ==
Extract a separate BugSubscriberPackageBugsOverView from BugSubscriberPackageBugsSearchListingView, for use with +packagebugs. The main commonality between the two views was URL generation, which is now a separate get_package_search_url method.
== Pre-implementation notes ==
Discussed portlet removal with deryck
== Implementation details ==
As a driveby, rip out unused +portlet-otherpackages portlet, and its associated getOtherBugSubscriberPackageLinks link.
Fix tests to distinguish between overview URL and search URL.
== Tests ==
bin/test -t person-bug-views.txt
== Demo and Q/A ==
Go to https://bugs.qastaging.launchpad.net/~ubuntu-server/
Click the 'Subscribed packages' link.
You should get a table listing subscribed packages, not an OOPS.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/bugs/browser/configure.zcml
lib/lp/registry/browser/person.py
lib/lp/bugs/browser/tests/person-bug-views.txt
These lint errors are in the original, not due to my changes:
./lib/lp/bugs/browser/tests/person-bug-views.txt
1: narrative uses a moin header.
19: narrative uses a moin header.
46: narrative uses a moin header.
131: narrative uses a moin header.
167: narrative uses a moin header.
240: want exceeds 78 characters.
244: want exceeds 78 characters.
248: want exceeds 78 characters.
252: want exceeds 78 characters.
259: want exceeds 78 characters.
266: want exceeds 78 characters.
282: want exceeds 78 characters.
294: want exceeds 78 characters.
296: want exceeds 78 characters.
298: want exceeds 78 characters.
300: want exceeds 78 characters.
456: narrative uses a moin header.
484: narrative uses a moin header.
495: narrative uses a moin header.
540: narrative uses a moin header.
571: narrative uses a moin header.
592: narrative uses a moin header.
614: narrative uses a moin header.
--
https://code.launchpad.net/~abentley/launchpad/packagebugs/+merge/84329
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/packagebugs into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml 2011-11-27 01:31:13 +0000
+++ lib/lp/bugs/browser/configure.zcml 2011-12-02 21:01:24 +0000
@@ -262,12 +262,6 @@
permission="zope.Public"
class="lp.app.browser.launchpad.Macro"/>
<browser:page
- for="lp.registry.interfaces.person.IPerson"
- name="+portlet-otherpackages"
- permission="zope.Public"
- class="lp.registry.browser.person.BugSubscriberPackageBugsSearchListingView"
- template="../templates/person-portlet-otherpackages.pt"/>
- <browser:page
name="+bugs"
for="lp.registry.interfaces.person.IPerson"
class="lp.registry.browser.person.PersonRelatedBugTaskSearchListingView"
@@ -300,7 +294,7 @@
<browser:page
name="+packagebugs"
for="lp.registry.interfaces.person.IPerson"
- class="lp.registry.browser.person.BugSubscriberPackageBugsSearchListingView"
+ class="lp.registry.browser.person.BugSubscriberPackageBugsOverView"
permission="zope.Public"
template="../templates/person-packagebugs-overview.pt"/>
<browser:page
=== modified file 'lib/lp/bugs/browser/tests/person-bug-views.txt'
--- lib/lp/bugs/browser/tests/person-bug-views.txt 2011-03-28 20:54:50 +0000
+++ lib/lp/bugs/browser/tests/person-bug-views.txt 2011-12-02 21:01:24 +0000
@@ -233,17 +233,22 @@
>>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
>>> ubuntu_mozilla_firefox = ubuntu.getSourcePackage("mozilla-firefox")
+ >>> packagebugs_view = create_view(name12, name='+packagebugs')
- >>> packagebugs_search_view.getOpenBugsURL(ubuntu_mozilla_firefox)
+ >>> packagebugs_view.getOpenBugsURL(
+ ... ubuntu_mozilla_firefox, u'/~name12')
u'.../~name12/+packagebugs-search?field.distribution=ubuntu&field.sourcepackagename=mozilla-firefox&field.status=New&field.status=Incomplete&field.status=Confirmed&field.status=Triaged&field.status=In+Progress&field.status=Fix+Committed&search=Search'
- >>> packagebugs_search_view.getCriticalBugsURL(ubuntu_mozilla_firefox)
+ >>> packagebugs_view.getCriticalBugsURL(
+ ... ubuntu_mozilla_firefox, u'/~name12')
u'.../~name12/+packagebugs-search?field.distribution=ubuntu&field.importance=Critical&field.sourcepackagename=mozilla-firefox&field.status=New&field.status=Incomplete&field.status=Confirmed&field.status=Triaged&field.status=In+Progress&field.status=Fix+Committed&search=Search'
- >>> packagebugs_search_view.getUnassignedBugsURL(ubuntu_mozilla_firefox)
+ >>> packagebugs_view.getUnassignedBugsURL(
+ ... ubuntu_mozilla_firefox, u'/~name12')
u'.../~name12/+packagebugs-search?field.distribution=ubuntu&field.sourcepackagename=mozilla-firefox&field.status=New&field.status=Incomplete&field.status=Confirmed&field.status=Triaged&field.status=In+Progress&field.status=Fix+Committed&field.unassigned=on&search=Search'
- >>> packagebugs_search_view.getInProgressBugsURL(ubuntu_mozilla_firefox)
+ >>> packagebugs_view.getInProgressBugsURL(
+ ... ubuntu_mozilla_firefox, u'/~name12')
u'.../~name12/+packagebugs-search?field.distribution=ubuntu&field.sourcepackagename=mozilla-firefox&field.status=In+Progress&search=Search'
A helper method is used to calculate the package bug search URL for the
@@ -418,7 +423,7 @@
... 'field.status': [s.title for s in UNRESOLVED_BUGTASK_STATUSES]}
>>> packagebugs_search_view = create_view(
- ... name16, name="+packagebugs", form=form)
+ ... name16, name="+packagebugs-search", form=form)
>>> print pretty(packagebugs_search_view.getMilestoneWidgetValues())
[]
@@ -431,7 +436,7 @@
>>> new_milestone = hoary_series.newMilestone(name='testing')
>>> packagebugs_search_view = create_view(
- ... name16, name="+packagebugs", form=form)
+ ... name16, name="+packagebugs-search", form=form)
>>> print pretty(packagebugs_search_view.getMilestoneWidgetValues())
[{'checked': False,
@@ -442,7 +447,7 @@
>>> new_milestone.active = False
>>> packagebugs_search_view = create_view(
- ... name16, name="+packagebugs", form=form)
+ ... name16, name="+packagebugs-search", form=form)
>>> print pretty(packagebugs_search_view.getMilestoneWidgetValues())
[]
=== removed file 'lib/lp/bugs/templates/person-portlet-otherpackages.pt'
--- lib/lp/bugs/templates/person-portlet-otherpackages.pt 2009-07-17 17:59:07 +0000
+++ lib/lp/bugs/templates/person-portlet-otherpackages.pt 1970-01-01 00:00:00 +0000
@@ -1,34 +0,0 @@
-<tal:root
- xmlns:tal="http://xml.zope.org/namespaces/tal";
- xmlns:metal="http://xml.zope.org/namespaces/metal";
- xmlns:i18n="http://xml.zope.org/namespaces/i18n";
- omit-tag="">
-<div class="portlet" id="portlet-other-packages">
- <h2 tal:condition="python:view.user == context">
- My other packages
- </h2>
- <h2 tal:condition="python:not view.user == context"
- tal:define="name context/fmt:displayname"
- tal:content="string: ${name}'s other packages">
- Sample Person's other packages
- </h2>
- <div class="portletBody portletContent"
- tal:define="package_being_shown view/current_package">
- <ul style="list-style: none;">
- <li style="margin-left: -15px;"
- tal:repeat="bug_subscriber_package_link view/getOtherBugSubscriberPackageLinks">
- <a href="#"
- tal:content="bug_subscriber_package_link/title"
- tal:attributes="href bug_subscriber_package_link/url">
- mozilla-firefox (Ubuntu)
- </a>
- </li>
- </ul>
- <br />
- <a href="#"
- tal:attributes="href string:${context/fmt:url}/+packagebugs">
- Package bugs overview
- </a>
- </div>
-</div>
-</tal:root>
=== 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 21:01:24 +0000
@@ -1417,48 +1417,40 @@
return self.context.specifications(filter=filter)
-class BugSubscriberPackageBugsSearchListingView(BugTaskSearchListingView):
- """Bugs reported on packages for a bug subscriber."""
-
- columns_to_show = ["id", "summary", "importance", "status"]
+def get_package_search_url(distributionsourcepackage, person_url,
+ advanced=False, extra_params=None):
+ """Construct a default search URL for a distributionsourcepackage.
+
+ Optional filter parameters can be specified as a dict with the
+ extra_params argument.
+ """
+ params = {
+ "field.distribution": distributionsourcepackage.distribution.name,
+ "field.sourcepackagename": distributionsourcepackage.name,
+ "search": "Search"}
+
+ if extra_params is not None:
+ # We must UTF-8 encode searchtext to play nicely with
+ # urllib.urlencode, because it may contain non-ASCII characters.
+ if 'field.searchtext' in extra_params:
+ extra_params["field.searchtext"] = (
+ extra_params["field.searchtext"].encode("utf8"))
+
+ params.update(extra_params)
+
+ query_string = urllib.urlencode(sorted(params.items()), doseq=True)
+
+ if advanced:
+ return (person_url + '/+packagebugs-search?advanced=1&%s'
+ % query_string)
+ else:
+ return person_url + '/+packagebugs-search?%s' % query_string
+
+
+class BugSubscriberPackageBugsOverView(LaunchpadView):
+
page_title = 'Package bugs'
- @property
- def current_package(self):
- """Get the package whose bugs are currently being searched."""
- if not (
- self.widgets['distribution'].hasValidInput() and
- self.widgets['distribution'].getInputValue()):
- raise UnexpectedFormData("A distribution is required")
- if not (
- self.widgets['sourcepackagename'].hasValidInput() and
- self.widgets['sourcepackagename'].getInputValue()):
- raise UnexpectedFormData("A sourcepackagename is required")
-
- distribution = self.widgets['distribution'].getInputValue()
- return distribution.getSourcePackage(
- self.widgets['sourcepackagename'].getInputValue())
-
- def search(self, searchtext=None):
- distrosourcepackage = self.current_package
- return BugTaskSearchListingView.search(
- self, searchtext=searchtext, context=distrosourcepackage)
-
- def getMilestoneWidgetValues(self):
- """See `BugTaskSearchListingView`.
-
- We return only the active milestones on the current distribution
- since any others are irrelevant.
- """
- current_distro = self.current_package.distribution
- vocabulary_registry = getVocabularyRegistry()
- vocabulary = vocabulary_registry.get(current_distro, 'Milestone')
-
- return helpers.shortlist([
- dict(title=milestone.title, value=milestone.token, checked=False)
- for milestone in vocabulary],
- longest_expected=10)
-
@cachedproperty
def total_bug_counts(self):
"""Return the totals of each type of package bug count as a dict."""
@@ -1482,98 +1474,43 @@
L = []
package_counts = getUtility(IBugTaskSet).getBugCountsForPackages(
self.user, self.context.getBugSubscriberPackages())
+ person_url = canonical_url(self.user)
for package_counts in package_counts:
package = package_counts['package']
L.append({
'package_name': package.displayname,
'package_search_url':
- self.getBugSubscriberPackageSearchURL(package),
+ get_package_search_url(package, person_url),
'open_bugs_count': package_counts['open'],
- 'open_bugs_url': self.getOpenBugsURL(package),
+ 'open_bugs_url': self.getOpenBugsURL(package, person_url),
'critical_bugs_count': package_counts['open_critical'],
- 'critical_bugs_url': self.getCriticalBugsURL(package),
+ 'critical_bugs_url': self.getCriticalBugsURL(
+ package, person_url),
'high_bugs_count': package_counts['open_high'],
- 'high_bugs_url': self.getHighBugsURL(package),
+ 'high_bugs_url': self.getHighBugsURL(package, person_url),
'unassigned_bugs_count': package_counts['open_unassigned'],
- 'unassigned_bugs_url': self.getUnassignedBugsURL(package),
+ 'unassigned_bugs_url': self.getUnassignedBugsURL(
+ package, person_url),
'inprogress_bugs_count': package_counts['open_inprogress'],
- 'inprogress_bugs_url': self.getInProgressBugsURL(package),
+ 'inprogress_bugs_url': self.getInProgressBugsURL(
+ package, person_url),
})
return sorted(L, key=itemgetter('package_name'))
- def getOtherBugSubscriberPackageLinks(self):
- """Return a list of the other packages for a bug subscriber.
-
- This excludes the current package.
- """
- current_package = self.current_package
-
- other_packages = [
- package for package in self.context.getBugSubscriberPackages()
- if package != current_package]
-
- package_links = []
- for other_package in other_packages:
- package_links.append({
- 'title': other_package.displayname,
- 'url': self.getBugSubscriberPackageSearchURL(other_package)})
-
- return package_links
-
- @cachedproperty
- def person_url(self):
- return canonical_url(self.context)
-
- def getBugSubscriberPackageSearchURL(self, distributionsourcepackage=None,
- advanced=False, extra_params=None):
- """Construct a default search URL for a distributionsourcepackage.
-
- Optional filter parameters can be specified as a dict with the
- extra_params argument.
- """
- if distributionsourcepackage is None:
- distributionsourcepackage = self.current_package
-
- params = {
- "field.distribution": distributionsourcepackage.distribution.name,
- "field.sourcepackagename": distributionsourcepackage.name,
- "search": "Search"}
-
- if extra_params is not None:
- # We must UTF-8 encode searchtext to play nicely with
- # urllib.urlencode, because it may contain non-ASCII characters.
- if 'field.searchtext' in extra_params:
- extra_params["field.searchtext"] = (
- extra_params["field.searchtext"].encode("utf8"))
-
- params.update(extra_params)
-
- query_string = urllib.urlencode(sorted(params.items()), doseq=True)
-
- if advanced:
- return (self.person_url + '/+packagebugs-search?advanced=1&%s'
- % query_string)
- else:
- return self.person_url + '/+packagebugs-search?%s' % query_string
-
- def getBugSubscriberPackageAdvancedSearchURL(self,
- distributionsourcepackage=None):
- """Build the advanced search URL for a distributionsourcepackage."""
- return self.getBugSubscriberPackageSearchURL(advanced=True)
-
- def getOpenBugsURL(self, distributionsourcepackage):
+ def getOpenBugsURL(self, distributionsourcepackage, person_url):
"""Return the URL for open bugs on distributionsourcepackage."""
status_params = {'field.status': []}
for status in UNRESOLVED_BUGTASK_STATUSES:
status_params['field.status'].append(status.title)
- return self.getBugSubscriberPackageSearchURL(
+ return get_package_search_url(
distributionsourcepackage=distributionsourcepackage,
+ person_url=person_url,
extra_params=status_params)
- def getCriticalBugsURL(self, distributionsourcepackage):
+ def getCriticalBugsURL(self, distributionsourcepackage, person_url):
"""Return the URL for critical bugs on distributionsourcepackage."""
critical_bugs_params = {
'field.status': [], 'field.importance': "Critical"}
@@ -1581,11 +1518,12 @@
for status in UNRESOLVED_BUGTASK_STATUSES:
critical_bugs_params["field.status"].append(status.title)
- return self.getBugSubscriberPackageSearchURL(
+ return get_package_search_url(
distributionsourcepackage=distributionsourcepackage,
+ person_url=person_url,
extra_params=critical_bugs_params)
- def getHighBugsURL(self, distributionsourcepackage):
+ def getHighBugsURL(self, distributionsourcepackage, person_url):
"""Return URL for high bugs on distributionsourcepackage."""
high_bugs_params = {
'field.status': [], 'field.importance': "High"}
@@ -1593,11 +1531,12 @@
for status in UNRESOLVED_BUGTASK_STATUSES:
high_bugs_params["field.status"].append(status.title)
- return self.getBugSubscriberPackageSearchURL(
+ return get_package_search_url(
distributionsourcepackage=distributionsourcepackage,
+ person_url=person_url,
extra_params=high_bugs_params)
- def getUnassignedBugsURL(self, distributionsourcepackage):
+ def getUnassignedBugsURL(self, distributionsourcepackage, person_url):
"""Return the URL for unassigned bugs on distributionsourcepackage."""
unassigned_bugs_params = {
"field.status": [], "field.unassigned": "on"}
@@ -1605,18 +1544,85 @@
for status in UNRESOLVED_BUGTASK_STATUSES:
unassigned_bugs_params["field.status"].append(status.title)
- return self.getBugSubscriberPackageSearchURL(
+ return get_package_search_url(
distributionsourcepackage=distributionsourcepackage,
+ person_url=person_url,
extra_params=unassigned_bugs_params)
- def getInProgressBugsURL(self, distributionsourcepackage):
+ def getInProgressBugsURL(self, distributionsourcepackage, person_url):
"""Return the URL for unassigned bugs on distributionsourcepackage."""
inprogress_bugs_params = {"field.status": "In Progress"}
- return self.getBugSubscriberPackageSearchURL(
+ return get_package_search_url(
distributionsourcepackage=distributionsourcepackage,
+ person_url=person_url,
extra_params=inprogress_bugs_params)
+
+class BugSubscriberPackageBugsSearchListingView(BugTaskSearchListingView):
+ """Bugs reported on packages for a bug subscriber."""
+
+ columns_to_show = ["id", "summary", "importance", "status"]
+ page_title = 'Package bugs'
+
+ @property
+ def current_package(self):
+ """Get the package whose bugs are currently being searched."""
+ if not (
+ self.widgets['distribution'].hasValidInput() and
+ self.widgets['distribution'].getInputValue()):
+ raise UnexpectedFormData("A distribution is required")
+ if not (
+ self.widgets['sourcepackagename'].hasValidInput() and
+ self.widgets['sourcepackagename'].getInputValue()):
+ raise UnexpectedFormData("A sourcepackagename is required")
+
+ distribution = self.widgets['distribution'].getInputValue()
+ return distribution.getSourcePackage(
+ self.widgets['sourcepackagename'].getInputValue())
+
+ def search(self, searchtext=None):
+ distrosourcepackage = self.current_package
+ return BugTaskSearchListingView.search(
+ self, searchtext=searchtext, context=distrosourcepackage)
+
+ def getMilestoneWidgetValues(self):
+ """See `BugTaskSearchListingView`.
+
+ We return only the active milestones on the current distribution
+ since any others are irrelevant.
+ """
+ current_distro = self.current_package.distribution
+ vocabulary_registry = getVocabularyRegistry()
+ vocabulary = vocabulary_registry.get(current_distro, 'Milestone')
+
+ return helpers.shortlist([
+ dict(title=milestone.title, value=milestone.token, checked=False)
+ for milestone in vocabulary],
+ longest_expected=10)
+
+ @cachedproperty
+ def person_url(self):
+ return canonical_url(self.context)
+
+ def getBugSubscriberPackageSearchURL(self, distributionsourcepackage=None,
+ advanced=False, extra_params=None):
+ """Construct a default search URL for a distributionsourcepackage.
+
+ Optional filter parameters can be specified as a dict with the
+ extra_params argument.
+ """
+ if distributionsourcepackage is None:
+ distributionsourcepackage = self.current_package
+ return get_package_search_url(
+ distributionsourcepackage, self.person_url, advanced,
+ extra_params)
+
+ def getBugSubscriberPackageAdvancedSearchURL(self,
+ distributionsourcepackage=None):
+ """Build the advanced search URL for a distributionsourcepackage."""
+ return self.getBugSubscriberPackageSearchURL(advanced=True)
+
def shouldShowSearchWidgets(self):
# XXX: Guilherme Salgado 2005-11-05:
# It's not possible to search amongst the bugs on maintained
@@ -1628,7 +1634,7 @@
return "Search bugs in %s" % self.current_package.displayname
def getSimpleSearchURL(self):
- return self.getBugSubscriberPackageSearchURL()
+ return get_package_search_url(self.current_package, self.person_url)
@property
def label(self):
