From f5bcf21c0601d1fe53b38df5bfeff83ec0ec7633 Mon Sep 17 00:00:00 2001 From: Julian Date: Tue, 30 Aug 2022 13:40:12 -0300 Subject: [PATCH 01/17] Bring already implemented code --- Cargo.lock | 66 +++++++--- Cargo.toml | 1 + parser/src/command.rs | 8 ++ parser/src/command/decision.rs | 189 +++++++++++++++++++++++++++++ src/config.rs | 10 ++ src/handlers.rs | 2 + src/handlers/decision.rs | 212 +++++++++++++++++++++++++++++++++ 7 files changed, 474 insertions(+), 14 deletions(-) create mode 100644 parser/src/command/decision.rs create mode 100644 src/handlers/decision.rs diff --git a/Cargo.lock b/Cargo.lock index ef0fa9e9..a69f5482 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -309,6 +309,16 @@ dependencies = [ "typenum", ] +[[package]] +name = "ctor" +version = "0.1.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cdffe87e1d521a10f9696f833fe502293ea446d7f256c06128293a4119bdf4cb" +dependencies = [ + "quote", + "syn", +] + [[package]] name = "cynic" version = "2.2.2" @@ -382,6 +392,12 @@ dependencies = [ "syn", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "digest" version = "0.8.1" @@ -1169,6 +1185,15 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "output_vt100" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "628223faebab4e3e40667ee0b2336d34a5b960ff60ea743ddfdbcf7770bcfb66" +dependencies = [ + "winapi", +] + [[package]] name = "parking_lot" version = "0.11.2" @@ -1372,13 +1397,25 @@ version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eb9f9e6e233e5c4a35559a617bf40a4ec447db2e84c20b55a6f83167b7e57872" +[[package]] +name = "pretty_assertions" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c89f989ac94207d048d92db058e4f6ec7342b0971fc58d1271ca148b799b3563" +dependencies = [ + "ansi_term", + "ctor", + "diff", + "output_vt100", +] + [[package]] name = "proc-macro2" -version = "1.0.37" +version = "1.0.43" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ec757218438d5fda206afc041538b2f6d889286160d649a86a24d37e1235afd1" +checksum = "0a2ca2c61bc9f3d74d2886294ab7b9853abd9c1ad903a3ac7815c58989bb7bab" dependencies = [ - "unicode-xid", + "unicode-ident", ] [[package]] @@ -1395,9 +1432,9 @@ dependencies = [ [[package]] name = "quote" -version = "1.0.18" +version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1feb54ed693b93a84e14094943b84b7c4eae204c512b7ccb95ab0c66d278ad1" +checksum = "bbe448f377a7d6961e30f5955f9b8d106c3f5e449d493ee1b125c1d43c2b5179" dependencies = [ "proc-macro2", ] @@ -1774,13 +1811,13 @@ checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601" [[package]] name = "syn" -version = "1.0.91" +version = "1.0.99" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b683b2b825c8eef438b77c36a06dc262294da3d5a5813fac20da149241dcd44d" +checksum = "58dbef6ec655055e20b86b15a8cc6d439cca19b667537ac6a1369572d151ab13" dependencies = [ "proc-macro2", "quote", - "unicode-xid", + "unicode-ident", ] [[package]] @@ -2093,6 +2130,7 @@ dependencies = [ "parser", "postgres-native-tls", "postgres-types", + "pretty_assertions", "rand", "regex", "reqwest", @@ -2217,6 +2255,12 @@ version = "0.3.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1a01404663e3db436ed2746d9fefef640d868edae3cceb81c3b8d5732fda678f" +[[package]] +name = "unicode-ident" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c4f5b37a154999a8f3f98cc23a628d850e154479cd94decf3414696e12e31aaf" + [[package]] name = "unicode-normalization" version = "0.1.19" @@ -2232,12 +2276,6 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ed742d4ea2bd1176e236172c8429aaf54486e7ac098db29ffe6529e0ce50973" -[[package]] -name = "unicode-xid" -version = "0.2.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" - [[package]] name = "unicode_categories" version = "0.1.1" diff --git a/Cargo.toml b/Cargo.toml index 26780fa6..2caa6fe8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,7 @@ rand = "0.8.5" ignore = "0.4.18" postgres-types = { version = "0.2.4", features = ["derive"] } cron = { version = "0.12.0" } +pretty_assertions = "1.2" [dependencies.serde] version = "1" diff --git a/parser/src/command.rs b/parser/src/command.rs index 7e40949d..94fe3d54 100644 --- a/parser/src/command.rs +++ b/parser/src/command.rs @@ -13,6 +13,7 @@ pub mod prioritize; pub mod relabel; pub mod second; pub mod shortcut; +pub mod decision; #[derive(Debug, PartialEq)] pub enum Command<'a> { @@ -26,6 +27,7 @@ pub enum Command<'a> { Shortcut(Result>), Close(Result>), Note(Result>), + Decision(Result>), } #[derive(Debug)] @@ -132,6 +134,11 @@ impl<'a> Input<'a> { Command::Close, &original_tokenizer, )); + success.extend(parse_single_command( + decision::DecisionCommand::parse, + Command::Decision, + &original_tokenizer, + )); if success.len() > 1 { panic!( @@ -207,6 +214,7 @@ impl<'a> Command<'a> { Command::Shortcut(r) => r.is_ok(), Command::Close(r) => r.is_ok(), Command::Note(r) => r.is_ok(), + Command::Decision(r) => r.is_ok(), } } diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs new file mode 100644 index 00000000..5881dc3a --- /dev/null +++ b/parser/src/command/decision.rs @@ -0,0 +1,189 @@ +//! The decision process command parser. +//! +//! This can parse arbitrary input, giving the user to be assigned. +//! +//! The grammar is as follows: +//! +//! ```text +//! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. +//! ``` + +use crate::error::Error; +use crate::token::{Token, Tokenizer}; +use std::fmt; + +/// A command as parsed and received from calling the bot with some arguments, +/// like `@rustbot merge` +#[derive(PartialEq, Eq, Debug)] +pub struct DecisionCommand { + user: String, + disposition: Resolution, + reversibility: Reversibility, + issue_id: String, + comment_id: String, +} + + +#[derive(Debug)] +pub enum Error { + /// The first command that was given to this bot is not a valid one. + /// Decision process must start with a resolution. + InvalidFirstCommand, +} + +use Error::*; + +#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] +enum Reversibility { + Reversible, + Irreversible, +} + +use Reversibility::*; + +impl fmt::Display for Reversibility { + fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + match self { + Reversible => write!(formatter, "a **reversible**"), + Irreversible => writeln!(formatter, "an **irreversible**"), + } + } +} + +#[derive(Debug, PartialEq, Serialize, Deserialize)] +enum Resolution { + Hold, + Custom(String), +} + +use Resolution::*; + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UserStatus { + name: String, + issue_id: String, + comment_id: String, +} + +impl UserStatus { + fn new(name: String, issue_id: String, comment_id: String) -> UserStatus { + UserStatus { + name, + issue_id, + comment_id, + } + } +} + +impl fmt::Display for UserStatus { + fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { + write!(formatter, "{}", self.name) + } +} +pub trait LinkRenderer { + fn render_link(&self, data: &UserStatus) -> String; +} + +#[derive(Debug, Serialize, Deserialize)] +pub struct State { + initiator: String, + team_members: Vec, + period_start: DateTime, + original_period_start: DateTime, + current_statuses: HashMap, + status_history: HashMap>, + reversibility: Reversibility, + resolution: Resolution, +} + +impl State { + /// Renders the current state to the form it will have when seen as a + /// comment in the github issue/PR + pub fn render(&self, renderer: &impl LinkRenderer) -> String { + let initiator = &self.initiator; + let reversibility = self.reversibility.to_string(); + let comment = format!("Hello! {initiator} has proposed to merge this. This is {reversibility} decision, which means that it will be affirmed once the \"final comment period\" of 10 days have passed, unless a team member places a \"hold\" on the decision (or cancels it).\n\n"); + + let mut table = String::from(if self.status_history.is_empty() { + "| Team member | State |\n\ + |-------------|-------|\n" + } else { + "| Team member | History | State |\n\ + |-------------|---------|-------|\n" + }); + + for member in self.team_members.iter() { + let current_status = self + .current_statuses + .get(member) + .map(|status| { + let link = renderer.render_link(status); + + format!("[{status}]({link})") + }) + .unwrap_or_else(|| "".into()); + + if self.status_history.is_empty() { + table.push_str(&format!("| {member} | {current_status} |\n")); + } else { + let status_history = self + .status_history + .get(member) + .map(|statuses| { + statuses + .iter() + .map(|status| { + let link = renderer.render_link(status); + + format!("[{status}]({link})") + }) + .collect::>() + .join(" ") + }) + .unwrap_or_else(|| "".into()); + + table.push_str(&format!( + "| {member} | {status_history} | {current_status} |\n" + )); + } + } + + comment + &table + } +} + +impl Command { + #[cfg(test)] + fn merge(user: String, issue_id: String, comment_id: String) -> Self { + Self { + user, + issue_id, + comment_id, + disposition: Custom("merge".to_owned()), + reversibility: Reversibility::Reversible, + } + } + + #[cfg(test)] + fn hold(user: String, issue_id: String, comment_id: String) -> Self { + Self { + user, + issue_id, + comment_id, + disposition: Hold, + reversibility: Reversibility::Reversible, + } + } +} + + +pub struct Context { + team_members: Vec, + now: DateTime, +} + +impl Context { + pub fn new(team_members: Vec, now: DateTime) -> Context { + Context { team_members, now } + } +} diff --git a/src/config.rs b/src/config.rs index 053aa682..203d2366 100644 --- a/src/config.rs +++ b/src/config.rs @@ -34,6 +34,7 @@ pub(crate) struct Config { pub(crate) note: Option, pub(crate) mentions: Option, pub(crate) no_merges: Option, + pub(crate) decision: Option, } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] @@ -257,6 +258,12 @@ pub(crate) struct GitHubReleasesConfig { pub(crate) changelog_branch: String, } +#[derive(PartialEq, Eq, Debug, serde::Deserialize)] +pub(crate) struct DecisionConfig { + #[serde(default)] + _empty: (), +} + fn get_cached_config(repo: &str) -> Option, ConfigurationError>> { let cache = CONFIG_CACHE.read().unwrap(); cache.get(repo).and_then(|(config, fetch_time)| { @@ -343,6 +350,8 @@ mod tests { infra = "T-infra" [shortcut] + + [decision-process] "#; let config = toml::from_str::(&config).unwrap(); let mut ping_teams = HashMap::new(); @@ -395,6 +404,7 @@ mod tests { review_submitted: None, mentions: None, no_merges: None, + decision: None } ); } diff --git a/src/handlers.rs b/src/handlers.rs index da39943b..1cac878e 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -45,6 +45,7 @@ mod review_submitted; mod rfc_helper; pub mod rustc_commits; mod shortcut; +mod decision; pub async fn handle(ctx: &Context, event: &Event) -> Vec { let config = config::get(&ctx.github, event.repo()).await; @@ -275,6 +276,7 @@ command_handlers! { shortcut: Shortcut, close: Close, note: Note, + decision: Decision, } pub struct Context { diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs new file mode 100644 index 00000000..6e7442a8 --- /dev/null +++ b/src/handlers/decision.rs @@ -0,0 +1,212 @@ +use parser::command::decision::{ + Context, DecisionCommand, Error, Resolution, Reversibility, State, UserStatus, InvalidFirstCommand +}; +use parser::command::decision::Resolution::*; +use std::collections::HashMap; + +/// Applies a command to the current state and returns the next state +pub(super) async fn handle_command( + ctx: &Context, + _config: &DecisionConfig, + event: &Event, + cmd: DecisionCommand, +) -> Result { + let DecisionCommand { + user, + issue_id, + comment_id, + disposition, + reversibility, + } = command; + + if let Some(state) = state { + let name = match disposition { + Hold => "hold".into(), + Custom(name) => name, + }; + + let mut current_statuses = state.current_statuses; + let mut status_history = state.status_history; + + if let Some(entry) = current_statuses.get_mut(&user) { + let past = status_history.entry(user).or_insert(Vec::new()); + + past.push(entry.clone()); + + *entry = UserStatus::new(name, issue_id, comment_id); + } else { + current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); + } + + Ok(State { + current_statuses, + status_history, + ..state + }) + } else { + // no state, this is the first call to the decision process + match disposition { + Hold => Err(InvalidFirstCommand), + + Custom(name) => { + let mut statuses = HashMap::new(); + + statuses.insert( + user.clone(), + UserStatus::new(name.clone(), issue_id, comment_id), + ); + + let Context { team_members, now } = context; + + Ok(State { + initiator: user, + team_members, + period_start: now, + original_period_start: now, + current_statuses: statuses, + status_history: HashMap::new(), + reversibility, + resolution: Custom(name), + }) + } + } + } +} + +#[cfg(test)] +mod tests { + use chrono::{Duration, Utc}; + use pretty_assertions::assert_eq; + + use super::*; + + struct TestRenderer {} + + impl LinkRenderer for TestRenderer { + fn render_link(&self, data: &UserStatus) -> String { + let issue_id = &data.issue_id; + let comment_id = &data.comment_id; + + format!("http://example.com/issue/{issue_id}#comment={comment_id}") + } + } + + /// Example 1 + /// + /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal + /// + /// * From the starting point of there not being any state, someone proposes + /// to merge a proposal + /// * then barbara holds + /// * 11 days pass + /// * barbara says merge, it immediatly merges + #[test] + fn example_merging_proposal() { + let team_members = vec![ + "@Alan".to_owned(), + "@Barbara".to_owned(), + "@Grace".to_owned(), + "@Niklaus".to_owned(), + ]; + let r = TestRenderer {}; + + // alan proposes to merge + let time1 = Utc::now(); + let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); + let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); + + assert_eq!(state.period_start, time1); + assert_eq!(state.original_period_start, time1); + assert_eq!( + state.current_statuses, + vec![( + "@Alan".into(), + UserStatus::new("merge".into(), "1".into(), "1".into()) + ),] + .into_iter() + .collect() + ); + assert_eq!(state.status_history, HashMap::new()); + assert_eq!(state.reversibility, Reversibility::Reversible); + assert_eq!(state.resolution, Custom("merge".into())); + assert_eq!( + state.render(&r), + include_str!("../../test/decision/res/01_merging_proposal__1.md") + ); + + // barbara holds + let time2 = Utc::now(); + let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); + let state = handle_command( + Some(state), + command, + Context::new(team_members.clone(), time2), + ) + .unwrap(); + + assert_eq!(state.period_start, time1); + assert_eq!(state.original_period_start, time1); + assert_eq!( + state.current_statuses, + vec![ + ( + "@Alan".into(), + UserStatus::new("merge".into(), "1".into(), "1".into()) + ), + ( + "@Barbara".into(), + UserStatus::new("hold".into(), "1".into(), "2".into()) + ), + ] + .into_iter() + .collect() + ); + assert_eq!(state.status_history, HashMap::new()); + assert_eq!(state.reversibility, Reversibility::Reversible); + assert_eq!(state.resolution, Custom("merge".into())); + assert_eq!( + state.render(&r), + include_str!("../../test/decision/res/01_merging_proposal__2.md") + ); + + // 11 days pass + let time3 = time2 + Duration::days(11); + + // Barbara says merge, it immediatly merges + let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); + let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); + + assert_eq!(state.period_start, time1); + assert_eq!(state.original_period_start, time1); + assert_eq!( + state.current_statuses, + vec![ + ( + "@Alan".into(), + UserStatus::new("merge".into(), "1".into(), "1".into()) + ), + ( + "@Barbara".into(), + UserStatus::new("merge".into(), "1".into(), "3".into()) + ), + ] + .into_iter() + .collect() + ); + assert_eq!( + state.status_history, + vec![( + "@Barbara".into(), + vec![UserStatus::new("hold".into(), "1".into(), "2".into())] + ),] + .into_iter() + .collect() + ); + assert_eq!(state.reversibility, Reversibility::Reversible); + assert_eq!(state.resolution, Custom("merge".into())); + assert_eq!( + state.render(&r), + include_str!("../../test/decision/01_merging_proposal__3.md") + ); + } +} From 2d9ff5b26a5260d5d56efc1d375682160300f52e Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 19 Oct 2022 17:03:20 -0300 Subject: [PATCH 02/17] Add decision state db support --- src/db.rs | 18 +++++++ src/db/decision_state.rs | 108 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 src/db/decision_state.rs diff --git a/src/db.rs b/src/db.rs index a696be99..1d644233 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,6 +8,7 @@ use std::sync::{Arc, Mutex}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; use tokio_postgres::Client as DbClient; +pub mod decision_state; pub mod issue_data; pub mod jobs; pub mod notifications; @@ -273,4 +274,21 @@ CREATE UNIQUE INDEX jobs_name_scheduled_at_unique_index name, scheduled_at ); ", + " +CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); +", + " +CREATE TYPE resolution AS ENUM ('hold', 'merge'); +", + "CREATE TABLE decision_state ( + issue_id BIGINT PRIMARY KEY, + initiator TEXT NOT NULL, + team_members JSONB NOT NULL, + start_date TIMESTAMP WITH TIME ZONE NOT NULL, + period_end_date TIMESTAMP WITH TIME ZONE NOT NULL, + current_statuses JSONB NOT NULL, + status_history JSONB, + reversibility reversibility NOT NULL DEFAULT 'reversible', + resolution resolution NOT NULL DEFAULT 'merge' +);", ]; diff --git a/src/db/decision_state.rs b/src/db/decision_state.rs new file mode 100644 index 00000000..cadf6b59 --- /dev/null +++ b/src/db/decision_state.rs @@ -0,0 +1,108 @@ +//! The decision state table provides a way to store the state of each issue + +use serde::{Deserialize, Serialize}; +use chrono::{DateTime, FixedOffset}; +use std::collections::HashMap; +use parser::command::decision::{Resolution, Reversibility}; +use anyhow::{Context as _, Result}; +use tokio_postgres::Client as DbClient; + +#[derive(Debug, Serialize, Deserialize)] +pub struct DecisionState { + issue_id: String, + initiator: String, + team_members: Vec, + start_date: DateTime, + period_date: DateTime, + current_statuses: HashMap, + status_history: HashMap>, + reversibility: Reversibility, + resolution: Resolution, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UserStatus { + name: String, + issue_id: String, + comment_id: String, +} + +pub async fn insert_decision_state( + db: &DbClient, + issue_id: &String, + initiator: &String, + team_members: &Vec, + start_date: &DateTime, + period_end_date: &DateTime, + current_statuses: &HashMap, + status_history: &HashMap>, + reversibility: &Reversibility, + resolution: &Resolution, +) -> Result<()> { + tracing::trace!("insert_decision_state(issue_id={})", issue_id); + + db.execute( + "INSERT INTO decision_state (issue_id, initiator, team_members, start_date, period_end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + ON CONFLICT issue_id DO NOTHING", + &[&issue_id, &initiator, &team_members, &start_date, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution], + ) + .await + .context("Inserting decision state")?; + + Ok(()) +} + +// pub async fn update_decision_state( +// db: &DbClient, +// issue_id: &String, +// period_end_date: &DateTime, +// current_statuses: &HashMap, +// status_history: &HashMap>, +// reversibility: &Reversibility, +// resolution: &Resolution +// ) -> Result<()> { +// tracing::trace!("update_decision_state(issue_id={})", issue_id); + +// db.execute("UPDATE decision_state SET period_end_date = $2, current_statuses = $3, status_history = $4, reversibility = $5, resolution = $6 WHERE issue_id = $1", +// &[&issue_id, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution] +// ) +// .await +// .context("Updating decision state")?; + +// Ok(()) +// } + +// pub async fn get_decision_state_for_issue(db: &DbClient, issue_id: &String) -> Result { +// let state = db +// .query( +// "SELECT * FROM decision_state WHERE issue_id = $1", +// &[&issue_id] +// ) +// .await +// .context("Getting decision state data")?; + + +// let issue_id: String = state.get(0); +// let initiator: String = state.get(1); +// let team_members: Vec = state.get(2); +// let start_date: DateTime = state.get(3); +// let period_date: DateTime = state.get(4); +// let current_statuses: HashMap = state.get(5); +// let status_history: HashMap> = state.get(6); +// let reversibility: Reversibility = state.get(7); +// let resolution: Resolution = state.get(8); + +// Ok( +// DecisionState { +// issue_id, +// initiator, +// team_members, +// start_date, +// period_date, +// current_statuses, +// status_history, +// reversibility, +// resolution, +// } +// ) +// } From 3b2a789f322d3ff47e56c264136be5997d2529ca Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 19 Oct 2022 17:03:27 -0300 Subject: [PATCH 03/17] Start working on handler --- Cargo.lock | 6 +- parser/Cargo.toml | 5 + parser/src/command/decision.rs | 191 +++------------ src/db/decision_state.rs | 10 +- src/handlers/decision.rs | 425 ++++++++++++++++++--------------- 5 files changed, 269 insertions(+), 368 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a69f5482..12bae674 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1224,8 +1224,10 @@ name = "parser" version = "0.1.0" dependencies = [ "log", + "postgres-types", "pulldown-cmark", "regex", + "serde", ] [[package]] @@ -1335,9 +1337,9 @@ checksum = "1df8c4ec4b0627e53bdf214615ad287367e482558cf84b109250b37464dc03ae" [[package]] name = "postgres-derive" -version = "0.4.2" +version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d0c2c18e40b92144b05e6f3ae9d1ee931f0d1afa9410ac8b97486c6eaaf91201" +checksum = "9e76c801e97c9cf696097369e517785b98056e98b21149384c812febfc5912f2" dependencies = [ "proc-macro2", "quote", diff --git a/parser/Cargo.toml b/parser/Cargo.toml index 22bf0aaf..d73d4aa9 100644 --- a/parser/Cargo.toml +++ b/parser/Cargo.toml @@ -8,3 +8,8 @@ edition = "2021" pulldown-cmark = "0.7.0" log = "0.4" regex = "1.6.0" +postgres-types = { version = "0.2.4", features = ["derive"] } + +[dependencies.serde] +version = "1" +features = ["derive"] diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 5881dc3a..31e22c97 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,182 +8,47 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` +use crate::token::{Tokenizer}; use crate::error::Error; -use crate::token::{Token, Tokenizer}; -use std::fmt; +use serde::{Deserialize, Serialize}; +use postgres_types::{FromSql, ToSql}; /// A command as parsed and received from calling the bot with some arguments, /// like `@rustbot merge` -#[derive(PartialEq, Eq, Debug)] +#[derive(Debug, Eq, PartialEq)] pub struct DecisionCommand { - user: String, - disposition: Resolution, - reversibility: Reversibility, - issue_id: String, - comment_id: String, + pub resolution: Resolution, + pub reversibility: Reversibility } - -#[derive(Debug)] -pub enum Error { - /// The first command that was given to this bot is not a valid one. - /// Decision process must start with a resolution. - InvalidFirstCommand, +impl DecisionCommand { + pub fn parse<'a>(_input: &mut Tokenizer<'a>) -> Result, Error<'a>> { + Ok(Some(Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible + })) + } } -use Error::*; +#[derive(Debug, Eq, PartialEq)] +pub enum ParseError { + InvalidFirstCommand +} -#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] -enum Reversibility { +#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[postgres(name = "reversibility")] +pub enum Reversibility { + #[postgres(name = "reversible")] Reversible, + #[postgres(name = "irreversible")] Irreversible, } -use Reversibility::*; - -impl fmt::Display for Reversibility { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - match self { - Reversible => write!(formatter, "a **reversible**"), - Irreversible => writeln!(formatter, "an **irreversible**"), - } - } -} - -#[derive(Debug, PartialEq, Serialize, Deserialize)] -enum Resolution { +#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[postgres(name = "resolution")] +pub enum Resolution { + #[postgres(name = "merge")] + Merge, + #[postgres(name = "hold")] Hold, - Custom(String), -} - -use Resolution::*; - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct UserStatus { - name: String, - issue_id: String, - comment_id: String, -} - -impl UserStatus { - fn new(name: String, issue_id: String, comment_id: String) -> UserStatus { - UserStatus { - name, - issue_id, - comment_id, - } - } -} - -impl fmt::Display for UserStatus { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - write!(formatter, "{}", self.name) - } -} -pub trait LinkRenderer { - fn render_link(&self, data: &UserStatus) -> String; -} - -#[derive(Debug, Serialize, Deserialize)] -pub struct State { - initiator: String, - team_members: Vec, - period_start: DateTime, - original_period_start: DateTime, - current_statuses: HashMap, - status_history: HashMap>, - reversibility: Reversibility, - resolution: Resolution, -} - -impl State { - /// Renders the current state to the form it will have when seen as a - /// comment in the github issue/PR - pub fn render(&self, renderer: &impl LinkRenderer) -> String { - let initiator = &self.initiator; - let reversibility = self.reversibility.to_string(); - let comment = format!("Hello! {initiator} has proposed to merge this. This is {reversibility} decision, which means that it will be affirmed once the \"final comment period\" of 10 days have passed, unless a team member places a \"hold\" on the decision (or cancels it).\n\n"); - - let mut table = String::from(if self.status_history.is_empty() { - "| Team member | State |\n\ - |-------------|-------|\n" - } else { - "| Team member | History | State |\n\ - |-------------|---------|-------|\n" - }); - - for member in self.team_members.iter() { - let current_status = self - .current_statuses - .get(member) - .map(|status| { - let link = renderer.render_link(status); - - format!("[{status}]({link})") - }) - .unwrap_or_else(|| "".into()); - - if self.status_history.is_empty() { - table.push_str(&format!("| {member} | {current_status} |\n")); - } else { - let status_history = self - .status_history - .get(member) - .map(|statuses| { - statuses - .iter() - .map(|status| { - let link = renderer.render_link(status); - - format!("[{status}]({link})") - }) - .collect::>() - .join(" ") - }) - .unwrap_or_else(|| "".into()); - - table.push_str(&format!( - "| {member} | {status_history} | {current_status} |\n" - )); - } - } - - comment + &table - } -} - -impl Command { - #[cfg(test)] - fn merge(user: String, issue_id: String, comment_id: String) -> Self { - Self { - user, - issue_id, - comment_id, - disposition: Custom("merge".to_owned()), - reversibility: Reversibility::Reversible, - } - } - - #[cfg(test)] - fn hold(user: String, issue_id: String, comment_id: String) -> Self { - Self { - user, - issue_id, - comment_id, - disposition: Hold, - reversibility: Reversibility::Reversible, - } - } -} - - -pub struct Context { - team_members: Vec, - now: DateTime, -} - -impl Context { - pub fn new(team_members: Vec, now: DateTime) -> Context { - Context { team_members, now } - } } diff --git a/src/db/decision_state.rs b/src/db/decision_state.rs index cadf6b59..28502451 100644 --- a/src/db/decision_state.rs +++ b/src/db/decision_state.rs @@ -11,9 +11,8 @@ use tokio_postgres::Client as DbClient; pub struct DecisionState { issue_id: String, initiator: String, - team_members: Vec, start_date: DateTime, - period_date: DateTime, + end_date: DateTime, current_statuses: HashMap, status_history: HashMap>, reversibility: Reversibility, @@ -31,9 +30,8 @@ pub async fn insert_decision_state( db: &DbClient, issue_id: &String, initiator: &String, - team_members: &Vec, start_date: &DateTime, - period_end_date: &DateTime, + end_date: &DateTime, current_statuses: &HashMap, status_history: &HashMap>, reversibility: &Reversibility, @@ -42,9 +40,9 @@ pub async fn insert_decision_state( tracing::trace!("insert_decision_state(issue_id={})", issue_id); db.execute( - "INSERT INTO decision_state (issue_id, initiator, team_members, start_date, period_end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + "INSERT INTO decision_state (issue_id, initiator, start_date, end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) ON CONFLICT issue_id DO NOTHING", - &[&issue_id, &initiator, &team_members, &start_date, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution], + &[&issue_id, &initiator, &start_date, &end_date, ¤t_statuses, &status_history, &reversibility, &resolution], ) .await .context("Inserting decision state")?; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 6e7442a8..6dfa27c7 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,212 +1,243 @@ -use parser::command::decision::{ - Context, DecisionCommand, Error, Resolution, Reversibility, State, UserStatus, InvalidFirstCommand +use parser::command::decision::{DecisionCommand, ParseError}; +use crate::{ + config::DecisionConfig, + github::{self, Event}, + handlers::Context, + interactions::ErrorComment, + db::decision_state::* }; -use parser::command::decision::Resolution::*; use std::collections::HashMap; +use chrono::{DateTime, Duration, Utc}; + +// get state for issue_id from db +// if no state (first call) + // initialize state + // schedule job if necessary + // send comment to github + // save state +// if state + // apply logic to decide what to do + // schedule job if necessary + // send comment to github + // save state -/// Applies a command to the current state and returns the next state pub(super) async fn handle_command( ctx: &Context, _config: &DecisionConfig, event: &Event, cmd: DecisionCommand, -) -> Result { +) -> anyhow::Result<()> { let DecisionCommand { - user, - issue_id, - comment_id, - disposition, - reversibility, - } = command; - - if let Some(state) = state { - let name = match disposition { - Hold => "hold".into(), - Custom(name) => name, - }; - - let mut current_statuses = state.current_statuses; - let mut status_history = state.status_history; - - if let Some(entry) = current_statuses.get_mut(&user) { - let past = status_history.entry(user).or_insert(Vec::new()); - - past.push(entry.clone()); - - *entry = UserStatus::new(name, issue_id, comment_id); - } else { - current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); - } + resolution, + reversibility + } = cmd; + + let issue = event.issue().unwrap(); + + let is_team_member = event + .user() + .is_team_member(&ctx.github) + .await + .unwrap_or(false); + + if !is_team_member { + let cmnt = ErrorComment::new(&issue, "Only team members can be part of the decision process."); + cmnt.post(&ctx.github).await?; + return Ok(()); + } - Ok(State { - current_statuses, - status_history, - ..state - }) - } else { - // no state, this is the first call to the decision process - match disposition { - Hold => Err(InvalidFirstCommand), - - Custom(name) => { - let mut statuses = HashMap::new(); - - statuses.insert( - user.clone(), - UserStatus::new(name.clone(), issue_id, comment_id), - ); - - let Context { team_members, now } = context; - - Ok(State { - initiator: user, - team_members, - period_start: now, - original_period_start: now, - current_statuses: statuses, - status_history: HashMap::new(), - reversibility, - resolution: Custom(name), - }) + // return Ok(()); + + match get_decision_state(issue.id) { + Some(state) => { + // let name = match disposition { + // Hold => "hold".into(), + // Custom(name) => name, + // }; + + // let mut current_statuses = state.current_statuses; + // let mut status_history = state.status_history; + + // if let Some(entry) = current_statuses.get_mut(&user) { + // let past = status_history.entry(user).or_insert(Vec::new()); + + // past.push(entry.clone()); + + // *entry = UserStatus::new(name, issue_id, comment_id); + // } else { + // current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); + // } + + // Ok(State { + // current_statuses, + // status_history, + // ..state + // }) + Ok() + }, + None => { + match resolution { + Hold => Err(ParseError::InvalidFirstCommand), + Merge => { + let start_date = chrono::Utc::now().into(); + let end_date = start_date.checked_add_signed(Duration::days(10)).unwrap(); + + let current_statuses = HashMap::new(); + let status_history = HashMap::new(); + + let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? + + insert_decision_state( + db, + issue.id, + user.login, + start_date, + end_date, + current_statuses, + status_history, + reversibility, + resolution, + ); + } } } } } -#[cfg(test)] -mod tests { - use chrono::{Duration, Utc}; - use pretty_assertions::assert_eq; - - use super::*; - - struct TestRenderer {} - - impl LinkRenderer for TestRenderer { - fn render_link(&self, data: &UserStatus) -> String { - let issue_id = &data.issue_id; - let comment_id = &data.comment_id; - - format!("http://example.com/issue/{issue_id}#comment={comment_id}") - } - } - - /// Example 1 - /// - /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal - /// - /// * From the starting point of there not being any state, someone proposes - /// to merge a proposal - /// * then barbara holds - /// * 11 days pass - /// * barbara says merge, it immediatly merges - #[test] - fn example_merging_proposal() { - let team_members = vec![ - "@Alan".to_owned(), - "@Barbara".to_owned(), - "@Grace".to_owned(), - "@Niklaus".to_owned(), - ]; - let r = TestRenderer {}; - - // alan proposes to merge - let time1 = Utc::now(); - let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); - let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); - - assert_eq!(state.period_start, time1); - assert_eq!(state.original_period_start, time1); - assert_eq!( - state.current_statuses, - vec![( - "@Alan".into(), - UserStatus::new("merge".into(), "1".into(), "1".into()) - ),] - .into_iter() - .collect() - ); - assert_eq!(state.status_history, HashMap::new()); - assert_eq!(state.reversibility, Reversibility::Reversible); - assert_eq!(state.resolution, Custom("merge".into())); - assert_eq!( - state.render(&r), - include_str!("../../test/decision/res/01_merging_proposal__1.md") - ); - - // barbara holds - let time2 = Utc::now(); - let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); - let state = handle_command( - Some(state), - command, - Context::new(team_members.clone(), time2), - ) - .unwrap(); - - assert_eq!(state.period_start, time1); - assert_eq!(state.original_period_start, time1); - assert_eq!( - state.current_statuses, - vec![ - ( - "@Alan".into(), - UserStatus::new("merge".into(), "1".into(), "1".into()) - ), - ( - "@Barbara".into(), - UserStatus::new("hold".into(), "1".into(), "2".into()) - ), - ] - .into_iter() - .collect() - ); - assert_eq!(state.status_history, HashMap::new()); - assert_eq!(state.reversibility, Reversibility::Reversible); - assert_eq!(state.resolution, Custom("merge".into())); - assert_eq!( - state.render(&r), - include_str!("../../test/decision/res/01_merging_proposal__2.md") - ); - - // 11 days pass - let time3 = time2 + Duration::days(11); - - // Barbara says merge, it immediatly merges - let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); - let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); - - assert_eq!(state.period_start, time1); - assert_eq!(state.original_period_start, time1); - assert_eq!( - state.current_statuses, - vec![ - ( - "@Alan".into(), - UserStatus::new("merge".into(), "1".into(), "1".into()) - ), - ( - "@Barbara".into(), - UserStatus::new("merge".into(), "1".into(), "3".into()) - ), - ] - .into_iter() - .collect() - ); - assert_eq!( - state.status_history, - vec![( - "@Barbara".into(), - vec![UserStatus::new("hold".into(), "1".into(), "2".into())] - ),] - .into_iter() - .collect() - ); - assert_eq!(state.reversibility, Reversibility::Reversible); - assert_eq!(state.resolution, Custom("merge".into())); - assert_eq!( - state.render(&r), - include_str!("../../test/decision/01_merging_proposal__3.md") - ); - } -} +// #[cfg(test)] +// mod tests { +// use chrono::{Duration, Utc}; +// use pretty_assertions::assert_eq; + +// use super::*; + +// struct TestRenderer {} + +// impl LinkRenderer for TestRenderer { +// fn render_link(&self, data: &UserStatus) -> String { +// let issue_id = &data.issue_id; +// let comment_id = &data.comment_id; + +// format!("http://example.com/issue/{issue_id}#comment={comment_id}") +// } +// } + +// /// Example 1 +// /// +// /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal +// /// +// /// * From the starting point of there not being any state, someone proposes +// /// to merge a proposal +// /// * then barbara holds +// /// * 11 days pass +// /// * barbara says merge, it immediatly merges +// #[test] +// fn example_merging_proposal() { +// let team_members = vec![ +// "@Alan".to_owned(), +// "@Barbara".to_owned(), +// "@Grace".to_owned(), +// "@Niklaus".to_owned(), +// ]; +// let r = TestRenderer {}; + +// // alan proposes to merge +// let time1 = Utc::now(); +// let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); +// let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); + +// assert_eq!(state.period_start, time1); +// assert_eq!(state.original_period_start, time1); +// assert_eq!( +// state.current_statuses, +// vec![( +// "@Alan".into(), +// UserStatus::new("merge".into(), "1".into(), "1".into()) +// ),] +// .into_iter() +// .collect() +// ); +// assert_eq!(state.status_history, HashMap::new()); +// assert_eq!(state.reversibility, Reversibility::Reversible); +// assert_eq!(state.resolution, Custom("merge".into())); +// assert_eq!( +// state.render(&r), +// include_str!("../../test/decision/res/01_merging_proposal__1.md") +// ); + +// // barbara holds +// let time2 = Utc::now(); +// let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); +// let state = handle_command( +// Some(state), +// command, +// Context::new(team_members.clone(), time2), +// ) +// .unwrap(); + +// assert_eq!(state.period_start, time1); +// assert_eq!(state.original_period_start, time1); +// assert_eq!( +// state.current_statuses, +// vec![ +// ( +// "@Alan".into(), +// UserStatus::new("merge".into(), "1".into(), "1".into()) +// ), +// ( +// "@Barbara".into(), +// UserStatus::new("hold".into(), "1".into(), "2".into()) +// ), +// ] +// .into_iter() +// .collect() +// ); +// assert_eq!(state.status_history, HashMap::new()); +// assert_eq!(state.reversibility, Reversibility::Reversible); +// assert_eq!(state.resolution, Custom("merge".into())); +// assert_eq!( +// state.render(&r), +// include_str!("../../test/decision/res/01_merging_proposal__2.md") +// ); + +// // 11 days pass +// let time3 = time2 + Duration::days(11); + +// // Barbara says merge, it immediatly merges +// let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); +// let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); + +// assert_eq!(state.period_start, time1); +// assert_eq!(state.original_period_start, time1); +// assert_eq!( +// state.current_statuses, +// vec![ +// ( +// "@Alan".into(), +// UserStatus::new("merge".into(), "1".into(), "1".into()) +// ), +// ( +// "@Barbara".into(), +// UserStatus::new("merge".into(), "1".into(), "3".into()) +// ), +// ] +// .into_iter() +// .collect() +// ); +// assert_eq!( +// state.status_history, +// vec![( +// "@Barbara".into(), +// vec![UserStatus::new("hold".into(), "1".into(), "2".into())] +// ),] +// .into_iter() +// .collect() +// ); +// assert_eq!(state.reversibility, Reversibility::Reversible); +// assert_eq!(state.resolution, Custom("merge".into())); +// assert_eq!( +// state.render(&r), +// include_str!("../../test/decision/01_merging_proposal__3.md") +// ); +// } +// } From c60424349ee9e3303124088a04f51f28dd074ac4 Mon Sep 17 00:00:00 2001 From: Julian Montes de Oca Date: Wed, 26 Oct 2022 15:18:44 -0300 Subject: [PATCH 04/17] Fix parse to test integration --- parser/src/command/decision.rs | 9 ++++++--- src/handlers/decision.rs | 19 ++++++++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 31e22c97..c6a6df18 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,7 +8,7 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` -use crate::token::{Tokenizer}; +use crate::token::{Token, Tokenizer}; use crate::error::Error; use serde::{Deserialize, Serialize}; use postgres_types::{FromSql, ToSql}; @@ -22,11 +22,14 @@ pub struct DecisionCommand { } impl DecisionCommand { - pub fn parse<'a>(_input: &mut Tokenizer<'a>) -> Result, Error<'a>> { + pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result, Error<'a>> { + if let Some(Token::Word("merge")) = input.peek_token()? { Ok(Some(Self { resolution: Resolution::Merge, reversibility: Reversibility::Reversible - })) + })) } else { + Ok(None) + } } } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 6dfa27c7..3644c3cf 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,3 +1,4 @@ +use anyhow::Context as Ctx; use parser::command::decision::{DecisionCommand, ParseError}; use crate::{ config::DecisionConfig, @@ -46,8 +47,6 @@ pub(super) async fn handle_command( return Ok(()); } - // return Ok(()); - match get_decision_state(issue.id) { Some(state) => { // let name = match disposition { @@ -73,7 +72,7 @@ pub(super) async fn handle_command( // status_history, // ..state // }) - Ok() + Ok(); }, None => { match resolution { @@ -98,6 +97,20 @@ pub(super) async fn handle_command( reversibility, resolution, ); + + let comment = format!( + "Wow, it looks like you want to merge this, {}.", event.user().login + ); + + let comment = format!( + "| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |"); + + issue + .post_comment(&ctx.github, &comment) + .await + .context("merge vote comment")?; + + Ok(); } } } From ec487925f232de9753b3adb7bc29b89635241536 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 9 Nov 2022 16:00:15 -0300 Subject: [PATCH 05/17] Add db support for issue_decision_state --- parser/src/command/decision.rs | 4 +- src/db.rs | 11 ++-- src/db/decision_state.rs | 106 ------------------------------- src/db/issue_decision_state.rs | 112 +++++++++++++++++++++++++++++++++ src/handlers/decision.rs | 76 ++++++++++++---------- 5 files changed, 161 insertions(+), 148 deletions(-) delete mode 100644 src/db/decision_state.rs create mode 100644 src/db/issue_decision_state.rs diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index c6a6df18..1d24829d 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -38,7 +38,7 @@ pub enum ParseError { InvalidFirstCommand } -#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "reversibility")] pub enum Reversibility { #[postgres(name = "reversible")] @@ -47,7 +47,7 @@ pub enum Reversibility { Irreversible, } -#[derive(Serialize, Deserialize, Debug, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "resolution")] pub enum Resolution { #[postgres(name = "merge")] diff --git a/src/db.rs b/src/db.rs index 1d644233..840a9159 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,7 +8,7 @@ use std::sync::{Arc, Mutex}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; use tokio_postgres::Client as DbClient; -pub mod decision_state; +pub mod issue_decision_state; pub mod issue_data; pub mod jobs; pub mod notifications; @@ -280,14 +280,13 @@ CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); " CREATE TYPE resolution AS ENUM ('hold', 'merge'); ", - "CREATE TABLE decision_state ( +"CREATE TABLE issue_decision_state ( issue_id BIGINT PRIMARY KEY, initiator TEXT NOT NULL, - team_members JSONB NOT NULL, start_date TIMESTAMP WITH TIME ZONE NOT NULL, - period_end_date TIMESTAMP WITH TIME ZONE NOT NULL, - current_statuses JSONB NOT NULL, - status_history JSONB, + end_date TIMESTAMP WITH TIME ZONE NOT NULL, + current JSONB NOT NULL, + history JSONB, reversibility reversibility NOT NULL DEFAULT 'reversible', resolution resolution NOT NULL DEFAULT 'merge' );", diff --git a/src/db/decision_state.rs b/src/db/decision_state.rs deleted file mode 100644 index 28502451..00000000 --- a/src/db/decision_state.rs +++ /dev/null @@ -1,106 +0,0 @@ -//! The decision state table provides a way to store the state of each issue - -use serde::{Deserialize, Serialize}; -use chrono::{DateTime, FixedOffset}; -use std::collections::HashMap; -use parser::command::decision::{Resolution, Reversibility}; -use anyhow::{Context as _, Result}; -use tokio_postgres::Client as DbClient; - -#[derive(Debug, Serialize, Deserialize)] -pub struct DecisionState { - issue_id: String, - initiator: String, - start_date: DateTime, - end_date: DateTime, - current_statuses: HashMap, - status_history: HashMap>, - reversibility: Reversibility, - resolution: Resolution, -} - -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -pub struct UserStatus { - name: String, - issue_id: String, - comment_id: String, -} - -pub async fn insert_decision_state( - db: &DbClient, - issue_id: &String, - initiator: &String, - start_date: &DateTime, - end_date: &DateTime, - current_statuses: &HashMap, - status_history: &HashMap>, - reversibility: &Reversibility, - resolution: &Resolution, -) -> Result<()> { - tracing::trace!("insert_decision_state(issue_id={})", issue_id); - - db.execute( - "INSERT INTO decision_state (issue_id, initiator, start_date, end_date, current_statuses, status_history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) - ON CONFLICT issue_id DO NOTHING", - &[&issue_id, &initiator, &start_date, &end_date, ¤t_statuses, &status_history, &reversibility, &resolution], - ) - .await - .context("Inserting decision state")?; - - Ok(()) -} - -// pub async fn update_decision_state( -// db: &DbClient, -// issue_id: &String, -// period_end_date: &DateTime, -// current_statuses: &HashMap, -// status_history: &HashMap>, -// reversibility: &Reversibility, -// resolution: &Resolution -// ) -> Result<()> { -// tracing::trace!("update_decision_state(issue_id={})", issue_id); - -// db.execute("UPDATE decision_state SET period_end_date = $2, current_statuses = $3, status_history = $4, reversibility = $5, resolution = $6 WHERE issue_id = $1", -// &[&issue_id, &period_end_date, ¤t_statuses, &status_history, &reversibility, &resolution] -// ) -// .await -// .context("Updating decision state")?; - -// Ok(()) -// } - -// pub async fn get_decision_state_for_issue(db: &DbClient, issue_id: &String) -> Result { -// let state = db -// .query( -// "SELECT * FROM decision_state WHERE issue_id = $1", -// &[&issue_id] -// ) -// .await -// .context("Getting decision state data")?; - - -// let issue_id: String = state.get(0); -// let initiator: String = state.get(1); -// let team_members: Vec = state.get(2); -// let start_date: DateTime = state.get(3); -// let period_date: DateTime = state.get(4); -// let current_statuses: HashMap = state.get(5); -// let status_history: HashMap> = state.get(6); -// let reversibility: Reversibility = state.get(7); -// let resolution: Resolution = state.get(8); - -// Ok( -// DecisionState { -// issue_id, -// initiator, -// team_members, -// start_date, -// period_date, -// current_statuses, -// status_history, -// reversibility, -// resolution, -// } -// ) -// } diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs new file mode 100644 index 00000000..a8120307 --- /dev/null +++ b/src/db/issue_decision_state.rs @@ -0,0 +1,112 @@ +//! The issue decision state table provides a way to store +//! the decision process state of each issue + +use serde::{Deserialize, Serialize}; +use chrono::{DateTime, FixedOffset}; +use std::collections::BTreeMap; +use parser::command::decision::{Resolution, Reversibility}; +use anyhow::{Context as _, Result}; +use tokio_postgres::Client as DbClient; + +#[derive(Debug, Serialize, Deserialize)] +pub struct IssueDecisionState { + pub issue_id: i64, + pub initiator: String, + pub start_date: DateTime, + pub end_date: DateTime, + pub current: BTreeMap, + pub history: BTreeMap>, + pub reversibility: Reversibility, + pub resolution: Resolution, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct UserStatus { + pub comment_id: String, + pub text: String, + pub reversibility: Reversibility, + pub resolution: Resolution, +} + +pub async fn insert_issue_decision_state( + db: &DbClient, + issue_number: &u64, + initiator: &String, + start_date: &DateTime, + end_date: &DateTime, + current: &BTreeMap, + history: &BTreeMap>, + reversibility: &Reversibility, + resolution: &Resolution, +) -> Result<()> { + tracing::trace!("insert_issue_decision_state(issue_id={})", issue_number); + let issue_id = *issue_number as i64; + + db.execute( + "INSERT INTO issue_decision_state (issue_id, initiator, start_date, end_date, current, history, reversibility, resolution) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + ON CONFLICT DO NOTHING", + &[&issue_id, &initiator, &start_date, &end_date, &serde_json::to_value(current).unwrap(), &serde_json::to_value(history).unwrap(), &reversibility, &resolution], + ) + .await + .context("Inserting decision state")?; + + Ok(()) +} + +pub async fn update_issue_decision_state( + db: &DbClient, + issue_number: &u64, + end_date: &DateTime, + current: &BTreeMap, + history: &BTreeMap>, + reversibility: &Reversibility, + resolution: &Resolution +) -> Result<()> { + tracing::trace!("update_issue_decision_state(issue_id={})", issue_number); + let issue_id = *issue_number as i64; + + db.execute("UPDATE issue_decision_state SET end_date = $2, current = $3, history = $4, reversibility = $5, resolution = $6 WHERE issue_id = $1", + &[&issue_id, &end_date, &serde_json::to_value(current).unwrap(), &serde_json::to_value(history).unwrap(), &reversibility, &resolution] + ) + .await + .context("Updating decision state")?; + + Ok(()) +} + +pub async fn get_issue_decision_state(db: &DbClient, issue_number: &u64) -> Result { + tracing::trace!("get_issue_decision_state(issue_id={})", issue_number); + let issue_id = *issue_number as i64; + + let state = db + .query_one( + "SELECT * FROM issue_decision_state WHERE issue_id = $1", + &[&issue_id] + ) + .await + .context("Getting decision state data")?; + + deserialize_issue_decision_state(&state) +} + +fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result { + let issue_id: i64 = row.try_get(0)?; + let initiator: String = row.try_get(1)?; + let start_date: DateTime = row.try_get(2)?; + let end_date: DateTime = row.try_get(3)?; + let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; + let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; + let reversibility: Reversibility = row.try_get(6)?; + let resolution: Resolution = row.try_get(7)?; + + Ok(IssueDecisionState { + issue_id, + initiator, + start_date, + end_date, + current, + history, + reversibility, + resolution, + }) +} diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 3644c3cf..c22b0b51 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,14 +1,15 @@ use anyhow::Context as Ctx; -use parser::command::decision::{DecisionCommand, ParseError}; use crate::{ config::DecisionConfig, - github::{self, Event}, + github::{Event}, handlers::Context, interactions::ErrorComment, - db::decision_state::* + db::issue_decision_state::* }; -use std::collections::HashMap; -use chrono::{DateTime, Duration, Utc}; +use chrono::{DateTime, Duration, FixedOffset}; +use parser::command::decision::{DecisionCommand, Resolution::*, Reversibility::*}; +use std::collections::BTreeMap; +use chrono::{DateTime, FixedOffset, Duration}; // get state for issue_id from db // if no state (first call) @@ -28,15 +29,18 @@ pub(super) async fn handle_command( event: &Event, cmd: DecisionCommand, ) -> anyhow::Result<()> { + let db = ctx.db.get().await; + let DecisionCommand { resolution, reversibility } = cmd; let issue = event.issue().unwrap(); + let user = event.user(); - let is_team_member = event - .user() + let is_team_member = + user .is_team_member(&ctx.github) .await .unwrap_or(false); @@ -47,8 +51,8 @@ pub(super) async fn handle_command( return Ok(()); } - match get_decision_state(issue.id) { - Some(state) => { + match get_issue_decision_state(&db, &issue.number).await { + Ok(_state) => { // let name = match disposition { // Hold => "hold".into(), // Custom(name) => name, @@ -72,45 +76,49 @@ pub(super) async fn handle_command( // status_history, // ..state // }) - Ok(); + Ok(()) }, - None => { + _ => { match resolution { - Hold => Err(ParseError::InvalidFirstCommand), + Hold => Ok(()), // change me! Merge => { - let start_date = chrono::Utc::now().into(); - let end_date = start_date.checked_add_signed(Duration::days(10)).unwrap(); - - let current_statuses = HashMap::new(); - let status_history = HashMap::new(); + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); - let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? + let mut current: BTreeMap = BTreeMap::new(); + current.insert("mcass19".to_string(), UserStatus{ + comment_id: "comment_id".to_string(), + text: "something".to_string(), + reversibility: Reversible, + resolution: Merge, + }); + let history: BTreeMap> = BTreeMap::new(); - insert_decision_state( - db, - issue.id, - user.login, - start_date, - end_date, - current_statuses, - status_history, - reversibility, - resolution, - ); + insert_issue_decision_state( + &db, + &issue.number, + &user.login, + &start_date, + &end_date, + ¤t, + &history, + &reversibility, + &Merge, + ).await?; - let comment = format!( - "Wow, it looks like you want to merge this, {}.", event.user().login - ); + // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? let comment = format!( - "| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |"); + "Wow, it looks like you want to merge this, {}.\n| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |", + user.login + ); issue .post_comment(&ctx.github, &comment) .await .context("merge vote comment")?; - Ok(); + Ok(()) } } } From 771325519eb2075aa8c558f43b31311df153937a Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 9 Nov 2022 16:01:59 -0300 Subject: [PATCH 06/17] Run format --- parser/src/command.rs | 2 +- parser/src/command/decision.rs | 17 +++++----- src/db.rs | 4 +-- src/db/issue_decision_state.rs | 24 ++++++++------ src/handlers.rs | 2 +- src/handlers/decision.rs | 58 +++++++++++++++++----------------- 6 files changed, 56 insertions(+), 51 deletions(-) diff --git a/parser/src/command.rs b/parser/src/command.rs index 94fe3d54..671ea41f 100644 --- a/parser/src/command.rs +++ b/parser/src/command.rs @@ -5,6 +5,7 @@ use regex::Regex; pub mod assign; pub mod close; +pub mod decision; pub mod glacier; pub mod nominate; pub mod note; @@ -13,7 +14,6 @@ pub mod prioritize; pub mod relabel; pub mod second; pub mod shortcut; -pub mod decision; #[derive(Debug, PartialEq)] pub enum Command<'a> { diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 1d24829d..9e55e27b 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,26 +8,27 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` -use crate::token::{Token, Tokenizer}; use crate::error::Error; -use serde::{Deserialize, Serialize}; +use crate::token::{Token, Tokenizer}; use postgres_types::{FromSql, ToSql}; +use serde::{Deserialize, Serialize}; /// A command as parsed and received from calling the bot with some arguments, /// like `@rustbot merge` #[derive(Debug, Eq, PartialEq)] pub struct DecisionCommand { pub resolution: Resolution, - pub reversibility: Reversibility + pub reversibility: Reversibility, } impl DecisionCommand { pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result, Error<'a>> { if let Some(Token::Word("merge")) = input.peek_token()? { - Ok(Some(Self { - resolution: Resolution::Merge, - reversibility: Reversibility::Reversible - })) } else { + Ok(Some(Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + })) + } else { Ok(None) } } @@ -35,7 +36,7 @@ impl DecisionCommand { #[derive(Debug, Eq, PartialEq)] pub enum ParseError { - InvalidFirstCommand + InvalidFirstCommand, } #[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] diff --git a/src/db.rs b/src/db.rs index 840a9159..bd3de5cc 100644 --- a/src/db.rs +++ b/src/db.rs @@ -8,8 +8,8 @@ use std::sync::{Arc, Mutex}; use tokio::sync::{OwnedSemaphorePermit, Semaphore}; use tokio_postgres::Client as DbClient; -pub mod issue_decision_state; pub mod issue_data; +pub mod issue_decision_state; pub mod jobs; pub mod notifications; pub mod rustc_commits; @@ -280,7 +280,7 @@ CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); " CREATE TYPE resolution AS ENUM ('hold', 'merge'); ", -"CREATE TABLE issue_decision_state ( + "CREATE TABLE issue_decision_state ( issue_id BIGINT PRIMARY KEY, initiator TEXT NOT NULL, start_date TIMESTAMP WITH TIME ZONE NOT NULL, diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs index a8120307..62973948 100644 --- a/src/db/issue_decision_state.rs +++ b/src/db/issue_decision_state.rs @@ -1,11 +1,11 @@ -//! The issue decision state table provides a way to store +//! The issue decision state table provides a way to store //! the decision process state of each issue -use serde::{Deserialize, Serialize}; +use anyhow::{Context as _, Result}; use chrono::{DateTime, FixedOffset}; -use std::collections::BTreeMap; use parser::command::decision::{Resolution, Reversibility}; -use anyhow::{Context as _, Result}; +use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; use tokio_postgres::Client as DbClient; #[derive(Debug, Serialize, Deserialize)] @@ -54,13 +54,13 @@ pub async fn insert_issue_decision_state( } pub async fn update_issue_decision_state( - db: &DbClient, + db: &DbClient, issue_number: &u64, end_date: &DateTime, current: &BTreeMap, history: &BTreeMap>, reversibility: &Reversibility, - resolution: &Resolution + resolution: &Resolution, ) -> Result<()> { tracing::trace!("update_issue_decision_state(issue_id={})", issue_number); let issue_id = *issue_number as i64; @@ -74,18 +74,21 @@ pub async fn update_issue_decision_state( Ok(()) } -pub async fn get_issue_decision_state(db: &DbClient, issue_number: &u64) -> Result { +pub async fn get_issue_decision_state( + db: &DbClient, + issue_number: &u64, +) -> Result { tracing::trace!("get_issue_decision_state(issue_id={})", issue_number); let issue_id = *issue_number as i64; let state = db .query_one( "SELECT * FROM issue_decision_state WHERE issue_id = $1", - &[&issue_id] + &[&issue_id], ) .await .context("Getting decision state data")?; - + deserialize_issue_decision_state(&state) } @@ -95,7 +98,8 @@ fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result = row.try_get(2)?; let end_date: DateTime = row.try_get(3)?; let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; - let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; + let history: BTreeMap> = + serde_json::from_value(row.try_get(5).unwrap())?; let reversibility: Reversibility = row.try_get(6)?; let resolution: Resolution = row.try_get(7)?; diff --git a/src/handlers.rs b/src/handlers.rs index 1cac878e..c0cf9dd1 100644 --- a/src/handlers.rs +++ b/src/handlers.rs @@ -26,6 +26,7 @@ impl fmt::Display for HandlerError { mod assign; mod autolabel; mod close; +mod decision; pub mod docs_update; mod github_releases; mod glacier; @@ -45,7 +46,6 @@ mod review_submitted; mod rfc_helper; pub mod rustc_commits; mod shortcut; -mod decision; pub async fn handle(ctx: &Context, event: &Event) -> Vec { let config = config::get(&ctx.github, event.repo()).await; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index c22b0b51..2841ede5 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,15 +1,11 @@ -use anyhow::Context as Ctx; use crate::{ - config::DecisionConfig, - github::{Event}, - handlers::Context, + config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, interactions::ErrorComment, - db::issue_decision_state::* }; +use anyhow::Context as Ctx; use chrono::{DateTime, Duration, FixedOffset}; use parser::command::decision::{DecisionCommand, Resolution::*, Reversibility::*}; use std::collections::BTreeMap; -use chrono::{DateTime, FixedOffset, Duration}; // get state for issue_id from db // if no state (first call) @@ -33,20 +29,19 @@ pub(super) async fn handle_command( let DecisionCommand { resolution, - reversibility + reversibility, } = cmd; let issue = event.issue().unwrap(); let user = event.user(); - let is_team_member = - user - .is_team_member(&ctx.github) - .await - .unwrap_or(false); + let is_team_member = user.is_team_member(&ctx.github).await.unwrap_or(false); if !is_team_member { - let cmnt = ErrorComment::new(&issue, "Only team members can be part of the decision process."); + let cmnt = ErrorComment::new( + &issue, + "Only team members can be part of the decision process.", + ); cmnt.post(&ctx.github).await?; return Ok(()); } @@ -57,41 +52,45 @@ pub(super) async fn handle_command( // Hold => "hold".into(), // Custom(name) => name, // }; - + // let mut current_statuses = state.current_statuses; // let mut status_history = state.status_history; - + // if let Some(entry) = current_statuses.get_mut(&user) { // let past = status_history.entry(user).or_insert(Vec::new()); - + // past.push(entry.clone()); - + // *entry = UserStatus::new(name, issue_id, comment_id); // } else { // current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); // } - + // Ok(State { // current_statuses, // status_history, // ..state // }) Ok(()) - }, + } _ => { match resolution { Hold => Ok(()), // change me! Merge => { let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); + let end_date: DateTime = + start_date.checked_add_signed(Duration::days(10)).unwrap(); let mut current: BTreeMap = BTreeMap::new(); - current.insert("mcass19".to_string(), UserStatus{ - comment_id: "comment_id".to_string(), - text: "something".to_string(), - reversibility: Reversible, - resolution: Merge, - }); + current.insert( + "mcass19".to_string(), + UserStatus { + comment_id: "comment_id".to_string(), + text: "something".to_string(), + reversibility: Reversible, + resolution: Merge, + }, + ); let history: BTreeMap> = BTreeMap::new(); insert_issue_decision_state( @@ -104,15 +103,16 @@ pub(super) async fn handle_command( &history, &reversibility, &Merge, - ).await?; + ) + .await?; // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? - + let comment = format!( "Wow, it looks like you want to merge this, {}.\n| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |", user.login ); - + issue .post_comment(&ctx.github, &comment) .await From dfe4f7af1bdfbebf484c78219380c3044b6863fa Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Tue, 6 Dec 2022 11:05:51 -0300 Subject: [PATCH 07/17] Schedule job for initial merge --- parser/src/command/decision.rs | 2 +- src/db/issue_decision_state.rs | 16 +++++----- src/github.rs | 25 +++++++++++++-- src/handlers/decision.rs | 57 +++++++++++++++++++++++++--------- src/handlers/jobs.rs | 34 +++++++++++++++++++- 5 files changed, 107 insertions(+), 27 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 9e55e27b..fe237a2a 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -48,7 +48,7 @@ pub enum Reversibility { Irreversible, } -#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "resolution")] pub enum Resolution { #[postgres(name = "merge")] diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs index 62973948..5091880b 100644 --- a/src/db/issue_decision_state.rs +++ b/src/db/issue_decision_state.rs @@ -2,7 +2,7 @@ //! the decision process state of each issue use anyhow::{Context as _, Result}; -use chrono::{DateTime, FixedOffset}; +use chrono::{DateTime, Utc}; use parser::command::decision::{Resolution, Reversibility}; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; @@ -12,8 +12,8 @@ use tokio_postgres::Client as DbClient; pub struct IssueDecisionState { pub issue_id: i64, pub initiator: String, - pub start_date: DateTime, - pub end_date: DateTime, + pub start_date: DateTime, + pub end_date: DateTime, pub current: BTreeMap, pub history: BTreeMap>, pub reversibility: Reversibility, @@ -32,8 +32,8 @@ pub async fn insert_issue_decision_state( db: &DbClient, issue_number: &u64, initiator: &String, - start_date: &DateTime, - end_date: &DateTime, + start_date: &DateTime, + end_date: &DateTime, current: &BTreeMap, history: &BTreeMap>, reversibility: &Reversibility, @@ -56,7 +56,7 @@ pub async fn insert_issue_decision_state( pub async fn update_issue_decision_state( db: &DbClient, issue_number: &u64, - end_date: &DateTime, + end_date: &DateTime, current: &BTreeMap, history: &BTreeMap>, reversibility: &Reversibility, @@ -95,8 +95,8 @@ pub async fn get_issue_decision_state( fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result { let issue_id: i64 = row.try_get(0)?; let initiator: String = row.try_get(1)?; - let start_date: DateTime = row.try_get(2)?; - let end_date: DateTime = row.try_get(3)?; + let start_date: DateTime = row.try_get(2)?; + let end_date: DateTime = row.try_get(3)?; let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; diff --git a/src/github.rs b/src/github.rs index 35309b2b..44cc97f3 100644 --- a/src/github.rs +++ b/src/github.rs @@ -402,7 +402,7 @@ impl fmt::Display for IssueRepository { } impl IssueRepository { - fn url(&self) -> String { + pub fn url(&self) -> String { format!( "https://api.github.com/repos/{}/{}", self.organization, self.repository @@ -779,6 +779,27 @@ impl Issue { Ok(()) } + pub async fn merge(&self, client: &GithubClient) -> anyhow::Result<()> { + let merge_url = format!("{}/pulls/{}/merge", self.repository().url(), self.number); + + // change defaults by reading from somewhere, maybe in .toml? + #[derive(serde::Serialize)] + struct MergeIssue<'a> { + commit_title: &'a str, + merge_method: &'a str + } + + client + ._send_req(client.put(&merge_url).json(&MergeIssue { + commit_title: "Merged by the bot!", + merge_method: "merge" + })) + .await + .context("failed to merge issue")?; + + Ok(()) + } + /// Returns the diff in this event, for Open and Synchronize events for now. pub async fn diff(&self, client: &GithubClient) -> anyhow::Result> { let (before, after) = if let (Some(base), Some(head)) = (&self.base, &self.head) { @@ -1827,7 +1848,7 @@ impl GithubClient { response.text().await.context("raw gist from url") } - fn get(&self, url: &str) -> RequestBuilder { + pub fn get(&self, url: &str) -> RequestBuilder { log::trace!("get {:?}", url); self.client.get(url).configure(self) } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 2841ede5..6cf6c9e5 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,23 +1,35 @@ +use crate::db::jobs::*; use crate::{ config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, interactions::ErrorComment, }; use anyhow::Context as Ctx; -use chrono::{DateTime, Duration, FixedOffset}; -use parser::command::decision::{DecisionCommand, Resolution::*, Reversibility::*}; +use chrono::{DateTime, Duration, Utc}; +use parser::command::decision::Resolution::{Hold, Merge}; +use parser::command::decision::*; +use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; // get state for issue_id from db -// if no state (first call) - // initialize state - // schedule job if necessary - // send comment to github - // save state -// if state - // apply logic to decide what to do - // schedule job if necessary - // send comment to github - // save state + // if no state (first call) + // initialize state + // schedule job if necessary + // send comment to github + // save state + // if state + // apply logic to decide what to do + // schedule job if necessary + // send comment to github + // save state + +pub const DECISION_PROCESS_JOB_NAME: &str = "decision_process_action"; + +#[derive(Serialize, Deserialize)] +pub struct DecisionProcessActionMetadata { + pub message: String, + pub get_issue_url: String, + pub status: Resolution, +} pub(super) async fn handle_command( ctx: &Context, @@ -77,8 +89,8 @@ pub(super) async fn handle_command( match resolution { Hold => Ok(()), // change me! Merge => { - let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); let mut current: BTreeMap = BTreeMap::new(); @@ -87,7 +99,7 @@ pub(super) async fn handle_command( UserStatus { comment_id: "comment_id".to_string(), text: "something".to_string(), - reversibility: Reversible, + reversibility: Reversibility::Reversible, resolution: Merge, }, ); @@ -106,6 +118,21 @@ pub(super) async fn handle_command( ) .await?; + let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { + message: "some message".to_string(), + get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + status: Merge, + }) + .unwrap(); + + insert_job( + &db, + &DECISION_PROCESS_JOB_NAME.to_string(), + &end_date, + &metadata, + ) + .await?; + // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? let comment = format!( diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index adb05e4a..2950a9a5 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -3,8 +3,12 @@ // the job name and the corresponding function. // Further info could be find in src/jobs.rs - use super::Context; +use crate::github::*; +use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_JOB_NAME}; +use parser::command::decision::Resolution::Merge; +use reqwest::Client; +use tracing as log; pub async fn handle_job( ctx: &Context, @@ -17,6 +21,9 @@ pub async fn handle_job( super::rustc_commits::synchronize_commits_inner(ctx, None).await; Ok(()) } + matched_name if *matched_name == DECISION_PROCESS_JOB_NAME.to_string() => { + decision_process_handler(&metadata).await + } _ => default(&name, &metadata), } } @@ -30,3 +37,28 @@ fn default(name: &String, metadata: &serde_json::Value) -> anyhow::Result<()> { Ok(()) } + +async fn decision_process_handler(metadata: &serde_json::Value) -> anyhow::Result<()> { + tracing::trace!( + "handle_job fell into decision process case: (metadata={:?})", + metadata + ); + + let metadata: DecisionProcessActionMetadata = serde_json::from_value(metadata.clone())?; + + match metadata.status { + Merge => { + let gh_client = GithubClient::new_with_default_token(Client::new().clone()); + + let request = gh_client.get(&metadata.get_issue_url); + + match gh_client.json::(request).await { + Ok(issue) => issue.merge(&gh_client).await?, + Err(e) => log::error!("Failed to get issue {}, error: {}", metadata.get_issue_url, e), + } + } + _ => {} + } + + Ok(()) +} From 026e92229ed38e477d74d2a3686358d8620add28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Montes=20de=20Oca?= Date: Thu, 8 Dec 2022 13:29:19 -0300 Subject: [PATCH 08/17] Add initial parsing for merge and hold commands --- parser/src/command/decision.rs | 104 ++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 9 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index fe237a2a..5857a500 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -8,6 +8,8 @@ //! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. //! ``` +use std::fmt; + use crate::error::Error; use crate::token::{Token, Tokenizer}; use postgres_types::{FromSql, ToSql}; @@ -23,20 +25,49 @@ pub struct DecisionCommand { impl DecisionCommand { pub fn parse<'a>(input: &mut Tokenizer<'a>) -> Result, Error<'a>> { - if let Some(Token::Word("merge")) = input.peek_token()? { - Ok(Some(Self { - resolution: Resolution::Merge, - reversibility: Reversibility::Reversible, - })) - } else { - Ok(None) + let mut toks = input.clone(); + + match toks.peek_token()? { + Some(Token::Word("merge")) => { + command_or_error(input, &mut toks, Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + }) + } + Some(Token::Word("hold")) => { + command_or_error(input, &mut toks, Self { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible, + }) + } + _ => Ok(None), } } } -#[derive(Debug, Eq, PartialEq)] +fn command_or_error<'a>(input: &mut Tokenizer<'a>, toks: &mut Tokenizer<'a>, command: DecisionCommand) -> Result, Error<'a>> { + toks.next_token()?; + if let Some(Token::Dot) | Some(Token::EndOfLine) = toks.peek_token()? { + *input = toks.clone(); + Ok(Some(command)) + } else { + Err(toks.error(ParseError::ExpectedEnd)) + } +} + +#[derive(PartialEq, Eq, Debug)] pub enum ParseError { - InvalidFirstCommand, + ExpectedEnd, +} + +impl std::error::Error for ParseError {} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + ParseError::ExpectedEnd => write!(f, "expected end of command"), + } + } } #[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] @@ -56,3 +87,58 @@ pub enum Resolution { #[postgres(name = "hold")] Hold, } +#[cfg(test)] +mod tests { + use super::*; + + fn parse<'a>(input: &'a str) -> Result, Error<'a>> { + let mut toks = Tokenizer::new(input); + Ok(DecisionCommand::parse(&mut toks)?) + } + + #[test] + fn test_correct_merge() { + assert_eq!( + parse("merge"), + Ok(Some(DecisionCommand { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible + })), + ); + } + + #[test] + fn test_correct_merge_final_dot() { + assert_eq!( + parse("merge."), + Ok(Some(DecisionCommand { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible + })), + ); + } + + #[test] + fn test_correct_hold() { + assert_eq!( + parse("hold"), + Ok(Some(DecisionCommand { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible + })), + ); + } + + #[test] + fn test_expected_end() { + use std::error::Error; + assert_eq!( + parse("hold my beer") + .unwrap_err() + .source() + .unwrap() + .downcast_ref(), + Some(&ParseError::ExpectedEnd), + ); + } +} From 73511bc2660d824f68a657390288dc9f4e6e5600 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juli=C3=A1n=20Montes=20de=20Oca?= Date: Tue, 3 Jan 2023 16:34:40 -0300 Subject: [PATCH 09/17] Decision state comment builder * add comment builder for the decision state * improve error handling * add factory for user status and add tests * cleanup unnecessary code --- Cargo.lock | 21 ++++ Cargo.toml | 1 + parser/src/command/decision.rs | 10 ++ src/db/issue_decision_state.rs | 7 +- src/handlers/decision.rs | 191 +++++++++++++++++++++++++++++++-- 5 files changed, 216 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 12bae674..f34ce977 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -471,6 +471,26 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b5320ae4c3782150d900b79807611a59a99fc9a1d61d686faafc24b93fc8d7ca" +[[package]] +name = "factori" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ff6b50917609e530c145de1c6aa8df9c38c40562375e8aa5eeaaf6c737a0b31" +dependencies = [ + "factori-impl", +] + +[[package]] +name = "factori-impl" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d6344ded92b0a4a1d90a816632f7ff2a12e01401d325d6295810dacca1dbdd6" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "fake-simd" version = "0.1.2" @@ -2117,6 +2137,7 @@ dependencies = [ "cron", "cynic", "dotenv", + "factori", "futures", "github-graphql", "glob", diff --git a/Cargo.toml b/Cargo.toml index 2caa6fe8..51be74a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,6 +45,7 @@ ignore = "0.4.18" postgres-types = { version = "0.2.4", features = ["derive"] } cron = { version = "0.12.0" } pretty_assertions = "1.2" +factori = "1.1.0" [dependencies.serde] version = "1" diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 5857a500..7291940a 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -87,6 +87,16 @@ pub enum Resolution { #[postgres(name = "hold")] Hold, } + +impl fmt::Display for Resolution { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Resolution::Merge => write!(f, "merge"), + Resolution::Hold => write!(f, "hold"), + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/db/issue_decision_state.rs b/src/db/issue_decision_state.rs index 5091880b..af673c3d 100644 --- a/src/db/issue_decision_state.rs +++ b/src/db/issue_decision_state.rs @@ -14,7 +14,7 @@ pub struct IssueDecisionState { pub initiator: String, pub start_date: DateTime, pub end_date: DateTime, - pub current: BTreeMap, + pub current: BTreeMap>, pub history: BTreeMap>, pub reversibility: Reversibility, pub resolution: Resolution, @@ -34,7 +34,7 @@ pub async fn insert_issue_decision_state( initiator: &String, start_date: &DateTime, end_date: &DateTime, - current: &BTreeMap, + current: &BTreeMap>, history: &BTreeMap>, reversibility: &Reversibility, resolution: &Resolution, @@ -97,7 +97,8 @@ fn deserialize_issue_decision_state(row: &tokio_postgres::row::Row) -> Result = row.try_get(2)?; let end_date: DateTime = row.try_get(3)?; - let current: BTreeMap = serde_json::from_value(row.try_get(4).unwrap())?; + let current: BTreeMap> = + serde_json::from_value(row.try_get(4).unwrap())?; let history: BTreeMap> = serde_json::from_value(row.try_get(5).unwrap())?; let reversibility: Reversibility = row.try_get(6)?; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 6cf6c9e5..34143fea 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,8 +1,10 @@ use crate::db::jobs::*; +use crate::github; use crate::{ config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, interactions::ErrorComment, }; +use anyhow::bail; use anyhow::Context as Ctx; use chrono::{DateTime, Duration, Utc}; use parser::command::decision::Resolution::{Hold, Merge}; @@ -93,16 +95,26 @@ pub(super) async fn handle_command( let end_date: DateTime = start_date.checked_add_signed(Duration::days(10)).unwrap(); - let mut current: BTreeMap = BTreeMap::new(); + //TODO: change this to be configurable in toml / ask user to provide the team name + // it should match the same team that we check for above when determining if the user is a member + let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); + + let mut current: BTreeMap> = BTreeMap::new(); + + for member in team.members { + current.insert(member.name, None); + } + current.insert( - "mcass19".to_string(), - UserStatus { + user.login.clone(), + Some(UserStatus { comment_id: "comment_id".to_string(), text: "something".to_string(), reversibility: Reversibility::Reversible, resolution: Merge, - }, + }), ); + let history: BTreeMap> = BTreeMap::new(); insert_issue_decision_state( @@ -120,7 +132,11 @@ pub(super) async fn handle_command( let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { message: "some message".to_string(), - get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + get_issue_url: format!( + "{}/issues/{}", + issue.repository().url(), + issue.number + ), status: Merge, }) .unwrap(); @@ -133,12 +149,7 @@ pub(super) async fn handle_command( ) .await?; - // let team = github::get_team(&ctx.github, &"T-lang"); // change this to be configurable in toml? - - let comment = format!( - "Wow, it looks like you want to merge this, {}.\n| Team member | State |\n|-------------|-------|\n| julmontesdeoca | merge |\n| mcass19 | |", - user.login - ); + let comment = build_status_comment(&history, ¤t)?; issue .post_comment(&ctx.github, &comment) @@ -152,6 +163,164 @@ pub(super) async fn handle_command( } } +fn build_status_comment( + history: &BTreeMap>, + current: &BTreeMap>, +) -> anyhow::Result { + let mut comment = "| Team member | State |\n|-------------|-------|".to_owned(); + for (user, statuses) in history { + let mut user_statuses = format!("\n| {} |", user); + + // previous stasuses + for status in statuses { + let status_item = format!(" ~~{}~~ ", status.resolution); + user_statuses.push_str(&status_item); + } + + // current status + let user_resolution = match current.get(user) { + Some(current_status) => { + if let Some(status) = current_status { + format!("**{}**", status.resolution) + } else { + "".to_string() + } + } + None => bail!("user {} not present in current statuses list", user), + }; + + let status_item = format!(" {} |", user_resolution); + user_statuses.push_str(&status_item); + + comment.push_str(&user_statuses); + } + + Ok(comment) +} + +#[cfg(test)] +mod tests { + use super::*; + use factori::{create, factori}; + + factori!(UserStatus, { + default { + comment_id = "the-comment-id".to_string(), + text = "this is my argument for making this decision".to_string(), + reversibility = Reversibility::Reversible, + resolution = Resolution::Merge + } + + mixin hold { + resolution = Resolution::Hold + } + }); + + #[test] + fn test_successfuly_build_comment() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + history.insert("Niklaus".to_string(), user_1_statuses); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + history.insert("Barbara".to_string(), user_2_statuses); + + current_statuses.insert("Barbara".to_string(), Some(create!(UserStatus))); + + let build_result = build_status_comment(&history, ¤t_statuses) + .expect("it shouldn't fail building the message"); + let expected_comment = "| Team member | State |\n\ + |-------------|-------|\n\ + | Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ + | Niklaus | ~~merge~~ ~~hold~~ **merge** |" + .to_string(); + + assert_eq!(build_result, expected_comment); + } + + #[test] + fn test_successfuly_build_comment_user_no_votes() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + history.insert("Niklaus".to_string(), user_1_statuses); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + history.insert("Barbara".to_string(), user_2_statuses); + + current_statuses.insert("Barbara".to_string(), Some(create!(UserStatus))); + + // user 3 + history.insert("Tom".to_string(), Vec::new()); + + current_statuses.insert("Tom".to_string(), None); + + let build_result = build_status_comment(&history, ¤t_statuses) + .expect("it shouldn't fail building the message"); + let expected_comment = "| Team member | State |\n\ + |-------------|-------|\n\ + | Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ + | Niklaus | ~~merge~~ ~~hold~~ **merge** |\n\ + | Tom | |" + .to_string(); + + assert_eq!(build_result, expected_comment); + } + + #[test] + fn test_build_comment_inconsistent_users() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + history.insert("Niklaus".to_string(), user_1_statuses); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + history.insert("Barbara".to_string(), user_2_statuses); + + current_statuses.insert("Martin".to_string(), Some(create!(UserStatus))); + + let build_result = build_status_comment(&history, ¤t_statuses); + assert_eq!( + format!("{}", build_result.unwrap_err()), + "user Barbara not present in current statuses list" + ); + } +} + // #[cfg(test)] // mod tests { // use chrono::{Duration, Utc}; From fe2064f6832ad54ed643832bead18faa9a9924fa Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Thu, 5 Jan 2023 11:31:22 -0300 Subject: [PATCH 10/17] Support hold as first command and post comment when no first vote --- parser/src/command/decision.rs | 28 +-- src/github.rs | 4 +- src/handlers/decision.rs | 310 +++++++-------------------------- src/handlers/jobs.rs | 26 +-- 4 files changed, 99 insertions(+), 269 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 7291940a..25eb3684 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -28,24 +28,32 @@ impl DecisionCommand { let mut toks = input.clone(); match toks.peek_token()? { - Some(Token::Word("merge")) => { - command_or_error(input, &mut toks, Self { + Some(Token::Word("merge")) => command_or_error( + input, + &mut toks, + Self { resolution: Resolution::Merge, reversibility: Reversibility::Reversible, - }) - } - Some(Token::Word("hold")) => { - command_or_error(input, &mut toks, Self { + }, + ), + Some(Token::Word("hold")) => command_or_error( + input, + &mut toks, + Self { resolution: Resolution::Hold, reversibility: Reversibility::Reversible, - }) - } + }, + ), _ => Ok(None), } } } -fn command_or_error<'a>(input: &mut Tokenizer<'a>, toks: &mut Tokenizer<'a>, command: DecisionCommand) -> Result, Error<'a>> { +fn command_or_error<'a>( + input: &mut Tokenizer<'a>, + toks: &mut Tokenizer<'a>, + command: DecisionCommand, +) -> Result, Error<'a>> { toks.next_token()?; if let Some(Token::Dot) | Some(Token::EndOfLine) = toks.peek_token()? { *input = toks.clone(); @@ -70,7 +78,7 @@ impl fmt::Display for ParseError { } } -#[derive(Serialize, Deserialize, Debug, Clone, ToSql, FromSql, Eq, PartialEq)] +#[derive(Serialize, Deserialize, Debug, Clone, Copy, ToSql, FromSql, Eq, PartialEq)] #[postgres(name = "reversibility")] pub enum Reversibility { #[postgres(name = "reversible")] diff --git a/src/github.rs b/src/github.rs index 44cc97f3..166ff1ae 100644 --- a/src/github.rs +++ b/src/github.rs @@ -786,13 +786,13 @@ impl Issue { #[derive(serde::Serialize)] struct MergeIssue<'a> { commit_title: &'a str, - merge_method: &'a str + merge_method: &'a str, } client ._send_req(client.put(&merge_url).json(&MergeIssue { commit_title: "Merged by the bot!", - merge_method: "merge" + merge_method: "merge", })) .await .context("failed to merge issue")?; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 34143fea..0f83af04 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -1,4 +1,3 @@ -use crate::db::jobs::*; use crate::github; use crate::{ config::DecisionConfig, db::issue_decision_state::*, github::Event, handlers::Context, @@ -7,23 +6,11 @@ use crate::{ use anyhow::bail; use anyhow::Context as Ctx; use chrono::{DateTime, Duration, Utc}; -use parser::command::decision::Resolution::{Hold, Merge}; +use parser::command::decision::Resolution; use parser::command::decision::*; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; -// get state for issue_id from db - // if no state (first call) - // initialize state - // schedule job if necessary - // send comment to github - // save state - // if state - // apply logic to decide what to do - // schedule job if necessary - // send comment to github - // save state - pub const DECISION_PROCESS_JOB_NAME: &str = "decision_process_action"; #[derive(Serialize, Deserialize)] @@ -62,103 +49,76 @@ pub(super) async fn handle_command( match get_issue_decision_state(&db, &issue.number).await { Ok(_state) => { - // let name = match disposition { - // Hold => "hold".into(), - // Custom(name) => name, - // }; - - // let mut current_statuses = state.current_statuses; - // let mut status_history = state.status_history; - - // if let Some(entry) = current_statuses.get_mut(&user) { - // let past = status_history.entry(user).or_insert(Vec::new()); + // TO DO + let cmnt = ErrorComment::new( + &issue, + "We don't support having more than one vote yet. Coming soon :)", + ); + cmnt.post(&ctx.github).await?; - // past.push(entry.clone()); - - // *entry = UserStatus::new(name, issue_id, comment_id); - // } else { - // current_statuses.insert(user, UserStatus::new("hold".into(), issue_id, comment_id)); - // } - - // Ok(State { - // current_statuses, - // status_history, - // ..state - // }) Ok(()) } _ => { - match resolution { - Hold => Ok(()), // change me! - Merge => { - let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = - start_date.checked_add_signed(Duration::days(10)).unwrap(); - - //TODO: change this to be configurable in toml / ask user to provide the team name - // it should match the same team that we check for above when determining if the user is a member - let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); - - let mut current: BTreeMap> = BTreeMap::new(); - - for member in team.members { - current.insert(member.name, None); - } - - current.insert( - user.login.clone(), - Some(UserStatus { - comment_id: "comment_id".to_string(), - text: "something".to_string(), - reversibility: Reversibility::Reversible, - resolution: Merge, - }), - ); - - let history: BTreeMap> = BTreeMap::new(); - - insert_issue_decision_state( - &db, - &issue.number, - &user.login, - &start_date, - &end_date, - ¤t, - &history, - &reversibility, - &Merge, - ) - .await?; - - let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { - message: "some message".to_string(), - get_issue_url: format!( - "{}/issues/{}", - issue.repository().url(), - issue.number - ), - status: Merge, - }) - .unwrap(); - - insert_job( - &db, - &DECISION_PROCESS_JOB_NAME.to_string(), - &end_date, - &metadata, - ) - .await?; - - let comment = build_status_comment(&history, ¤t)?; - - issue - .post_comment(&ctx.github, &comment) - .await - .context("merge vote comment")?; - - Ok(()) - } + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = + start_date.checked_add_signed(Duration::days(10)).unwrap(); + + let mut current: BTreeMap> = BTreeMap::new(); + let history: BTreeMap> = BTreeMap::new(); + + // TODO + // change this to be entered by the user as part of the command + // it should match the same team that we check for above when determining if the user is a member + let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); + for member in team.members { + current.insert(member.name, None); } + + current.insert( + user.login.clone(), + Some(UserStatus { + comment_id: "comment_id".to_string(), + text: "something".to_string(), + reversibility: reversibility, + resolution: resolution, + }), + ); + + insert_issue_decision_state( + &db, + &issue.number, + &user.login, + &start_date, + &end_date, + ¤t, + &history, + &reversibility, + &resolution, + ) + .await?; + + // TO DO -- Do not insert this job until we support more votes + // let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { + // message: "some message".to_string(), + // get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + // status: resolution, + // }) + // .unwrap(); + // insert_job( + // &db, + // &DECISION_PROCESS_JOB_NAME.to_string(), + // &end_date, + // &metadata, + // ) + // .await?; + + let comment = build_status_comment(&history, ¤t)?; + issue + .post_comment(&ctx.github, &comment) + .await + .context("merge vote comment")?; + + Ok(()) } } } @@ -320,141 +280,3 @@ mod tests { ); } } - -// #[cfg(test)] -// mod tests { -// use chrono::{Duration, Utc}; -// use pretty_assertions::assert_eq; - -// use super::*; - -// struct TestRenderer {} - -// impl LinkRenderer for TestRenderer { -// fn render_link(&self, data: &UserStatus) -> String { -// let issue_id = &data.issue_id; -// let comment_id = &data.comment_id; - -// format!("http://example.com/issue/{issue_id}#comment={comment_id}") -// } -// } - -// /// Example 1 -// /// -// /// https://lang-team.rust-lang.org/decision_process/examples.html#reversible-decision-merging-a-proposal -// /// -// /// * From the starting point of there not being any state, someone proposes -// /// to merge a proposal -// /// * then barbara holds -// /// * 11 days pass -// /// * barbara says merge, it immediatly merges -// #[test] -// fn example_merging_proposal() { -// let team_members = vec![ -// "@Alan".to_owned(), -// "@Barbara".to_owned(), -// "@Grace".to_owned(), -// "@Niklaus".to_owned(), -// ]; -// let r = TestRenderer {}; - -// // alan proposes to merge -// let time1 = Utc::now(); -// let command = DecisionCommand::merge("@Alan".into(), "1".into(), "1".into()); -// let state = handle_command(None, command, Context::new(team_members.clone(), time1)).unwrap(); - -// assert_eq!(state.period_start, time1); -// assert_eq!(state.original_period_start, time1); -// assert_eq!( -// state.current_statuses, -// vec![( -// "@Alan".into(), -// UserStatus::new("merge".into(), "1".into(), "1".into()) -// ),] -// .into_iter() -// .collect() -// ); -// assert_eq!(state.status_history, HashMap::new()); -// assert_eq!(state.reversibility, Reversibility::Reversible); -// assert_eq!(state.resolution, Custom("merge".into())); -// assert_eq!( -// state.render(&r), -// include_str!("../../test/decision/res/01_merging_proposal__1.md") -// ); - -// // barbara holds -// let time2 = Utc::now(); -// let command = DecisionCommand::hold("@Barbara".into(), "1".into(), "2".into()); -// let state = handle_command( -// Some(state), -// command, -// Context::new(team_members.clone(), time2), -// ) -// .unwrap(); - -// assert_eq!(state.period_start, time1); -// assert_eq!(state.original_period_start, time1); -// assert_eq!( -// state.current_statuses, -// vec![ -// ( -// "@Alan".into(), -// UserStatus::new("merge".into(), "1".into(), "1".into()) -// ), -// ( -// "@Barbara".into(), -// UserStatus::new("hold".into(), "1".into(), "2".into()) -// ), -// ] -// .into_iter() -// .collect() -// ); -// assert_eq!(state.status_history, HashMap::new()); -// assert_eq!(state.reversibility, Reversibility::Reversible); -// assert_eq!(state.resolution, Custom("merge".into())); -// assert_eq!( -// state.render(&r), -// include_str!("../../test/decision/res/01_merging_proposal__2.md") -// ); - -// // 11 days pass -// let time3 = time2 + Duration::days(11); - -// // Barbara says merge, it immediatly merges -// let command = DecisionCommand::merge("@Barbara".into(), "1".into(), "3".into()); -// let state = handle_command(Some(state), command, Context::new(team_members, time3)).unwrap(); - -// assert_eq!(state.period_start, time1); -// assert_eq!(state.original_period_start, time1); -// assert_eq!( -// state.current_statuses, -// vec![ -// ( -// "@Alan".into(), -// UserStatus::new("merge".into(), "1".into(), "1".into()) -// ), -// ( -// "@Barbara".into(), -// UserStatus::new("merge".into(), "1".into(), "3".into()) -// ), -// ] -// .into_iter() -// .collect() -// ); -// assert_eq!( -// state.status_history, -// vec![( -// "@Barbara".into(), -// vec![UserStatus::new("hold".into(), "1".into(), "2".into())] -// ),] -// .into_iter() -// .collect() -// ); -// assert_eq!(state.reversibility, Reversibility::Reversible); -// assert_eq!(state.resolution, Custom("merge".into())); -// assert_eq!( -// state.render(&r), -// include_str!("../../test/decision/01_merging_proposal__3.md") -// ); -// } -// } diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index 2950a9a5..480b1c29 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -6,7 +6,7 @@ use super::Context; use crate::github::*; use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_JOB_NAME}; -use parser::command::decision::Resolution::Merge; +use parser::command::decision::Resolution::{Hold, Merge}; use reqwest::Client; use tracing as log; @@ -45,19 +45,19 @@ async fn decision_process_handler(metadata: &serde_json::Value) -> anyhow::Resul ); let metadata: DecisionProcessActionMetadata = serde_json::from_value(metadata.clone())?; + let gh_client = GithubClient::new_with_default_token(Client::new().clone()); + let request = gh_client.get(&metadata.get_issue_url); - match metadata.status { - Merge => { - let gh_client = GithubClient::new_with_default_token(Client::new().clone()); - - let request = gh_client.get(&metadata.get_issue_url); - - match gh_client.json::(request).await { - Ok(issue) => issue.merge(&gh_client).await?, - Err(e) => log::error!("Failed to get issue {}, error: {}", metadata.get_issue_url, e), - } - } - _ => {} + match gh_client.json::(request).await { + Ok(issue) => match metadata.status { + Merge => issue.merge(&gh_client).await?, + Hold => issue.close(&gh_client).await?, + }, + Err(e) => log::error!( + "Failed to get issue {}, error: {}", + metadata.get_issue_url, + e + ), } Ok(()) From 130c25acae94f6daca6f35b02448bf6b0232a2fc Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Wed, 11 Jan 2023 10:59:40 -0300 Subject: [PATCH 11/17] Fix status comment builder --- src/handlers/decision.rs | 65 ++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 0f83af04..b22bb21a 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -64,14 +64,15 @@ pub(super) async fn handle_command( start_date.checked_add_signed(Duration::days(10)).unwrap(); let mut current: BTreeMap> = BTreeMap::new(); - let history: BTreeMap> = BTreeMap::new(); + let mut history: BTreeMap> = BTreeMap::new(); // TODO // change this to be entered by the user as part of the command // it should match the same team that we check for above when determining if the user is a member let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); for member in team.members { - current.insert(member.name, None); + current.insert(member.name.clone(), None); + history.insert(member.name.clone(), Vec::new()); } current.insert( @@ -128,25 +129,24 @@ fn build_status_comment( current: &BTreeMap>, ) -> anyhow::Result { let mut comment = "| Team member | State |\n|-------------|-------|".to_owned(); - for (user, statuses) in history { + for (user, status) in current { let mut user_statuses = format!("\n| {} |", user); // previous stasuses - for status in statuses { - let status_item = format!(" ~~{}~~ ", status.resolution); - user_statuses.push_str(&status_item); + match history.get(user) { + Some(statuses) => { + for status in statuses { + let status_item = format!(" ~~{}~~ ", status.resolution); + user_statuses.push_str(&status_item); + } + } + None => bail!("user {} not present in history statuses list", user), } // current status - let user_resolution = match current.get(user) { - Some(current_status) => { - if let Some(status) = current_status { - format!("**{}**", status.resolution) - } else { - "".to_string() - } - } - None => bail!("user {} not present in current statuses list", user), + let user_resolution = match status { + Some(status) => format!("**{}**", status.resolution), + _ => "".to_string(), }; let status_item = format!(" {} |", user_resolution); @@ -276,7 +276,40 @@ mod tests { let build_result = build_status_comment(&history, ¤t_statuses); assert_eq!( format!("{}", build_result.unwrap_err()), - "user Barbara not present in current statuses list" + "user Martin not present in history statuses list" ); } + + #[test] + fn test_successfuly_build_comment_no_history() { + let mut history: BTreeMap> = BTreeMap::new(); + let mut current_statuses: BTreeMap> = BTreeMap::new(); + + // user 1 + let mut user_1_statuses: Vec = Vec::new(); + user_1_statuses.push(create!(UserStatus)); + user_1_statuses.push(create!(UserStatus, :hold)); + + current_statuses.insert("Niklaus".to_string(), Some(create!(UserStatus))); + history.insert("Niklaus".to_string(), Vec::new()); + + // user 2 + let mut user_2_statuses: Vec = Vec::new(); + user_2_statuses.push(create!(UserStatus, :hold)); + user_2_statuses.push(create!(UserStatus)); + + current_statuses.insert("Barbara".to_string(), Some(create!(UserStatus))); + history.insert("Barbara".to_string(), Vec::new()); + + let build_result = build_status_comment(&history, ¤t_statuses) + .expect("it shouldn't fail building the message"); + let expected_comment = "| Team member | State |\n\ + |-------------|-------|\n\ + | Barbara | **merge** |\n\ + | Niklaus | **merge** |\ + " + .to_string(); + + assert_eq!(build_result, expected_comment); + } } From 92f67b0257b2178f41c8ef02a55878dc6ee36f04 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Thu, 19 Jan 2023 15:35:53 -0300 Subject: [PATCH 12/17] Parse and handle team in command * Parse team in command * Use parsed team in handler --- parser/src/command/decision.rs | 94 ++++++++++++++++----- src/handlers/decision.rs | 145 +++++++++++++++++++-------------- 2 files changed, 158 insertions(+), 81 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 25eb3684..289c78e4 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -21,6 +21,7 @@ use serde::{Deserialize, Serialize}; pub struct DecisionCommand { pub resolution: Resolution, pub reversibility: Reversibility, + pub team: Option, } impl DecisionCommand { @@ -28,33 +29,57 @@ impl DecisionCommand { let mut toks = input.clone(); match toks.peek_token()? { - Some(Token::Word("merge")) => command_or_error( - input, - &mut toks, - Self { - resolution: Resolution::Merge, - reversibility: Reversibility::Reversible, - }, - ), - Some(Token::Word("hold")) => command_or_error( - input, - &mut toks, - Self { - resolution: Resolution::Hold, - reversibility: Reversibility::Reversible, - }, - ), + Some(Token::Word("merge")) => { + toks.next_token()?; + + let team: Option = get_team(&mut toks)?; + + command_or_error( + input, + &mut toks, + Self { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + team, + }, + ) + } + Some(Token::Word("hold")) => { + toks.next_token()?; + + let team: Option = get_team(&mut toks)?; + + command_or_error( + input, + &mut toks, + Self { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible, + team, + }, + ) + } _ => Ok(None), } } } +fn get_team<'a>(toks: &mut Tokenizer<'a>) -> Result, Error<'a>> { + match toks.peek_token()? { + Some(Token::Word(team)) => { + toks.next_token()?; + + Ok(Some(team.to_string())) + } + _ => Ok(None), + } +} + fn command_or_error<'a>( input: &mut Tokenizer<'a>, toks: &mut Tokenizer<'a>, command: DecisionCommand, ) -> Result, Error<'a>> { - toks.next_token()?; if let Some(Token::Dot) | Some(Token::EndOfLine) = toks.peek_token()? { *input = toks.clone(); Ok(Some(command)) @@ -120,7 +145,8 @@ mod tests { parse("merge"), Ok(Some(DecisionCommand { resolution: Resolution::Merge, - reversibility: Reversibility::Reversible + reversibility: Reversibility::Reversible, + team: None })), ); } @@ -131,7 +157,8 @@ mod tests { parse("merge."), Ok(Some(DecisionCommand { resolution: Resolution::Merge, - reversibility: Reversibility::Reversible + reversibility: Reversibility::Reversible, + team: None })), ); } @@ -142,7 +169,8 @@ mod tests { parse("hold"), Ok(Some(DecisionCommand { resolution: Resolution::Hold, - reversibility: Reversibility::Reversible + reversibility: Reversibility::Reversible, + team: None })), ); } @@ -151,7 +179,7 @@ mod tests { fn test_expected_end() { use std::error::Error; assert_eq!( - parse("hold my beer") + parse("hold lang beer") .unwrap_err() .source() .unwrap() @@ -159,4 +187,28 @@ mod tests { Some(&ParseError::ExpectedEnd), ); } + + #[test] + fn test_correct_merge_with_team() { + assert_eq!( + parse("merge lang"), + Ok(Some(DecisionCommand { + resolution: Resolution::Merge, + reversibility: Reversibility::Reversible, + team: Some("lang".to_string()) + })), + ); + } + + #[test] + fn test_correct_hold_with_team() { + assert_eq!( + parse("hold lang"), + Ok(Some(DecisionCommand { + resolution: Resolution::Hold, + reversibility: Reversibility::Reversible, + team: Some("lang".to_string()) + })), + ); + } } diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index b22bb21a..8818cdc2 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -31,6 +31,7 @@ pub(super) async fn handle_command( let DecisionCommand { resolution, reversibility, + team: team_name, } = cmd; let issue = event.issue().unwrap(); @@ -59,67 +60,91 @@ pub(super) async fn handle_command( Ok(()) } _ => { - let start_date: DateTime = chrono::Utc::now().into(); - let end_date: DateTime = - start_date.checked_add_signed(Duration::days(10)).unwrap(); - - let mut current: BTreeMap> = BTreeMap::new(); - let mut history: BTreeMap> = BTreeMap::new(); - - // TODO - // change this to be entered by the user as part of the command - // it should match the same team that we check for above when determining if the user is a member - let team = github::get_team(&ctx.github, &"T-lang").await?.unwrap(); - for member in team.members { - current.insert(member.name.clone(), None); - history.insert(member.name.clone(), Vec::new()); + match team_name { + None => { + let cmnt = ErrorComment::new( + &issue, + "In the first vote, is necessary to specify the team name that will be involved in the decision process.", + ); + cmnt.post(&ctx.github).await?; + + Ok(()) + } + Some(team_name) => { + match github::get_team(&ctx.github, &team_name).await { + Ok(Some(team)) => { + let start_date: DateTime = chrono::Utc::now().into(); + let end_date: DateTime = + start_date.checked_add_signed(Duration::days(10)).unwrap(); + + let mut current: BTreeMap> = BTreeMap::new(); + let mut history: BTreeMap> = BTreeMap::new(); + + // Add team members to current and history + for member in team.members { + current.insert(member.github.clone(), None); + history.insert(member.github.clone(), Vec::new()); + } + + // Add issue user to current and history + current.insert( + user.login.clone(), + Some(UserStatus { + comment_id: event.html_url().unwrap().to_string(), + text: event.comment_body().unwrap().to_string(), + reversibility: reversibility, + resolution: resolution, + }), + ); + history.insert(user.login.clone(), Vec::new()); + + // Initialize issue decision state + insert_issue_decision_state( + &db, + &issue.number, + &user.login, + &start_date, + &end_date, + ¤t, + &history, + &reversibility, + &resolution, + ) + .await?; + + // TO DO -- Do not insert this job until we support more votes + // let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { + // message: "some message".to_string(), + // get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), + // status: resolution, + // }) + // .unwrap(); + // insert_job( + // &db, + // &DECISION_PROCESS_JOB_NAME.to_string(), + // &end_date, + // &metadata, + // ) + // .await?; + + let comment = build_status_comment(&history, ¤t)?; + issue + .post_comment(&ctx.github, &comment) + .await + .context("merge vote comment")?; + + Ok(()) + } + _ => { + let cmnt = + ErrorComment::new(&issue, "Failed to resolve to a known team."); + cmnt.post(&ctx.github).await?; + + Ok(()) + } + } + } } - - current.insert( - user.login.clone(), - Some(UserStatus { - comment_id: "comment_id".to_string(), - text: "something".to_string(), - reversibility: reversibility, - resolution: resolution, - }), - ); - - insert_issue_decision_state( - &db, - &issue.number, - &user.login, - &start_date, - &end_date, - ¤t, - &history, - &reversibility, - &resolution, - ) - .await?; - - // TO DO -- Do not insert this job until we support more votes - // let metadata = serde_json::value::to_value(DecisionProcessActionMetadata { - // message: "some message".to_string(), - // get_issue_url: format!("{}/issues/{}", issue.repository().url(), issue.number), - // status: resolution, - // }) - // .unwrap(); - // insert_job( - // &db, - // &DECISION_PROCESS_JOB_NAME.to_string(), - // &end_date, - // &metadata, - // ) - // .await?; - - let comment = build_status_comment(&history, ¤t)?; - issue - .post_comment(&ctx.github, &comment) - .await - .context("merge vote comment")?; - - Ok(()) } } } From 85725c6565bc0cf7b45f6c5ba7a8a039a2a64482 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Thu, 23 Feb 2023 15:04:26 -0300 Subject: [PATCH 13/17] Minor improvements * Move dependencies only to dev * Limit to team member when initializing the process * Display @ on user names in table --- Cargo.toml | 6 ++++-- src/handlers/decision.rs | 38 +++++++++++++++++++------------------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 51be74a7..9167e56d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,8 +44,6 @@ rand = "0.8.5" ignore = "0.4.18" postgres-types = { version = "0.2.4", features = ["derive"] } cron = { version = "0.12.0" } -pretty_assertions = "1.2" -factori = "1.1.0" [dependencies.serde] version = "1" @@ -55,5 +53,9 @@ features = ["derive"] version = "1.3.1" default-features = false +[dev-dependencies] +pretty_assertions = "1.2" +factori = "1.1.0" + [profile.release] debug = 2 diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index 8818cdc2..a0339ac3 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -37,17 +37,6 @@ pub(super) async fn handle_command( let issue = event.issue().unwrap(); let user = event.user(); - let is_team_member = user.is_team_member(&ctx.github).await.unwrap_or(false); - - if !is_team_member { - let cmnt = ErrorComment::new( - &issue, - "Only team members can be part of the decision process.", - ); - cmnt.post(&ctx.github).await?; - return Ok(()); - } - match get_issue_decision_state(&db, &issue.number).await { Ok(_state) => { // TO DO @@ -60,6 +49,17 @@ pub(super) async fn handle_command( Ok(()) } _ => { + let is_team_member = user.is_team_member(&ctx.github).await.unwrap_or(false); + if !is_team_member { + let cmnt = ErrorComment::new( + &issue, + "Only team members can be part of the decision process.", + ); + cmnt.post(&ctx.github).await?; + + return Ok(()); + } + match team_name { None => { let cmnt = ErrorComment::new( @@ -155,7 +155,7 @@ fn build_status_comment( ) -> anyhow::Result { let mut comment = "| Team member | State |\n|-------------|-------|".to_owned(); for (user, status) in current { - let mut user_statuses = format!("\n| {} |", user); + let mut user_statuses = format!("\n| @{} |", user); // previous stasuses match history.get(user) { @@ -228,8 +228,8 @@ mod tests { .expect("it shouldn't fail building the message"); let expected_comment = "| Team member | State |\n\ |-------------|-------|\n\ - | Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ - | Niklaus | ~~merge~~ ~~hold~~ **merge** |" + | @Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ + | @Niklaus | ~~merge~~ ~~hold~~ **merge** |" .to_string(); assert_eq!(build_result, expected_comment); @@ -267,9 +267,9 @@ mod tests { .expect("it shouldn't fail building the message"); let expected_comment = "| Team member | State |\n\ |-------------|-------|\n\ - | Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ - | Niklaus | ~~merge~~ ~~hold~~ **merge** |\n\ - | Tom | |" + | @Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ + | @Niklaus | ~~merge~~ ~~hold~~ **merge** |\n\ + | @Tom | |" .to_string(); assert_eq!(build_result, expected_comment); @@ -330,8 +330,8 @@ mod tests { .expect("it shouldn't fail building the message"); let expected_comment = "| Team member | State |\n\ |-------------|-------|\n\ - | Barbara | **merge** |\n\ - | Niklaus | **merge** |\ + | @Barbara | **merge** |\n\ + | @Niklaus | **merge** |\ " .to_string(); From 050821a81b0a7bf0648d529a3d227fdcac6629cc Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Fri, 2 Jun 2023 15:30:50 +0200 Subject: [PATCH 14/17] Show the user event link in the table that triggered the vote (#11) * Fix top comment in decision parser * Display vote with link to the event that caused it --- parser/src/command/decision.rs | 10 ++++++++-- src/handlers/decision.rs | 20 +++++++++++--------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/parser/src/command/decision.rs b/parser/src/command/decision.rs index 289c78e4..2002c928 100644 --- a/parser/src/command/decision.rs +++ b/parser/src/command/decision.rs @@ -1,11 +1,17 @@ //! The decision process command parser. //! -//! This can parse arbitrary input, giving the user to be assigned. +//! This can parse arbitrary input, giving the command with which we would like +//! to vote that will potentially change the issue in its resolution, +//! reversibility and/or more. +//! +//! In the first one, we must also assign a valid team to the issue decision process. //! //! The grammar is as follows: //! //! ```text -//! Command: `@bot merge`, `@bot hold`, `@bot restart`, `@bot dissent`, `@bot stabilize` or `@bot close`. +//! Command: `@bot merge`, `@bot hold`, `@bot close` +//! +//! First comment: `@bot merge lang`, `@bot hold lang` //! ``` use std::fmt; diff --git a/src/handlers/decision.rs b/src/handlers/decision.rs index a0339ac3..4ba26de3 100644 --- a/src/handlers/decision.rs +++ b/src/handlers/decision.rs @@ -161,7 +161,8 @@ fn build_status_comment( match history.get(user) { Some(statuses) => { for status in statuses { - let status_item = format!(" ~~{}~~ ", status.resolution); + let status_item = + format!(" [~~{}~~]({}) ", status.resolution, status.comment_id); user_statuses.push_str(&status_item); } } @@ -170,7 +171,7 @@ fn build_status_comment( // current status let user_resolution = match status { - Some(status) => format!("**{}**", status.resolution), + Some(status) => format!("[**{}**]({})", status.resolution, status.comment_id), _ => "".to_string(), }; @@ -190,13 +191,14 @@ mod tests { factori!(UserStatus, { default { - comment_id = "the-comment-id".to_string(), + comment_id = "https://some-comment-id-for-merge.com".to_string(), text = "this is my argument for making this decision".to_string(), reversibility = Reversibility::Reversible, resolution = Resolution::Merge } mixin hold { + comment_id = "https://some-comment-id-for-hold.com".to_string(), resolution = Resolution::Hold } }); @@ -228,8 +230,8 @@ mod tests { .expect("it shouldn't fail building the message"); let expected_comment = "| Team member | State |\n\ |-------------|-------|\n\ - | @Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ - | @Niklaus | ~~merge~~ ~~hold~~ **merge** |" + | @Barbara | [~~hold~~](https://some-comment-id-for-hold.com) [~~merge~~](https://some-comment-id-for-merge.com) [**merge**](https://some-comment-id-for-merge.com) |\n\ + | @Niklaus | [~~merge~~](https://some-comment-id-for-merge.com) [~~hold~~](https://some-comment-id-for-hold.com) [**merge**](https://some-comment-id-for-merge.com) |" .to_string(); assert_eq!(build_result, expected_comment); @@ -267,8 +269,8 @@ mod tests { .expect("it shouldn't fail building the message"); let expected_comment = "| Team member | State |\n\ |-------------|-------|\n\ - | @Barbara | ~~hold~~ ~~merge~~ **merge** |\n\ - | @Niklaus | ~~merge~~ ~~hold~~ **merge** |\n\ + | @Barbara | [~~hold~~](https://some-comment-id-for-hold.com) [~~merge~~](https://some-comment-id-for-merge.com) [**merge**](https://some-comment-id-for-merge.com) |\n\ + | @Niklaus | [~~merge~~](https://some-comment-id-for-merge.com) [~~hold~~](https://some-comment-id-for-hold.com) [**merge**](https://some-comment-id-for-merge.com) |\n\ | @Tom | |" .to_string(); @@ -330,8 +332,8 @@ mod tests { .expect("it shouldn't fail building the message"); let expected_comment = "| Team member | State |\n\ |-------------|-------|\n\ - | @Barbara | **merge** |\n\ - | @Niklaus | **merge** |\ + | @Barbara | [**merge**](https://some-comment-id-for-merge.com) |\n\ + | @Niklaus | [**merge**](https://some-comment-id-for-merge.com) |\ " .to_string(); From 555cb074e2bfab1786cbb1f175b372c4d683db93 Mon Sep 17 00:00:00 2001 From: Mauricio Cassola Date: Fri, 2 Jun 2023 15:31:27 +0200 Subject: [PATCH 15/17] Do not perform a merge instead post comment (#12) --- src/github.rs | 21 --------------------- src/handlers/jobs.rs | 28 +++++++++++++++++++++++++--- src/interactions.rs | 14 ++++++++++++-- 3 files changed, 37 insertions(+), 26 deletions(-) diff --git a/src/github.rs b/src/github.rs index 4bf0e2c2..8b9dafbd 100644 --- a/src/github.rs +++ b/src/github.rs @@ -779,27 +779,6 @@ impl Issue { Ok(()) } - pub async fn merge(&self, client: &GithubClient) -> anyhow::Result<()> { - let merge_url = format!("{}/pulls/{}/merge", self.repository().url(), self.number); - - // change defaults by reading from somewhere, maybe in .toml? - #[derive(serde::Serialize)] - struct MergeIssue<'a> { - commit_title: &'a str, - merge_method: &'a str, - } - - client - ._send_req(client.put(&merge_url).json(&MergeIssue { - commit_title: "Merged by the bot!", - merge_method: "merge", - })) - .await - .context("failed to merge issue")?; - - Ok(()) - } - /// Returns the diff in this event, for Open and Synchronize events for now. pub async fn diff(&self, client: &GithubClient) -> anyhow::Result> { let (before, after) = if let (Some(base), Some(head)) = (&self.base, &self.head) { diff --git a/src/handlers/jobs.rs b/src/handlers/jobs.rs index 480b1c29..33d25c08 100644 --- a/src/handlers/jobs.rs +++ b/src/handlers/jobs.rs @@ -4,10 +4,13 @@ // Further info could be find in src/jobs.rs use super::Context; +use crate::db::issue_decision_state::get_issue_decision_state; use crate::github::*; use crate::handlers::decision::{DecisionProcessActionMetadata, DECISION_PROCESS_JOB_NAME}; +use crate::interactions::PingComment; use parser::command::decision::Resolution::{Hold, Merge}; use reqwest::Client; +use tokio_postgres::Client as DbClient; use tracing as log; pub async fn handle_job( @@ -22,7 +25,8 @@ pub async fn handle_job( Ok(()) } matched_name if *matched_name == DECISION_PROCESS_JOB_NAME.to_string() => { - decision_process_handler(&metadata).await + let db = ctx.db.get().await; + decision_process_handler(&db, &metadata).await } _ => default(&name, &metadata), } @@ -38,7 +42,10 @@ fn default(name: &String, metadata: &serde_json::Value) -> anyhow::Result<()> { Ok(()) } -async fn decision_process_handler(metadata: &serde_json::Value) -> anyhow::Result<()> { +async fn decision_process_handler( + db: &DbClient, + metadata: &serde_json::Value, +) -> anyhow::Result<()> { tracing::trace!( "handle_job fell into decision process case: (metadata={:?})", metadata @@ -50,7 +57,22 @@ async fn decision_process_handler(metadata: &serde_json::Value) -> anyhow::Resul match gh_client.json::(request).await { Ok(issue) => match metadata.status { - Merge => issue.merge(&gh_client).await?, + Merge => { + let users: Vec = get_issue_decision_state(&db, &issue.number) + .await + .unwrap() + .current + .into_keys() + .collect(); + let users_ref: Vec<&str> = users.iter().map(|x| x.as_ref()).collect(); + + let cmnt = PingComment::new( + &issue, + &users_ref, + "The final comment period has resolved, with a decision to **merge**. Ping involved once again.", + ); + cmnt.post(&gh_client).await?; + } Hold => issue.close(&gh_client).await?, }, Err(e) => log::error!( diff --git a/src/interactions.rs b/src/interactions.rs index f3d6a115..e78050d7 100644 --- a/src/interactions.rs +++ b/src/interactions.rs @@ -33,15 +33,25 @@ impl<'a> ErrorComment<'a> { pub struct PingComment<'a> { issue: &'a Issue, users: &'a [&'a str], + message: String, } impl<'a> PingComment<'a> { - pub fn new(issue: &'a Issue, users: &'a [&str]) -> PingComment<'a> { - PingComment { issue, users } + pub fn new(issue: &'a Issue, users: &'a [&str], message: T) -> PingComment<'a> + where + T: Into, + { + PingComment { + issue, + users, + message: message.into(), + } } pub async fn post(&self, client: &GithubClient) -> anyhow::Result<()> { let mut body = String::new(); + writeln!(body, "{}", self.message)?; + writeln!(body)?; for user in self.users { write!(body, "@{} ", user)?; } From 558ee453fa450940c57cc23160e7b2a5bd32a155 Mon Sep 17 00:00:00 2001 From: Julian Montes de Oca Date: Fri, 8 Mar 2024 18:15:21 -0300 Subject: [PATCH 16/17] Fix format --- src/db.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/db.rs b/src/db.rs index 33c13f24..b4d00c97 100644 --- a/src/db.rs +++ b/src/db.rs @@ -326,22 +326,22 @@ CREATE UNIQUE INDEX jobs_name_scheduled_at_unique_index name, scheduled_at ); ", -" + " CREATE table review_prefs ( id UUID DEFAULT gen_random_uuid() PRIMARY KEY, user_id BIGINT REFERENCES users(user_id), assigned_prs INT[] NOT NULL DEFAULT array[]::INT[] );", -" + " CREATE UNIQUE INDEX review_prefs_user_id ON review_prefs(user_id); ", -" + " CREATE TYPE reversibility AS ENUM ('reversible', 'irreversible'); ", -" + " CREATE TYPE resolution AS ENUM ('hold', 'merge'); ", -"CREATE TABLE issue_decision_state ( + "CREATE TABLE issue_decision_state ( issue_id BIGINT PRIMARY KEY, initiator TEXT NOT NULL, start_date TIMESTAMP WITH TIME ZONE NOT NULL, From 5b8f25da7018fbecf36c1f01a56c9cebbe6e90c2 Mon Sep 17 00:00:00 2001 From: Julian Montes de Oca Date: Fri, 8 Mar 2024 18:27:36 -0300 Subject: [PATCH 17/17] Update config entry name --- src/config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 93ac941a..a43231bc 100644 --- a/src/config.rs +++ b/src/config.rs @@ -416,7 +416,7 @@ mod tests { [shortcut] - [decision-process] + [decision] "#; let config = toml::from_str::(&config).unwrap(); let mut ping_teams = HashMap::new(); @@ -471,7 +471,7 @@ mod tests { review_requested: None, mentions: None, no_merges: None, - decision: None, + decision: Some(DecisionConfig { _empty: () }), validate_config: Some(ValidateConfig {}), } );