From 7b219f44d00a58130f70a1f102ba51f532f80775 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Tue, 5 May 2026 11:56:32 +0100 Subject: [PATCH 1/4] Update to octocrab 0.50.0 --- Cargo.lock | 5 ++--- Cargo.toml | 4 ++-- src/endpoints.rs | 2 +- src/prs.rs | 16 ++++------------ 4 files changed, 9 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 88ca405..f1edf20 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2046,8 +2046,8 @@ dependencies = [ [[package]] name = "octocrab" -version = "0.49.5" -source = "git+https://github.com/XAMPPRocky/octocrab.git?rev=460733dd3f49863b159ffb48050ab9523300ff84#460733dd3f49863b159ffb48050ab9523300ff84" +version = "0.50.0" +source = "git+https://github.com/XAMPPRocky/octocrab.git?rev=986289adc94b856c04a912b2c4cae07c63444d6d#986289adc94b856c04a912b2c4cae07c63444d6d" dependencies = [ "arc-swap", "async-trait", @@ -2056,7 +2056,6 @@ dependencies = [ "cargo_metadata", "cfg-if", "chrono", - "either", "futures", "futures-util", "getrandom 0.2.17", diff --git a/Cargo.toml b/Cargo.toml index 4db3b45..5e1b0d6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -37,8 +37,8 @@ itertools = "0.14.0" maplit = "1.0.2" md5 = "0.8.0" moka = { version = "0.12.10", features = ["future"] } -# Until https://github.com/XAMPPRocky/octocrab/pull/854 is released. -octocrab = { git = "https://github.com/XAMPPRocky/octocrab.git", rev = "460733dd3f49863b159ffb48050ab9523300ff84" } +# Until https://github.com/XAMPPRocky/octocrab/issues/884 is resolved +octocrab = { git = "https://github.com/XAMPPRocky/octocrab.git", rev = "986289adc94b856c04a912b2c4cae07c63444d6d" } octocrab-rate-limiter = "0.1.1" regex = "1.11.1" reqwest = { version = "0.12.20", default-features = false, features = [ diff --git a/src/endpoints.rs b/src/endpoints.rs index 1402913..b70fa8f 100644 --- a/src/endpoints.rs +++ b/src/endpoints.rs @@ -355,7 +355,7 @@ pub async fn started_itp( .await?; let usernames: BTreeSet<_> = prs .into_iter() - .filter_map(|pr| Some(GithubLogin::from(pr.user?.login))) + .filter_map(|pr| Some(GithubLogin::from(pr.user.login))) .collect(); Ok(Json(usernames)) } diff --git a/src/prs.rs b/src/prs.rs index e353914..61c6edb 100644 --- a/src/prs.rs +++ b/src/prs.rs @@ -100,17 +100,13 @@ pub async fn get_prs( .. }| { // If a user is deleted from GitHub, their User will be None - ignore PRs from deleted users. - let author = GithubLogin::from(user?.login); + let author = GithubLogin::from(user.login); - let labels = labels - .into_iter() - .flatten() - .map(|label| label.name) - .collect(); + let labels = labels.into_iter().map(|label| label.name).collect(); let pr_state = PrState::from(&labels); - let is_closed = state.unwrap_or(IssueState::Open) == IssueState::Closed; + let is_closed = state == IssueState::Closed; if is_closed && pr_state != PrState::Complete { return None; } @@ -118,11 +114,7 @@ pub async fn get_prs( // For some reason repo is generally None, but we know it, so... let repo_name = module.to_owned(); - // Unclear when they API would return None for these, ignore them. - let updated_at = updated_at?; - let created_at = created_at?; - let url = html_url?.to_string(); - let title = title?; + let url = html_url.to_string(); let body = body.unwrap_or_default(); Some(Pr { From 25a38ab4558ac9660dd7ca017f96a11df940c5d1 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Tue, 5 May 2026 11:57:19 +0100 Subject: [PATCH 2/4] Handle invalid durations more gracefully --- src/frontend.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/frontend.rs b/src/frontend.rs index bc47228..1e50ecd 100644 --- a/src/frontend.rs +++ b/src/frontend.rs @@ -339,10 +339,14 @@ pub struct ModuleReviewMetrics { impl ReviewMetricsTemplate { pub fn format_duration(&self, duration: &Option) -> String { if let Some(duration) = duration { - let secs = duration.to_std().unwrap().as_secs(); - let secs_without_hours = secs - (secs % (60 * 60)); - humantime::format_duration(std::time::Duration::from_secs(secs_without_hours)) - .to_string() + if let Ok(duration) = duration.to_std() { + let secs = duration.as_secs(); + let secs_without_hours = secs - (secs % (60 * 60)); + humantime::format_duration(std::time::Duration::from_secs(secs_without_hours)) + .to_string() + } else { + format!("Invalid duration: {:?}", duration) + } } else { "Not yet".to_owned() } From 79c7a357a0cd7292fc5a4432f2bbde79263c3769 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Tue, 5 May 2026 11:58:37 +0100 Subject: [PATCH 3/4] Ignore self-added Reviewed/Complete labels --- src/prs.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/prs.rs b/src/prs.rs index 61c6edb..8963c2e 100644 --- a/src/prs.rs +++ b/src/prs.rs @@ -329,26 +329,30 @@ impl PrMetrics { pr: Pr, created_at: chrono::DateTime, label_add_events: Vec, + staff: &BTreeSet, ) -> PrMetrics { let mut first_needs_review = None; let mut first_reviewed = None; let mut first_complete = None; let mut iterations = 0; + let mut reviewed_by = ReviewedBy::NoOne; for event in &label_add_events { if event.label == "Needs Review" { if first_needs_review.is_none() { first_needs_review = Some(event.time); } - } else if event.label == "Reviewed" { - iterations += 1; - if first_reviewed.is_none() { - first_reviewed = Some(event.time); - } - } else if event.label == "Complete" { - iterations += 1; - if first_complete.is_none() { - first_complete = Some(event.time); + } else if event.actor != pr.author { + if event.label == "Reviewed" { + iterations += 1; + if first_reviewed.is_none() { + first_reviewed = Some(event.time); + } + } else if event.label == "Complete" { + iterations += 1; + if first_complete.is_none() { + first_complete = Some(event.time); + } } } } From cf86a808147a6c477b85c6ea4939d260f131559a Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Tue, 5 May 2026 11:58:59 +0100 Subject: [PATCH 4/4] Track review counts by staff/volunteers/both --- config.prod.json | 3 ++- src/config.rs | 32 +++++++++++++++++++++++-- src/frontend.rs | 15 ++++++++++-- src/prs.rs | 44 ++++++++++++++++++++++++++++++++++- templates/review-metrics.html | 20 +++++++++++++++- 5 files changed, 107 insertions(+), 7 deletions(-) diff --git a/config.prod.json b/config.prod.json index 9908d55..cede811 100644 --- a/config.prod.json +++ b/config.prod.json @@ -871,5 +871,6 @@ } } } - } + }, + "staff_github_team_name": "$CYF_STAFF_GITHUB_TEAM_NAME" } diff --git a/src/config.rs b/src/config.rs index 9ed1ced..43e0b29 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,11 +1,19 @@ -use std::{collections::BTreeMap, net::IpAddr}; +use std::{ + collections::{BTreeMap, BTreeSet}, + net::IpAddr, +}; use chrono::NaiveDate; use indexmap::IndexMap; +use octocrab::Octocrab; use serde::Deserialize; use serde_env_field::EnvField; -use crate::newtypes::Region; +use crate::{ + Error, + newtypes::{GithubLogin, Region}, + octocrab::all_pages, +}; #[derive(Clone, Deserialize)] pub struct Config { @@ -39,6 +47,8 @@ pub struct Config { pub mentoring_records_sheet_id: String, pub reviewer_staff_info_sheet_id: String, + + pub staff_github_team_name: EnvField, } #[derive(Clone, Deserialize)] @@ -77,6 +87,24 @@ impl Config { None } } + + pub async fn get_staff_github_usernames( + &self, + octocrab: &Octocrab, + ) -> Result, Error> { + let members = all_pages("members", &octocrab, async || { + octocrab + .teams(&self.github_org) + .members(self.staff_github_team_name.as_str()) + .send() + .await + }) + .await?; + Ok(members + .into_iter() + .map(|member| GithubLogin::from(member.login)) + .collect()) + } } #[derive(Clone, Deserialize)] diff --git a/src/frontend.rs b/src/frontend.rs index 1e50ecd..c60e910 100644 --- a/src/frontend.rs +++ b/src/frontend.rs @@ -275,6 +275,12 @@ pub async fn get_review_metrics( let octocrab = octocrab(&session, &server_state, original_uri).await?; + let staff_usernames = server_state + .config + .get_staff_github_usernames(&octocrab) + .await + .map_err(|err| err.context("Unable to get staff usernames"))?; + let module_futures = module_names .into_iter() .map(async |module_name| { @@ -288,8 +294,13 @@ pub async fn get_review_metrics( let metrics_futures: Vec<_> = prs .into_iter() .map(async |pr| { - crate::prs::get_review_metrics(&octocrab, &server_state.config.github_org, pr) - .await + crate::prs::get_review_metrics( + &octocrab, + &server_state.config.github_org, + pr, + &staff_usernames, + ) + .await }) .collect(); let metrics = join_all(metrics_futures).await; diff --git a/src/prs.rs b/src/prs.rs index 8963c2e..9ce81b1 100644 --- a/src/prs.rs +++ b/src/prs.rs @@ -245,6 +245,8 @@ pub struct AggregatePrMetrics { pub p100_needs_review_to_complete: Option, pub iteration_counts: BTreeMap, + + pub reviewed_by_counts: BTreeMap, } impl AggregatePrMetrics { @@ -271,10 +273,12 @@ impl AggregatePrMetrics { }); let mut iteration_counts = BTreeMap::new(); + let mut reviewed_by_counts = BTreeMap::new(); for metric in metrics { if metric.first_complete.is_some() { *iteration_counts.entry(metric.iterations).or_default() += 1; } + *reviewed_by_counts.entry(metric.reviewed_by).or_default() += 1; } AggregatePrMetrics { @@ -288,6 +292,7 @@ impl AggregatePrMetrics { p90_needs_review_to_complete, p100_needs_review_to_complete, iteration_counts, + reviewed_by_counts, } } @@ -322,6 +327,7 @@ pub struct PrMetrics { pub first_reviewed: Option>, pub first_complete: Option>, pub iterations: usize, + pub reviewed_by: ReviewedBy, } impl PrMetrics { @@ -343,6 +349,7 @@ impl PrMetrics { first_needs_review = Some(event.time); } } else if event.actor != pr.author { + reviewed_by.observe_also(&event.actor, staff); if event.label == "Reviewed" { iterations += 1; if first_reviewed.is_none() { @@ -365,6 +372,7 @@ impl PrMetrics { first_reviewed, first_complete, iterations, + reviewed_by, } } @@ -385,6 +393,39 @@ impl PrMetrics { } } +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, PartialOrd, Ord, strum_macros::Display)] +pub enum ReviewedBy { + NoOne, + OnlyStaff, + OnlyVolunteer, + StaffAndVolunteer, +} + +impl ReviewedBy { + fn observe_also(&mut self, user: &GithubLogin, staff: &BTreeSet) { + match self { + ReviewedBy::NoOne => { + if staff.contains(user) { + *self = ReviewedBy::OnlyStaff; + } else { + *self = ReviewedBy::OnlyVolunteer; + } + } + ReviewedBy::OnlyStaff => { + if !staff.contains(user) { + *self = ReviewedBy::StaffAndVolunteer; + } + } + ReviewedBy::OnlyVolunteer => { + if staff.contains(user) { + *self = ReviewedBy::StaffAndVolunteer; + } + } + ReviewedBy::StaffAndVolunteer => {} + } + } +} + #[derive(Clone, Debug, PartialEq, Eq, Serialize)] pub struct LabelAddEvent { pub actor: GithubLogin, @@ -521,6 +562,7 @@ pub(crate) async fn get_review_metrics( octocrab: &Octocrab, github_org: &str, pr: Pr, + staff: &BTreeSet, ) -> Result { let events = all_pages("timeline events", octocrab, async || { octocrab @@ -561,7 +603,7 @@ pub(crate) async fn get_review_metrics( ) .collect(); let created_at = pr.created_at; - Ok(PrMetrics::new(pr, created_at, label_add_events)) + Ok(PrMetrics::new(pr, created_at, label_add_events, staff)) } // Ideally this would be a more general shared function, but async closures aren't super stable yet. diff --git a/templates/review-metrics.html b/templates/review-metrics.html index 98505fb..d17e842 100644 --- a/templates/review-metrics.html +++ b/templates/review-metrics.html @@ -10,7 +10,7 @@ } .stats-container { display: grid; - grid-template-columns: repeat(4, 1fr); + grid-template-columns: repeat(5, 1fr); gap: 40px; } .reviews-container { @@ -65,6 +65,14 @@

Iterations

{% endfor %} +
+

Reviews by

+
    + {% for (reviewed_by, count) in aggregate_metrics.reviewed_by_counts %} +
  • {{reviewed_by}}: {{count}} PRs
    • + {% endfor %} +
+
{% for module in modules %} @@ -105,6 +113,15 @@

Iterations

{% endfor %} +
+

Reviews by

+
    + {% for (reviewed_by, count) in module.aggregate_metrics.reviewed_by_counts %} +
  • {{reviewed_by}}: {{count}} PRs
    • + {% endfor %} +
+
+
{% for pr in module.metrics %} @@ -114,6 +131,7 @@

Iterations

Needs Review to Complete: {{format_duration(&pr.needs_review_to_complete())}}
{{pr.iterations}} iterations
Time since created: {{format_duration(&Some(pr.time_since_created()))}}
+
Reviewed by: {{pr.reviewed_by}}
{% endfor %}