launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21661
[Merge] lp:~cjwatson/launchpad/refactor-branch-listing-sort into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/refactor-branch-listing-sort into lp:launchpad.
Commit message:
Push BranchListingSort and friends down to BranchCollection.
This allows lp.code.feed.branch to avoid importing lp.code.model.branch.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/refactor-branch-listing-sort/+merge/325708
This enumeration is a bit view-ish, but on balance I think having it at the model level is the lesser evil.
importfascist is now clean!
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/refactor-branch-listing-sort into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py 2016-06-28 21:10:18 +0000
+++ lib/lp/code/browser/branchlisting.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Base class view for branch listings."""
@@ -34,10 +34,7 @@
EnumeratedType,
Item,
)
-from storm.expr import (
- Asc,
- Desc,
- )
+from storm.expr import Desc
from z3c.ptcompat import ViewPageTemplateFile
from zope.component import getUtility
from zope.formlib import form
@@ -71,6 +68,7 @@
from lp.code.enums import (
BranchLifecycleStatus,
BranchLifecycleStatusFilter,
+ BranchListingSort,
BranchType,
)
from lp.code.interfaces.branch import (
@@ -215,69 +213,6 @@
return '<BranchListingItem %r (%d)>' % (self.unique_name, self.id)
-class BranchListingSort(EnumeratedType):
- """Choices for how to sort branch listings."""
-
- # XXX: MichaelHudson 2007-10-17 bug=153891: We allow sorting on quantities
- # that are not visible in the listing!
-
- DEFAULT = Item("""
- by most interesting
-
- Sort branches by the default ordering for the view.
- """)
-
- PRODUCT = Item("""
- by project name
-
- Sort branches by name of the project the branch is for.
- """)
-
- LIFECYCLE = Item("""
- by status
-
- Sort branches by their status.
- """)
-
- NAME = Item("""
- by branch name
-
- Sort branches by the name of the branch.
- """)
-
- OWNER = Item("""
- by owner name
-
- Sort branches by the name of the owner.
- """)
-
- MOST_RECENTLY_CHANGED_FIRST = Item("""
- most recently changed first
-
- Sort branches from the most recently to the least recently
- changed.
- """)
-
- LEAST_RECENTLY_CHANGED_FIRST = Item("""
- most neglected first
-
- Sort branches from the least recently to the most recently
- changed.
- """)
-
- NEWEST_FIRST = Item("""
- newest first
-
- Sort branches from newest to oldest.
- """)
-
- OLDEST_FIRST = Item("""
- oldest first
-
- Sort branches from oldest to newest.
- """)
-
-
class PersonBranchCategory(EnumeratedType):
"""Choices for filtering lists of branches related to people."""
@@ -627,7 +562,7 @@
len(self.branches().visible_branches_for_view) == 0 and
not self.branch_count)
- def _branches(self, lifecycle_status):
+ def _branches(self, lifecycle_status, sort_by=None):
"""Return a sequence of branches.
This method is overridden in the derived classes to perform the
@@ -639,8 +574,11 @@
if lifecycle_status is not None:
collection = collection.withLifecycleStatus(*lifecycle_status)
collection = collection.visibleByUser(self.user)
- return collection.getBranches(eager_load=False).order_by(
- self._listingSortToOrderBy(self.sort_by))
+ if sort_by is None:
+ sort_by = self.sort_by
+ if sort_by is None:
+ sort_by = BranchListingSort.DEFAULT
+ return collection.getBranches(eager_load=False, sort_by=sort_by)
@property
def no_branch_message(self):
@@ -714,44 +652,6 @@
# If a derived view has specified a default sort_by, use that.
return self.initial_values.get('sort_by')
- @staticmethod
- def _listingSortToOrderBy(sort_by):
- """Compute a value to pass as orderBy to Branch.select().
-
- :param sort_by: an item from the BranchListingSort enumeration.
- """
- from lp.code.model.branch import Branch
-
- DEFAULT_BRANCH_LISTING_SORT = [
- BranchListingSort.PRODUCT,
- BranchListingSort.LIFECYCLE,
- BranchListingSort.OWNER,
- BranchListingSort.NAME,
- ]
-
- LISTING_SORT_TO_COLUMN = {
- BranchListingSort.PRODUCT: (Asc, Branch.target_suffix),
- BranchListingSort.LIFECYCLE: (Desc, Branch.lifecycle_status),
- BranchListingSort.NAME: (Asc, Branch.name),
- BranchListingSort.OWNER: (Asc, Branch.owner_name),
- BranchListingSort.MOST_RECENTLY_CHANGED_FIRST: (
- Desc, Branch.date_last_modified),
- BranchListingSort.LEAST_RECENTLY_CHANGED_FIRST: (
- Asc, Branch.date_last_modified),
- BranchListingSort.NEWEST_FIRST: (Desc, Branch.date_created),
- BranchListingSort.OLDEST_FIRST: (Asc, Branch.date_created),
- }
-
- order_by = map(
- LISTING_SORT_TO_COLUMN.get, DEFAULT_BRANCH_LISTING_SORT)
-
- if sort_by is not None and sort_by != BranchListingSort.DEFAULT:
- direction, column = LISTING_SORT_TO_COLUMN[sort_by]
- order_by = (
- [(direction, column)] +
- [sort for sort in order_by if sort[1] is not column])
- return [direction(column) for direction, column in order_by]
-
def setUpWidgets(self, context=None):
"""Set up the 'sort_by' widget with only the applicable choices."""
fields = []
@@ -1267,9 +1167,8 @@
"""Return the series branches, followed by most recently changed."""
series_branches = self._getSeriesBranches()
branch_query = super(ProductBranchesView, self)._branches(
- self.selected_lifecycle_status)
- branch_query.order_by(self._listingSortToOrderBy(
- BranchListingSort.MOST_RECENTLY_CHANGED_FIRST))
+ self.selected_lifecycle_status,
+ sort_by=BranchListingSort.MOST_RECENTLY_CHANGED_FIRST)
# We don't want the initial branch listing to be batched, so only get
# the batch size - the number of series_branches.
batch_size = config.launchpad.branchlisting_batch_size
=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py 2016-06-30 17:25:25 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for branch listing."""
@@ -13,22 +13,15 @@
from lazr.uri import URI
from lxml import html
import soupmatchers
-from storm.expr import (
- Asc,
- Desc,
- )
from testtools.matchers import Not
from zope.component import getUtility
from lp.app.enums import InformationType
from lp.app.interfaces.services import IService
from lp.code.browser.branchlisting import (
- BranchListingSort,
- BranchListingView,
GroupedDistributionSourcePackageBranchesView,
SourcePackageBranchesView,
)
-from lp.code.model.branch import Branch
from lp.code.model.seriessourcepackagebranch import (
SeriesSourcePackageBranchSet,
)
@@ -39,8 +32,6 @@
from lp.registry.interfaces.person import IPerson
from lp.registry.interfaces.personproduct import IPersonProductFactory
from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.registry.model.person import Owner
-from lp.registry.model.product import Product
from lp.services.features.testing import FeatureFixture
from lp.services.webapp import canonical_url
from lp.services.webapp.servers import LaunchpadTestRequest
@@ -50,7 +41,6 @@
login_person,
normalize_whitespace,
person_logged_in,
- TestCase,
TestCaseWithFactory,
time_counter,
)
@@ -71,60 +61,6 @@
)
-class TestListingToSortOrder(TestCase):
- """Tests for the BranchSet._listingSortToOrderBy static method.
-
- This method translates values from the BranchListingSort enumeration into
- values suitable to pass to orderBy in queries against BranchWithSortKeys.
- """
-
- DEFAULT_BRANCH_LISTING_SORT = [
- Asc(Product.name),
- Desc(Branch.lifecycle_status),
- Asc(Owner.name),
- Asc(Branch.name),
- ]
-
- def assertSortsEqual(self, sort_one, sort_two):
- """Assert that one list of sort specs is equal to another."""
-
- def sort_data(sort):
- return sort.suffix, sort.expr
- self.assertEqual(map(sort_data, sort_one), map(sort_data, sort_two))
-
- def test_default(self):
- """Test that passing None results in the default list."""
- self.assertSortsEqual(
- self.DEFAULT_BRANCH_LISTING_SORT,
- BranchListingView._listingSortToOrderBy(None))
-
- def test_lifecycle(self):
- """Test with an option that's part of the default sort.
-
- Sorting on LIFECYCYLE moves the lifecycle reference to the
- first element of the output."""
- # Check that this isn't a no-op.
- lifecycle_order = BranchListingView._listingSortToOrderBy(
- BranchListingSort.LIFECYCLE)
- self.assertSortsEqual(
- [Desc(Branch.lifecycle_status),
- Asc(Product.name),
- Asc(Owner.name),
- Asc(Branch.name)], lifecycle_order)
-
- def test_sortOnColumNotInDefaultSortOrder(self):
- """Test with an option that's not part of the default sort.
-
- This should put the passed option first in the list, but leave
- the rest the same.
- """
- registrant_order = BranchListingView._listingSortToOrderBy(
- BranchListingSort.OLDEST_FIRST)
- self.assertSortsEqual(
- [Asc(Branch.date_created)] + self.DEFAULT_BRANCH_LISTING_SORT,
- registrant_order)
-
-
class AjaxBatchNavigationMixin:
def _test_search_batch_request(self, context, user=None,
view_name='+branches'):
=== modified file 'lib/lp/code/enums.py'
--- lib/lp/code/enums.py 2016-12-02 12:04:11 +0000
+++ lib/lp/code/enums.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Enumerations used in the lp/code modules."""
@@ -7,6 +7,7 @@
__all__ = [
'BranchLifecycleStatus',
'BranchLifecycleStatusFilter',
+ 'BranchListingSort',
'BranchMergeProposalStatus',
'BranchSubscriptionDiffSize',
'BranchSubscriptionNotificationLevel',
@@ -199,6 +200,69 @@
""")
+class BranchListingSort(EnumeratedType):
+ """Choices for how to sort branch listings."""
+
+ # XXX: MichaelHudson 2007-10-17 bug=153891: We allow sorting on quantities
+ # that are not visible in the listing!
+
+ DEFAULT = Item("""
+ by most interesting
+
+ Sort branches by the default ordering for the view.
+ """)
+
+ PRODUCT = Item("""
+ by project name
+
+ Sort branches by name of the project the branch is for.
+ """)
+
+ LIFECYCLE = Item("""
+ by status
+
+ Sort branches by their status.
+ """)
+
+ NAME = Item("""
+ by branch name
+
+ Sort branches by the name of the branch.
+ """)
+
+ OWNER = Item("""
+ by owner name
+
+ Sort branches by the name of the owner.
+ """)
+
+ MOST_RECENTLY_CHANGED_FIRST = Item("""
+ most recently changed first
+
+ Sort branches from the most recently to the least recently
+ changed.
+ """)
+
+ LEAST_RECENTLY_CHANGED_FIRST = Item("""
+ most neglected first
+
+ Sort branches from the least recently to the most recently
+ changed.
+ """)
+
+ NEWEST_FIRST = Item("""
+ newest first
+
+ Sort branches from newest to oldest.
+ """)
+
+ OLDEST_FIRST = Item("""
+ oldest first
+
+ Sort branches from oldest to newest.
+ """)
+
+
class BranchMergeProposalStatus(DBEnumeratedType):
"""Branch Merge Proposal Status
=== modified file 'lib/lp/code/feed/branch.py'
--- lib/lp/code/feed/branch.py 2015-07-08 16:05:11 +0000
+++ lib/lp/code/feed/branch.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Branch feed (syndication) views."""
@@ -15,23 +15,19 @@
'ProjectRevisionFeed',
]
-from storm.locals import (
- Asc,
- Desc,
- )
from z3c.ptcompat import ViewPageTemplateFile
from zope.component import getUtility
from zope.interface import implementer
from zope.security.interfaces import Unauthorized
from lp.code.browser.branch import BranchView
+from lp.code.enums import BranchListingSort
from lp.code.interfaces.branch import (
DEFAULT_BRANCH_STATUS_IN_LISTING,
IBranch,
)
from lp.code.interfaces.branchcollection import IAllBranches
from lp.code.interfaces.revisioncache import IRevisionCache
-from lp.code.model.branch import Branch
from lp.registry.interfaces.person import IPerson
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.projectgroup import IProjectGroup
@@ -165,11 +161,10 @@
"""
collection = self._getCollection().visibleByUser(
None).withLifecycleStatus(*DEFAULT_BRANCH_STATUS_IN_LISTING)
- branches = collection.getBranches(eager_load=False)
- return list(branches.order_by(
- Desc(Branch.date_last_modified), Asc(Branch.target_suffix),
- Desc(Branch.lifecycle_status), Asc(Branch.name)).config(
- limit=self.quantity))
+ branches = collection.getBranches(
+ eager_load=False,
+ sort_by=BranchListingSort.MOST_RECENTLY_CHANGED_FIRST)
+ return list(branches.config(limit=self.quantity))
class ProductBranchFeed(BranchListingFeed):
=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py 2015-10-12 12:58:32 +0000
+++ lib/lp/code/interfaces/branchcollection.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""A collection of branches.
@@ -58,16 +58,18 @@
of individuals and teams that own branches in this collection.
"""
- def getBranches(eager_load=False):
+ def getBranches(eager_load=False, sort_by=None):
"""Return a result set of all branches in this collection.
The returned result set will also join across the specified tables as
defined by the arguments to this function. These extra tables are
- joined specificly to allow the caller to sort on values not in the
+ joined specifically to allow the caller to sort on values not in the
Branch table itself.
:param eager_load: If True trigger eager loading of all the related
objects in the collection.
+ :param sort_by: an item from the `BranchListingSort` enumeration, or
+ None to return an unordered result set.
"""
def getBranchIds():
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py 2015-10-12 12:58:32 +0000
+++ lib/lp/code/model/branchcollection.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Implementations of `IBranchCollection`."""
@@ -18,6 +18,7 @@
)
from storm.expr import (
And,
+ Asc,
Count,
Desc,
In,
@@ -38,7 +39,10 @@
from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
from lp.bugs.model.bugbranch import BugBranch
from lp.bugs.model.bugtask import BugTask
-from lp.code.enums import BranchMergeProposalStatus
+from lp.code.enums import (
+ BranchListingSort,
+ BranchMergeProposalStatus,
+ )
from lp.code.interfaces.branch import user_has_special_branch_access
from lp.code.interfaces.branchcollection import (
IBranchCollection,
@@ -271,13 +275,52 @@
cache = caches[code_import.branchID]
cache.code_import = code_import
- def getBranches(self, find_expr=Branch, eager_load=False):
+ @staticmethod
+ def _convertListingSortToOrderBy(sort_by):
+ """Compute a value to pass to `order_by` on a collection of branches.
+
+ :param sort_by: an item from the `BranchListingSort` enumeration.
+ """
+ DEFAULT_BRANCH_LISTING_SORT = [
+ BranchListingSort.PRODUCT,
+ BranchListingSort.LIFECYCLE,
+ BranchListingSort.OWNER,
+ BranchListingSort.NAME,
+ ]
+
+ LISTING_SORT_TO_COLUMN = {
+ BranchListingSort.PRODUCT: (Asc, Branch.target_suffix),
+ BranchListingSort.LIFECYCLE: (Desc, Branch.lifecycle_status),
+ BranchListingSort.NAME: (Asc, Branch.name),
+ BranchListingSort.OWNER: (Asc, Branch.owner_name),
+ BranchListingSort.MOST_RECENTLY_CHANGED_FIRST: (
+ Desc, Branch.date_last_modified),
+ BranchListingSort.LEAST_RECENTLY_CHANGED_FIRST: (
+ Asc, Branch.date_last_modified),
+ BranchListingSort.NEWEST_FIRST: (Desc, Branch.date_created),
+ BranchListingSort.OLDEST_FIRST: (Asc, Branch.date_created),
+ }
+
+ order_by = map(
+ LISTING_SORT_TO_COLUMN.get, DEFAULT_BRANCH_LISTING_SORT)
+
+ if sort_by is not None and sort_by != BranchListingSort.DEFAULT:
+ direction, column = LISTING_SORT_TO_COLUMN[sort_by]
+ order_by = (
+ [(direction, column)] +
+ [sort for sort in order_by if sort[1] is not column])
+ return [direction(column) for direction, column in order_by]
+
+ def getBranches(self, find_expr=Branch, eager_load=False, sort_by=None):
"""See `IBranchCollection`."""
all_tables = set(
self._tables.values() + self._asymmetric_tables.values())
tables = [Branch] + list(all_tables)
expressions = self._getBranchExpressions()
resultset = self.store.using(*tables).find(find_expr, *expressions)
+ if sort_by is not None:
+ resultset = resultset.order_by(
+ *self._convertListingSortToOrderBy(sort_by))
def do_eager_load(rows):
branch_ids = set(branch.id for branch in rows)
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py 2015-10-12 12:58:32 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py 2017-06-15 01:08:14 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for branch collections."""
@@ -12,6 +12,10 @@
from operator import attrgetter
import pytz
+from storm.expr import (
+ Asc,
+ Desc,
+ )
from storm.store import (
EmptyResultSet,
Store,
@@ -25,6 +29,7 @@
from lp.app.interfaces.services import IService
from lp.code.enums import (
BranchLifecycleStatus,
+ BranchListingSort,
BranchMergeProposalStatus,
BranchSubscriptionDiffSize,
BranchSubscriptionNotificationLevel,
@@ -43,12 +48,15 @@
from lp.registry.enums import PersonVisibility
from lp.registry.interfaces.person import TeamMembershipPolicy
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.model.person import Owner
+from lp.registry.model.product import Product
from lp.services.database.interfaces import IStore
from lp.services.webapp.publisher import canonical_url
from lp.testing import (
person_logged_in,
run_with_login,
StormStatementRecorder,
+ TestCase,
TestCaseWithFactory,
)
from lp.testing.layers import (
@@ -150,6 +158,21 @@
self.assertTrue(branch.visibleByUser(someone))
self.assertThat(recorder, HasQueryCount(Equals(0)))
+ def test_getBranches_sort_by(self):
+ product_a = self.factory.makeProduct(name='a')
+ product_b = self.factory.makeProduct(name='b')
+ branch_a_b = self.factory.makeProductBranch(
+ product=product_a, name='b')
+ branch_b_a = self.factory.makeProductBranch(
+ product=product_b, name='a')
+ collection = GenericBranchCollection(self.store)
+ self.assertEqual(
+ [branch_a_b, branch_b_a],
+ list(collection.getBranches(sort_by=BranchListingSort.DEFAULT)))
+ self.assertEqual(
+ [branch_b_a, branch_a_b],
+ list(collection.getBranches(sort_by=BranchListingSort.NAME)))
+
def test_getBranchIds(self):
branch = self.factory.makeProductBranch()
self.factory.makeAnyBranch()
@@ -1378,3 +1401,57 @@
*DEFAULT_BRANCH_STATUS_IN_LISTING)
person_count, team_count = collection.ownerCounts()
self.assertEqual(1, person_count)
+
+
+class TestBranchCollectionConvertListingSortToOrderBy(TestCase):
+
+ DEFAULT_BRANCH_LISTING_SORT = [
+ Asc(Product.name),
+ Desc(Branch.lifecycle_status),
+ Asc(Owner.name),
+ Asc(Branch.name),
+ ]
+
+ def assertSortsEqual(self, sort_one, sort_two):
+ """Assert that one list of sort specs is equal to another."""
+ def sort_data(sort):
+ return sort.suffix, sort.expr
+ self.assertEqual(map(sort_data, sort_one), map(sort_data, sort_two))
+
+ def test_default(self):
+ """Test that passing None or DEFAULT results in the default list."""
+ self.assertSortsEqual(
+ self.DEFAULT_BRANCH_LISTING_SORT,
+ GenericBranchCollection._convertListingSortToOrderBy(None))
+ self.assertSortsEqual(
+ self.DEFAULT_BRANCH_LISTING_SORT,
+ GenericBranchCollection._convertListingSortToOrderBy(
+ BranchListingSort.DEFAULT))
+
+ def test_lifecycle(self):
+ """Test with an option that's part of the default sort.
+
+ Sorting on LIFECYCLE moves the lifecycle reference to the first
+ element of the output.
+ """
+ # Check that this isn't a no-op.
+ lifecycle_order = GenericBranchCollection._convertListingSortToOrderBy(
+ BranchListingSort.LIFECYCLE)
+ self.assertSortsEqual(
+ [Desc(Branch.lifecycle_status),
+ Asc(Product.name),
+ Asc(Owner.name),
+ Asc(Branch.name)], lifecycle_order)
+
+ def test_sort_on_column_not_in_default_sort_order(self):
+ """Test with an option that's not part of the default sort.
+
+ This should put the passed option first in the list, but leave the
+ rest the same.
+ """
+ registrant_order = (
+ GenericBranchCollection._convertListingSortToOrderBy(
+ BranchListingSort.OLDEST_FIRST))
+ self.assertSortsEqual(
+ [Asc(Branch.date_created)] + self.DEFAULT_BRANCH_LISTING_SORT,
+ registrant_order)
Follow ups
