launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/registry/browser/person.py'
> --- lib/lp/registry/browser/person.py	2015-03-05 11:39:06 +0000
> +++ lib/lp/registry/browser/person.py	2015-03-09 11:42:57 +0000
> @@ -2028,7 +2028,8 @@
>      def label(self):
>          return 'Team participation for ' + self.context.displayname
>  
> -    def _asParticipation(self, membership=None, team=None, via=None):
> +    def _asParticipation(self, membership=None, team=None, via=None,
> +                         mailing_list=None, subscription=None):
>          """Return a dict of participation information for the membership.
>  
>          Method requires membership or team, not both.
> @@ -2057,15 +2058,14 @@
>              # The member is a direct member; use the membership data.
>              team = membership.team
>              datejoined = membership.datejoined
> -            if membership.person == team.teamowner:
> +            if membership.personID == team.teamownerID:
>                  role = 'Owner'
>              elif membership.status == TeamMembershipStatus.ADMIN:
>                  role = 'Admin'
>              else:
>                  role = 'Member'
>  
> -        if team.mailing_list is not None and team.mailing_list.is_usable:
> -            subscription = team.mailing_list.getSubscription(self.context)
> +        if mailing_list is not None:
>              if subscription is None:
>                  subscribed = 'Not subscribed'
>              else:
> @@ -2081,28 +2081,37 @@
>      def active_participations(self):
>          """Return the participation information for active memberships."""
>          paths, memberships = self.context.getPathsToTeams()
> +        memberships = [
> +            membership for membership in memberships
> +            if check_permission('launchpad.View', membership.team)]
>          direct_teams = [membership.team for membership in memberships]
>          indirect_teams = [
>              team for team in paths.keys()
> -            if team not in direct_teams]
> +            if team not in direct_teams and
> +               check_permission('launchpad.View', team)]
>          participations = []
>  
> +        # Bulk-load mailing list subscriptions.
> +        subscriptions = getUtility(IMailingListSet).getSubscriptionsForTeams(
> +            self.context, direct_teams + indirect_teams)
> +
>          # First, create a participation for all direct memberships.
>          for membership in memberships:
> -            # Add a participation record for the membership if allowed.
> -            if check_permission('launchpad.View', membership.team):
> -                participations.append(
> -                    self._asParticipation(membership=membership))
> +            mailing_list, subscription = subscriptions.get(
> +                membership.team.id, (None, None))
> +            participations.append(self._asParticipation(
> +                membership=membership, mailing_list=mailing_list,
> +                subscription=subscription))
>  
>          # Second, create a participation for all indirect memberships,
>          # using the remaining paths.
>          for indirect_team in indirect_teams:
> -            if not check_permission('launchpad.View', indirect_team):
> -                continue
> +            mailing_list, subscription = subscriptions.get(
> +                indirect_team.id, (None, None))
>              participations.append(
>                  self._asParticipation(
> -                    via=paths[indirect_team],
> -                    team=indirect_team))
> +                    via=paths[indirect_team], team=indirect_team,
> +                    mailing_list=mailing_list, subscription=subscription))
>          return sorted(participations, key=itemgetter('displayname'))

I wonder if you can refactor this to create a single list of teams, some of which have a membership and some of which have a via? Would remove the duplicated check_permissions etc.

>  
>      @cachedproperty
> 
> === modified file 'lib/lp/registry/browser/tests/test_person.py'
> --- lib/lp/registry/browser/tests/test_person.py	2015-03-05 15:28:11 +0000
> +++ lib/lp/registry/browser/tests/test_person.py	2015-03-09 11:42:57 +0000
> @@ -47,6 +47,7 @@
>  from lp.services.identity.interfaces.emailaddress import IEmailAddressSet
>  from lp.services.log.logger import FakeLogger
>  from lp.services.mail import stub
> +from lp.services.propertycache import clear_property_cache
>  from lp.services.verification.interfaces.authtoken import LoginTokenType
>  from lp.services.verification.interfaces.logintoken import ILoginTokenSet
>  from lp.services.verification.tests.logintoken import get_token_url_from_email
> @@ -68,6 +69,7 @@
>      login_person,
>      monkey_patch,
>      person_logged_in,
> +    record_two_runs,
>      StormStatementRecorder,
>      TestCaseWithFactory,
>      )
> @@ -878,6 +880,26 @@
>          self.assertEqual(1, len(participations))
>          self.assertEqual(True, self.view.has_participations)
>  
> +    def test_mailing_list_subscriptions_query_count(self):
> +        # Additional mailing list subscriptions do not add additional queries.
> +        def create_subscriptions():
> +            direct_team = self.factory.makeTeam(members=[self.user])
> +            direct_list = self.factory.makeMailingList(
> +                direct_team, direct_team.teamowner)
> +            direct_list.subscribe(self.user)
> +            indirect_team = self.factory.makeTeam(members=[direct_team])
> +            indirect_list = self.factory.makeMailingList(
> +                indirect_team, indirect_team.teamowner)
> +            indirect_list.subscribe(self.user)
> +
> +        def get_participations():
> +            clear_property_cache(self.view)
> +            return list(self.view.active_participations)
> +
> +        recorder1, recorder2 = record_two_runs(
> +            get_participations, create_subscriptions, 5)
> +        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
> +
>  
>  class TestPersonRelatedPackagesView(TestCaseWithFactory):
>      """Test the related software view."""
> 
> === modified file 'lib/lp/registry/configure.zcml'
> --- lib/lp/registry/configure.zcml	2015-02-26 11:34:47 +0000
> +++ lib/lp/registry/configure.zcml	2015-03-09 11:42:57 +0000
> @@ -37,6 +37,7 @@
>                  id
>                  team
>                  person
> +                personID
>                  status
>                  proposed_by
>                  acknowledged_by
> 
> === modified file 'lib/lp/registry/interfaces/mailinglist.py'
> --- lib/lp/registry/interfaces/mailinglist.py	2013-01-07 02:40:55 +0000
> +++ lib/lp/registry/interfaces/mailinglist.py	2015-03-09 11:42:57 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Mailing list interfaces."""
> @@ -464,6 +464,17 @@
>          :raises AssertionError: When `team_name` is not a string.
>          """
>  
> +    def getSubscriptionsForTeams(person, teams):
> +        """Return a person's subscriptions to usable lists for a set of teams.
> +
> +        :param person: An `IPerson`.
> +        :param teams: A list of `ITeam`s.
> +        :return: A dictionary mapping team IDs to tuples of `IMailingList`
> +            IDs and `IMailingListSubscription` IDs; the second element will
> +            be None if a team has a list to which the person is not
> +            subscribed.
> +        """
> +
>      def getSubscribedAddresses(team_names):
>          """Return the set of subscribed email addresses for members.
>  
> 
> === modified file 'lib/lp/registry/interfaces/teammembership.py'
> --- lib/lp/registry/interfaces/teammembership.py	2013-01-07 02:40:55 +0000
> +++ lib/lp/registry/interfaces/teammembership.py	2015-03-09 11:42:57 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  """Team membership interfaces."""
> @@ -131,6 +131,7 @@
>          Reference(title=_("Member"), required=True, readonly=True,
>                    schema=Interface),  # Specified in interfaces/person.py.
>          exported_as='member')
> +    personID = Int(title=_("Person ID"), required=True, readonly=True)
>      proposed_by = Attribute(_('Proponent'))
>      reviewed_by = Attribute(
>          _("The team admin who approved/rejected the member."))
> 
> === modified file 'lib/lp/registry/model/mailinglist.py'
> --- lib/lp/registry/model/mailinglist.py	2013-06-20 05:50:00 +0000
> +++ lib/lp/registry/model/mailinglist.py	2015-03-09 11:42:57 +0000
> @@ -1,4 +1,4 @@
> -# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
> +# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  __metaclass__ = type
> @@ -100,6 +100,13 @@
>      PostedMessageStatus.APPROVAL_PENDING)
>  
>  
> +USABLE_STATUSES = (
> +    MailingListStatus.ACTIVE,
> +    MailingListStatus.MODIFIED,
> +    MailingListStatus.UPDATING,
> +    MailingListStatus.MOD_FAILED)
> +
> +
>  class MessageApproval(SQLBase):
>      """A held message."""
>  
> @@ -344,10 +351,7 @@
>      @property
>      def is_usable(self):
>          """See `IMailingList`."""
> -        return self.status in [MailingListStatus.ACTIVE,
> -                               MailingListStatus.MODIFIED,
> -                               MailingListStatus.UPDATING,
> -                               MailingListStatus.MOD_FAILED]
> +        return self.status in USABLE_STATUSES
>  
>      def _get_welcome_message(self):
>          return self.welcome_message_
> @@ -547,6 +551,24 @@
>              """ % sqlvalues(team_name),
>              clauseTables=['Person'])
>  
> +    def getSubscriptionsForTeams(self, person, teams):
> +        """See `IMailingListSet`."""
> +        store = IStore(MailingList)
> +        team_ids = set(map(operator.attrgetter("id"), teams))
> +        lists = dict(store.find(
> +            (MailingList.teamID, MailingList.id),
> +            MailingList.teamID.is_in(team_ids),
> +            MailingList.status.is_in(USABLE_STATUSES)))
> +        subscriptions = dict(store.find(
> +            (MailingListSubscription.mailing_listID,
> +             MailingListSubscription.id),
> +            MailingListSubscription.person == person,
> +            MailingListSubscription.mailing_listID.is_in(lists.values())))
> +        by_team = {}
> +        for team, mailing_list in lists.items():
> +            by_team[team] = (mailing_list, subscriptions.get(mailing_list))
> +        return by_team

Is there a particular reason for this to return two IDs, rather than just the MailingListSubscription object?

> +
>      def _getTeamIdsAndMailingListIds(self, team_names):
>          """Return a tuple of team and mailing list Ids for the team names."""
>          store = IStore(MailingList)
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/participation-query-count/+merge/252267
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.