diff --git a/src/bin/project_goals.rs b/src/bin/project_goals.rs deleted file mode 100644 index c2542e4bd..000000000 --- a/src/bin/project_goals.rs +++ /dev/null @@ -1,40 +0,0 @@ -use clap::Parser; -use triagebot::team_data::TeamClient; -use triagebot::zulip::client::ZulipClient; -use triagebot::{github::GithubClient, handlers::project_goals}; - -/// A basic example -#[derive(Parser, Debug)] -struct Opt { - /// If specified, no messages are sent. - #[arg(long)] - dry_run: bool, - - /// Goals with an updated within this threshold will not be pinged. - days_threshold: i64, - - /// A string like "on Sep-5" when the update blog post will be written. - next_meeting_date: String, -} - -#[tokio::main(flavor = "current_thread")] -async fn main() -> anyhow::Result<()> { - dotenvy::dotenv().ok(); - tracing_subscriber::fmt::init(); - - let opt = Opt::parse(); - let gh = GithubClient::new_from_env(); - let zulip = ZulipClient::new_from_env(); - let team_api = TeamClient::new_from_env(); - project_goals::ping_project_goals_owners( - &gh, - &zulip, - &team_api, - opt.dry_run, - opt.days_threshold, - &opt.next_meeting_date, - ) - .await?; - - Ok(()) -} diff --git a/src/github/queries/mod.rs b/src/github/queries/mod.rs index a1bfa2f81..36e8b009b 100644 --- a/src/github/queries/mod.rs +++ b/src/github/queries/mod.rs @@ -1,5 +1,6 @@ pub(crate) mod issue_with_comments; pub(crate) mod least_recently_reviewed; +pub(crate) mod open_goal_issues; pub(crate) mod recent_commits; pub(crate) mod user_comments_in_org; pub(crate) mod user_contributions; diff --git a/src/github/queries/open_goal_issues.rs b/src/github/queries/open_goal_issues.rs new file mode 100644 index 000000000..4927ceac4 --- /dev/null +++ b/src/github/queries/open_goal_issues.rs @@ -0,0 +1,197 @@ +use anyhow::Context; + +use crate::github::GithubClient; + +const ORG: &str = "rust-lang"; +const REPO: &str = "rust-project-goals"; +const LABEL: &str = "C-tracking-issue"; + +pub struct GoalIssue { + pub number: u64, + pub title: String, + pub assignees: Vec, + pub created_at: chrono::DateTime, + pub labels: Vec, + pub last_comment: Option, +} + +pub struct LastGoalComment { + pub created_at: chrono::DateTime, +} + +#[derive(serde::Deserialize)] +struct GraphQlConnection { + nodes: Vec>, + #[serde(rename = "pageInfo")] + page_info: GraphQlPageInfo, +} + +#[derive(serde::Deserialize)] +struct GraphQlPageInfo { + #[serde(rename = "hasNextPage")] + has_next_page: bool, + #[serde(rename = "endCursor")] + end_cursor: Option, +} + +#[derive(serde::Deserialize)] +struct GraphQlIssue { + number: u64, + title: String, + #[serde(rename = "createdAt")] + created_at: chrono::DateTime, + assignees: GraphQlNodes, + labels: Option>, + comments: GraphQlNodes, +} + +#[derive(serde::Deserialize)] +struct GraphQlNodes { + nodes: Vec>, +} + +#[derive(serde::Deserialize)] +struct GraphQlUser { + login: String, +} + +#[derive(serde::Deserialize)] +struct GraphQlLabel { + name: String, +} + +#[derive(serde::Deserialize)] +struct GraphQlComment { + #[serde(rename = "createdAt")] + created_at: chrono::DateTime, +} + +impl From for GoalIssue { + fn from(issue: GraphQlIssue) -> Self { + let assignees = issue + .assignees + .nodes + .into_iter() + .flatten() + .map(|assignee| assignee.login) + .collect(); + + let labels = issue + .labels + .map(|labels| { + labels + .nodes + .into_iter() + .flatten() + .map(|label| label.name) + .collect() + }) + .unwrap_or_default(); + + let last_comment = issue + .comments + .nodes + .into_iter() + .flatten() + .next() + .map(|comment| LastGoalComment { + created_at: comment.created_at, + }); + + Self { + number: issue.number, + title: issue.title, + assignees, + created_at: issue.created_at, + labels, + last_comment, + } + } +} + +impl GithubClient { + /// Get every open tracking issue in `rust-lang/rust-project-goals`, + /// including the latest comment's date. + pub async fn open_goal_issues(&self) -> anyhow::Result> { + let mut cursor = None::; + let mut issues = Vec::new(); + + loop { + let mut response = self + .graphql_query( + r#" +query ( + $owner: String! + $repo: String! + $label: String! + $cursor: String +) { + repository(owner: $owner, name: $repo) { + issues( + first: 100 + after: $cursor + states: [OPEN] + labels: [$label] + orderBy: { + field: CREATED_AT + direction: ASC + } + ) { + nodes { + number + title + createdAt + assignees(first: 100) { + nodes { + login + } + } + labels(first: 100) { + nodes { + name + } + } + comments(last: 1) { + nodes { + createdAt + } + } + } + pageInfo { + hasNextPage + endCursor + } + } + } +} +"#, + serde_json::json!({ + "owner": ORG, + "repo": REPO, + "label": LABEL, + "cursor": cursor.as_deref(), + }), + ) + .await + .context("failed to fetch goal issues")?; + + let page = response + .pointer_mut("/data/repository/issues") + .context("data.repository.issues is missing from response")? + .take(); + + let page: GraphQlConnection = + serde_json::from_value(page).context("failed to deserialize page")?; + + issues.extend(page.nodes.into_iter().flatten().map(GoalIssue::from)); + + if !page.page_info.has_next_page { + break; + } + + cursor = page.page_info.end_cursor; + } + + Ok(issues) + } +} diff --git a/src/handlers/project_goals.rs b/src/handlers/project_goals.rs index 9941f0dad..b472d1a59 100644 --- a/src/handlers/project_goals.rs +++ b/src/handlers/project_goals.rs @@ -1,203 +1,925 @@ -use super::Context; -use crate::github::{ - self, GitHubUser, GithubClient, IssueCommentAction, IssueCommentEvent, IssuesAction, - IssuesEvent, +use crate::{ + github::{ + self, Event, GithubClient, Issue, IssueCommentAction, IssueCommentEvent, IssuesAction, + IssuesEvent, queries::open_goal_issues::GoalIssue, + }, + handlers::Context, + jobs::Job, + team_data::TeamClient, + zulip::{MessageApiRequest, api::Recipient, client::ZulipClient}, }; -use crate::github::{Event, Issue}; -use crate::jobs::Job; -use crate::team_data::TeamClient; -use crate::zulip::api::Recipient; -use crate::zulip::client::ZulipClient; -use anyhow::Context as _; use async_trait::async_trait; -use chrono::{Datelike, NaiveDate, Utc}; -use tracing::{self as log}; +use chrono::{DateTime, Datelike, Duration, NaiveDate, Utc, Weekday}; +use itertools::Itertools; +use std::collections::BTreeMap; +use tracing as log; -const MAX_ZULIP_TOPIC: usize = 60; const RUST_PROJECT_GOALS_REPO: &str = "rust-lang/rust-project-goals"; +const GOALS_TEAM: &str = "goals"; + +const FIRST_REPORT_GRACE_DAYS: i64 = 7; +const REPORT_LABELS: &[(&str, Period)] = &[ + ("R-every-week", Period::EveryWeek), + ("R-every-2-weeks", Period::Every2Weeks), + ("R-every-4-weeks", Period::Every4Weeks), +]; + const GOALS_STREAM: u64 = 435_869; // #project-goals -const C_TRACKING_ISSUE: &str = "C-tracking-issue"; - -fn message( - zulip_owners: &str, - days: &str, - issue_number: u64, - issue_title: &str, - next_update: &str, -) -> String { - format!( - r#" -Dear {zulip_owners}, it's been {days} days since the last update to your goal *{issue_title}*. +const GOALS_META_STREAM: u64 = 478_266; // #project-goals/meta +const TRIAGEBOT_TOPIC: &str = "triagebot reports"; +const MAX_ZULIP_TOPIC: usize = 60; + +/// The weekday of the job execution (must match [`crate::jobs`]). +const JOB_WEEKDAY: Weekday = Weekday::Thu; +/// The UTC hour of the job execution (must match [`crate::jobs`]). +const JOB_UTC_HOUR: u32 = 14; +/// The UTC minute of the job execution (must match [`crate::jobs`]). +const JOB_UTC_MINUTE: u32 = 0; + +/// An arbitrary date to keep reporting periods anchored. +/// +/// The phase of the biweekly and 4-week periods depends on this day. +const EPOCH: NaiveDate = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap(); + +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] +struct ZulipId(u64); + +impl ZulipId { + fn mention(self, muted: bool) -> String { + if muted { + format!("@_**|{}**", self.0) + } else { + format!("@**|{}**", self.0) + } + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] +struct GhUsername<'gh>(&'gh str); + +impl<'gh> GhUsername<'gh> { + fn link(self) -> String { + format!("[@{login}](https://github.com/{login})", login = self.0) + } + + fn team_link(self) -> String { + format!( + "[@{login}](https://github.com/rust-lang/team/tree/main/people/{login}.toml)", + login = self.0, + ) + } +} + +#[derive(Clone, Copy, Debug)] +enum OwnerContact { + Reachable(ZulipId), + MissingZulipId, + MissingTeamEntry, +} + +#[derive(Clone, Copy, Debug)] +struct Owner<'gh> { + github: GhUsername<'gh>, + contact: OwnerContact, +} + +impl<'gh> Owner<'gh> { + async fn from_username(team: &TeamClient, username: &'gh str) -> anyhow::Result { + Ok(Self { + github: GhUsername(username), + contact: match team.get_gh_id_from_username(username).await? { + Some(gh_id) => match team.github_to_zulip_id(gh_id).await? { + Some(zulip_id) => OwnerContact::Reachable(ZulipId(zulip_id)), + None => OwnerContact::MissingZulipId, + }, + None => OwnerContact::MissingTeamEntry, + }, + }) + } + + async fn from_id_and_username( + team: &TeamClient, + gh_id: u64, + username: &'gh str, + ) -> anyhow::Result { + Ok(Self { + github: GhUsername(username), + contact: match team.github_to_zulip_id(gh_id).await? { + Some(zulip_id) => OwnerContact::Reachable(ZulipId(zulip_id)), + None => OwnerContact::MissingZulipId, + }, + }) + } + + fn display_mention(self, muted: bool) -> String { + match self.contact { + OwnerContact::Reachable(zulip_id) => zulip_id.mention(muted), + OwnerContact::MissingZulipId | OwnerContact::MissingTeamEntry => self.github.link(), + } + } +} + +fn join_mentions(mentions: Vec) -> Option { + let joined = match mentions.as_slice() { + [] => return None, + [owner] => owner.clone(), + [first, second] => format!("{first} and {second}"), + [rest @ .., last] => format!("{}, and {last}", rest.iter().join(", ")), + }; + Some(joined) +} + +#[derive(Debug)] +struct Owners<'gh>(Vec>); + +impl<'gh> Owners<'gh> { + async fn resolve_goal(team: &TeamClient, issue: &'gh GoalIssue) -> anyhow::Result { + let mut owners = Vec::with_capacity(issue.assignees.len()); + for username in &issue.assignees { + owners.push(Owner::from_username(team, username).await?); + } + Ok(Self(owners)) + } + + async fn resolve_event(team: &TeamClient, issue: &'gh Issue) -> anyhow::Result { + let mut owners = Vec::with_capacity(issue.assignees.len()); + for assignee in &issue.assignees { + owners.push(Owner::from_id_and_username(team, assignee.id, &assignee.login).await?); + } + Ok(Self(owners)) + } + + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn has_multiple(&self) -> bool { + self.0.len() > 1 + } + + fn has_missing_zulip_id(&self) -> bool { + self.0 + .iter() + .any(|owner| matches!(owner.contact, OwnerContact::MissingZulipId)) + } + + fn has_missing_team_entry(&self) -> bool { + self.0 + .iter() + .any(|owner| matches!(owner.contact, OwnerContact::MissingTeamEntry)) + } + + fn has_problem(&self) -> bool { + self.has_multiple() || self.has_missing_zulip_id() || self.has_missing_team_entry() + } + + fn reachable(&self) -> impl Iterator + '_ { + self.0.iter().filter_map(|owner| match owner.contact { + OwnerContact::Reachable(zulip_id) => Some(zulip_id), + OwnerContact::MissingZulipId | OwnerContact::MissingTeamEntry => None, + }) + } + + fn missing_zulip_ids(&self) -> impl Iterator> + '_ { + self.0.iter().filter_map(|owner| { + matches!(owner.contact, OwnerContact::MissingZulipId).then_some(owner.github) + }) + } + + fn missing_team_entries(&self) -> impl Iterator> + '_ { + self.0.iter().filter_map(|owner| { + matches!(owner.contact, OwnerContact::MissingTeamEntry).then_some(owner.github) + }) + } + + fn all_mentions(&self, muted: bool) -> Option { + join_mentions( + self.0 + .iter() + .copied() + .map(|owner| owner.display_mention(muted)) + .collect_vec(), + ) + } + + fn reachable_mentions(&self) -> Option { + join_mentions(self.reachable().map(|id| id.mention(true)).collect_vec()) + } + + fn missing_zulip_team_links(&self) -> Option { + join_mentions( + self.missing_zulip_ids() + .map(GhUsername::team_link) + .collect_vec(), + ) + } + + fn missing_team_entry_links(&self) -> Option { + join_mentions( + self.missing_team_entries() + .map(GhUsername::link) + .collect_vec(), + ) + } +} + +/// Every goal has its own reporting schedule. +/// This is how often the goal owner is prompted to author an update. +/// +/// This is set via a label (see [`REPORT_LABELS`]) on the goal's tracking issue. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)] +enum Period { + /// Start a new reporting period every week. + EveryWeek = 0, + /// Start a new reporting period every 2 weeks. + Every2Weeks = 1, + /// Start a new reporting period every 4 weeks. + Every4Weeks = 2, +} + +impl Period { + fn weeks(self) -> i64 { + match self { + Self::EveryWeek => 1, + Self::Every2Weeks => 2, + Self::Every4Weeks => 4, + } + } + + fn adjective(self) -> &'static str { + match self { + Self::EveryWeek => "weekly", + Self::Every2Weeks => "biweekly", + Self::Every4Weeks => "4-week", + } + } + + /// Returns the starting date of the period that includes this day. + /// + /// Every reporting period begins on a [`JOB_WEEKDAY`] + /// and lasts [`Period::weeks`], depending on the goal. + /// + /// Biweekly and 4-week cycles are aligned to [`EPOCH`]. + fn start(self, today: NaiveDate) -> NaiveDate { + // Depending on the chosen date, `EPOCH` may not fall on the `JOB_WEEKDAY`. + // `days_until_job` is needed to calculate an anchor from the `EPOCH` that + // falls on the `JOB_WEEKDAY` and can be used to compute the relevant dates. + let epoch_weekday = i64::from(EPOCH.weekday().num_days_from_monday()); + let job_weekday = i64::from(JOB_WEEKDAY.num_days_from_monday()); + let days_until_job = (job_weekday - epoch_weekday).rem_euclid(7); + + // Dates are computed relative to this date. + let anchor = EPOCH + Duration::days(days_until_job); + + // The number of full weeks since the anchor. + let weeks_since_anchor = today.signed_duration_since(anchor).num_weeks(); + let period_weeks = self.weeks(); + // The number of full periods that passed since the anchor date. + let periods_since_anchor = weeks_since_anchor.div_euclid(period_weeks); + // The number of weeks since the anchor, quantized to the period. + let weeks_since_anchor = periods_since_anchor * period_weeks; -We will begin drafting the next blog post collecting goal updates {next_update}. + anchor + Duration::weeks(weeks_since_anchor) + } + + /// Returns the starting date of the next period, + /// i.e. this period's starting date plus the duration of a period. + fn next_start(self, period_start: NaiveDate) -> NaiveDate { + period_start + Duration::weeks(self.weeks()) + } +} + +#[derive(Debug)] +struct Schedule { + period: Period, + conflict: Option, +} + +impl Schedule { + fn from_issue(issue: &GoalIssue) -> Self { + let selected = REPORT_LABELS + .iter() + .filter_map(|&(label, period)| { + issue + .labels + .iter() + .any(|issue_label| issue_label == label) + .then_some((label, period)) + }) + .collect_vec(); + + match selected.as_slice() { + [] => Self { + period: Period::Every4Weeks, + conflict: None, + }, + [(_, period)] => Self { + period: *period, + conflict: None, + }, + multiple => { + let (minimal_label, minimal) = multiple + .iter() + .copied() + .min_by_key(|(_, period)| *period) + .expect("multiple contains at least two schedules"); + Self { + period: minimal, + conflict: Some(format!( + "{} (`{minimal_label}` was used)", + multiple + .iter() + .map(|(label, _)| format!("`{label}`")) + .join(", "), + )), + } + } + } + } +} + +#[derive(Clone, Copy, Debug)] +struct Goal<'gh> { + issue: u64, + title: &'gh str, + created_at: DateTime, + last_comment_at: Option>, +} + +impl<'gh> Goal<'gh> { + fn from_issue(issue: &'gh GoalIssue) -> Self { + Self { + issue: issue.number, + title: &issue.title, + created_at: issue.created_at, + last_comment_at: issue.last_comment.as_ref().map(|c| c.created_at), + } + } + + /// Returns a string representing an issue in the `rust-lang/rust-project-goals` repo. + /// Zulip recognizes strings like `goals#123` and turns them into links. + fn link(&self) -> String { + format!("goals#{number}", number = self.issue) + } -Please comment on the github tracking issue goals#{issue_number} before then. Thanks! <3 + /// Same as [`Goal::link`], but also includes the goal title. + fn named_link(&self) -> String { + format!( + "**{title}** (goals#{number})", + title = self.title, + number = self.issue + ) + } -Here is a suggested template for updates (feel free to drop the items that don't apply): + fn latest_update(&self) -> String { + match self.last_comment_at { + None => format!( + "goal started: {} (no updates so far)", + display_datetime(self.created_at) + ), + Some(dt) => { + format!("latest update: {}", display_datetime(dt)) + } + } + } +} -* **Key developments:** *What has happened since the last time. It's perfectly ok to list "nothing" if that's the truth, we know people get busy.* -* **Blockers:** *List any Rust teams you are waiting on and what you are waiting for.* -* **Help wanted:** *Are there places where you are looking for contribution or feedback from the broader community?* -"# +fn display_job_date(date: NaiveDate) -> String { + format!( + "", + date.format("%Y-%m-%d"), ) } -pub struct ProjectGoalsUpdateJob; +fn display_datetime(date: DateTime) -> String { + format!("", date.format("%Y-%m-%dT%H:%M%:z"),) +} -#[async_trait] -impl Job for ProjectGoalsUpdateJob { - fn name(&self) -> &'static str { - "project_goals_update_job" +#[derive(Clone, Copy, Debug)] +struct Reminder<'gh> { + goal: Goal<'gh>, + /// The reporting period of this goal. + period: Period, + /// The start date of this goal's current reporting period. + period_start: NaiveDate, + /// The start date of this goal's next reporting period. + /// + /// (This acts as a deadline for the current update.) + next_period_start: NaiveDate, +} + +impl<'gh> Reminder<'gh> { + fn from_issue(issue: &'gh GoalIssue, now: DateTime) -> (Self, Option) { + let schedule = Schedule::from_issue(issue); + let today = now.date_naive(); + let period_start = schedule.period.start(today); + ( + Self { + goal: Goal::from_issue(issue), + period: schedule.period, + period_start, + next_period_start: schedule.period.next_start(period_start), + }, + schedule.conflict, + ) + } + + fn is_required(&self, now: DateTime) -> bool { + // Give new goals a grace period before reminders begin. + let grace_end = self.goal.created_at + Duration::days(FIRST_REPORT_GRACE_DAYS); + + if now < grace_end { + return false; + } + + self.goal + .last_comment_at + .is_none_or(|d| d.date_naive() < self.period_start) } - async fn run(&self, ctx: &super::Context, _metadata: &serde_json::Value) -> anyhow::Result<()> { - ping_project_goals_owners_automatically(&ctx.github, &ctx.zulip, &ctx.team).await + fn list_item(&self) -> String { + format!( + "+ {goal}\n - {latest}\n - next *{period}* cycle starts {next}", + goal = self.goal.named_link(), + latest = self.goal.latest_update(), + next = display_job_date(self.next_period_start), + period = self.period.adjective(), + ) } } -/// Returns true if the user with the given github id is allowed to ping all group people -/// and do other "project group adminstrative" tasks. -pub async fn check_project_goal_acl(team_client: &TeamClient, gh_id: u64) -> anyhow::Result { - const GOALS_TEAM: &str = "goals"; +#[derive(Debug)] +struct OwnershipProblem<'gh> { + goal: Goal<'gh>, + owners: Owners<'gh>, +} - let team = match team_client.get_team(GOALS_TEAM).await { - Ok(Some(team)) => team, - Ok(None) => { - log::info!("team ({GOALS_TEAM}) failed to resolve to a known team"); - return Ok(false); +#[derive(Debug)] +struct PeriodConflict<'gh> { + goal: Goal<'gh>, + reason: String, +} + +#[derive(Default)] +struct ReminderErrors<'gh> { + unowned: Vec>, + ownership: Vec>, + schedule: Vec>, +} + +impl ReminderErrors<'_> { + fn is_empty(&self) -> bool { + self.unowned.is_empty() && self.ownership.is_empty() && self.schedule.is_empty() + } + + fn count(&self) -> usize { + self.unowned.len() + + self.schedule.len() + + self + .ownership + .iter() + .map(|problem| { + problem.owners.has_multiple() as usize + + problem.owners.has_missing_zulip_id() as usize + + problem.owners.has_missing_team_entry() as usize + }) + .sum::() + } +} + +#[derive(Default)] +struct ReminderPlan<'gh> { + goals_by_owner: BTreeMap>>, + errors: ReminderErrors<'gh>, +} + +impl<'gh> ReminderPlan<'gh> { + fn add_conflicts(&mut self, goal: Goal<'gh>, reason: String) { + self.errors.schedule.push(PeriodConflict { goal, reason }); + } + + fn add_goal(&mut self, reminder: Reminder<'gh>, owners: Owners<'gh>) { + if owners.is_empty() { + self.errors.unowned.push(reminder.goal); + return; } - Err(err) => { - log::error!("team ({GOALS_TEAM}) failed to resolve to a known team: {err:?}"); - return Ok(false); + + for owner in owners.reachable() { + self.goals_by_owner.entry(owner).or_default().push(reminder); } - }; - Ok(team - .members - .into_iter() - .any(|member| member.github_id == gh_id)) + if owners.has_problem() { + self.errors.ownership.push(OwnershipProblem { + goal: reminder.goal, + owners, + }); + } + } } -async fn ping_project_goals_owners_automatically( - gh: &GithubClient, - zulip: &ZulipClient, - team_api: &TeamClient, -) -> anyhow::Result<()> { - // Predicted schedule is to author a blog post on the 3rd week of the month. - // We start pinging when the month starts until we see an update in this month - // or the last 7 days of previous month. - // - // Therefore, we compute: - // * Days since start of this month -- threshold will be this number of days + 7. - // * Date of the 3rd Monday in the month -- this will be the next update (e.g., `on Sep-5`). - let now = Utc::now(); +fn owner_message(owner: ZulipId, goals: &[Reminder<'_>]) -> String { + format!( + r#" +Hi {owner}! - // We want to ping people unless they've written an update since the last week of the previous month. - let days_threshold = now.day() + 7; +This is your reminder to post updates for the following goals: - // Format the 3rd Monday of the month, e.g. "on Sep-5", for inclusion. - let third_monday = - NaiveDate::from_weekday_of_month_opt(now.year(), now.month(), chrono::Weekday::Mon, 3) - .unwrap() - .format("on %b-%d") - .to_string(); +{goals} - ping_project_goals_owners( - gh, - zulip, - team_api, - false, - i64::from(days_threshold), - &third_monday, +Some questions to guide you (you don't have to follow this): + ++ What has happened since your last update? ++ Are there any relevant PRs, issues, docs, or discussions to link? ++ Are you blocked on any issue, PR, or team? ++ Do you need help or feedback? Where should people look? ++ What do you plan to work on before the next update? + +Even if there's little to say, a brief message provides reassurance that the goal is still alive. + +Please leave your updates as comments on the tracking issues. Thanks! <3 + +--- + +*Note: Two- and four-week goals are pinged weekly until an update is posted for the current reporting period.* + +*By default, the reporting period is 4 weeks. If you'd like to post updates more often, you can override the period per goal by labeling the issue with `R-every-week`, `R-every-2-weeks`, or `R-every-4-weeks`.* +"#, + owner = owner.mention(false), + goals = goals.iter().map(Reminder::list_item).join("\n"), ) - .await } -/// Sends a ping message to all project goal owners if -/// they have not posted an update in the last `days_threshold` days. -/// -/// `next_update` is a human readable description of when the next update -/// will be drafted (e.g., `"on Sep 5"`). -pub async fn ping_project_goals_owners( - gh: &GithubClient, - zulip: &ZulipClient, - team_client: &TeamClient, - dry_run: bool, - days_threshold: i64, - next_update: &str, -) -> anyhow::Result<()> { - let goals_repo = gh.repository(RUST_PROJECT_GOALS_REPO).await?; +fn unowned_errors(goals: &[Goal<'_>]) -> String { + format!( + r#" +The following goals have no owner assigned: - let tracking_issues_query = github::issue_query::Query { - filters: vec![("state", "open"), ("is", "issue")], - include_labels: vec!["C-tracking-issue"], - exclude_labels: vec![], - }; - let issues = goals_repo - .get_issues(gh, &tracking_issues_query) - .await - .with_context(|| "Unable to get issues.")?; +{unowned} - for issue in issues { - let comments = issue.comments.unwrap_or(0); +Please assign an owner and reach out to them! +"#, + unowned = goals + .iter() + .map(|g| format!("+ {}", g.named_link())) + .join("\n") + ) +} + +fn multiple_owner_warnings(problems: &[OwnershipProblem<'_>]) -> String { + format!( + r#" +The following goals have more than one owner assigned: + +{multiple_owner} + +A goal should have exactly one owner. All owners with a Zulip account were still notified separately. +"#, + multiple_owner = problems + .iter() + .filter(|p| p.owners.has_multiple()) + .map(|p| { + format!( + "+ {goal}: {owners}", + goal = p.goal.link(), + owners = p.owners.all_mentions(true).expect("has multiple"), + ) + }) + .join("\n") + ) +} + +fn missing_zulip_errors(problems: &[OwnershipProblem<'_>]) -> String { + format!( + r#" +The following goal owners were not pinged because they don't have a Zulip account specified in the `team` repo: + +{missing_zulip} + +Please make sure to register their `zulip-id` and reach out to them! +"#, + missing_zulip = problems + .iter() + .filter(|p| p.owners.has_missing_zulip_id()) + .map(|p| { + format!( + "+ {goal}: {unreachable}\n - {notified}", + goal = p.goal.link(), + unreachable = p + .owners + .missing_zulip_team_links() + .expect("has missing Zulip ID"), + notified = match p.owners.reachable_mentions() { + None => "Nobody was notified on Zulip.".to_owned(), + Some(owners) => format!("{owners} got notified on Zulip."), + } + ) + }) + .join("\n") + ) +} + +fn missing_team_entry_warnings(problems: &[OwnershipProblem<'_>]) -> String { + format!( + r#" +The following assignees could not be found in the `team` repo, so Triagebot could not look up their Zulip accounts: + +{missing_team_entries} + +Please check the assignee usernames and their entries in the `team` repo. +"#, + missing_team_entries = problems + .iter() + .filter(|p| p.owners.has_missing_team_entry()) + .map(|p| { + format!( + "+ {goal}: {owners}\n - {notified}", + goal = p.goal.link(), + owners = p + .owners + .missing_team_entry_links() + .expect("has missing team entry"), + notified = match p.owners.reachable_mentions() { + None => "Nobody was notified on Zulip.".to_owned(), + Some(owners) => format!("{owners} got notified on Zulip."), + }, + ) + }) + .join("\n") + ) +} + +fn schedule_warnings(conflicts: &[PeriodConflict<'_>]) -> String { + format!( + r#" +The following goals have conflicting reporting period labels: + +{conflicts} + +Unlabeled goals use the default period of 4 weeks. +"#, + conflicts = conflicts + .iter() + .map(|e| format!("+ {}: {}", e.goal.link(), e.reason)) + .join("\n") + ) +} + +fn error_sections(errors: &ReminderErrors<'_>) -> String { + let mut sections = Vec::new(); + + if !errors.unowned.is_empty() { + sections.push(unowned_errors(&errors.unowned)); + } + if errors.ownership.iter().any(|p| p.owners.has_multiple()) { + sections.push(multiple_owner_warnings(&errors.ownership)); + } + if errors + .ownership + .iter() + .any(|p| p.owners.has_missing_zulip_id()) + { + sections.push(missing_zulip_errors(&errors.ownership)); + } + if errors + .ownership + .iter() + .any(|p| p.owners.has_missing_team_entry()) + { + sections.push(missing_team_entry_warnings(&errors.ownership)); + } + if !errors.schedule.is_empty() { + sections.push(schedule_warnings(&errors.schedule)); + } + + sections.iter().join("\n\n---\n\n") +} + +async fn build_plan<'gh>( + issues: &'gh [GoalIssue], + team: &TeamClient, + now: DateTime, +) -> anyhow::Result> { + let mut plan = ReminderPlan::default(); - // Find the time of the last comment posted. - let days_since_last_comment = (Utc::now() - issue.updated_at).num_days(); + for issue in issues { + let (reminder, conflict) = Reminder::from_issue(issue, now); - // Start pinging 3 weeks after the last update. - // As a special case, if the last update was within a day of creation, that means no initial update, so ping anyway. log::debug!( - "issue #{}: days_since_last_comment = {} days, number of comments = {}", + "issue #{}: period_start = {}, next_deadline = {}, last_comment = {:?}", issue.number, - days_since_last_comment, - comments, + reminder.period_start, + reminder.next_period_start, + issue.last_comment.as_ref().map(|c| c.created_at), ); - if days_since_last_comment < days_threshold && comments > 1 { - continue; + + if let Some(conflict) = conflict { + plan.add_conflicts(reminder.goal, conflict); } - let zulip_topic_name = zulip_topic_name(&issue); - let Some(zulip_owners) = zulip_owners(team_client, &issue).await? else { - log::debug!("no owners assigned"); + if !reminder.is_required(now) { continue; - }; - - let message = message( - &zulip_owners, - &if comments <= 1 { - "∞".to_string() - } else { - days_since_last_comment.to_string() - }, - issue.number, - &issue.title, - next_update, + } + + let owners = Owners::resolve_goal(team, issue).await?; + plan.add_goal(reminder, owners); + } + + Ok(plan) +} + +async fn send_dm(zulip: &ZulipClient, owner: ZulipId, content: &str, dry_run: bool) { + if dry_run { + log::debug!("(DRY) Would send DM to user {}: {}", owner.0, content); + return; + } + + let req = MessageApiRequest { + recipient: Recipient::Private { + id: owner.0, + email: "", + }, + content, + }; + + if let Err(err) = req.send(zulip).await { + log::error!("failed to send a DM on Zulip: {err}") + } +} + +async fn send_triagebot_topic( + zulip: &ZulipClient, + content: &str, + dry_run: bool, +) -> anyhow::Result<()> { + if dry_run { + log::debug!( + "(DRY) Would send to topic {GOALS_META_STREAM}>{TRIAGEBOT_TOPIC}: {}", + content, ); + return Ok(()); + } - let zulip_req = crate::zulip::MessageApiRequest { - recipient: Recipient::Stream { - id: GOALS_STREAM, - topic: &zulip_topic_name, - }, - content: &message, - }; + MessageApiRequest { + recipient: Recipient::Stream { + id: GOALS_META_STREAM, + topic: TRIAGEBOT_TOPIC, + }, + content, + } + .send(zulip) + .await?; - log::debug!("zulip_topic_name = {zulip_topic_name:#?}"); - log::debug!("message = {message:#?}"); + Ok(()) +} - if dry_run { - eprintln!(); - eprintln!("-- Dry Run ------------------------------------"); - eprintln!("Would send to {zulip_topic_name}: {}", zulip_req.content); - } else { - zulip_req.send(zulip).await?; +#[derive(Default)] +struct PeriodCounts { + weekly: usize, + every_2_weeks: usize, + every_4_weeks: usize, +} + +impl PeriodCounts { + fn from_reminders<'gh>(reminders: impl Iterator>) -> Self { + let mut counts = Self::default(); + + for reminder in reminders { + match reminder.period { + Period::EveryWeek => counts.weekly += 1, + Period::Every2Weeks => counts.every_2_weeks += 1, + Period::Every4Weeks => counts.every_4_weeks += 1, + } } + + counts + } + + fn total(&self) -> usize { + self.weekly + self.every_2_weeks + self.every_4_weeks + } +} + +fn report( + errors: &ReminderErrors<'_>, + total_owners: usize, + counts: &PeriodCounts, + today: NaiveDate, +) -> String { + let next_week = Period::EveryWeek.next_start(Period::EveryWeek.start(today)); + let next_2_weeks = Period::Every2Weeks.next_start(Period::Every2Weeks.start(today)); + let next_4_weeks = Period::Every4Weeks.next_start(Period::Every4Weeks.start(today)); + + let error_summary = if errors.is_empty() { + "No errors happened in the process.".to_owned() + } else { + format!( + "{count} errors happened in the process.\n\n---\n\n{details}", + count = errors.count(), + details = error_sections(errors), + ) + }; + + format!( + r#" +Hi @*T-goals*! + +Weekly run finished. + +{total_owners} owners were notified about {total_goals} goals: ++ Weekly reports: {weekly} (next cycle: {next_week}) ++ Biweekly reports: {every_2_weeks} (next cycle: {next_2_weeks}) ++ Four-week reports: {every_4_weeks} (next cycle: {next_4_weeks}) + +{error_summary} + +Until next week! <3 +"#, + total_goals = counts.total(), + weekly = counts.weekly, + every_2_weeks = counts.every_2_weeks, + every_4_weeks = counts.every_4_weeks, + next_week = display_job_date(next_week), + next_2_weeks = display_job_date(next_2_weeks), + next_4_weeks = display_job_date(next_4_weeks), + ) +} + +async fn execute_plan( + zulip: &ZulipClient, + plan: ReminderPlan<'_>, + today: NaiveDate, + dry_run: bool, +) -> anyhow::Result<()> { + let ReminderPlan { + goals_by_owner, + errors, + } = plan; + + let total_owners = goals_by_owner.len(); + let counts = PeriodCounts::from_reminders( + goals_by_owner + .values() + .flatten() + .copied() + .unique_by(|reminder| reminder.goal.issue), + ); + + for (owner, goals) in goals_by_owner { + send_dm(zulip, owner, &owner_message(owner, &goals), dry_run).await; } + send_triagebot_topic( + zulip, + &report(&errors, total_owners, &counts, today), + dry_run, + ) + .await?; + Ok(()) } -fn zulip_topic_name(issue: &Issue) -> String { +pub async fn ping_project_goals_owners( + gh: &GithubClient, + zulip: &ZulipClient, + team: &TeamClient, + dry_run: bool, +) -> anyhow::Result<()> { + let now = Utc::now(); + let issues = gh.open_goal_issues().await?; + let plan = build_plan(&issues, team, now).await?; + execute_plan(zulip, plan, now.date_naive(), dry_run).await +} + +pub struct PingProjectGoalsOwnersJob; + +#[async_trait] +impl Job for PingProjectGoalsOwnersJob { + fn name(&self) -> &'static str { + "ping_project_goal_owners_job" + } + + async fn run(&self, ctx: &Context, _metadata: &serde_json::Value) -> anyhow::Result<()> { + ping_project_goals_owners(&ctx.github, &ctx.zulip, &ctx.team, false).await + } +} + +/// Returns true if the GitHub user is part of the Goals team. +pub async fn is_goals_member(team_client: &TeamClient, github_id: u64) -> anyhow::Result { + let team = match team_client.get_team(GOALS_TEAM).await? { + Some(team) => team, + None => { + log::info!("team ({GOALS_TEAM}) failed to resolve to a known team"); + return Ok(false); + } + }; + + Ok(team + .members + .into_iter() + .any(|member| member.github_id == github_id)) +} + +fn goal_zulip_topic(issue: &Issue) -> String { let goal_number = format!("(goals#{})", issue.number); let mut title = String::new(); for word in issue.title.split_whitespace() { @@ -212,38 +934,87 @@ fn zulip_topic_name(issue: &Issue) -> String { title } -async fn zulip_owners(team_client: &TeamClient, issue: &Issue) -> anyhow::Result> { - use std::fmt::Write; - - Ok(match &issue.assignees[..] { - [] => None, - [string0] => Some(owner_string(team_client, string0).await?), - [string0, string1] => Some(format!( - "{} and {}", - owner_string(team_client, string0).await?, - owner_string(team_client, string1).await? - )), - [string0 @ .., string1] => { - let mut out = String::new(); - for s in string0 { - write!(out, "{}, ", owner_string(team_client, s).await?).unwrap(); - } - write!(out, "{}, ", owner_string(team_client, string1).await?).unwrap(); - Some(out) - } - }) +fn is_tracking_issue(issue: &Issue) -> bool { + issue + .labels + .iter() + .any(|label| label.name == "C-tracking-issue") } -async fn owner_string(team_api: &TeamClient, assignee: &GitHubUser) -> anyhow::Result { - if let Some(zulip_id) = team_api.github_to_zulip_id(assignee.id).await? { - Ok(format!("@**|{zulip_id}**")) - } else { - // No zulip-id? Fallback to github user name. - Ok(format!( - "@{login} ([register your zulip-id here to get a real ping!](https://github.com/rust-lang/team/tree/master/people/{login}.toml))", - login = assignee.login, - )) +async fn create_goal_topic(issue: &Issue, ctx: &Context) -> anyhow::Result<()> { + if !is_tracking_issue(issue) { + return Ok(()); + } + + let owners = Owners::resolve_event(&ctx.team, issue).await?; + let topic = goal_zulip_topic(issue); + let content = format!( + "Goal *{title}* (goals#{number}) has been accepted. It's owned by {owners}.", + title = issue.title, + number = issue.number, + owners = owners + .all_mentions(false) + .unwrap_or_else(|| "nobody (@*T-goals* should fix this)".to_owned()), + ); + + MessageApiRequest { + recipient: Recipient::Stream { + id: GOALS_STREAM, + topic: &topic, + }, + content: &content, + } + .send(&ctx.zulip) + .await?; + + Ok(()) +} + +fn quote_fence(text: &str) -> String { + let mut ticks = "````".to_owned(); + + while text.contains(&ticks) { + ticks.push('`'); + } + + ticks +} + +async fn echo_comment_to_zulip( + issue: &Issue, + comment: &github::Comment, + ctx: &Context, +) -> anyhow::Result<()> { + if !is_tracking_issue(issue) { + return Ok(()); + } + + let author = + Owner::from_id_and_username(&ctx.team, comment.user.id, &comment.user.login).await?; + let text = &comment.body; + + let content = format!( + "[Comment posted]({url}) on goals#{number} by {author}:\n\ + {ticks}quote\n\ + {text}\n\ + {ticks}", + url = comment.html_url, + number = issue.number, + author = author.display_mention(true), + ticks = quote_fence(text), + ); + + MessageApiRequest { + recipient: Recipient::Stream { + id: GOALS_STREAM, + topic: &goal_zulip_topic(issue), + }, + content: &content, } + .send(&ctx.zulip) + .await?; + + Ok(()) } pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> { @@ -252,93 +1023,19 @@ pub async fn handle(ctx: &Context, event: &Event) -> anyhow::Result<()> { } match event { - // When a new issue is opened that is tagged as a tracking issue, - // automatically create a Zulip topic for it and post a comment to the issue. Event::Issue(IssuesEvent { action: IssuesAction::Opened, issue, .. - }) => { - if !issue.labels.iter().any(|l| l.name == C_TRACKING_ISSUE) { - return Ok(()); - } - let zulip_topic_name = zulip_topic_name(issue); - let zulip_owners = zulip_owners(&ctx.team, issue).await?; - let zulip_owners = zulip_owners.as_deref().unwrap_or("(no owners assigned)"); - let title = &issue.title; - let goalnum = issue.number; - let zulip_req = crate::zulip::MessageApiRequest { - recipient: Recipient::Stream { - id: GOALS_STREAM, - topic: &zulip_topic_name, - }, - content: &format!( - r"New tracking issue goals#{goalnum}.\n* Goal title: {title}\n* Goal owners: {zulip_owners}" - ), - }; - zulip_req.send(&ctx.zulip).await?; - Ok(()) - } + }) => create_goal_topic(issue, ctx).await, - // When a new comment is posted on a tracking issue, post it to Zulip. Event::IssueComment(IssueCommentEvent { - action, + action: IssueCommentAction::Created, issue, comment, .. - }) => { - // Only comments on tracking issues should be forwarded to Zulip. - if !issue.labels.iter().any(|l| l.name == C_TRACKING_ISSUE) { - return Ok(()); - } + }) => echo_comment_to_zulip(issue, comment, ctx).await, - let number = issue.number; - let action_str = match action { - IssueCommentAction::Created => "posted", - - // Don't spam for updates, deletes - _ => return Ok(()), - }; - let zulip_topic_name = zulip_topic_name(issue); - let url = &comment.html_url; - let text = &comment.body; - let zulip_author = owner_string(&ctx.team, &comment.user).await?; - - let mut ticks = "````".to_string(); - while text.contains(&ticks) { - ticks.push('`'); - } - - match action { - IssueCommentAction::Created | IssueCommentAction::Edited => { - let zulip_req = crate::zulip::MessageApiRequest { - recipient: Recipient::Stream { - id: GOALS_STREAM, - topic: &zulip_topic_name, - }, - content: &format!( - "[Comment {action_str}]({url}) on goals#{number} by {zulip_author}:\n\ - {ticks}quote\n\ - {text}\n\ - {ticks}" - ), - }; - zulip_req.send(&ctx.zulip).await?; - } - - IssueCommentAction::Deleted - | IssueCommentAction::Pinned - | IssueCommentAction::Unpinned => { - // Do we really care? - } - } - - Ok(()) - } - - _ => { - /* No action for other cases */ - Ok(()) - } + _ => Ok(()), } } diff --git a/src/jobs.rs b/src/jobs.rs index e2d23c104..7c31fd2cf 100644 --- a/src/jobs.rs +++ b/src/jobs.rs @@ -50,6 +50,7 @@ use std::str::FromStr; use async_trait::async_trait; use cron::Schedule; +use crate::handlers::project_goals::PingProjectGoalsOwnersJob; use crate::handlers::pull_requests_assignment_update::PullRequestAssignmentUpdate; use crate::{ db::jobs::JobSchedule, @@ -77,6 +78,7 @@ pub fn jobs() -> Vec> { Box::new(MajorChangeAcceptanceJob), Box::new(GithubRateLimitLoggingJob), Box::new(AddReviewChangesSinceLinkJob), + Box::new(PingProjectGoalsOwnersJob), ] } @@ -107,6 +109,12 @@ pub fn default_jobs() -> Vec { schedule: Schedule::from_str("* */15 * * * * *").unwrap(), metadata: serde_json::Value::Null, }, + JobSchedule { + name: PingProjectGoalsOwnersJob.name(), + // Around 6/7am Pacific time on every Thursday. + schedule: Schedule::from_str("0 00 14 * * Thu *").unwrap(), + metadata: serde_json::Value::Null, + }, ] } diff --git a/src/zulip.rs b/src/zulip.rs index 16fcf1b9d..f44348613 100644 --- a/src/zulip.rs +++ b/src/zulip.rs @@ -14,14 +14,13 @@ use crate::github::{self, PullRequestNumber, Repository}; use crate::handlers::Context; use crate::handlers::docs_update::docs_update; use crate::handlers::pr_tracking::{ReviewerWorkqueue, get_assigned_prs}; -use crate::handlers::project_goals::{self, ping_project_goals_owners}; use crate::interactions::ErrorComment; use crate::utils::pluralize; use crate::zulip::api::{MessageApiResponse, Recipient}; use crate::zulip::client::ZulipClient; use crate::zulip::commands::{ - BackportChannelArgs, BackportVerbArgs, ChatCommand, IssuePrio, LookupCmd, PingGoalsArgs, - StreamCommand, WorkqueueCmd, WorkqueueLimit, parse_cli, + BackportChannelArgs, BackportVerbArgs, ChatCommand, IssuePrio, LookupCmd, StreamCommand, + WorkqueueCmd, WorkqueueLimit, parse_cli, }; use anyhow::{Context as _, format_err}; use axum::Json; @@ -266,9 +265,6 @@ async fn handle_command<'a>( id, } => unlock_cmd(&ctx, gh_id, organization, repo, *id).await, ChatCommand::Work(cmd) => workqueue_commands(&ctx, gh_id, cmd).await, - ChatCommand::PingGoals(args) => { - ping_goals_cmd(ctx.clone(), gh_id, message_data, args).await - } ChatCommand::DocsUpdate => trigger_docs_update(&ctx.zulip, message_data), ChatCommand::UserInfo { username, @@ -358,9 +354,6 @@ async fn handle_command<'a>( .await .map_err(|e| format_err!("Failed to await at this time: {e:?}")) } - StreamCommand::PingGoals(args) => { - ping_goals_cmd(ctx, gh_id, message_data, &args).await - } StreamCommand::DocsUpdate => trigger_docs_update(&ctx.zulip, message_data), StreamCommand::Backport { verb, @@ -632,56 +625,6 @@ async fn assign_issue_prio( Ok(None) } -async fn ping_goals_cmd( - ctx: Arc, - gh_id: u64, - message: &Message, - args: &PingGoalsArgs, -) -> anyhow::Result> { - if project_goals::check_project_goal_acl(&ctx.team, gh_id).await? { - let args = args.clone(); - let message = message.clone(); - tokio::spawn(async move { - let res = ping_project_goals_owners( - &ctx.github, - &ctx.zulip, - &ctx.team, - false, - args.threshold as i64, - &format!("on {}", args.next_update), - ) - .await; - - let status = match res { - Ok(_res) => "OK".to_string(), - Err(err) => { - tracing::error!("ping_project_goals_owners: {err:?}"); - format!("ERROR\n\n```\n{err:#?}\n```\n") - } - }; - - let res = MessageApiRequest { - recipient: message.sender_to_recipient(), - content: &format!("End pinging project groups owners: {status}"), - } - .send(&ctx.zulip) - .await; - - if let Err(err) = res { - tracing::error!( - "error sending project goals ping reply: {err:?} for status: {status}" - ); - } - }); - - Ok(Some("Started pinging project groups owners...".to_string())) - } else { - Err(format_err!( - "That command is only permitted for those running the project-goal program.", - )) - } -} - /// Unlock a specific issue in our managed repos. /// This command can only be used by team members. async fn unlock_cmd( @@ -1203,7 +1146,6 @@ enum ImpersonationMode { fn get_cmd_impersonation_mode(cmd: &ChatCommand) -> ImpersonationMode { match cmd { ChatCommand::DocsUpdate - | ChatCommand::PingGoals(_) | ChatCommand::UserInfo { .. } | ChatCommand::TeamStats { .. } | ChatCommand::Unlock { .. } diff --git a/src/zulip/commands.rs b/src/zulip/commands.rs index 64785e3f2..771d15304 100644 --- a/src/zulip/commands.rs +++ b/src/zulip/commands.rs @@ -27,8 +27,6 @@ pub enum ChatCommand { /// Issue or pull-request number to unlock. id: PullRequestNumber, }, - /// Ping project goal owners. - PingGoals(PingGoalsArgs), /// Update docs DocsUpdate, /// Show recent GitHub activity of a user. @@ -149,8 +147,6 @@ pub enum StreamCommand { EndMeeting, /// Read a document. Read, - /// Ping project goal owners. - PingGoals(PingGoalsArgs), /// Update docs. DocsUpdate, /// Accept or decline a backport. @@ -189,14 +185,6 @@ pub enum StreamCommand { }, } -#[derive(clap::Parser, Debug, PartialEq, Clone)] -pub struct PingGoalsArgs { - /// Number of days before an update is considered stale - pub threshold: u64, - /// Date of next update - pub next_update: String, -} - /// Backport release channels #[derive(Clone, clap::ValueEnum, Debug, PartialEq)] pub enum BackportChannelArgs {