Merge pull request #2040 from Kobzol/require-zulip-id

Make Zulip IDs required

Author: marcoieni

config.toml

@@ -52,9 +52,32 @@ special-org-members = [
     "rust-log-analyzer",
     "rust-timer",
     "rustbot",
-
     # Ferrous systems employees who manage the github sponsors
     # for rust-analyzer.
     "MissHolmes",
     "skade",
 ]
+
+members-without-zulip-id = [
+    "arshiamufti",
+    "bnchi",
+    "brotzeit",
+    "celaus",
+    "chris-morgan",
+    "codesections",
+    "da-x",
+    "ecstatic-morse",
+    "gdr-at-ms",
+    "kennykerr",
+    "mathk",
+    "nico-abram",
+    "opeolluwa",
+    "patchfx",
+    "pvdrz",
+    "reitermarkus",
+    "ryankurte",
+    "Stammark",
+    "therealprof",
+    "U007D",
+    "zeenix"
+]

docs/toml-schema.md

@@ -9,7 +9,7 @@ The file structure is this:
 name = "John Doe"  # Display name of the person for the website (required)
 github = "johndoe"  # GitHub username of the person (required)
 github-id = 123456  # GitHub ID of the person (required)
-zulip-id = 123456   # Zulip ID of the person (optional)
+zulip-id = 123456   # Zulip ID of the person (required)
 discord-id = 123456 # Discord ID of the person (optional)
 # You can also set `email = false` to explicitly disable the email for the user.
 # This will, for example, avoid adding the person to the mailing lists.

src/schema.rs

@@ -15,6 +15,7 @@ pub(crate) struct Config {
     permissions_bools: HashSet<String>,
     // Use a BTreeSet for consistent ordering in tests
     special_org_members: BTreeSet<String>,
+    members_without_zulip_id: BTreeSet<String>,
 }
 
 impl Config {
@@ -41,6 +42,10 @@ impl Config {
     pub(crate) fn special_org_members(&self) -> &BTreeSet<String> {
         &self.special_org_members
     }
+
+    pub(crate) fn members_without_zulip_id(&self) -> &BTreeSet<String> {
+        &self.members_without_zulip_id
+    }
 }
 
 // This is an enum to allow two kinds of values for the email field:

src/validate.rs

@@ -49,6 +49,8 @@ static CHECKS: &[Check<fn(&Data, &mut Vec<String>)>] = checks![
     validate_zulip_group_extra_people,
     validate_unique_zulip_streams,
     validate_unique_zulip_user_ids,
+    validate_present_zulip_id,
+    validate_zulip_id_allowlist,
     validate_zulip_stream_ids,
     validate_zulip_stream_extra_people,
     validate_repos,
@@ -715,6 +717,53 @@ fn validate_unique_zulip_user_ids(data: &Data, errors: &mut Vec<String>) {
     })
 }
 
