launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01998
[Merge] lp:~adeuring/launchpad/bug-675595-2 into lp:launchpad/devel
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-675595-2 into lp:launchpad/devel with lp:~adeuring/launchpad/bug-675595 as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#675595 BugTaskSet.getAssignedMilestonesFromSearch() can call BugTaskSet._search() directly and retrieve the milestones with one SQL query instead of two
https://bugs.launchpad.net/bugs/675595
This branch fixes bug 675595: "BugTaskSet.getAssignedMilestonesFromSearch()
can call BugTaskSet._search() directly and retrieve the milestones with
one SQL query instead of two"
Working on the branch, I realized that the method
BugTaskSet.getAssignedMilestonesFromSearch() is in fact unnecessary:
The only callsite was RelevantMilestonesMixin.getMilestoneWidgetValues();
since we can now retrieve milestones by pre-joining the table in
BugTaskSet.searchTasks(), I simply removed getAssignedMilestonesFromSearch().
getMilestoneWidgetValues() calls searchTasks() via the method
BugTaskSearchListingView.searchUnbatched(), so I added the parameter
"prejoins" to this method.
tests:
./bin/test -vvt test_buglisting -t test_person_view
--
https://code.launchpad.net/~adeuring/launchpad/bug-675595-2/+merge/41604
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-675595-2 into lp:launchpad/devel.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2010-10-31 20:18:45 +0000
+++ lib/lp/bugs/browser/bugtask.py 2010-11-23 14:43:50 +0000
@@ -2629,7 +2629,7 @@
return self._getBatchNavigator(unbatchedTasks)
def searchUnbatched(self, searchtext=None, context=None,
- extra_params=None):
+ extra_params=None, prejoins=[]):
"""Return a `SelectResults` object for the GET search criteria.
:param searchtext: Text that must occur in the bug report. If
@@ -2647,7 +2647,7 @@
search_params = self.buildSearchParams(
searchtext=searchtext, extra_params=extra_params)
search_params.user = self.user
- tasks = context.searchTasks(search_params)
+ tasks = context.searchTasks(search_params, prejoins=prejoins)
return tasks
def getWidgetValues(
=== modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
--- lib/lp/bugs/browser/tests/test_buglisting.py 2010-10-06 22:05:15 +0000
+++ lib/lp/bugs/browser/tests/test_buglisting.py 2010-11-23 14:43:50 +0000
@@ -3,6 +3,10 @@
__metaclass__ = type
+from storm.expr import LeftJoin
+from storm.store import Store
+from testtools.matchers import Equals
+
from canonical.launchpad.testing.pages import (
extract_text,
find_tag_by_id,
@@ -10,7 +14,15 @@
)
from canonical.launchpad.webapp import canonical_url
from canonical.testing.layers import LaunchpadFunctionalLayer
-from lp.testing import BrowserTestCase
+from lp.bugs.model.bugtask import BugTask
+from lp.registry.model.person import Person
+from lp.testing import (
+ BrowserTestCase,
+ StormStatementRecorder,
+ )
+
+from lp.testing.matchers import HasQueryCount
+from lp.testing.views import create_initialized_view
class TestBugTaskSearchListingView(BrowserTestCase):
@@ -120,3 +132,19 @@
self.assertIs(None,
find_tag_by_id(browser.contents, 'portlet-tags'),
"portlet-tags should not be shown.")
+
+ def test_searchUnbatched_can_preload_objects(self):
+ # BugTaskSearchListingView.searchUnbatched() can optionally
+ # preload objects while retureving the bugtasks.
+ product = self.factory.makeProduct()
+ bugtask_1 = self.factory.makeBug(product=product).default_bugtask
+ bugtask_2 = self.factory.makeBug(product=product).default_bugtask
+ view = create_initialized_view(product, '+bugs')
+ Store.of(product).invalidate()
+ with StormStatementRecorder() as recorder:
+ prejoins=[(Person, LeftJoin(Person, BugTask.owner==Person.id))]
+ bugtasks = list(view.searchUnbatched(prejoins=prejoins))
+ self.assertEqual(
+ [bugtask_1, bugtask_2], bugtasks)
+ [bugtask.owner for bugtask in bugtasks]
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
=== removed file 'lib/lp/bugs/doc/milestones-from-bugtask-search.txt'
--- lib/lp/bugs/doc/milestones-from-bugtask-search.txt 2010-09-14 15:32:53 +0000
+++ lib/lp/bugs/doc/milestones-from-bugtask-search.txt 1970-01-01 00:00:00 +0000
@@ -1,54 +0,0 @@
-= Discover the assigned milestones from a BugTask search =
-
-The IBugTaskSet method getAssignedMilestonesFromSearch() accepts a
-result set that yields BugTasks. It attempt to efficiently find all
-the distinct milestones assigned to those BugTasks. Typically this
-should be called with the results from calling IHasBugs.searchTasks()
-or IBugTaskSet.search().
-
- >>> from lp.bugs.interfaces.bugtask import (
- ... BugTaskSearchParams, IBugTaskSet)
-
- >>> person = factory.makePerson()
- >>> login(person.preferredemail.email)
-
- >>> milestone = factory.makeMilestone()
-
- >>> bugtask = factory.makeBugTask(target=milestone.product)
- >>> bugtask.milestone = milestone
- >>> bugtask.subscribe(person, person)
- <lp.bugs.model.bugsubscription.BugSubscription ...>
- >>> transaction.commit()
-
-The results of a search for all bug tasks related to a person can be
-passed to getAssignedMilestonesFromSearch() to discover all the
-milestones used.
-
- >>> bugtask_set = getUtility(IBugTaskSet)
-
- >>> bugtasks = bugtask_set.search(
- ... BugTaskSearchParams(person, assignee=person),
- ... BugTaskSearchParams(person, subscriber=person),
- ... BugTaskSearchParams(person, owner=person, bug_reporter=person),
- ... BugTaskSearchParams(person, bug_commenter=person))
-
- >>> milestones = bugtask_set.getAssignedMilestonesFromSearch(
- ... bugtasks)
-
- >>> milestones
- [<Milestone ...>]
- >>> len(milestones)
- 1
- >>> milestone in milestones
- True
-
-Because get_assigned_milestones_from_bugtasks() attempts to be
-efficient, by customising the result set passed in, it rejects
-attempts to pass in lists of bug tasks.
-
- >>> milestones = bugtask_set.getAssignedMilestonesFromSearch(
- ... list(bugtasks))
- Traceback (most recent call last):
- ...
- AssertionError: search_results must provide
- IResultSet or ISQLObjectResultSet
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2010-11-23 14:43:50 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2010-11-23 14:43:50 +0000
@@ -1414,13 +1414,6 @@
:param params: the BugTaskSearchParams to search on.
"""
- def getAssignedMilestonesFromSearch(search_results):
- """Returns distinct milestones for the given tasks.
-
- :param search_results: A result set yielding BugTask objects,
- typically the result of calling `BugTaskSet.search()`.
- """
-
def getStatusCountsForProductSeries(user, product_series):
"""Returns status counts for a product series' bugs.
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2010-11-23 14:43:50 +0000
+++ lib/lp/bugs/model/bugtask.py 2010-11-23 14:43:50 +0000
@@ -2309,44 +2309,6 @@
"""See `IBugTaskSet`."""
return self._search(BugTask.bugID, [], params).result_set
- def getAssignedMilestonesFromSearch(self, search_results):
- """See `IBugTaskSet`."""
- # XXX: Gavin Panella 2009-03-05 bug=338184: There is currently
- # no clean way to get the underlying Storm ResultSet from an
- # SQLObjectResultSet, so we must remove the security proxy for
- # a moment.
- if ISQLObjectResultSet.providedBy(search_results):
- search_results = removeSecurityProxy(search_results)._result_set
- # Check that we have a Storm result set before we start doing
- # things with it.
- assert IResultSet.providedBy(search_results), (
- "search_results must provide IResultSet or ISQLObjectResultSet")
- # Remove ordering and make distinct.
- search_results = search_results.order_by().config(distinct=True)
- # Get milestone IDs.
- milestone_ids = [
- milestone_id for milestone_id in (
- search_results.values(BugTask.milestoneID))
- if milestone_id is not None]
- # Query for milestones.
- if len(milestone_ids) == 0:
- return []
- else:
- # Import here because of cyclic references.
- from lp.registry.model.milestone import (
- Milestone, milestone_sort_key)
- # We need the store that was used, we have no objects to key off
- # of other than the search result, and Store.of() doesn't
- # currently work on result sets. Additionally it may be a
- # DecoratedResultSet.
- if zope_isinstance(search_results, DecoratedResultSet):
- store = removeSecurityProxy(search_results).result_set._store
- else:
- store = search_results._store
- milestones = store.find(
- Milestone, Milestone.id.is_in(milestone_ids))
- return sorted(milestones, key=milestone_sort_key, reverse=True)
-
def createTask(self, bug, owner, product=None, productseries=None,
distribution=None, distroseries=None,
sourcepackagename=None,
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-11-18 12:05:34 +0000
+++ lib/lp/registry/browser/person.py 2010-11-23 14:43:50 +0000
@@ -105,6 +105,7 @@
URI,
)
import pytz
+from storm.expr import Join
from storm.zope.interfaces import IResultSet
from z3c.ptcompat import ViewPageTemplateFile
from zope.app.form.browser import (
@@ -240,6 +241,7 @@
IBugTaskSet,
UNRESOLVED_BUGTASK_STATUSES,
)
+from lp.bugs.model.bugtask import BugTask
from lp.buildmaster.enums import BuildStatus
from lp.code.browser.sourcepackagerecipelisting import HasRecipesMenuMixin
from lp.code.errors import InvalidNamespace
@@ -302,6 +304,10 @@
IWikiName,
IWikiNameSet,
)
+from lp.registry.model.milestone import (
+ Milestone,
+ milestone_sort_key,
+ )
from lp.services.fields import LocationField
from lp.services.geoip.interfaces import IRequestPreferredLanguages
from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
@@ -2131,8 +2137,12 @@
def getMilestoneWidgetValues(self):
"""Return data used to render the milestone checkboxes."""
- milestones = getUtility(IBugTaskSet).getAssignedMilestonesFromSearch(
- self.searchUnbatched())
+ prejoins = [
+ (Milestone, Join(Milestone, BugTask.milestone == Milestone.id))]
+ milestones = [
+ bugtask.milestone
+ for bugtask in self.searchUnbatched(prejoins=prejoins)]
+ milestones = sorted(milestones, key=milestone_sort_key, reverse=True)
return [
dict(title=milestone.title, value=milestone.id, checked=False)
for milestone in milestones]
@@ -2148,7 +2158,7 @@
page_title = 'Related bugs'
def searchUnbatched(self, searchtext=None, context=None,
- extra_params=None):
+ extra_params=None, prejoins=[]):
"""Return the open bugs related to a person.
:param extra_params: A dict that provides search params added to
@@ -2179,7 +2189,7 @@
return context.searchTasks(
assignee_params, subscriber_params, owner_params,
- commenter_params)
+ commenter_params, prejoins=prejoins)
def getSearchPageHeading(self):
return "Bugs related to %s" % self.context.displayname
@@ -2208,7 +2218,7 @@
page_title = 'Assigned bugs'
def searchUnbatched(self, searchtext=None, context=None,
- extra_params=None):
+ extra_params=None, prejoins=[]):
"""Return the open bugs assigned to a person."""
if context is None:
context = self.context
@@ -2220,7 +2230,8 @@
extra_params['assignee'] = context
sup = super(PersonAssignedBugTaskSearchListingView, self)
- return sup.searchUnbatched(searchtext, context, extra_params)
+ return sup.searchUnbatched(
+ searchtext, context, extra_params, prejoins)
def shouldShowAssigneeWidget(self):
"""Should the assignee widget be shown on the advanced search page?"""
@@ -2265,7 +2276,7 @@
page_title = 'Commented bugs'
def searchUnbatched(self, searchtext=None, context=None,
- extra_params=None):
+ extra_params=None, prejoins=[]):
"""Return the open bugs commented on by a person."""
if context is None:
context = self.context
@@ -2277,7 +2288,8 @@
extra_params['bug_commenter'] = context
sup = super(PersonCommentedBugTaskSearchListingView, self)
- return sup.searchUnbatched(searchtext, context, extra_params)
+ return sup.searchUnbatched(
+ searchtext, context, extra_params, prejoins)
def getSearchPageHeading(self):
"""The header for the search page."""
@@ -2310,7 +2322,7 @@
page_title = 'Reported bugs'
def searchUnbatched(self, searchtext=None, context=None,
- extra_params=None):
+ extra_params=None, prejoins=[]):
"""Return the bugs reported by a person."""
if context is None:
context = self.context
@@ -2325,7 +2337,8 @@
extra_params['bug_reporter'] = context
sup = super(PersonReportedBugTaskSearchListingView, self)
- return sup.searchUnbatched(searchtext, context, extra_params)
+ return sup.searchUnbatched(
+ searchtext, context, extra_params, prejoins)
def getSearchPageHeading(self):
"""The header for the search page."""
@@ -2366,7 +2379,7 @@
page_title = 'Subscribed bugs'
def searchUnbatched(self, searchtext=None, context=None,
- extra_params=None):
+ extra_params=None, prejoins=[]):
"""Return the bugs subscribed to by a person."""
if context is None:
context = self.context
@@ -2378,7 +2391,8 @@
extra_params['subscriber'] = context
sup = super(PersonSubscribedBugTaskSearchListingView, self)
- return sup.searchUnbatched(searchtext, context, extra_params)
+ return sup.searchUnbatched(
+ searchtext, context, extra_params, prejoins)
def getSearchPageHeading(self):
"""The header for the search page."""
=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py 2010-10-06 08:16:37 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py 2010-11-23 14:43:50 +0000
@@ -4,8 +4,10 @@
__metaclass__ = type
import transaction
+from storm.expr import LeftJoin
+from storm.store import Store
+from testtools.matchers import Equals
from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
from canonical.config import config
from canonical.launchpad.ftests import (
@@ -25,6 +27,7 @@
)
from lp.app.errors import NotFoundError
+from lp.bugs.model.bugtask import BugTask
from lp.buildmaster.enums import BuildStatus
from lp.registry.browser.person import (
PersonEditView,
@@ -43,15 +46,20 @@
)
from lp.registry.model.karma import KarmaCategory
+from lp.registry.model.milestone import milestone_sort_key
from lp.soyuz.enums import (
ArchiveStatus,
PackagePublishingStatus,
)
+from lp.registry.model.person import Person
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
from lp.testing import (
login_person,
+ person_logged_in,
+ StormStatementRecorder,
TestCaseWithFactory,
)
+from lp.testing.matchers import HasQueryCount
from lp.testing.views import (
create_initialized_view,
create_view,
@@ -809,3 +817,132 @@
login_person(self.person)
view = create_initialized_view(self.team, '+subscriptions')
self.assertTrue(view.canUnsubscribeFromBugTasks())
+
+
+class BugTaskViewsTestBase:
+ """A base class for bugtask search related tests."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(BugTaskViewsTestBase, self).setUp()
+ self.person = self.factory.makePerson()
+ with person_logged_in(self.person):
+ self.subscribed_bug = self.factory.makeBug()
+ self.subscribed_bug.subscribe(
+ self.person, subscribed_by=self.person)
+ self.assigned_bug = self.factory.makeBug()
+ self.assigned_bug.default_bugtask.transitionToAssignee(
+ self.person)
+ self.owned_bug = self.factory.makeBug(owner=self.person)
+ self.commented_bug = self.factory.makeBug()
+ self.commented_bug.newMessage(owner=self.person)
+
+ for bug in (self.subscribed_bug, self.assigned_bug, self.owned_bug,
+ self.commented_bug):
+ with person_logged_in(bug.default_bugtask.product.owner):
+ milestone = self.factory.makeMilestone(
+ product=bug.default_bugtask.product)
+ bug.default_bugtask.transitionToMilestone(
+ milestone, bug.default_bugtask.product.owner)
+
+ def test_searchUnbatched(self):
+ view = create_initialized_view(self.person, self.view_name)
+ self.assertEqual(
+ self.expected_for_search_unbatched, list(view.searchUnbatched()))
+
+ def test_searchUnbatched_with_prejoins(self):
+ view = create_initialized_view(self.person, self.view_name)
+ Store.of(self.subscribed_bug).invalidate()
+ with StormStatementRecorder() as recorder:
+ prejoins=[(Person, LeftJoin(Person, BugTask.owner==Person.id))]
+ bugtasks = view.searchUnbatched(prejoins=prejoins)
+ [bugtask.owner for bugtask in bugtasks]
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+ def test_getMilestoneWidgetValues(self):
+ view = create_initialized_view(self.person, self.view_name)
+ milestones = [
+ bugtask.milestone
+ for bugtask in self.expected_for_search_unbatched]
+ milestones = sorted(milestones, key=milestone_sort_key, reverse=True)
+ expected = [
+ {
+ 'title': milestone.title,
+ 'value': milestone.id,
+ 'checked': False,
+ }
+ for milestone in milestones]
+ Store.of(milestones[0]).invalidate()
+ with StormStatementRecorder() as recorder:
+ self.assertEqual(expected, view.getMilestoneWidgetValues())
+ self.assertThat(recorder, HasQueryCount(Equals(1)))
+
+
+class TestPersonRelatedBugTaskSearchListingView(
+ BugTaskViewsTestBase, TestCaseWithFactory):
+ """Tests for PersonRelatedBugTaskSearchListingView."""
+
+ view_name = '+bugs'
+
+ def setUp(self):
+ super(TestPersonRelatedBugTaskSearchListingView, self).setUp()
+ self.expected_for_search_unbatched = [
+ self.subscribed_bug.default_bugtask,
+ self.assigned_bug.default_bugtask,
+ self.owned_bug.default_bugtask,
+ self.commented_bug.default_bugtask,
+ ]
+
+
+class TestPersonAssignedBugTaskSearchListingView(
+ BugTaskViewsTestBase, TestCaseWithFactory):
+ """Tests for PersonAssignedBugTaskSearchListingView."""
+
+ view_name = '+assignedbugs'
+
+ def setUp(self):
+ super(TestPersonAssignedBugTaskSearchListingView, self).setUp()
+ self.expected_for_search_unbatched = [
+ self.assigned_bug.default_bugtask,
+ ]
+
+
+class TestPersonCommentedBugTaskSearchListingView(
+ BugTaskViewsTestBase, TestCaseWithFactory):
+ """Tests for PersonAssignedBugTaskSearchListingView."""
+
+ view_name = '+commentedbugs'
+
+ def setUp(self):
+ super(TestPersonCommentedBugTaskSearchListingView, self).setUp()
+ self.expected_for_search_unbatched = [
+ self.commented_bug.default_bugtask,
+ ]
+
+
+class TestPersonReportedBugTaskSearchListingView(
+ BugTaskViewsTestBase, TestCaseWithFactory):
+ """Tests for PersonAssignedBugTaskSearchListingView."""
+
+ view_name = '+reportedbugs'
+
+ def setUp(self):
+ super(TestPersonReportedBugTaskSearchListingView, self).setUp()
+ self.expected_for_search_unbatched = [
+ self.owned_bug.default_bugtask,
+ ]
+
+
+class TestPersonSubscribedBugTaskSearchListingView(
+ BugTaskViewsTestBase, TestCaseWithFactory):
+ """Tests for PersonAssignedBugTaskSearchListingView."""
+
+ view_name = '+subscribedbugs'
+
+ def setUp(self):
+ super(TestPersonSubscribedBugTaskSearchListingView, self).setUp()
+ self.expected_for_search_unbatched = [
+ self.subscribed_bug.default_bugtask,
+ self.owned_bug.default_bugtask,
+ ]
