Require all people in Zulip user groups to have a Zulip id in their config

rylev · rylev · commit 52eaa791fe83 · 2023-03-25T14:40:07.000+01:00
diff --git a/rust_team_data/src/v1.rs b/rust_team_data/src/v1.rs
@@ -103,6 +103,8 @@ pub struct ZulipGroup {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
 pub enum ZulipGroupMember {
+    // TODO(rylev): this variant can be removed once
+    // it is verified that noone is relying on it
     Email(String),
     Id(usize),
 }
diff --git a/src/check_synced.rs b/src/check_synced.rs
@@ -24,11 +24,6 @@ fn check_zulip(data: &Data) -> Result<(), failure::Error> {
         .map(|g| (g.name.clone(), g))
         .collect::<HashMap<_, _>>();
     let remote_users = zulip.get_users()?;
-    let email_to_zulip_id = remote_users
-        .iter()
-        .cloned()
-        .map(|u| (u.email, u.user_id))
-        .collect::<HashMap<_, _>>();
     let zulip_id_to_name = remote_users
         .into_iter()
         .map(|u| (u.user_id, u.name))
@@ -44,16 +39,10 @@ fn check_zulip(data: &Data) -> Result<(), failure::Error> {
                 let mut remote_members = rg.members.iter().collect::<HashSet<_>>();
                 for local_member in local_group.members() {
                     let i = match local_member {
-                        ZulipGroupMember::Id(i) => *i,
-                        ZulipGroupMember::Email(e) => match email_to_zulip_id.get(e) {
-                            Some(i) => *i,
-                            None => {
-                                error!("No user on Zulip uses the email '{e}'");
-                                continue;
-                            }
-                        },
-                        ZulipGroupMember::Missing => {
-                            error!("Member of Zulip user group '{}' does not have an email or Zulip id", local_group.name());
+                        ZulipGroupMember::MemberWithId { zulip_id, .. } => *zulip_id,
+                        ZulipGroupMember::JustId(zulip_id) => *zulip_id,
+                        ZulipGroupMember::MemberWithoutId { github } => {
+                            error!("Member '{github}' of Zulip user group '{}' does not have a Zulip id", local_group.name());
                             continue;
                         }
                     };
diff --git a/src/schema.rs b/src/schema.rs
@@ -369,15 +369,14 @@ impl Team {
                 let member = data.person(member).ok_or_else(|| {
                     err_msg(format!("{} does not have a person configuration", member))
                 })?;
-                let member = match (member.zulip_id, member.email()) {
-                    (Some(zulip_id), _) => ZulipGroupMember::Id(zulip_id),
-                    (_, Email::Present(email)) => ZulipGroupMember::Email(email.to_string()),
-                    _ => ZulipGroupMember::Missing,
+                let member = match (member.github.clone(), member.zulip_id) {
+                    (github, Some(zulip_id)) => ZulipGroupMember::MemberWithId { github, zulip_id },
+                    (github, _) => ZulipGroupMember::MemberWithoutId { github },
                 };
                 group.members.push(member);
             }
             for &extra in &raw_group.extra_zulip_ids {
-                group.members.push(ZulipGroupMember::Id(extra));
+                group.members.push(ZulipGroupMember::JustId(extra));
             }
             groups.push(group);
         }
@@ -654,9 +653,9 @@ impl ZulipGroup {
 
 #[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
 pub(crate) enum ZulipGroupMember {
-    Id(usize),
-    Email(String),
-    Missing,
+    MemberWithId { github: String, zulip_id: usize },
+    JustId(usize),
+    MemberWithoutId { github: String },
 }
 
 fn default_true() -> bool {
diff --git a/src/static_api.rs b/src/static_api.rs
@@ -237,9 +237,13 @@ impl<'a> Generator<'a> {
                     members: members
                         .into_iter()
                         .filter_map(|m| match m {
-                            ZulipGroupMember::Email(e) => Some(v1::ZulipGroupMember::Email(e)),
-                            ZulipGroupMember::Id(i) => Some(v1::ZulipGroupMember::Id(i)),
-                            ZulipGroupMember::Missing => None,
+                            ZulipGroupMember::MemberWithId { zulip_id, .. } => {
+                                Some(v1::ZulipGroupMember::Id(zulip_id))
+                            }
+                            ZulipGroupMember::JustId(zulip_id) => {
+                                Some(v1::ZulipGroupMember::Id(zulip_id))
+                            }
+                            ZulipGroupMember::MemberWithoutId { .. } => None,
                         })
                         .collect(),
                 },
diff --git a/src/validate.rs b/src/validate.rs
@@ -609,13 +609,10 @@ fn validate_discord_team_members_have_discord_ids(data: &Data, errors: &mut Vec<
     });
 }
 
-/// Ensure every member of a team that has a Zulip group either has a Zulip id or an email address
+/// Ensure every member of a team that has a Zulip group has a Zulip id
 fn validate_zulip_users(data: &Data, zulip: &ZulipApi, errors: &mut Vec<String>) {
-    let (by_id, by_email) = match zulip.get_users() {
-        Ok(u) => (
-            u.iter().map(|u| u.user_id).collect::<HashSet<_>>(),
-            u.into_iter().map(|u| u.email).collect::<HashSet<_>>(),
-        ),
+    let by_id = match zulip.get_users() {
+        Ok(u) => u.iter().map(|u| u.user_id).collect::<HashSet<_>>(),
         Err(err) => {
             errors.push(format!("couldn't verify Zulip users: {}", err));
             return;
@@ -633,8 +630,15 @@ fn validate_zulip_users(data: &Data, zulip: &ZulipApi, errors: &mut Vec<String>)
             .members()
             .iter()
             .filter_map(|m| match m {
-                ZulipGroupMember::Id(i) if !by_id.contains(i) => Some(format!("{}", i)),
-                ZulipGroupMember::Email(e) if !by_email.contains(e) => Some(e.clone()),
+                ZulipGroupMember::MemberWithId { github, zulip_id }
+                    if !by_id.contains(zulip_id) =>
+                {
+                    Some(github.clone())
+                }
+                ZulipGroupMember::JustId(zulip_id) if !by_id.contains(zulip_id) => {
+                    Some(format!("ID: {zulip_id}"))
+                }
+                ZulipGroupMember::MemberWithoutId { github } => Some(github.clone()),
                 _ => None,
             })
             .collect::<HashSet<_>>();
@@ -649,7 +653,7 @@ fn validate_zulip_users(data: &Data, zulip: &ZulipApi, errors: &mut Vec<String>)
     })
 }
 
-/// Ensure every member of a team that has a Zulip group either has a Zulip id or an email address
+/// Ensure every member of a team that has a Zulip group either has a Zulip id
 fn validate_zulip_group_ids(data: &Data, errors: &mut Vec<String>) {
     wrapper(data.teams(), errors, |team, errors| {
         let groups = team.zulip_groups(data)?;
@@ -659,11 +663,9 @@ fn validate_zulip_group_ids(data: &Data, errors: &mut Vec<String>) {
         }
         wrapper(team.members(data).iter(), errors, |member, _| {
             if let Some(member) = data.person(member) {
-                if member.zulip_id().is_none()
-                    && matches!(member.email(), Email::Missing | Email::Disabled)
-                {
+                if member.zulip_id().is_none() {
                     bail!(
-                        "person `{}` in '{}' is a member of a Zulip user group but has no Zulip id nor an enabled email address",
+                        "person `{}` in '{}' is a member of a Zulip user group but has no Zulip id",
                         member.github(),
                         team.name()
                     );
diff --git a/src/zulip.rs b/src/zulip.rs
@@ -98,8 +98,6 @@ struct ZulipUsers {
 /// A single Zulip user
 #[derive(Clone, Deserialize, PartialEq, Eq, Hash)]
 pub(crate) struct ZulipUser {
-    #[serde(rename = "delivery_email")]
-    pub(crate) email: String,
     pub(crate) user_id: usize,
     #[serde(rename = "full_name")]
     pub(crate) name: String,
diff --git a/tests/static-api/_expected/v1/zulip-groups.json b/tests/static-api/_expected/v1/zulip-groups.json
@@ -7,7 +7,7 @@
           "id": 1234
         },
         {
-          "email": "user1@example.com"
+          "id": 4321
         }
       ]
     }
diff --git a/tests/static-api/_expected/v1/zulip-map.json b/tests/static-api/_expected/v1/zulip-map.json
@@ -1,6 +1,7 @@
 {
   "users": {
     "2": 2,
-    "1234": 0
+    "1234": 0,
+    "4321": 0
   }
 }
diff --git a/tests/static-api/people/user-1.toml b/tests/static-api/people/user-1.toml
@@ -3,3 +3,4 @@ github = 'user-1'
 github-id = 0
 email = "user1@example.com"
 discord-id = 1
+zulip-id = 4321

Original file line number	Diff line number	Diff line change
`@@ -103,6 +103,8 @@ pub struct ZulipGroup {`
`103`	`103`	`#[derive(Debug, Clone, Serialize, Deserialize)]`
`104`	`104`	`#[serde(rename_all = "snake_case")]`
`105`	`105`	`pub enum ZulipGroupMember {`
	`106`	`+ // TODO(rylev): this variant can be removed once`
	`107`	`+ // it is verified that noone is relying on it`
`106`	`108`	`Email(String),`
`107`	`109`	`Id(usize),`
`108`	`110`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`"id": 1234`
`8`	`8`	`},`
`9`	`9`	`{`
`10`		`- "email": "[email protected]"`
	`10`	`+ "id": 4321`
`11`	`11`	`}`
`12`	`12`	`]`
`13`	`13`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"users": {`
`3`	`3`	`"2": 2,`
`4`		`- "1234": 0`
	`4`	`+ "1234": 0,`
	`5`	`+ "4321": 0`
`5`	`6`	`}`
`6`	`7`	`}`