+/// Ensure that every active member except a few remaining people on an allowlist have a Zulip ID.
+fn validate_present_zulip_id(data: &Data, errors: &mut Vec<String>) {
+    wrapper(
+        data.active_members().unwrap().iter(),
+        errors,
+        |person, _| {
+            let person = data.person(person).expect("Person not found");
+            if person.zulip_id().is_none()
+                && !data
+                    .config()
+                    .members_without_zulip_id()
+                    .contains(person.github())
+            {
+                return Err(anyhow::anyhow!(
+                    "User {} does not have a Zulip ID",
+                    person.github()
+                ));
+            }
+            Ok(())
+        },
+    )
+}
+
+/// Ensure people in the missing Zulip ID allowlist do not actually have Zulip ID configured.
+fn validate_zulip_id_allowlist(data: &Data, errors: &mut Vec<String>) {
+    // Sort for deterministic output
+    let mut members = data
+        .config()
+        .members_without_zulip_id()
+        .iter()
+        .collect::<Vec<_>>();
+    members.sort();
+    for member in members {
+        let Some(person) = data.person(member) else {
+            errors.push(format!(
+                "Person {member} in Zulip ID allowlist does not exist"
+            ));
+            continue;
+        };
+        if person.zulip_id().is_some() {
+            errors.push(format!(
+                "Person {member} in Zulip ID allowlist has Zulip ID configured. Please remove them from the `members-without-zulip-id` list in `config.toml`"
+            ));
+        }
+    }
+}
+
 /// Ensure team members in Zulip groups have a Zulip id
 fn validate_zulip_group_ids(data: &Data, errors: &mut Vec<String>) {
     wrapper(data.teams(), errors, |team, errors| {
@@ -731,15 +780,17 @@ fn validate_zulip_group_ids(data: &Data, errors: &mut Vec<String>) {
                         // Okay, have zulip IDs.
                     }
                     ZulipMember::MemberWithoutId { github } => {
-                        // Bad, only github handle, no zulip ID, even though included in zulip user
-                        // group
-                        bail!(
-                            "person `{}` in '{}' is a member of a Zulip user group '{}' but has no \
-                            Zulip id",
-                            github,
-                            team.name(),
-                            group.name()
-                        );
+                        if !data.config().members_without_zulip_id().contains(github) {
+                            // Bad, only github handle, no zulip ID, even though included in zulip user
+                            // group
+                            bail!(
+                                "person `{}` in '{}' is a member of a Zulip user group '{}' but has no \
+                                Zulip id",
+                                github,
+                                team.name(),
+                                group.name()
+                            );
+                        }
                     }
                 }
                 Ok(())
@@ -764,11 +815,13 @@ fn validate_zulip_stream_ids(data: &Data, errors: &mut Vec<String>) {
                 match member {
                     ZulipMember::MemberWithId { .. } | ZulipMember::JustId(_) => {}
                     ZulipMember::MemberWithoutId { github } => {
-                        bail!(
-                            "person `{github}` is a member of a Zulip stream `{}` defined in team `{}`, but has no Zulip id",
-                            stream.name(),
-                            team.name()
-                        );
+                        if !data.config().members_without_zulip_id().contains(github) {
+                            bail!(
+                                "person `{github}` is a member of a Zulip stream `{}` defined in team `{}`, but has no Zulip id",
+                                stream.name(),
+                                team.name()
+                            );
+                        }
                     }
                 }
                 Ok(())

teams/all.toml

@@ -31,26 +31,7 @@ extra-emails = [
 
 [[zulip-groups]]
 name = "T-all"
-# Exclude the following people from the Zulip group for grandfathering purposes,
-# where previously we didn't require all project team members to have Zulip IDs
-# (since there wasn't a `T-all` Zulip group previously).
-excluded-people = [
-    "U007D",
-    "andrewpollack",
-    "bnchi",
-    "celaus",
-    "opeolluwa",
-]
 
 # Private channel with all team members, so that we can have an easy way of reaching them.
 [[zulip-streams]]
 name = "all/private"
-# Exclude the following people from the Zulip stream for grandfathering purposes,
-# where previously we didn't require all project team members to have Zulip IDs.
-excluded-people = [
-    "U007D",
-    "andrewpollack",
-    "bnchi",
-    "celaus",
-    "opeolluwa",
-]

tests/static-api/_expected/v1/zulip-map.json

@@ -1,6 +1,7 @@
 {
   "users": {
     "2": 2,
+    "6": 6,
     "1234": 0,
     "4321": 0
   }

tests/static-api/config.toml

@@ -22,3 +22,9 @@ permissions-bools = [
 special-org-members = [
     "test-bot",
 ]
+
+members-without-zulip-id = [
+    "test-admin",
+    "user-3",
+    "user-4"
+]

tests/static-api/people/user-6.toml

@@ -2,3 +2,4 @@ name = 'Sixth user'
 github = 'user-6'
 github-id = 6
 email = "[email protected]"
+zulip-id = 6