Improve PR metrics

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 = [

config.prod.json

@@ -871,5 +871,6 @@
         }
       }
     }
-  }
+  },
+  "staff_github_team_name": "$CYF_STAFF_GITHUB_TEAM_NAME"
 }

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<String>,
 }
 
 #[derive(Clone, Deserialize)]
@@ -77,6 +87,24 @@ impl Config {
             None
         }
     }
+
+    pub async fn get_staff_github_usernames(
+        &self,
+        octocrab: &Octocrab,
+    ) -> Result<BTreeSet<GithubLogin>, 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)]

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))
 }

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;
@@ -339,10 +350,14 @@ pub struct ModuleReviewMetrics {
 impl ReviewMetricsTemplate {
     pub fn format_duration(&self, duration: &Option<TimeDelta>) -> 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()
         }

src/prs.rs

@@ -100,29 +100,21 @@ 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;
                 }
 
                 // 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 {
@@ -253,6 +245,8 @@ pub struct AggregatePrMetrics {
     pub p100_needs_review_to_complete: Option<TimeDelta>,
 
     pub iteration_counts: BTreeMap<usize, usize>,
+
+    pub reviewed_by_counts: BTreeMap<ReviewedBy, usize>,
 }
 
 impl AggregatePrMetrics {
@@ -279,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 {
@@ -296,6 +292,7 @@ impl AggregatePrMetrics {
             p90_needs_review_to_complete,
             p100_needs_review_to_complete,
             iteration_counts,
+            reviewed_by_counts,
         }
     }
 
@@ -330,33 +327,39 @@ pub struct PrMetrics {
     pub first_reviewed: Option<chrono::DateTime<chrono::Utc>>,
     pub first_complete: Option<chrono::DateTime<chrono::Utc>>,
     pub iterations: usize,
+    pub reviewed_by: ReviewedBy,
 }
 
 impl PrMetrics {
     fn new(
         pr: Pr,
         created_at: chrono::DateTime<chrono::Utc>,
         label_add_events: Vec<LabelAddEvent>,
+        staff: &BTreeSet<GithubLogin>,
     ) -> 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 {
+                reviewed_by.observe_also(&event.actor, staff);
+                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);
+                    }
                 }
             }
         }
@@ -369,6 +372,7 @@ impl PrMetrics {
             first_reviewed,
             first_complete,
             iterations,
+            reviewed_by,
         }
     }
 
@@ -389,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<GithubLogin>) {
+        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,
@@ -525,6 +562,7 @@ pub(crate) async fn get_review_metrics(
     octocrab: &Octocrab,
     github_org: &str,
     pr: Pr,
+    staff: &BTreeSet<GithubLogin>,
 ) -> Result<PrMetrics, Error> {
     let events = all_pages("timeline events", octocrab, async || {
         octocrab
@@ -565,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.

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 @@ <h3>Iterations</h3>
                     {% endfor %}
                 </ul>
             </div>
+            <div class="stats-card">
+                <h3>Reviews by</h3>
+                <ul>
+                    {% for (reviewed_by, count) in aggregate_metrics.reviewed_by_counts %}
+                        <li>{{reviewed_by}}: {{count}} PRs</li>
+                    {% endfor %}
+                </ul>
+            </div>
         </div>
 
         {% for module in modules %}
@@ -105,6 +113,15 @@ <h3>Iterations</h3>
                                 {% endfor %}
                             </ul>
                         </div>
+                        <div class="stats-card">
+                            <h3>Reviews by</h3>
+                            <ul>
+                                {% for (reviewed_by, count) in module.aggregate_metrics.reviewed_by_counts %}
+                                    <li>{{reviewed_by}}: {{count}} PRs</li>
+                                {% endfor %}
+                            </ul>
+                        </div>
+
                     </div>
                     <div class="reviews-container">
                         {% for pr in module.metrics %}
@@ -114,6 +131,7 @@ <h3>Iterations</h3>
                                 <div>Needs Review to Complete: {{format_duration(&pr.needs_review_to_complete())}}</div>
                                 <div>{{pr.iterations}} iterations</div>
                                 <div>Time since created: {{format_duration(&Some(pr.time_since_created()))}}</div>
+                                <div>Reviewed by: {{pr.reviewed_by}}</div>
                             </div>
                         {% endfor %}
                     </div>