launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04390
lp:~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798873 in Launchpad itself: "Being able to filter by subscribed packages for a given team or user."
https://bugs.launchpad.net/launchpad/+bug/798873
For more details, see:
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873/+merge/69349
This branch adds a changed_by parameter to DistroSeriesDifference
.getForDistroSeries(). It can be a person, a team, or a collection of
either. The effect is to restrict the results to only those DSDs where
the associated SPR was created by one of the given persons or teams
(well, members of).
There's quite a lot in the diff but it's not too complicated if you
bear two things in mind:
- The local function eager_load() has been moved from the body of
getForDistroSeries() into a module-level function called
eager_load_dsds(). The implementation has not changed at all.
- Limiting the results was done using an INTERSECT. From the code:
# Identify all DSDs referring to SPRs created by changed_by for
# this distroseries. The set of DSDs for the given distroseries
# can then be discovered as the intersection between this set and
# the already established differences.
An alternative would be to do an IN, in the same way that the name
filter and packagesets filter have been done.
--
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873/+merge/69349
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/localpackagediffs-filter-by-person-or-team-bug-798873 into lp:launchpad.
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2011-07-22 15:47:49 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2011-07-26 19:43:09 +0000
@@ -66,12 +66,16 @@
IDistroSeriesDifferenceCommentSource,
)
from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet
-from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.person import (
+ IPerson,
+ IPersonSet,
+ )
from lp.registry.model.distroseriesdifferencecomment import (
DistroSeriesDifferenceComment,
)
from lp.registry.model.gpgkey import GPGKey
from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.registry.model.teammembership import TeamParticipation
from lp.services.database import bulk
from lp.services.database.stormbase import StormBase
from lp.services.messages.model.message import (
@@ -106,6 +110,12 @@
from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+ACTIVE_STATUSES = (
+ PackagePublishingStatus.PUBLISHED,
+ PackagePublishingStatus.PENDING,
+ )
+
+
def most_recent_publications(dsds, in_parent, statuses, match_version=False):
"""The most recent publications for the given `DistroSeriesDifference`s.
@@ -262,6 +272,108 @@
return grouped
+def eager_load_dsds(dsds):
+ """Eager load dependencies of the given `DistroSeriesDifference`s.
+
+ :param dsds: A concrete sequence (i.e. not a generator) of
+ `DistroSeriesDifference` to eager load for.
+ """
+ source_pubs = dict(
+ most_recent_publications(
+ dsds, statuses=ACTIVE_STATUSES,
+ in_parent=False, match_version=False))
+ parent_source_pubs = dict(
+ most_recent_publications(
+ dsds, statuses=ACTIVE_STATUSES,
+ in_parent=True, match_version=False))
+ source_pubs_for_release = dict(
+ most_recent_publications(
+ dsds, statuses=ACTIVE_STATUSES,
+ in_parent=False, match_version=True))
+ parent_source_pubs_for_release = dict(
+ most_recent_publications(
+ dsds, statuses=ACTIVE_STATUSES,
+ in_parent=True, match_version=True))
+
+ latest_comment_by_dsd_id = dict(
+ (comment.distro_series_difference_id, comment)
+ for comment in most_recent_comments(dsds))
+ latest_comments = latest_comment_by_dsd_id.values()
+
+ # SourcePackageReleases of the parent and source pubs are often
+ # referred to.
+ sprs = bulk.load_related(
+ SourcePackageRelease, chain(
+ source_pubs.itervalues(),
+ parent_source_pubs.itervalues(),
+ source_pubs_for_release.itervalues(),
+ parent_source_pubs_for_release.itervalues()),
+ ("sourcepackagereleaseID",))
+
+ # Get packagesets and parent_packagesets for each DSD.
+ dsd_packagesets = get_packagesets(dsds, in_parent=False)
+ dsd_parent_packagesets = get_packagesets(dsds, in_parent=True)
+
+ # Cache latest messages contents (MessageChunk).
+ messages = bulk.load_related(
+ Message, latest_comments, ['message_id'])
+ chunks = message_chunks(messages)
+ for msg in messages:
+ cache = get_property_cache(msg)
+ cache.text_contents = Message.chunks_text(
+ chunks.get(msg.id, []))
+
+ for dsd in dsds:
+ spn_id = dsd.source_package_name_id
+ cache = get_property_cache(dsd)
+ cache.source_pub = source_pubs.get(spn_id)
+ cache.parent_source_pub = parent_source_pubs.get(spn_id)
+ cache.packagesets = dsd_packagesets.get(dsd.id)
+ cache.parent_packagesets = dsd_parent_packagesets.get(dsd.id)
+ if spn_id in source_pubs_for_release:
+ spph = source_pubs_for_release[spn_id]
+ cache.source_package_release = (
+ DistroSeriesSourcePackageRelease(
+ dsd.derived_series,
+ spph.sourcepackagerelease))
+ else:
+ cache.source_package_release = None
+ if spn_id in parent_source_pubs_for_release:
+ spph = parent_source_pubs_for_release[spn_id]
+ cache.parent_source_package_release = (
+ DistroSeriesSourcePackageRelease(
+ dsd.parent_series, spph.sourcepackagerelease))
+ else:
+ cache.parent_source_package_release = None
+ cache.latest_comment = latest_comment_by_dsd_id.get(dsd.id)
+
+ # SourcePackageRelease.uploader can end up getting the requester
+ # for a source package recipe build.
+ sprbs = bulk.load_related(
+ SourcePackageRecipeBuild, sprs,
+ ("source_package_recipe_build_id",))
+
+ # SourcePackageRelease.uploader can end up getting the owner of
+ # the DSC signing key.
+ gpgkeys = bulk.load_related(GPGKey, sprs, ("dscsigningkeyID",))
+
+ # Load DistroSeriesDifferenceComment owners,
+ # SourcePackageRecipeBuild requesters and GPGKey owners, and
+ # SourcePackageRelease creators.
+ person_ids = set().union(
+ (dsdc.message.ownerID for dsdc in latest_comments),
+ (sprb.requester_id for sprb in sprbs),
+ (gpgkey.ownerID for gpgkey in gpgkeys),
+ (spr.creatorID for spr in sprs))
+ uploaders = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+ person_ids, need_validity=True)
+ list(uploaders)
+
+ # Load SourcePackageNames.
+ bulk.load_related(
+ SourcePackageName, dsds, ("source_package_name_id",))
+
+
class DistroSeriesDifference(StormBase):
"""See `DistroSeriesDifference`."""
implements(IDistroSeriesDifference)
@@ -341,31 +453,44 @@
status=None,
child_version_higher=False,
parent_series=None,
- packagesets=None):
+ packagesets=None,
+ changed_by=None):
"""See `IDistroSeriesDifferenceSource`."""
if difference_type is None:
difference_type = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
if status is None:
- status = (
- DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
- )
+ status = (DistroSeriesDifferenceStatus.NEEDS_ATTENTION,)
elif isinstance(status, DBItem):
status = (status, )
+ if IPerson.providedBy(changed_by):
+ changed_by = (changed_by,)
+
+ # Aliases, to improve readability.
+ DSD = DistroSeriesDifference
+ PSS = PackagesetSources
+ SPN = SourcePackageName
+ SPPH = SourcePackagePublishingHistory
+ SPR = SourcePackageRelease
+ TP = TeamParticipation
conditions = [
- DistroSeriesDifference.derived_series == distro_series,
- DistroSeriesDifference.difference_type == difference_type,
- DistroSeriesDifference.status.is_in(status),
- DistroSeriesDifference.source_package_name ==
- SourcePackageName.id,
- ]
+ DSD.derived_series == distro_series,
+ DSD.difference_type == difference_type,
+ DSD.source_package_name == SPN.id, # For ordering.
+ DSD.status.is_in(status),
+ ]
+
+ if child_version_higher:
+ conditions.append(DSD.source_version > DSD.parent_source_version)
if parent_series:
- conditions.extend([
- DistroSeriesDifference.parent_series == parent_series.id])
+ conditions.append(DSD.parent_series == parent_series.id)
+
+ # Take a copy of the conditions specified thus far.
+ basic_conditions = list(conditions)
if name_filter:
- name_matches = [SourcePackageName.name == name_filter]
+ name_matches = [SPN.name == name_filter]
try:
packageset = getUtility(IPackagesetSet).getByName(
name_filter, distroseries=distro_series)
@@ -373,131 +498,49 @@
packageset = None
if packageset is not None:
name_matches.append(
- DistroSeriesDifference.source_package_name_id.is_in(
- Select(
- PackagesetSources.sourcepackagename_id,
- PackagesetSources.packageset == packageset)))
- conditions.extend([Or(*name_matches)])
-
- if child_version_higher:
- conditions.extend([
- DistroSeriesDifference.source_version >
- DistroSeriesDifference.parent_source_version])
+ DSD.source_package_name_id.is_in(
+ Select(PSS.sourcepackagename_id,
+ PSS.packageset == packageset)))
+ conditions.append(Or(*name_matches))
if packagesets is not None:
set_ids = [packageset.id for packageset in packagesets]
- conditions.extend([
- DistroSeriesDifference.source_package_name_id.is_in(
- Select(
- PackagesetSources.sourcepackagename_id,
- PackagesetSources.packageset_id.is_in(set_ids)))])
-
- differences = IStore(DistroSeriesDifference).find(
- DistroSeriesDifference,
- And(*conditions)).order_by(SourcePackageName.name)
-
- def eager_load(dsds):
- active_statuses = (
- PackagePublishingStatus.PUBLISHED,
- PackagePublishingStatus.PENDING,
- )
- source_pubs = dict(
- most_recent_publications(
- dsds, statuses=active_statuses,
- in_parent=False, match_version=False))
- parent_source_pubs = dict(
- most_recent_publications(
- dsds, statuses=active_statuses,
- in_parent=True, match_version=False))
- source_pubs_for_release = dict(
- most_recent_publications(
- dsds, statuses=active_statuses,
- in_parent=False, match_version=True))
- parent_source_pubs_for_release = dict(
- most_recent_publications(
- dsds, statuses=active_statuses,
- in_parent=True, match_version=True))
-
- latest_comment_by_dsd_id = dict(
- (comment.distro_series_difference_id, comment)
- for comment in most_recent_comments(dsds))
- latest_comments = latest_comment_by_dsd_id.values()
-
- # SourcePackageReleases of the parent and source pubs are often
- # referred to.
- sprs = bulk.load_related(
- SourcePackageRelease, chain(
- source_pubs.itervalues(),
- parent_source_pubs.itervalues(),
- source_pubs_for_release.itervalues(),
- parent_source_pubs_for_release.itervalues()),
- ("sourcepackagereleaseID",))
-
- # Get packagesets and parent_packagesets for each DSD.
- dsd_packagesets = get_packagesets(dsds, in_parent=False)
- dsd_parent_packagesets = get_packagesets(dsds, in_parent=True)
-
- # Cache latest messages contents (MessageChunk).
- messages = bulk.load_related(
- Message, latest_comments, ['message_id'])
- chunks = message_chunks(messages)
- for msg in messages:
- cache = get_property_cache(msg)
- cache.text_contents = Message.chunks_text(
- chunks.get(msg.id, []))
-
- for dsd in dsds:
- spn_id = dsd.source_package_name_id
- cache = get_property_cache(dsd)
- cache.source_pub = source_pubs.get(spn_id)
- cache.parent_source_pub = parent_source_pubs.get(spn_id)
- cache.packagesets = dsd_packagesets.get(dsd.id)
- cache.parent_packagesets = dsd_parent_packagesets.get(dsd.id)
- if spn_id in source_pubs_for_release:
- spph = source_pubs_for_release[spn_id]
- cache.source_package_release = (
- DistroSeriesSourcePackageRelease(
- dsd.derived_series,
- spph.sourcepackagerelease))
- else:
- cache.source_package_release = None
- if spn_id in parent_source_pubs_for_release:
- spph = parent_source_pubs_for_release[spn_id]
- cache.parent_source_package_release = (
- DistroSeriesSourcePackageRelease(
- dsd.parent_series, spph.sourcepackagerelease))
- else:
- cache.parent_source_package_release = None
- cache.latest_comment = latest_comment_by_dsd_id.get(dsd.id)
-
- # SourcePackageRelease.uploader can end up getting the requester
- # for a source package recipe build.
- sprbs = bulk.load_related(
- SourcePackageRecipeBuild, sprs,
- ("source_package_recipe_build_id",))
-
- # SourcePackageRelease.uploader can end up getting the owner of
- # the DSC signing key.
- gpgkeys = bulk.load_related(GPGKey, sprs, ("dscsigningkeyID",))
-
- # Load DistroSeriesDifferenceComment owners,
- # SourcePackageRecipeBuild requesters and GPGKey owners, and
- # SourcePackageRelease creators.
- person_ids = set().union(
- (dsdc.message.ownerID for dsdc in latest_comments),
- (sprb.requester_id for sprb in sprbs),
- (gpgkey.ownerID for gpgkey in gpgkeys),
- (spr.creatorID for spr in sprs))
- uploaders = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
- person_ids, need_validity=True)
- list(uploaders)
-
- # Load SourcePackageNames.
- bulk.load_related(
- SourcePackageName, dsds, ("source_package_name_id",))
-
- return DecoratedResultSet(
- differences, pre_iter_hook=eager_load)
+ conditions.append(
+ DSD.source_package_name_id.is_in(
+ Select(PSS.sourcepackagename_id,
+ PSS.packageset_id.is_in(set_ids))))
+
+ store = IStore(DSD)
+ columns = (DSD, SPN.name)
+ differences = store.find(columns, And(*conditions))
+
+ if changed_by is not None:
+ # Identify all DSDs referring to SPRs created by changed_by for
+ # this distroseries. The set of DSDs for the given distroseries
+ # can then be discovered as the intersection between this set and
+ # the already established differences.
+ differences_changed_by_conditions = And(
+ basic_conditions,
+ SPPH.archiveID == distro_series.main_archive.id,
+ SPPH.distroseriesID == distro_series.id,
+ SPPH.sourcepackagereleaseID == SPR.id,
+ SPPH.status.is_in(ACTIVE_STATUSES),
+ SPR.creatorID == TP.personID,
+ SPR.sourcepackagenameID == DSD.source_package_name_id,
+ TP.teamID.is_in(person.id for person in changed_by))
+ differences_changed_by = store.find(
+ columns, differences_changed_by_conditions)
+ differences = differences.intersection(differences_changed_by)
+
+ differences = differences.order_by(SPN.name)
+
+ def pre_iter_hook(rows):
+ # Each row is (dsd, spn.name). Modify the results in place.
+ rows[:] = (dsd for (dsd, spn_name) in rows)
+ # Eager load everything to do with DSDs.
+ return eager_load_dsds(rows)
+
+ return DecoratedResultSet(differences, pre_iter_hook=pre_iter_hook)
@staticmethod
def getByDistroSeriesNameAndParentSeries(distro_series,
@@ -672,14 +715,9 @@
self.derived_series, self.source_version)
def _package_release(self, distro_series, version):
- statuses = (
- PackagePublishingStatus.PUBLISHED,
- PackagePublishingStatus.PENDING)
pubs = distro_series.main_archive.getPublishedSources(
- name=self.source_package_name.name,
- version=version,
- status=statuses,
- distroseries=distro_series,
+ name=self.source_package_name.name, version=version,
+ status=ACTIVE_STATUSES, distroseries=distro_series,
exact_match=True)
# Get the most recent publication (pubs are ordered by
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-07-08 09:06:24 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-07-26 19:43:09 +0000
@@ -982,10 +982,10 @@
self.makeDiffsForDistroSeries(derived_series)
diff_for_other_series = self.factory.makeDistroSeriesDifference()
- result = getUtility(IDistroSeriesDifferenceSource).getForDistroSeries(
- derived_series)
+ dsd_source = getUtility(IDistroSeriesDifferenceSource)
+ results = set(dsd_source.getForDistroSeries(derived_series))
- self.assertFalse(diff_for_other_series in result)
+ self.assertFalse(diff_for_other_series in results)
def test_getForDistroSeries_filters_by_type(self):
# Only differences for the specified types are returned.
@@ -1138,6 +1138,47 @@
[], dsd_source.getForDistroSeries(
dsd.derived_series, packagesets=(packageset, )))
+ def makeDistroSeriesDifferenceForUser(self, series, user):
+ dsd = self.factory.makeDistroSeriesDifference(derived_series=series)
+ removeSecurityProxy(dsd.source_package_release).creator = user
+ return dsd
+
+ def test_getForDistroSeries_filters_by_spr_creator(self):
+ # Specifiying changed_by limits the DSDs returned to those where the
+ # associated SPR was created by the given user or team.
+ megatron = self.factory.makePersonByName("Megatron")
+ alderney = self.factory.makePersonByName("Alderney")
+ bulgaria = self.factory.makePersonByName("Bulgaria")
+ # Create the derived distroseries and a DSD for each of the users
+ # above.
+ derived_distroseries = self.factory.makeDistroSeries()
+ dsd_megatron = self.makeDistroSeriesDifferenceForUser(
+ derived_distroseries, megatron)
+ dsd_alderney = self.makeDistroSeriesDifferenceForUser(
+ derived_distroseries, alderney)
+ dsd_bulgaria = self.makeDistroSeriesDifferenceForUser(
+ derived_distroseries, bulgaria)
+ # When changed_by is a person we see DSDs created only by that person.
+ dsd_source = getUtility(IDistroSeriesDifferenceSource)
+ self.assertContentEqual(
+ [dsd_alderney],
+ dsd_source.getForDistroSeries(
+ derived_distroseries, changed_by=alderney))
+ # When changed_by is a team we see DSDs created by any member of the
+ # team.
+ wombles = self.factory.makeTeam(members=(alderney, bulgaria))
+ self.assertContentEqual(
+ [dsd_alderney, dsd_bulgaria],
+ dsd_source.getForDistroSeries(
+ derived_distroseries, changed_by=wombles))
+ # When changed_by is not a person or team it is treated as a
+ # collection, and we see DSDs created by any person in the collection
+ # or member of a team in the collection.
+ self.assertContentEqual(
+ [dsd_alderney, dsd_bulgaria, dsd_megatron],
+ dsd_source.getForDistroSeries(
+ derived_distroseries, changed_by=(megatron, wombles)))
+
def test_getByDistroSeriesNameAndParentSeries(self):
# An individual difference is obtained using the name.
ds_diff = self.factory.makeDistroSeriesDifference(
References
