launchpad-reviewers team mailing list archive

[Colin Watson <mp+413581@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
> index 1a0c380..1902f8a 100644
> --- a/lib/lp/registry/model/person.py
> +++ b/lib/lp/registry/model/person.py
> @@ -1342,6 +1342,48 @@ class Person(
>          self._inTeam_cache[team.id] = in_team
>          return in_team
>  
> +    def inAnyTeam(self, teams):
> +        """See `IPerson`."""
> +        if not teams:
> +            return False
> +
> +        # Translate each team name to an ITeam if we were passed
> +        # a list of teams.
> +        if all(isinstance(team, str) for team in teams):
> +            teams = PersonSet.getByNames(teams)
> +            if not teams:
> +                return False

You don't need the string-to-team conversion feature for the bulk method.  (Indeed it's pretty rarely used in general, and the original use case for it no longer exists, so it might be worth looking into dropping it from `Person.isTeam` at some point.)  I'd drop this, which would also allow you to drop `PersonSet.getByNames`.

> +
> +        team_ids = {team.id for team in teams if team.is_team}
> +        if self.id in team_ids:
> +            # A team is always a member of itself.
> +            return True
> +
> +        if self._inTeam_cache is None:
> +            # Initialize cache
> +            self._inTeam_cache = {}
> +        else:
> +            if any(self._inTeam_cache.get(team_id) for team_id in team_ids):
> +                return True
> +
> +        team_participations = IStore(TeamParticipation).find(
> +            TeamParticipation,
> +            And(
> +                TeamParticipation.teamID.is_in(team_ids),
> +                TeamParticipation.personID == self.id
> +            )
> +        )
> +
> +        in_any_team = False
> +
> +        for team_participation in team_participations:
> +            in_any_team = True
> +            if team_participation.team.id not in self._inTeam_cache:
> +                self._inTeam_cache[team_participation.team.id] = True

This should be lifted up to a bit earlier, I think.  Basic algorithm:

 1) find elements of `team_ids` that aren't in `_inTeam_cache`, and query for just those `TeamParticipation` rows
 2) populate cache with newly-retrieved `TeamParticipation` rows - remember to populate both True and False cases here, as otherwise the cache is significantly less effective!
 3) `return any(self._inTeam_cache.get(team_id) for team_id in team_ids)`

> +
> +        return in_any_team
> +
> +
>      def hasParticipationEntryFor(self, team):
>          """See `IPerson`."""
>          return bool(TeamParticipation.selectOneBy(person=self, team=team))
> diff --git a/lib/lp/registry/model/personroles.py b/lib/lp/registry/model/personroles.py
> index f8bf273..bfb631f 100644
> --- a/lib/lp/registry/model/personroles.py
> +++ b/lib/lp/registry/model/personroles.py
> @@ -71,3 +72,15 @@ class PersonRoles:
>              if self.inTeam(role):
>                  return True
>          return False
> +
> +    def hasAnyRole(self, bug_target_roles):
> +        """See IPersonRoles."""
> +        teams = []
> +        for bug_target, roles in bug_target_roles.items():
> +            for role_ in roles:
> +                role = getattr(bug_target, role_)
> +                if role:
> +                    teams.append(role)
> +        if self.inAnyTeam(teams):
> +            return True
> +        return False

This is specific to bug targets, so I don't think it belongs in `PersonRoles`.  Can you move this back to the security adapter?



-- 
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/413581
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:optimize-has-any-bug-role-bulk-team-membership-checks into launchpad:master.