From 298cd1844e19779f1b47255fea1a54923876afb0 Mon Sep 17 00:00:00 2001
From: Jieyou Xu <jieyouxu@outlook.com>
Date: Sat, 6 Sep 2025 14:47:25 +0800
Subject: [PATCH] Fix zulip group zulip-id availability check

The previous `validate_zulip_group_ids` check logic is not quite correct
in two ways:

1. The previous logic was too *strict*, in that it does not properly
   account for the case where a team member may not be included in any
   team-associated zulip user groups (such as via the `excluded-people`
   mechanism).
2. The previous logic I think was also too *loose*, in that it is
   possible to attach additional extra people via `extra-people`. It is
   possible to include a person whose `people/` entry contain only a
   github handle, and not also a zulip id.

# Example

Consider the `cloud-compute` marker-team:

```markdown
name = "cloud-compute"
kind = "marker-team"

[people]
leads = []
members = [
   "have_zulip_id",
   "no_zulip_id",
]

[[zulip-groups]]
name = "cloud-compute"
extra-people = [
   "extra_no_zulip_id",
]
excluded-people = [
   "no_zulip_id",
]
```

The "all zulip group members must have a zulip-id" validation should
fail, but not against `no_zulip_id`, because that member is not part of
any zulip groups associated with `cloud-compute` team. It *should* error
on `extra_no_zulip_id` however.
---
 src/validate.rs | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/src/validate.rs b/src/validate.rs
index f5885c34a..77b433f82 100644
--- a/src/validate.rs
+++ b/src/validate.rs
@@ -728,18 +728,29 @@ fn validate_zulip_group_ids(data: &Data, errors: &mut Vec<String>) {
         if groups.is_empty() || groups.iter().all(|g| !g.includes_team_members()) {
             return Ok(());
         }
-        wrapper(team.members(data)?.iter(), errors, |member, _| {
-            if let Some(member) = data.person(member) {
-                if member.zulip_id().is_none() {
-                    bail!(
-                        "person `{}` in '{}' is a member of a Zulip user group but has no Zulip id",
-                        member.github(),
-                        team.name()
-                    );
+
+        for group in groups {
+            wrapper(group.members().iter(), errors, |member, _| {
+                match member {
+                    ZulipMember::MemberWithId { .. } | ZulipMember::JustId(_) => {
+                        // 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()
+                        );
+                    }
                 }
-            }
-            Ok(())
-        });
+                Ok(())
+            });
+        }
+
         Ok(())
     });
 }
@@ -752,6 +763,7 @@ fn validate_zulip_stream_ids(data: &Data, errors: &mut Vec<String>) {
         if streams.is_empty() || streams.iter().all(|s| !s.includes_team_members()) {
             return Ok(());
         }
+
         wrapper(team.members(data)?.iter(), errors, |member, _| {
             if let Some(member) = data.person(member) {
                 if member.zulip_id().is_none() {