launchpad-reviewers team mailing list archive

[Colin Watson <mp+455245@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:refactor-team-membership-active-states into launchpad:master.

Commit message:
Use ACTIVE_STATES in more places for team membership status checks

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/455245

This provides better abstraction than writing out something like `(TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED)` in lots of places.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:refactor-team-membership-active-states into launchpad:master.

diff --git a/lib/lp/registry/browser/team.py b/lib/lp/registry/browser/team.py
index 5674164..67c91b2 100644
--- a/lib/lp/registry/browser/team.py
+++ b/lib/lp/registry/browser/team.py
@@ -115,6 +115,7 @@ from lp.registry.interfaces.person import (
 from lp.registry.interfaces.poll import IPollSet
 from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.interfaces.teammembership import (
+    ACTIVE_STATES,
     DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT,
     CyclicalTeamMembershipError,
     ITeamMembership,
@@ -1408,8 +1409,6 @@ class TeamMembershipSelfRenewalView(LaunchpadFormView):
         """Return text describing why the membership can't be renewed."""
         context = self.context
         ondemand = TeamMembershipRenewalPolicy.ONDEMAND
-        admin = TeamMembershipStatus.ADMIN
-        approved = TeamMembershipStatus.APPROVED
         # We add a grace period of one day to the limit to
         # cover the fencepost error when `date_limit` is
         # earlier than `self.dateexpires`, which happens later
@@ -1417,7 +1416,7 @@ class TeamMembershipSelfRenewalView(LaunchpadFormView):
         date_limit = datetime.now(timezone.utc) + timedelta(
             days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT + 1
         )
-        if context.status not in (admin, approved):
+        if context.status not in ACTIVE_STATES:
             text = "it is not active."
         elif context.team.renewal_policy != ondemand:
             text = (
diff --git a/lib/lp/registry/browser/teammembership.py b/lib/lp/registry/browser/teammembership.py
index e554738..07ccaa2 100644
--- a/lib/lp/registry/browser/teammembership.py
+++ b/lib/lp/registry/browser/teammembership.py
@@ -18,7 +18,10 @@ from zope.schema import Date
 from lp import _
 from lp.app.errors import UnexpectedFormData
 from lp.app.widgets.date import DateWidget
-from lp.registry.interfaces.teammembership import TeamMembershipStatus
+from lp.registry.interfaces.teammembership import (
+    ACTIVE_STATES,
+    TeamMembershipStatus,
+)
 from lp.services.webapp import LaunchpadView, canonical_url
 from lp.services.webapp.breadcrumb import Breadcrumb
 
@@ -92,10 +95,7 @@ class TeamMembershipEditView(LaunchpadView):
 
     # Boolean helpers
     def isActive(self):
-        return self.context.status in [
-            TeamMembershipStatus.APPROVED,
-            TeamMembershipStatus.ADMIN,
-        ]
+        return self.context.status in ACTIVE_STATES
 
     def isInactive(self):
         return self.context.status in [
diff --git a/lib/lp/registry/mail/teammembership.py b/lib/lp/registry/mail/teammembership.py
index e92b439..b305930 100644
--- a/lib/lp/registry/mail/teammembership.py
+++ b/lib/lp/registry/mail/teammembership.py
@@ -13,6 +13,7 @@ from zope.component import getUtility
 from lp.app.browser.tales import DurationFormatterAPI
 from lp.registry.enums import TeamMembershipPolicy, TeamMembershipRenewalPolicy
 from lp.registry.interfaces.teammembership import (
+    ACTIVE_STATES,
     ITeamMembershipSet,
     TeamMembershipStatus,
 )
@@ -135,10 +136,7 @@ class TeamMembershipMailer(BaseMailer):
         notification_type = "team-membership-new"
         recipients = OrderedDict()
         reviewer = membership.proposed_by
-        if reviewer != member and membership.status in [
-            TeamMembershipStatus.APPROVED,
-            TeamMembershipStatus.ADMIN,
-        ]:
+        if reviewer != member and membership.status in ACTIVE_STATES:
             reviewer = membership.reviewed_by
             # Somebody added this person as a member, we better send a
             # notification to the person too.
@@ -161,10 +159,7 @@ class TeamMembershipMailer(BaseMailer):
         # Open teams do not notify admins about new members.
         if team.membership_policy != TeamMembershipPolicy.OPEN:
             reply_to = None
-            if membership.status in [
-                TeamMembershipStatus.APPROVED,
-                TeamMembershipStatus.ADMIN,
-            ]:
+            if membership.status in ACTIVE_STATES:
                 template_name = "new-member-notification-for-admins.txt"
                 subject = "%s joined %s" % (member.name, team.name)
             elif membership.status == TeamMembershipStatus.PROPOSED:
diff --git a/lib/lp/registry/model/person.py b/lib/lp/registry/model/person.py
index a99809e..f166706 100644
--- a/lib/lp/registry/model/person.py
+++ b/lib/lp/registry/model/person.py
@@ -169,6 +169,7 @@ from lp.registry.interfaces.ssh import (
     SSHKeyType,
 )
 from lp.registry.interfaces.teammembership import (
+    ACTIVE_STATES,
     IJoinTeamEvent,
     ITeamInvitationEvent,
     TeamMembershipStatus,
@@ -1035,9 +1036,7 @@ class Person(
         clauses = (
             TeamMembership.team_id == team.id,
             TeamMembership.person_id == Person.id,
-            TeamMembership.status.is_in(
-                (TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED)
-            ),
+            TeamMembership.status.is_in(ACTIVE_STATES),
             TeamParticipation.team_id == Person.id,
             TeamParticipation.person_id == self.id,
         )
@@ -2505,9 +2504,7 @@ class Person(
     def api_activemembers(self):
         """See `IPerson`."""
         return self._members(
-            direct=True,
-            status=(TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN),
-            preload_for_api=True,
+            direct=True, status=ACTIVE_STATES, preload_for_api=True
         )
 
     @property
@@ -2545,12 +2542,7 @@ class Person(
             TeamMembership,
             TeamMembership.person == self,
             TeamMembership.team_id == Team.id,
-            TeamMembership.status.is_in(
-                (
-                    TeamMembershipStatus.APPROVED,
-                    TeamMembershipStatus.ADMIN,
-                )
-            ),
+            TeamMembership.status.is_in(ACTIVE_STATES),
         ).order_by(Upper(Team.display_name), Upper(Team.name))
 
     def anyone_can_join(self):
@@ -2879,9 +2871,7 @@ class Person(
     @property
     def member_memberships(self):
         """See `IPerson`."""
-        return self._getMembershipsByStatuses(
-            [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
-        )
+        return self._getMembershipsByStatuses(ACTIVE_STATES)
 
     def getInactiveMemberships(self):
         """See `IPerson`."""
@@ -2946,12 +2936,7 @@ class Person(
                 [team.id for team in teams] + [self.id]
             ),
             TeamMembership.team != self,
-            TeamMembership.status.is_in(
-                (
-                    TeamMembershipStatus.APPROVED,
-                    TeamMembershipStatus.ADMIN,
-                )
-            ),
+            TeamMembership.status.is_in(ACTIVE_STATES),
         ).order_by(Desc(TeamMembership.datejoined), Desc(TeamMembership.id))
         # Cast the results to list now, because they will be iterated over
         # several times.
@@ -3009,12 +2994,7 @@ class Person(
                 And(
                     TeamMembership.person == self.id,
                     TeamMembership.team_id == TeamParticipation.team_id,
-                    TeamMembership.status.is_in(
-                        [
-                            TeamMembershipStatus.APPROVED,
-                            TeamMembershipStatus.ADMIN,
-                        ]
-                    ),
+                    TeamMembership.status.is_in(ACTIVE_STATES),
                 ),
             ),
         ]
@@ -5597,12 +5577,7 @@ def _get_recipients_for_team(team):
         # account, or are a team, or both.
         intermediate_transitive_results = source.find(
             (TeamMembership.person_id, EmailAddress.person_id),
-            TeamMembership.status.is_in(
-                (
-                    TeamMembershipStatus.ADMIN,
-                    TeamMembershipStatus.APPROVED,
-                )
-            ),
+            TeamMembership.status.is_in(ACTIVE_STATES),
             TeamMembership.team_id.is_in(pending_team_ids),
             Or(
                 And(
diff --git a/lib/lp/registry/model/teammembership.py b/lib/lp/registry/model/teammembership.py
index 0d5396b..b53350e 100644
--- a/lib/lp/registry/model/teammembership.py
+++ b/lib/lp/registry/model/teammembership.py
@@ -113,8 +113,6 @@ class TeamMembership(StormBase):
     def canBeRenewedByMember(self):
         """See `ITeamMembership`."""
         ondemand = TeamMembershipRenewalPolicy.ONDEMAND
-        admin = TeamMembershipStatus.APPROVED
-        approved = TeamMembershipStatus.ADMIN
         # We add a grace period of one day to the limit to
         # cover the fencepost error when `date_limit` is
         # earlier than `self.dateexpires`, which happens later
@@ -123,7 +121,7 @@ class TeamMembership(StormBase):
             days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT + 1
         )
         return (
-            self.status in (admin, approved)
+            self.status in ACTIVE_STATES
             and self.team.renewal_policy == ondemand
             and self.dateexpires is not None
             and self.dateexpires < date_limit
@@ -338,7 +336,7 @@ class TeamMembershipSet:
         tm.proposed_by = user
         tm.date_proposed = now
         tm.proponent_comment = comment
-        if status in [approved, admin]:
+        if status in ACTIVE_STATES:
             tm.datejoined = now
             tm.reviewed_by = user
             tm.date_reviewed = now
@@ -372,9 +370,7 @@ class TeamMembershipSet:
             when = datetime.now(timezone.utc)
         conditions = [
             TeamMembership.dateexpires <= when,
-            TeamMembership.status.is_in(
-                [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
-            ),
+            TeamMembership.status.is_in(ACTIVE_STATES),
         ]
         return IStore(TeamMembership).find(TeamMembership, *conditions)
 
@@ -401,9 +397,7 @@ class TeamMembershipSet:
         return IStore(TeamMembership).find(
             TeamMembership,
             Func("date_trunc", "day", TeamMembership.dateexpires).is_in(dates),
-            TeamMembership.status.is_in(
-                [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED]
-            ),
+            TeamMembership.status.is_in(ACTIVE_STATES),
         )
 
     def deactivateActiveMemberships(self, team, comment, reviewer):
diff --git a/lib/lp/registry/personmerge.py b/lib/lp/registry/personmerge.py
index cd412e9..151ff30 100644
--- a/lib/lp/registry/personmerge.py
+++ b/lib/lp/registry/personmerge.py
@@ -20,6 +20,7 @@ from lp.registry.interfaces.mailinglist import (
 )
 from lp.registry.interfaces.personnotification import IPersonNotificationSet
 from lp.registry.interfaces.teammembership import (
+    ACTIVE_STATES,
     ITeamMembershipSet,
     TeamMembershipStatus,
 )
@@ -620,11 +621,10 @@ def _mergeCodeReviewVote(cur, from_id, to_id):
 
 def _mergeTeamMembership(cur, from_id, to_id):
     # Transfer active team memberships
-    approved = TeamMembershipStatus.APPROVED
     admin = TeamMembershipStatus.ADMIN
     cur.execute(
         "SELECT team, status FROM TeamMembership WHERE person = %s "
-        "AND status IN (%s,%s)" % sqlvalues(from_id, approved, admin)
+        "AND status IN %s" % sqlvalues(from_id, ACTIVE_STATES)
     )
     for team_id, status in cur.fetchall():
         cur.execute(
@@ -650,7 +650,7 @@ def _mergeTeamMembership(cur, from_id, to_id):
             # while from_person is either admin or approved. That means we
             # can safely set from_person's membership status on
             # to_person's membership.
-            assert status in (approved.value, admin.value)
+            assert status in {s.value for s in ACTIVE_STATES}
             cur.execute(
                 "UPDATE TeamMembership SET status = %s WHERE person = %s "
                 "AND team = %s" % sqlvalues(status, to_id, team_id)