Skip to content

Commit 298cd18

Browse files
committed
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.
1 parent f632438 commit 298cd18

File tree

1 file changed

+23
-11
lines changed

1 file changed

+23
-11
lines changed

src/validate.rs

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -728,18 +728,29 @@ fn validate_zulip_group_ids(data: &Data, errors: &mut Vec<String>) {
728728
if groups.is_empty() || groups.iter().all(|g| !g.includes_team_members()) {
729729
return Ok(());
730730
}
731-
wrapper(team.members(data)?.iter(), errors, |member, _| {
732-
if let Some(member) = data.person(member) {
733-
if member.zulip_id().is_none() {
734-
bail!(
735-
"person `{}` in '{}' is a member of a Zulip user group but has no Zulip id",
736-
member.github(),
737-
team.name()
738-
);
731+
732+
for group in groups {
733+
wrapper(group.members().iter(), errors, |member, _| {
734+
match member {
735+
ZulipMember::MemberWithId { .. } | ZulipMember::JustId(_) => {
736+
// Okay, have zulip IDs.
737+
}
738+
ZulipMember::MemberWithoutId { github } => {
739+
// Bad, only github handle, no zulip ID, even though included in zulip user
740+
// group
741+
bail!(
742+
"person `{}` in '{}' is a member of a Zulip user group '{}' but has no \
743+
Zulip id",
744+
github,
745+
team.name(),
746+
group.name()
747+
);
748+
}
739749
}
740-
}
741-
Ok(())
742-
});
750+
Ok(())
751+
});
752+
}
753+
743754
Ok(())
744755
});
745756
}
@@ -752,6 +763,7 @@ fn validate_zulip_stream_ids(data: &Data, errors: &mut Vec<String>) {
752763
if streams.is_empty() || streams.iter().all(|s| !s.includes_team_members()) {
753764
return Ok(());
754765
}
766+
755767
wrapper(team.members(data)?.iter(), errors, |member, _| {
756768
if let Some(member) = data.person(member) {
757769
if member.zulip_id().is_none() {

0 commit comments

Comments
 (0)