diff --git a/docs/config/.gitvote.yml b/docs/config/.gitvote.yml index 6e51be9..bfb7aa8 100644 --- a/docs/config/.gitvote.yml +++ b/docs/config/.gitvote.yml @@ -83,10 +83,56 @@ profiles: #聽Percentage of votes in favor required to pass the vote # # The percentage is calculated based on the number of votes in favor and the - # number of allowed voters (see allowed_voters field below for more details). + # pass threshold base, which is the number of allowed voters by default (see + # the pass_threshold_base and allowed_voters fields below for more details). # It must be greater than 0 and not greater than 100. pass_threshold: 50 + # Pass threshold base (optional) + # + # Number the in favor and against percentages are calculated over, which is + # also used to decide if the vote passes. Only binding votes are counted. + # + # - allowed_voters (default): all users with binding votes, so users who + # do not vote count towards the percentage calculation too. + # + # - votes_cast: binding votes cast (in favor, against and abstain). + # Abstentions are included in the percentage calculation by default. + # When `exclude_abstentions` is set to true, only votes in favor and + # against are included. Abstentions are still displayed and recorded + # as binding votes, though. + # + # If no votes are counted, the vote does not pass. Please note that there is + # no minimum participation required when using votes_cast, so a low turnout + # can decide a vote. + # + # Each profile sets this option independently (the value set in the default + # profile is not inherited by other profiles). + # + # pass_threshold_base: allowed_voters + # + # pass_threshold_base: + # votes_cast: {} + # + # pass_threshold_base: + # votes_cast: + # exclude_abstentions: true + # + # More votes in favor than against + # + # To require more votes in favor than against (ignoring abstentions and users + # who do not vote), use votes_cast excluding abstentions with a pass + # threshold of 50.01. A tie does not pass with 50.01, but it would pass with + # 50. This is exact for up to 2,499 votes against. Comments will display + # `50.01%` as the passing threshold. + # + # pass_threshold: 50.01 + # pass_threshold_base: + # votes_cast: + # exclude_abstentions: true + # + pass_threshold_base: allowed_voters + # Allowed voters (optional) # # List of GitHub teams and users who have binding votes @@ -156,6 +202,12 @@ profiles: # The close on passing feature makes this possible. Open votes where this # feature has been enabled will be checked once daily and, if GitVote # detects that the vote has passed, it will automatically close it. + # + # Whatever the pass_threshold_base is set to, a vote is only closed early + # once the pass threshold is met over all allowed voters, so that votes from + # the users who have not voted yet cannot make it fail. When using + # votes_cast, this means that a vote that is currently passing may stay open + # until its duration ends. #聽 # close_on_passing: true # diff --git a/src/cfg_repo.rs b/src/cfg_repo.rs index 4aac2d8..a25e4a0 100644 --- a/src/cfg_repo.rs +++ b/src/cfg_repo.rs @@ -98,6 +98,14 @@ pub(crate) struct CfgProfile { pub allowed_voters: Option, #[serde(skip_serializing_if = "Option::is_none")] pub announcements: Option, + // The singleton map representation allows using the `votes_cast:` map syntax + // in YAML, instead of the `!votes_cast` tag expected by default + #[serde( + default, + skip_serializing_if = "Option::is_none", + with = "serde_yaml::with::singleton_map" + )] + pub pass_threshold_base: Option, #[serde(skip_serializing_if = "Option::is_none")] pub periodic_status_check: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -174,6 +182,20 @@ pub(crate) struct DiscussionsAnnouncements { pub category: String, } +/// Base used to calculate the pass threshold percentage. +#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum PassThresholdBase { + /// All users with binding votes. + #[default] + AllowedVoters, + /// Binding votes cast. + VotesCast { + #[serde(skip_serializing_if = "Option::is_none")] + exclude_abstentions: Option, + }, +} + /// Errors that may occur while getting the configuration profile. #[derive(Debug, Error, Clone, PartialEq, Serialize, Deserialize)] pub(crate) enum CfgError { diff --git a/src/cfg_repo/tests.rs b/src/cfg_repo/tests.rs index 51822fb..e1aeeaa 100644 --- a/src/cfg_repo/tests.rs +++ b/src/cfg_repo/tests.rs @@ -149,6 +149,69 @@ fn automation_rule_matches_anchored_pattern_only_from_root() { ); } +#[test] +fn cfg_profile_deserialize_pass_threshold_base() { + // Setup cases (YAML snippet, expected pass threshold base) + let cases = [ + ("", None), + ( + "pass_threshold_base: allowed_voters", + Some(PassThresholdBase::AllowedVoters), + ), + ( + "pass_threshold_base:\n votes_cast: {}", + Some(PassThresholdBase::VotesCast { + exclude_abstentions: None, + }), + ), + ( + "pass_threshold_base:\n votes_cast:", + Some(PassThresholdBase::VotesCast { + exclude_abstentions: None, + }), + ), + ( + "pass_threshold_base:\n votes_cast:\n exclude_abstentions: true", + Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), + ), + ( + "pass_threshold_base:\n votes_cast:\n exclude_abstentions: false", + Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(false), + }), + ), + ]; + + for (snippet, expected) in cases { + // Check the profile parses with the expected pass threshold base + let yaml = format!("duration: 5m\npass_threshold: 50\n{snippet}\n"); + let cfg: CfgProfile = serde_yaml::from_str(&yaml).unwrap_or_else(|err| panic!("{snippet}: {err}")); + assert_eq!(cfg.pass_threshold_base, expected, "snippet: {snippet}"); + } +} + +#[test] +fn cfg_profile_deserialize_pass_threshold_base_invalid() { + // Setup invalid YAML snippets + let cases = [ + "pass_threshold_base: other", + "pass_threshold_base: votes_cast", + "pass_threshold_base:\n allowed_voters: {}", + "pass_threshold_base:\n votes_cast:\n exclude_abstentions: \"yes\"", + ]; + + for snippet in cases { + // Check the profile is rejected + let yaml = format!("duration: 5m\npass_threshold: 50\n{snippet}\n"); + assert!( + serde_yaml::from_str::(&yaml).is_err(), + "snippet: {snippet}" + ); + } +} + #[test] fn cfg_profile_deserialize_stored_json() { // Setup JSON as stored in the vote cfg column @@ -187,6 +250,7 @@ fn cfg_profile_deserialize_stored_json() { category: DISCUSSIONS_CATEGORY.to_string(), }), }), + pass_threshold_base: None, periodic_status_check: Some("1 week".to_string()), close_on_passing: Some(true), close_on_passing_min_wait: Some("2 days".to_string()), @@ -214,6 +278,53 @@ fn cfg_profile_deserialize_stored_json_minimal() { ); } +#[test] +fn cfg_profile_deserialize_stored_json_pass_threshold_base() { + // Setup cases (stored JSON value, expected pass threshold base) + let cases = [ + (json!("allowed_voters"), PassThresholdBase::AllowedVoters), + ( + json!({"votes_cast": {}}), + PassThresholdBase::VotesCast { + exclude_abstentions: None, + }, + ), + ( + json!({"votes_cast": {"exclude_abstentions": true}}), + PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }, + ), + ]; + + for (stored_base, expected) in cases { + // Check the stored profile deserializes into the expected one + let stored = json!({ + "duration": "5m", + "pass_threshold": 50.0, + "pass_threshold_base": stored_base.clone() + }); + let cfg: CfgProfile = serde_json::from_value(stored.clone()).unwrap(); + assert_eq!( + cfg, + CfgProfile { + duration: Duration::from_mins(5), + pass_threshold: 50.0, + pass_threshold_base: Some(expected), + ..Default::default() + }, + "stored: {stored_base}" + ); + + // Check it serializes back into the same JSON + assert_eq!( + serde_json::to_value(&cfg).unwrap(), + stored, + "stored: {stored_base}" + ); + } +} + #[test] fn cfg_profile_serialize_full() { // Setup profile with all fields set @@ -230,6 +341,9 @@ fn cfg_profile_serialize_full() { category: DISCUSSIONS_CATEGORY.to_string(), }), }), + pass_threshold_base: Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), periodic_status_check: Some("1 day".to_string()), close_on_passing: Some(true), close_on_passing_min_wait: Some("1 hour".to_string()), @@ -251,6 +365,11 @@ fn cfg_profile_serialize_full() { "category": DISCUSSIONS_CATEGORY } }, + "pass_threshold_base": { + "votes_cast": { + "exclude_abstentions": true + } + }, "periodic_status_check": "1 day", "close_on_passing": true, "close_on_passing_min_wait": "1 hour" @@ -407,6 +526,9 @@ profiles: announcements: discussions: category: announcements + pass_threshold_base: + votes_cast: + exclude_abstentions: true periodic_status_check: 1 week close_on_passing: true close_on_passing_min_wait: 1 day @@ -433,6 +555,9 @@ profiles: category: DISCUSSIONS_CATEGORY.to_string(), }), }), + pass_threshold_base: Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), periodic_status_check: Some("1 week".to_string()), close_on_passing: Some(true), close_on_passing_min_wait: Some("1 day".to_string()), @@ -531,6 +656,32 @@ async fn get_cfg_profile_invalid_config_invalid_yaml() { )); } +#[tokio::test] +async fn get_cfg_profile_invalid_config_pass_threshold_base() { + // Setup GitHub expectations + let mut gh = MockGH::new(); + gh.expect_get_config_file() + .with(eq(INST_ID), eq(OWNER), eq(REPO)) + .times(1) + .returning(|_, _, _| { + let config = r" +profiles: + default: + duration: 5m + pass_threshold: 50 + pass_threshold_base: other +"; + Box::pin(future::ready(Some(config.to_string()))) + }); + let gh = Arc::new(gh); + + // Run and check the error returned + assert!(matches!( + CfgProfile::get(gh, INST_ID, OWNER, OWNER_IS_ORG, REPO, None).await.unwrap_err(), + CfgError::InvalidConfig(_) + )); +} + #[tokio::test] async fn get_cfg_profile_invalid_config_teams_owner_not_org() { // Setup GitHub expectations @@ -557,6 +708,41 @@ async fn get_cfg_profile_invalid_config_teams_owner_not_org() { ); } +#[tokio::test] +async fn get_cfg_profile_pass_threshold_base_votes_cast() { + // Setup GitHub expectations + let mut gh = MockGH::new(); + gh.expect_get_config_file() + .with(eq(INST_ID), eq(OWNER), eq(REPO)) + .times(1) + .returning(|_, _, _| { + let config = r" +profiles: + default: + duration: 5m + pass_threshold: 50.01 + pass_threshold_base: + votes_cast: + exclude_abstentions: true +"; + Box::pin(future::ready(Some(config.to_string()))) + }); + let gh = Arc::new(gh); + + // Run and check the profile returned + assert_eq!( + CfgProfile::get(gh, INST_ID, OWNER, OWNER_IS_ORG, REPO, None).await.unwrap(), + CfgProfile { + duration: Duration::from_mins(5), + pass_threshold: 50.01, + pass_threshold_base: Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), + ..Default::default() + } + ); +} + #[tokio::test] async fn get_cfg_profile_profile1() { // Setup GitHub expectations diff --git a/src/processor.rs b/src/processor.rs index 2b4a225..d8ddb3a 100644 --- a/src/processor.rs +++ b/src/processor.rs @@ -519,8 +519,9 @@ impl StatusChecker { } } -/// Worker that periodically auto closes votes that have already passed. This -/// only applies to votes with the `close_on_passing` feature enabled. +/// Worker that periodically auto closes votes that have already passed over all +/// the allowed voters. This only applies to votes with the `close_on_passing` +/// feature enabled. struct VotesAutoCloser { db: DynDB, gh: DynGH, @@ -546,8 +547,12 @@ impl VotesAutoCloser { // Calculate vote results match results::calculate(self.gh.clone(), owner, repo, vote).await { Ok(results) => { - // If the vote has already passed, update its ending timestamp - if results.passed + // Close the vote early (update its ending + // timestamp) only once it passes over all the + // allowed voters, so that further votes from + // pending voters cannot make it fail. A vote + // passing over the votes cast may stay open. + if results.passes_with_all_allowed_voters() && let Err(err) = self.db.update_vote_ends_at(vote_id).await { error!(?err, ?vote.vote_id, "error updating vote ends at"); diff --git a/src/processor/tests.rs b/src/processor/tests.rs index 186ac6a..ba8f553 100644 --- a/src/processor/tests.rs +++ b/src/processor/tests.rs @@ -6,7 +6,11 @@ use time::ext::NumericalDuration; use crate::results::{REACTION_IN_FAVOR, Vote}; use crate::testutil::*; -use crate::{cfg_repo::AllowedVoters, db::MockDB, github::*}; +use crate::{ + cfg_repo::{AllowedVoters, PassThresholdBase}, + db::MockDB, + github::*, +}; use super::*; @@ -1990,6 +1994,58 @@ async fn status_checker_stops_when_requested_none_pending() { assert!(cmds_rx.is_empty()); } +#[tokio::test] +async fn votes_auto_closer_closes_vote_only_when_passing_over_all_allowed_voters() { + // Setup cases (exclude abstentions, allowed voters, expected ends at updates). + // In all cases USER1 votes in favor and the vote passes over the votes cast. + let cases = [ + (None, vec![USER1, USER2, USER3], 0), + (None, vec![USER1, USER2], 1), + (Some(true), vec![USER1, USER2], 1), + (None, vec![], 0), + ]; + + for (exclude_abstentions, allowed_voters, expected_updates) in cases { + // Setup vote using the votes cast as the pass threshold base + let mut vote = setup_test_vote(); + vote.cfg.pass_threshold_base = Some(PassThresholdBase::VotesCast { exclude_abstentions }); + + // Setup database expectations + let mut db = MockDB::new(); + db.expect_get_open_votes_with_close_on_passing() + .times(1) + .return_once(move || Box::pin(future::ready(Ok(vec![vote])))); + db.expect_update_vote_ends_at() + .with(eq(Uuid::parse_str(VOTE_ID).unwrap())) + .times(expected_updates) + .returning(|_| Box::pin(future::ready(Ok(())))); + + // Setup GitHub expectations + let mut gh = MockGH::new(); + gh.expect_get_comment_reactions() + .with(eq(INST_ID), eq(ORG), eq(REPO), eq(COMMENT_ID)) + .times(1) + .returning(|_, _, _, _| Box::pin(future::ready(Ok(vec![in_favor_reaction(USER1)])))); + let allowed_voters: Vec = allowed_voters.into_iter().map(ToString::to_string).collect(); + gh.expect_get_allowed_voters() + .withf(|inst_id, _, owner, repo, _| *inst_id == INST_ID && owner == ORG && repo == REPO) + .times(1) + .return_once(move |_, _, _, _, _| Box::pin(future::ready(Ok(allowed_voters)))); + + // Run the votes auto closer until it's asked to stop + let cancel_token = CancellationToken::new(); + let votes_auto_closer = VotesAutoCloser::new(Arc::new(db), Arc::new(gh)); + let votes_auto_closer_handle = votes_auto_closer.run(cancel_token.clone()); + cancel_token.cancel(); + + // Check the worker completed (expectations are verified on drop) + assert!( + votes_auto_closer_handle.await.is_ok(), + "exclude abstentions: {exclude_abstentions:?}, expected updates: {expected_updates}" + ); + } +} + #[tokio::test] async fn votes_auto_closer_continues_after_error_calculating_results() { // Setup votes diff --git a/src/results.rs b/src/results.rs index d935b5f..74ad710 100644 --- a/src/results.rs +++ b/src/results.rs @@ -9,7 +9,7 @@ use tokio_postgres::{Row, types::Json}; use uuid::Uuid; use crate::{ - cfg_repo::CfgProfile, + cfg_repo::{CfgProfile, PassThresholdBase}, github::{DynGH, UserName}, }; @@ -117,6 +117,15 @@ pub(crate) struct VoteResults { pub pending_voters: Vec, } +impl VoteResults { + /// Check if the vote passes when calculated over all the allowed voters, so + /// that further votes from pending voters cannot make it fail (assuming the + /// votes already cast and the allowed voters do not change). + pub(crate) fn passes_with_all_allowed_voters(&self) -> bool { + threshold_reached(self.in_favor, self.pass_threshold, self.allowed_voters) + } +} + /// User's vote details. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub(crate) struct UserVote { @@ -175,7 +184,8 @@ pub(crate) async fn calculate<'a>( ); } - // Prepare results and return them + // Count binding votes per option and the number of binding and + // non-binding voters (only binding votes count towards the result) let (mut in_favor, mut against, mut abstain, mut binding, mut non_binding) = (0, 0, 0, 0, 0); for user_vote in votes.values() { if user_vote.binding { @@ -189,27 +199,38 @@ pub(crate) async fn calculate<'a>( non_binding += 1; } } + + // Select the base used to calculate the percentages and the pass threshold + let allowed_voters_count = allowed_voters.len() as i64; + let threshold_base = match vote.cfg.pass_threshold_base.clone().unwrap_or_default() { + PassThresholdBase::AllowedVoters => allowed_voters_count, + PassThresholdBase::VotesCast { exclude_abstentions } => { + if exclude_abstentions.unwrap_or(false) { + in_favor + against + } else { + binding + } + } + }; + + // Calculate the in favor and against percentages over the threshold base let mut in_favor_percentage = 0.0; let mut against_percentage = 0.0; #[allow(clippy::cast_precision_loss)] - if !allowed_voters.is_empty() { - in_favor_percentage = in_favor as f64 / allowed_voters.len() as f64 * 100.0; - against_percentage = against as f64 / allowed_voters.len() as f64 * 100.0; + if threshold_base > 0 { + in_favor_percentage = in_favor as f64 / threshold_base as f64 * 100.0; + against_percentage = against as f64 / threshold_base as f64 * 100.0; } + + // Collect the allowed voters who have not voted yet let pending_voters: Vec = allowed_voters .iter() .filter(|user| !votes.keys().any(|voter| voter.eq_ignore_ascii_case(user))) .cloned() .collect(); - // Check if the vote passed, comparing without dividing so that votes in - // favor that match the pass threshold exactly are not lost to rounding - #[allow(clippy::cast_precision_loss)] - let passed = if allowed_voters.is_empty() { - in_favor_percentage >= vote.cfg.pass_threshold - } else { - in_favor as f64 * 100.0 >= vote.cfg.pass_threshold * allowed_voters.len() as f64 - }; + // Check if the vote passed + let passed = threshold_reached(in_favor, vote.cfg.pass_threshold, threshold_base); Ok(VoteResults { passed, @@ -222,11 +243,18 @@ pub(crate) async fn calculate<'a>( not_voted: pending_voters.len() as i64, binding, non_binding, - allowed_voters: allowed_voters.len() as i64, + allowed_voters: allowed_voters_count, votes, pending_voters, }) } +/// Check if the votes in favor reach the pass threshold over the base provided, +/// comparing without dividing so that exact matches are not lost to rounding. +#[allow(clippy::cast_precision_loss)] +fn threshold_reached(in_favor: i64, pass_threshold: f64, base: i64) -> bool { + base > 0 && in_favor as f64 * 100.0 >= pass_threshold * base as f64 +} + #[cfg(test)] mod tests; diff --git a/src/results/tests.rs b/src/results/tests.rs index b904a3d..d09d241 100644 --- a/src/results/tests.rs +++ b/src/results/tests.rs @@ -5,7 +5,9 @@ use futures::future::{self}; use mockall::predicate::eq; use proptest::{collection, prelude::*, sample}; use serde_json::json; +use tokio_postgres::types::{FromSql, Type}; +use crate::cfg_repo::AllowedVoters; use crate::github::{MockGH, Reaction, User, UserName}; use crate::testutil::*; @@ -19,9 +21,27 @@ const PROPTEST_REACTIONS: [(&str, Option); 4] = [ ("heart", None), ]; +/// Pass threshold bases used in randomly generated votes. +const PROPTEST_THRESHOLD_BASES: [Option; 5] = [ + None, + Some(PassThresholdBase::AllowedVoters), + Some(PassThresholdBase::VotesCast { + exclude_abstentions: None, + }), + Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(false), + }), + Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), +]; + /// Number of users that can take part in randomly generated votes. const PROPTEST_USERS: usize = 6; +/// Pass threshold used to require more votes in favor than against. +const STRICT_MAJORITY_THRESHOLD: f64 = 50.01; + /// Additional deterministic timestamps used to check each vote keeps its own. const TIMESTAMP2: &str = "2022-11-30T11:00:00Z"; const TIMESTAMP3: &str = "2022-11-30T12:00:00Z"; @@ -67,6 +87,7 @@ proptest! { allowed_users_uppercase in collection::vec(any::(), PROPTEST_USERS), users_reactions in collection::vec((1..=PROPTEST_USERS, 0..PROPTEST_REACTIONS.len()), 0..20), pass_threshold in 1..=100_u32, + pass_threshold_base in 0..PROPTEST_THRESHOLD_BASES.len(), ) { // Setup allowed voters, spelled with random casing let allowed_voters: Vec = allowed_users @@ -77,36 +98,20 @@ proptest! { }) .collect(); - // Setup reactions and vote + // Setup reactions and vote profile let reactions: Vec = users_reactions .iter() - .map(|(i, r)| Reaction { - user: User { login: format!("user{i}") }, - content: PROPTEST_REACTIONS[*r].0.to_string(), - created_at: TIMESTAMP.to_string(), - }) + .map(|(i, r)| reaction(&format!("user{i}"), PROPTEST_REACTIONS[*r].0)) .collect(); - let mut vote = setup_test_vote(); - vote.cfg.pass_threshold = f64::from(pass_threshold); - - // Setup GitHub expectations - let mut gh = MockGH::new(); - let reactions_returned = reactions.clone(); - gh.expect_get_comment_reactions() - .with(eq(INST_ID), eq(OWNER), eq(REPO), eq(COMMENT_ID)) - .times(1) - .returning(move |_, _, _, _| Box::pin(future::ready(Ok(reactions_returned.clone())))); - let allowed_voters_returned = allowed_voters.clone(); - gh.expect_get_allowed_voters() - .withf(|inst_id, _, owner, repo, _| *inst_id == INST_ID && owner == OWNER && repo == REPO) - .times(1) - .returning(move |_, _, _, _, _| { - Box::pin(future::ready(Ok(allowed_voters_returned.clone()))) - }); + let cfg = CfgProfile { + pass_threshold: f64::from(pass_threshold), + pass_threshold_base: PROPTEST_THRESHOLD_BASES[pass_threshold_base].clone(), + ..setup_test_vote().cfg + }; // Calculate vote results let rt = tokio::runtime::Builder::new_current_thread().build().unwrap(); - let results = rt.block_on(calculate(Arc::new(gh), OWNER, REPO, &vote)).unwrap(); + let results = rt.block_on(calculate_results(cfg.clone(), reactions, allowed_voters.clone())); // Build expected votes (only users with a single supported reaction count) let mut users_options: BTreeMap> = BTreeMap::new(); @@ -132,6 +137,7 @@ proptest! { .count() as i64 }; let expected_in_favor = count_binding(Some(VoteOption::InFavor)); + let expected_against = count_binding(Some(VoteOption::Against)); let expected_binding = count_binding(None); let expected_pending: Vec = allowed_voters .iter() @@ -139,7 +145,20 @@ proptest! { .cloned() .collect(); let allowed = allowed_voters.len() as i64; - let expected_passed = allowed > 0 && expected_in_favor * 100 >= i64::from(pass_threshold) * allowed; + let expected_base = match cfg.pass_threshold_base.clone().unwrap_or_default() { + PassThresholdBase::AllowedVoters => allowed, + PassThresholdBase::VotesCast { exclude_abstentions: Some(true) } => { + expected_in_favor + expected_against + } + PassThresholdBase::VotesCast { .. } => expected_binding, + }; + let reaches_threshold = + |base: i64| base > 0 && expected_in_favor * 100 >= i64::from(pass_threshold) * base; + let expected_passed = reaches_threshold(expected_base); + #[allow(clippy::cast_precision_loss)] + let expected_percentage = |count: i64| { + if expected_base > 0 { count as f64 / expected_base as f64 * 100.0 } else { 0.0 } + }; // Check the votes counted let votes: BTreeMap = results @@ -151,8 +170,10 @@ proptest! { // Check the results summary prop_assert_eq!(results.passed, expected_passed); + prop_assert!((results.in_favor_percentage - expected_percentage(expected_in_favor)).abs() < 1e-9); + prop_assert!((results.against_percentage - expected_percentage(expected_against)).abs() < 1e-9); prop_assert_eq!(results.in_favor, expected_in_favor); - prop_assert_eq!(results.against, count_binding(Some(VoteOption::Against))); + prop_assert_eq!(results.against, expected_against); prop_assert_eq!(results.abstain, count_binding(Some(VoteOption::Abstain))); prop_assert_eq!(results.binding, expected_binding); prop_assert_eq!(results.non_binding, expected_votes.len() as i64 - expected_binding); @@ -163,6 +184,14 @@ proptest! { // Check the results invariants prop_assert_eq!(results.in_favor + results.against + results.abstain, results.binding); prop_assert_eq!(results.binding + results.not_voted, results.allowed_voters); + for percentage in [results.in_favor_percentage, results.against_percentage] { + prop_assert!(percentage.is_finite() && (0.0..=100.0).contains(&percentage)); + } + + // Check the early close check uses all the allowed voters as the base, and + // that a vote that passes over them also passes over the base configured + prop_assert_eq!(results.passes_with_all_allowed_voters(), reaches_threshold(allowed)); + prop_assert!(!results.passes_with_all_allowed_voters() || results.passed); } } @@ -179,48 +208,8 @@ macro_rules! test_calculate { $( #[tokio::test] async fn $func() { - // Prepare test data - let vote = Vote { - vote_id: Uuid::parse_str(VOTE_ID).unwrap(), - vote_comment_id: COMMENT_ID, - created_at: OffsetDateTime::now_utc(), - created_by: USER.to_string(), - ends_at: OffsetDateTime::now_utc(), - closed: false, - closed_at: None, - checked_at: None, - cfg: $cfg.clone(), - installation_id: INST_ID as i64, - issue_id: ISSUE_ID, - issue_number: ISSUE_NUM, - issue_title: Some(TITLE.to_string()), - is_pull_request: false, - repository_full_name: REPOFN.to_string(), - organization: Some(ORG.to_string()), - results: None, - }; - - // Setup mocks and expectations - let mut gh = MockGH::new(); - gh.expect_get_comment_reactions() - .with(eq(INST_ID), eq(OWNER), eq(REPO), eq(COMMENT_ID)) - .times(1) - .returning(|_, _, _, _| Box::pin(future::ready(Ok($reactions)))); - gh.expect_get_allowed_voters() - .withf(|inst_id, cfg, owner, repo, org| { - *inst_id == INST_ID - && *cfg == $cfg - && owner == OWNER - && repo == REPO - && *org == Some(ORG.to_string()).as_ref() - }) - .times(1) - .returning(|_, _, _, _, _| Box::pin(future::ready(Ok($allowed_voters)))); - // Calculate vote results and check we get what we expect - let results = calculate(Arc::new(gh), OWNER, REPO, &vote) - .await - .unwrap(); + let results = calculate_results($cfg, $reactions, $allowed_voters).await; assert_eq!(results, $expected_results); } )* @@ -230,22 +219,9 @@ macro_rules! test_calculate { test_calculate!( calculate_allowed_voters_are_matched_case_insensitively: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, - reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - } - ], - allowed_voters: vec![ - USER1.to_uppercase(), - USER2.to_uppercase() - ], + cfg: threshold_cfg(50.0), + reactions: reactions(&votes_cast(1, 0, 0)), + allowed_voters: vec![USER1.to_uppercase(), USER2.to_uppercase()], expected_results: VoteResults { passed: true, in_favor_percentage: 50.0, @@ -274,485 +250,158 @@ test_calculate!( calculate_do_not_count_votes_from_multiple_options_voters: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, + cfg: threshold_cfg(50.0), reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_AGAINST.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_ABSTAIN.to_string(), - created_at: TIMESTAMP.to_string(), - } + reaction(USER1, REACTION_AGAINST), + reaction(USER1, REACTION_ABSTAIN), ], - allowed_voters: vec![ - USER1.to_string() - ], - expected_results: VoteResults { - passed: false, - in_favor_percentage: 0.0, - pass_threshold: 50.0, - in_favor: 0, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 1, - binding: 0, - non_binding: 0, - votes: BTreeMap::new(), - allowed_voters: 1, - pending_voters: vec![USER1.to_string()], - } + allowed_voters: users(1), + expected_results: expected_results(50.0, &[], &users(1), (false, 0.0, 0.0)) }, calculate_do_not_count_votes_from_non_binding_multiple_options_voters: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, + cfg: threshold_cfg(50.0), reactions: vec![ - Reaction { - user: User { login: USER5.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER5.to_string() }, - content: REACTION_AGAINST.to_string(), - created_at: TIMESTAMP.to_string(), - } + reaction(USER5, REACTION_IN_FAVOR), + reaction(USER5, REACTION_AGAINST), ], - allowed_voters: vec![ - USER1.to_string() - ], - expected_results: VoteResults { - passed: false, - in_favor_percentage: 0.0, - pass_threshold: 50.0, - in_favor: 0, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 1, - binding: 0, - non_binding: 0, - votes: BTreeMap::new(), - allowed_voters: 1, - pending_voters: vec![USER1.to_string()], - } + allowed_voters: users(1), + expected_results: expected_results(50.0, &[], &users(1), (false, 0.0, 0.0)) }, calculate_ignore_further_reactions_from_multiple_options_voters: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, + cfg: threshold_cfg(50.0), reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_AGAINST.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_ABSTAIN.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER2.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - } + reaction(USER1, REACTION_IN_FAVOR), + reaction(USER1, REACTION_AGAINST), + reaction(USER1, REACTION_ABSTAIN), + reaction(USER2, REACTION_IN_FAVOR), ], - allowed_voters: vec![ - USER1.to_string(), - USER2.to_string() - ], - expected_results: VoteResults { - passed: true, - in_favor_percentage: 50.0, - pass_threshold: 50.0, - in_favor: 1, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 1, - binding: 1, - non_binding: 0, - votes: BTreeMap::from([ - ( - USER2.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ) - ]), - allowed_voters: 2, - pending_voters: vec![USER1.to_string()], - } + allowed_voters: users(2), + expected_results: expected_results( + 50.0, + &[(USER2.to_string(), VoteOption::InFavor)], + &users(2), + (true, 50.0, 0.0) + ) }, calculate_no_allowed_voters: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, - reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - } - ], + cfg: threshold_cfg(50.0), + reactions: reactions(&votes_cast(1, 0, 0)), allowed_voters: Vec::::new(), - expected_results: VoteResults { - passed: false, - in_favor_percentage: 0.0, - pass_threshold: 50.0, - in_favor: 0, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 0, - binding: 0, - non_binding: 1, - votes: BTreeMap::from([ - ( - USER1.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: false, - }, - ) - ]), - allowed_voters: 0, - pending_voters: vec![], - } + expected_results: expected_results(50.0, &votes_cast(1, 0, 0), &[], (false, 0.0, 0.0)) }, calculate_no_reactions: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, + cfg: threshold_cfg(50.0), reactions: Vec::::new(), - allowed_voters: vec![ - USER1.to_string(), - USER2.to_string() - ], - expected_results: VoteResults { - passed: false, - in_favor_percentage: 0.0, - pass_threshold: 50.0, - in_favor: 0, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 2, - binding: 0, - non_binding: 0, - votes: BTreeMap::new(), - allowed_voters: 2, - pending_voters: vec![USER1.to_string(), USER2.to_string()], - } + allowed_voters: users(2), + expected_results: expected_results(50.0, &[], &users(2), (false, 0.0, 0.0)) }, - calculate_unsupported_reactions_are_ignored: + calculate_pass_threshold_base_example_allowed_voters: { cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() + pass_threshold_base: Some(PassThresholdBase::AllowedVoters), + ..threshold_cfg(60.0) }, + reactions: reactions(&votes_cast(2, 1, 1)), + allowed_voters: users(10), + expected_results: expected_results( + 60.0, + &votes_cast(2, 1, 1), + &users(10), + (false, 2.0 / 10.0 * 100.0, 1.0 / 10.0 * 100.0) + ) + }, + + calculate_pass_threshold_base_example_votes_cast: + { + cfg: votes_cast_cfg(60.0, None), + reactions: reactions(&votes_cast(2, 1, 1)), + allowed_voters: users(10), + expected_results: expected_results( + 60.0, + &votes_cast(2, 1, 1), + &users(10), + (false, 2.0 / 4.0 * 100.0, 1.0 / 4.0 * 100.0) + ) + }, + + calculate_pass_threshold_base_example_votes_cast_excluding_abstentions: + { + cfg: votes_cast_cfg(60.0, Some(true)), + reactions: reactions(&votes_cast(2, 1, 1)), + allowed_voters: users(10), + expected_results: expected_results( + 60.0, + &votes_cast(2, 1, 1), + &users(10), + (true, 2.0 / 3.0 * 100.0, 1.0 / 3.0 * 100.0) + ) + }, + + calculate_unsupported_reactions_are_ignored: + { + cfg: threshold_cfg(50.0), reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: "unsupported".to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_AGAINST.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER1.to_string() }, - content: "unsupported".to_string(), - created_at: TIMESTAMP.to_string(), - } - ], - allowed_voters: vec![ - USER1.to_string() + reaction(USER1, "unsupported"), + reaction(USER1, REACTION_AGAINST), + reaction(USER1, "unsupported"), ], - expected_results: VoteResults { - passed: false, - in_favor_percentage: 0.0, - pass_threshold: 50.0, - in_favor: 0, - against: 1, - against_percentage: 100.0, - abstain: 0, - not_voted: 0, - binding: 1, - non_binding: 0, - votes: BTreeMap::from([ - ( - USER1.to_string(), - UserVote { - vote_option: VoteOption::Against, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ) - ]), - allowed_voters: 1, - pending_voters: vec![], - } + allowed_voters: users(1), + expected_results: expected_results( + 50.0, + &votes_cast(0, 1, 0), + &users(1), + (false, 0.0, 100.0) + ) }, calculate_vote_does_not_pass_when_in_favor_percentage_is_below_pass_threshold: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 66.67, - ..Default::default() - }, - reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER2.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER3.to_string() }, - content: REACTION_ABSTAIN.to_string(), - created_at: TIMESTAMP.to_string(), - } - ], - allowed_voters: vec![ - USER1.to_string(), - USER2.to_string(), - USER3.to_string() - ], - expected_results: VoteResults { - passed: false, - in_favor_percentage: 2.0 / 3.0 * 100.0, - pass_threshold: 66.67, - in_favor: 2, - against: 0, - against_percentage: 0.0, - abstain: 1, - not_voted: 0, - binding: 3, - non_binding: 0, - votes: BTreeMap::from([ - ( - USER1.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ( - USER2.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ( - USER3.to_string(), - UserVote { - vote_option: VoteOption::Abstain, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ]), - allowed_voters: 3, - pending_voters: vec![], - } + cfg: threshold_cfg(66.67), + reactions: reactions(&votes_cast(2, 0, 1)), + allowed_voters: users(3), + expected_results: expected_results( + 66.67, + &votes_cast(2, 0, 1), + &users(3), + (false, 2.0 / 3.0 * 100.0, 0.0) + ) }, calculate_vote_passes_when_in_favor_percentage_reaches_pass_threshold: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 75.0, - ..Default::default() - }, - reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER2.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER3.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - } - ], - allowed_voters: vec![ - USER1.to_string(), - USER2.to_string(), - USER3.to_string(), - USER4.to_string() - ], - expected_results: VoteResults { - passed: true, - in_favor_percentage: 75.0, - pass_threshold: 75.0, - in_favor: 3, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 1, - binding: 3, - non_binding: 0, - votes: BTreeMap::from([ - ( - USER1.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ( - USER2.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ( - USER3.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ]), - allowed_voters: 4, - pending_voters: vec![USER4.to_string()], - } + cfg: threshold_cfg(75.0), + reactions: reactions(&votes_cast(3, 0, 0)), + allowed_voters: users(4), + expected_results: expected_results(75.0, &votes_cast(3, 0, 0), &users(4), (true, 75.0, 0.0)) }, calculate_vote_passes_when_in_favor_votes_exactly_reach_pass_threshold: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 58.0, - ..Default::default() - }, - reactions: (1..=29) - .map(|i| Reaction { - user: User { login: format!("user{i}") }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }) - .collect::>(), - allowed_voters: (1..=50).map(|i| format!("user{i}")).collect::>(), - expected_results: VoteResults { - passed: true, - in_favor_percentage: 29.0 / 50.0 * 100.0, - pass_threshold: 58.0, - in_favor: 29, - against: 0, - against_percentage: 0.0, - abstain: 0, - not_voted: 21, - binding: 29, - non_binding: 0, - votes: (1..=29) - .map(|i| { - ( - format!("user{i}"), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ) - }) - .collect(), - allowed_voters: 50, - pending_voters: (30..=50).map(|i| format!("user{i}")).collect(), - } + cfg: threshold_cfg(58.0), + reactions: reactions(&votes_cast(29, 0, 0)), + allowed_voters: users(50), + expected_results: expected_results( + 58.0, + &votes_cast(29, 0, 0), + &users(50), + (true, 29.0 / 50.0 * 100.0, 0.0) + ) }, calculate_votes_are_counted_correctly: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, - reactions: vec![ - Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER2.to_string() }, - content: REACTION_AGAINST.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER3.to_string() }, - content: REACTION_ABSTAIN.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER5.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - } - ], - allowed_voters: vec![ - USER1.to_string(), - USER2.to_string(), - USER3.to_string(), - USER4.to_string() - ], + cfg: threshold_cfg(50.0), + reactions: reactions(&[votes_cast(1, 1, 1), vec![(USER5.to_string(), VoteOption::InFavor)]].concat()), + allowed_voters: users(4), expected_results: VoteResults { passed: false, in_favor_percentage: 25.0, @@ -803,77 +452,238 @@ test_calculate!( } }, + calculate_votes_cast_does_not_pass_when_in_favor_percentage_is_below_pass_threshold: + { + cfg: votes_cast_cfg(66.67, None), + reactions: reactions(&votes_cast(2, 1, 0)), + allowed_voters: users(5), + expected_results: expected_results( + 66.67, + &votes_cast(2, 1, 0), + &users(5), + (false, 2.0 / 3.0 * 100.0, 1.0 / 3.0 * 100.0) + ) + }, + + calculate_votes_cast_excluding_abstentions_does_not_pass_with_only_abstentions: + { + cfg: votes_cast_cfg(50.0, Some(true)), + reactions: reactions(&votes_cast(0, 0, 2)), + allowed_voters: users(3), + expected_results: expected_results(50.0, &votes_cast(0, 0, 2), &users(3), (false, 0.0, 0.0)) + }, + + calculate_votes_cast_excluding_abstentions_passes_when_in_favor_votes_exactly_reach_pass_threshold: + { + cfg: votes_cast_cfg(58.0, Some(true)), + reactions: reactions(&votes_cast(29, 21, 5)), + allowed_voters: users(60), + expected_results: expected_results( + 58.0, + &votes_cast(29, 21, 5), + &users(60), + (true, 29.0 / 50.0 * 100.0, 21.0 / 50.0 * 100.0) + ) + }, + + calculate_votes_cast_no_allowed_voters: + { + cfg: votes_cast_cfg(50.0, None), + reactions: reactions(&votes_cast(1, 0, 0)), + allowed_voters: Vec::::new(), + expected_results: expected_results(50.0, &votes_cast(1, 0, 0), &[], (false, 0.0, 0.0)) + }, + + calculate_votes_cast_no_reactions: + { + cfg: votes_cast_cfg(50.0, None), + reactions: Vec::::new(), + allowed_voters: users(2), + expected_results: expected_results(50.0, &[], &users(2), (false, 0.0, 0.0)) + }, + + calculate_votes_cast_only_non_binding_votes: + { + cfg: votes_cast_cfg(50.0, None), + reactions: reactions(&votes_cast(1, 0, 0)), + allowed_voters: vec![USER2.to_string(), USER3.to_string()], + expected_results: expected_results( + 50.0, + &votes_cast(1, 0, 0), + &[USER2.to_string(), USER3.to_string()], + (false, 0.0, 0.0) + ) + }, + + calculate_votes_cast_passes_when_in_favor_votes_exactly_reach_pass_threshold: + { + cfg: votes_cast_cfg(58.0, None), + reactions: reactions(&votes_cast(29, 21, 0)), + allowed_voters: users(60), + expected_results: expected_results( + 58.0, + &votes_cast(29, 21, 0), + &users(60), + (true, 29.0 / 50.0 * 100.0, 21.0 / 50.0 * 100.0) + ) + }, + calculate_votes_keep_each_user_timestamp: { - cfg: CfgProfile { - duration: Duration::from_secs(1), - pass_threshold: 50.0, - ..Default::default() - }, + cfg: threshold_cfg(50.0), reactions: vec![ + reaction(USER1, REACTION_IN_FAVOR), Reaction { - user: User { login: USER1.to_string() }, - content: REACTION_IN_FAVOR.to_string(), - created_at: TIMESTAMP.to_string(), - }, - Reaction { - user: User { login: USER2.to_string() }, - content: REACTION_AGAINST.to_string(), created_at: TIMESTAMP2.to_string(), + ..reaction(USER2, REACTION_AGAINST) }, Reaction { - user: User { login: USER5.to_string() }, - content: REACTION_IN_FAVOR.to_string(), created_at: TIMESTAMP3.to_string(), - } - ], - allowed_voters: vec![ - USER1.to_string(), - USER2.to_string() + ..reaction(USER5, REACTION_IN_FAVOR) + }, ], - expected_results: VoteResults { - passed: true, - in_favor_percentage: 50.0, - pass_threshold: 50.0, - in_favor: 1, - against: 1, - against_percentage: 50.0, - abstain: 0, - not_voted: 0, - binding: 2, - non_binding: 1, - votes: BTreeMap::from([ - ( - USER1.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(), - binding: true, - }, - ), - ( - USER2.to_string(), - UserVote { - vote_option: VoteOption::Against, - timestamp: OffsetDateTime::parse(TIMESTAMP2, &Rfc3339).unwrap(), - binding: true, - }, - ), - ( - USER5.to_string(), - UserVote { - vote_option: VoteOption::InFavor, - timestamp: OffsetDateTime::parse(TIMESTAMP3, &Rfc3339).unwrap(), - binding: false, - }, - ), - ]), - allowed_voters: 2, - pending_voters: vec![], + allowed_voters: users(2), + expected_results: { + let mut results = expected_results( + 50.0, + &[votes_cast(1, 1, 0), vec![(USER5.to_string(), VoteOption::InFavor)]].concat(), + &users(2), + (true, 50.0, 50.0) + ); + results.votes.get_mut(USER2).unwrap().timestamp = + OffsetDateTime::parse(TIMESTAMP2, &Rfc3339).unwrap(); + results.votes.get_mut(USER5).unwrap().timestamp = + OffsetDateTime::parse(TIMESTAMP3, &Rfc3339).unwrap(); + results } }, ); +#[tokio::test] +async fn calculate_strict_majority_recipe() { + // Setup cases (in favor, against, abstain, pass threshold, expected passed) + let cases = [ + (2, 0, 10, STRICT_MAJORITY_THRESHOLD, true), + (2, 2, 0, STRICT_MAJORITY_THRESHOLD, false), + (3, 2, 5, STRICT_MAJORITY_THRESHOLD, true), + (0, 1, 0, STRICT_MAJORITY_THRESHOLD, false), + (0, 0, 3, STRICT_MAJORITY_THRESHOLD, false), + // A tie passes with 50, so it cannot be used to require a strict majority + (2, 2, 0, 50.0, true), + ]; + + for (in_favor, against, abstain, pass_threshold, expected_passed) in cases { + // Calculate the results, leaving one allowed voter pending + let votes = votes_cast(in_favor, against, abstain); + let allowed_voters = users(in_favor + against + abstain + 1); + let cfg = votes_cast_cfg(pass_threshold, Some(true)); + let results = calculate_results(cfg, reactions(&votes), allowed_voters).await; + + // Check the vote passes only when expected + assert_eq!( + results.passed, expected_passed, + "votes: ({in_favor}, {against}, {abstain}), pass threshold: {pass_threshold}" + ); + } +} + +#[tokio::test] +async fn calculate_votes_cast_omitted_exclude_abstentions_equals_false() { + for (in_favor, against, abstain) in [(2, 1, 1), (1, 1, 0), (0, 0, 2)] { + // Calculate the results with the flag omitted and set to false + let votes = votes_cast(in_favor, against, abstain); + let mut results = vec![]; + for exclude_abstentions in [None, Some(false)] { + let cfg = votes_cast_cfg(50.0, exclude_abstentions); + results.push(calculate_results(cfg, reactions(&votes), users(5)).await); + } + + // Check both results are identical + assert_eq!( + results[0], results[1], + "votes: ({in_favor}, {against}, {abstain})" + ); + } +} + +#[test] +fn threshold_reached_strict_majority_limit() { + // Setup cases (in favor, against, expected result) + let cases = [(2500, 2499, true), (2501, 2500, false)]; + + for (in_favor, against, expected) in cases { + // Check the result over the votes cast excluding abstentions + assert_eq!( + threshold_reached(in_favor, STRICT_MAJORITY_THRESHOLD, in_favor + against), + expected, + "votes: ({in_favor}, {against})" + ); + } +} + +#[test] +fn vote_cfg_decode_stored_jsonb() { + // Setup cases (stored pass threshold base, expected pass threshold base) + let cases = [ + (None, None), + ( + Some(json!("allowed_voters")), + Some(PassThresholdBase::AllowedVoters), + ), + ( + Some(json!({"votes_cast": {}})), + Some(PassThresholdBase::VotesCast { + exclude_abstentions: None, + }), + ), + ( + Some(json!({"votes_cast": {"exclude_abstentions": false}})), + Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(false), + }), + ), + ( + Some(json!({"votes_cast": {"exclude_abstentions": true}})), + Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), + ), + ]; + + for (stored_base, expected_base) in cases { + // Setup JSONB value as stored in the vote cfg column (version byte + JSON) + let mut stored = json!({ + "duration": "5m", + "pass_threshold": 50.0, + "allowed_voters": {"users": [USER1]}, + "close_on_passing": true + }); + if let Some(stored_base) = &stored_base { + stored["pass_threshold_base"] = stored_base.clone(); + } + let mut raw = vec![1_u8]; + raw.extend(serde_json::to_vec(&stored).unwrap()); + + // Check it decodes into the expected profile, as done when reading votes + let Json(cfg) = as FromSql>::from_sql(&Type::JSONB, &raw).unwrap(); + assert_eq!( + cfg, + CfgProfile { + duration: Duration::from_mins(5), + pass_threshold: 50.0, + allowed_voters: Some(AllowedVoters { + users: Some(vec![USER1.to_string()]), + ..Default::default() + }), + close_on_passing: Some(true), + pass_threshold_base: expected_base, + ..Default::default() + }, + "stored: {stored_base:?}" + ); + } +} + #[test] fn vote_option_display() { // Check display strings @@ -980,6 +790,37 @@ fn vote_results_deserialize_stored_json() { ); } +#[test] +fn vote_results_passes_with_all_allowed_voters() { + // Setup cases (in favor, allowed voters, pass threshold, expected result) + let cases = [ + (0, 0, 50.0, false), + (1, 2, 50.0, true), + (1, 3, 50.0, false), + (29, 50, 58.0, true), + (28, 50, 58.0, false), + (2, 3, 66.67, false), + (3, 3, 100.0, true), + ]; + + for (in_favor, allowed_voters, pass_threshold, expected) in cases { + // Setup results + let results = VoteResults { + pass_threshold, + in_favor, + allowed_voters, + ..setup_test_vote_results() + }; + + // Check the result over all the allowed voters + assert_eq!( + results.passes_with_all_allowed_voters(), + expected, + "in favor: {in_favor}, allowed voters: {allowed_voters}, pass threshold: {pass_threshold}" + ); + } +} + #[test] fn vote_results_serialize() { // Check serialized vote results @@ -1008,3 +849,159 @@ fn vote_results_serialize() { }) ); } + +// Helpers. + +/// Calculate the results of a vote using the configuration, reactions and +/// allowed voters provided. +async fn calculate_results( + cfg: CfgProfile, + reactions: Vec, + allowed_voters: Vec, +) -> VoteResults { + // Setup GitHub expectations + let mut gh = MockGH::new(); + gh.expect_get_comment_reactions() + .with(eq(INST_ID), eq(OWNER), eq(REPO), eq(COMMENT_ID)) + .times(1) + .return_once(move |_, _, _, _| Box::pin(future::ready(Ok(reactions)))); + let expected_cfg = cfg.clone(); + gh.expect_get_allowed_voters() + .withf(move |inst_id, cfg, owner, repo, org| { + *inst_id == INST_ID + && *cfg == expected_cfg + && owner == OWNER + && repo == REPO + && *org == Some(ORG.to_string()).as_ref() + }) + .times(1) + .return_once(move |_, _, _, _, _| Box::pin(future::ready(Ok(allowed_voters)))); + + // Calculate vote results + let vote = Vote { + cfg, + ..setup_test_vote() + }; + calculate(Arc::new(gh), OWNER, REPO, &vote).await.unwrap() +} + +/// Build the expected results of a vote from the votes cast and allowed voters +/// provided, using the outcome given (passed, in favor and against percentages). +fn expected_results( + pass_threshold: f64, + votes_cast: &[(UserName, VoteOption)], + allowed_voters: &[UserName], + (passed, in_favor_percentage, against_percentage): (bool, f64, f64), +) -> VoteResults { + // Prepare votes, which are binding when cast by allowed voters (matched + // case insensitively) + let is_allowed_voter = + |user: &UserName| allowed_voters.iter().any(|voter| voter.eq_ignore_ascii_case(user)); + let timestamp = OffsetDateTime::parse(TIMESTAMP, &Rfc3339).unwrap(); + let votes: BTreeMap = votes_cast + .iter() + .map(|(user, vote_option)| { + let user_vote = UserVote { + vote_option: vote_option.clone(), + timestamp, + binding: is_allowed_voter(user), + }; + (user.clone(), user_vote) + }) + .collect(); + + // Prepare counts + let count_binding = |option: Option| { + votes + .values() + .filter(|v| v.binding && option.as_ref().is_none_or(|o| *o == v.vote_option)) + .count() as i64 + }; + let binding = count_binding(None); + let pending_voters: Vec = allowed_voters + .iter() + .filter(|voter| !votes.keys().any(|user| user.eq_ignore_ascii_case(voter))) + .cloned() + .collect(); + + VoteResults { + passed, + in_favor_percentage, + pass_threshold, + in_favor: count_binding(Some(VoteOption::InFavor)), + against: count_binding(Some(VoteOption::Against)), + against_percentage, + abstain: count_binding(Some(VoteOption::Abstain)), + not_voted: pending_voters.len() as i64, + binding, + non_binding: votes.len() as i64 - binding, + allowed_voters: allowed_voters.len() as i64, + votes, + pending_voters, + } +} + +/// Setup a reaction from the user provided with the content given. +fn reaction(user: &str, content: &str) -> Reaction { + Reaction { + user: User { + login: user.to_string(), + }, + content: content.to_string(), + created_at: TIMESTAMP.to_string(), + } +} + +/// Setup reactions for the votes provided. +fn reactions(votes: &[(UserName, VoteOption)]) -> Vec { + votes + .iter() + .map(|(user, vote_option)| { + let content = match vote_option { + VoteOption::InFavor => REACTION_IN_FAVOR, + VoteOption::Against => REACTION_AGAINST, + VoteOption::Abstain => REACTION_ABSTAIN, + }; + reaction(user, content) + }) + .collect() +} + +/// Setup a profile using the pass threshold provided and the default pass +/// threshold base. +fn threshold_cfg(pass_threshold: f64) -> CfgProfile { + CfgProfile { + duration: Duration::from_secs(1), + pass_threshold, + ..Default::default() + } +} + +/// Setup a list with the first users (`user1`, `user2`, ...). +fn users(count: usize) -> Vec { + (1..=count).map(|i| format!("user{i}")).collect() +} + +/// Setup the votes cast by the first users: in favor first, then against and +/// then abstain (i.e. `(2, 1, 0)` means `user1` and `user2` in favor and `user3` +/// against). +fn votes_cast(in_favor: usize, against: usize, abstain: usize) -> Vec<(UserName, VoteOption)> { + [ + (in_favor, VoteOption::InFavor), + (against, VoteOption::Against), + (abstain, VoteOption::Abstain), + ] + .into_iter() + .flat_map(|(count, vote_option)| std::iter::repeat_n(vote_option, count)) + .enumerate() + .map(|(i, vote_option)| (format!("user{}", i + 1), vote_option)) + .collect() +} + +/// Setup a profile using the votes cast as the pass threshold base. +fn votes_cast_cfg(pass_threshold: f64, exclude_abstentions: Option) -> CfgProfile { + CfgProfile { + pass_threshold_base: Some(PassThresholdBase::VotesCast { exclude_abstentions }), + ..threshold_cfg(pass_threshold) + } +} diff --git a/src/testdata/templates/vote-closed-announcement.golden b/src/testdata/templates/vote-closed-announcement.golden index feecb7e..fb207a2 100644 --- a/src/testdata/templates/vote-closed-announcement.golden +++ b/src/testdata/templates/vote-closed-announcement.golden @@ -4,7 +4,7 @@ The vote for "**Implement RFC-42** (**#123**)" is now closed. The vote **passed**! 馃帀 -`66.67%` of the users with binding vote were in favor and `0.00%` were against (passing threshold: `50%`). +In favor: `66.67%` 路 Against: `0.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-closed-failed.golden b/src/testdata/templates/vote-closed-failed.golden index 17ee44e..3c13475 100644 --- a/src/testdata/templates/vote-closed-failed.golden +++ b/src/testdata/templates/vote-closed-failed.golden @@ -2,7 +2,7 @@ The vote **did not pass**. -`40.00%` of the users with binding vote were in favor and `60.00%` were against (passing threshold: `50%`). +In favor: `40.00%` 路 Against: `60.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-closed-no-votes.golden b/src/testdata/templates/vote-closed-no-votes.golden index 3a15dea..c50601f 100644 --- a/src/testdata/templates/vote-closed-no-votes.golden +++ b/src/testdata/templates/vote-closed-no-votes.golden @@ -2,7 +2,7 @@ The vote **did not pass**. -`0.00%` of the users with binding vote were in favor and `0.00%` were against (passing threshold: `50%`). +In favor: `0.00%` 路 Against: `0.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-closed-non-binding-only.golden b/src/testdata/templates/vote-closed-non-binding-only.golden index 7e51523..328f233 100644 --- a/src/testdata/templates/vote-closed-non-binding-only.golden +++ b/src/testdata/templates/vote-closed-non-binding-only.golden @@ -2,7 +2,7 @@ The vote **did not pass**. -`0.00%` of the users with binding vote were in favor and `0.00%` were against (passing threshold: `50%`). +In favor: `0.00%` 路 Against: `0.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-closed-non-binding-truncated.golden b/src/testdata/templates/vote-closed-non-binding-truncated.golden index c09f54f..f76fc13 100644 --- a/src/testdata/templates/vote-closed-non-binding-truncated.golden +++ b/src/testdata/templates/vote-closed-non-binding-truncated.golden @@ -2,7 +2,7 @@ The vote **passed**! 馃帀 -`100.00%` of the users with binding vote were in favor and `0.00%` were against (passing threshold: `50%`). +In favor: `100.00%` 路 Against: `0.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-closed-passed.golden b/src/testdata/templates/vote-closed-passed.golden index ef17b3b..637e018 100644 --- a/src/testdata/templates/vote-closed-passed.golden +++ b/src/testdata/templates/vote-closed-passed.golden @@ -2,7 +2,7 @@ The vote **passed**! 馃帀 -`80.00%` of the users with binding vote were in favor and `20.00%` were against (passing threshold: `50%`). +In favor: `80.00%` 路 Against: `20.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-created-votes-cast-excluding-abstentions.golden b/src/testdata/templates/vote-created-votes-cast-excluding-abstentions.golden new file mode 100644 index 0000000..7bca372 --- /dev/null +++ b/src/testdata/templates/vote-created-votes-cast-excluding-abstentions.golden @@ -0,0 +1,19 @@ +## Vote created + +**@user** has called for a vote on `Test title` (#1). + +All repository collaborators have binding votes. + +Non-binding votes are also appreciated as a sign of support! + +## How to vote + +You can cast your vote by reacting to `this` comment. The following reactions are supported: + +| In favor | Against | Abstain | +| :------: | :-----: | :-----: | +| 馃憤 | 馃憥 | 馃憖 | + +*Please note that voting for multiple options is not allowed and those votes won't be counted.* + +The vote will be open for `1day`. It will pass if at least `50.01%` of the binding votes cast are `In favor 馃憤` (abstentions are recorded but not included in the percentage calculation). Once it's closed, results will be published here as a new comment. \ No newline at end of file diff --git a/src/testdata/templates/vote-created-votes-cast.golden b/src/testdata/templates/vote-created-votes-cast.golden new file mode 100644 index 0000000..8fe4e23 --- /dev/null +++ b/src/testdata/templates/vote-created-votes-cast.golden @@ -0,0 +1,19 @@ +## Vote created + +**@user** has called for a vote on `Test title` (#1). + +All repository collaborators have binding votes. + +Non-binding votes are also appreciated as a sign of support! + +## How to vote + +You can cast your vote by reacting to `this` comment. The following reactions are supported: + +| In favor | Against | Abstain | +| :------: | :-----: | :-----: | +| 馃憤 | 馃憥 | 馃憖 | + +*Please note that voting for multiple options is not allowed and those votes won't be counted.* + +The vote will be open for `1day`. It will pass if at least `60%` of the binding votes cast are `In favor 馃憤` (abstentions are included in the percentage calculation). Once it's closed, results will be published here as a new comment. \ No newline at end of file diff --git a/src/testdata/templates/vote-status-in-progress.golden b/src/testdata/templates/vote-status-in-progress.golden index 334b851..7ec9a1e 100644 --- a/src/testdata/templates/vote-status-in-progress.golden +++ b/src/testdata/templates/vote-status-in-progress.golden @@ -1,6 +1,6 @@ ## Vote status -So far `33.33%` of the users with binding vote are in favor and `0.00%` are against (passing threshold: `50%`). +In favor: `33.33%` 路 Against: `0.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/testdata/templates/vote-status-non-binding-truncated.golden b/src/testdata/templates/vote-status-non-binding-truncated.golden index 1096552..f09602e 100644 --- a/src/testdata/templates/vote-status-non-binding-truncated.golden +++ b/src/testdata/templates/vote-status-non-binding-truncated.golden @@ -1,6 +1,6 @@ ## Vote status -So far `100.00%` of the users with binding vote are in favor and `0.00%` are against (passing threshold: `50%`). +In favor: `100.00%` 路 Against: `0.00%` 路 Passing threshold: `50%` ### Summary diff --git a/src/tmpl.rs b/src/tmpl.rs index be88725..ab2ae92 100644 --- a/src/tmpl.rs +++ b/src/tmpl.rs @@ -7,7 +7,7 @@ use serde::Serialize; use time::{OffsetDateTime, format_description::well_known::Rfc3339}; use crate::{ - cfg_repo::CfgProfile, + cfg_repo::{CfgProfile, PassThresholdBase}, cmd::CreateVoteInput, github::{TeamSlug, UserName}, results::{Vote, VoteOption, VoteResults}, @@ -238,6 +238,8 @@ pub(crate) struct VoteCreated<'a> { issue_number: i64, duration: String, pass_threshold: f64, + pass_threshold_base_votes_cast: bool, + exclude_abstentions: bool, org: &'a str, teams: &'a [TeamSlug], users: &'a [UserName], @@ -263,12 +265,23 @@ impl<'a> VoteCreated<'a> { None => "", }; + // Prepare pass threshold base details + let (pass_threshold_base_votes_cast, exclude_abstentions) = + match cfg.pass_threshold_base.clone().unwrap_or_default() { + PassThresholdBase::AllowedVoters => (false, false), + PassThresholdBase::VotesCast { exclude_abstentions } => { + (true, exclude_abstentions.unwrap_or(false)) + } + }; + Self { creator: &input.created_by, issue_title: &input.issue_title, issue_number: input.issue_number, duration: humantime::format_duration(cfg.duration).to_string(), pass_threshold: cfg.pass_threshold, + pass_threshold_base_votes_cast, + exclude_abstentions, org, teams, users, diff --git a/src/tmpl/tests.rs b/src/tmpl/tests.rs index 12f4227..c20deb3 100644 --- a/src/tmpl/tests.rs +++ b/src/tmpl/tests.rs @@ -695,6 +695,63 @@ fn test_vote_created_users_only() { check_golden_file("vote-created-users-only", &output); } +#[test] +fn test_vote_created_votes_cast() { + // Setup input and configuration + let input = CreateVoteInput::new(None, &Event::Issue(setup_test_issue_event())); + let cfg = CfgProfile { + duration: std::time::Duration::from_hours(24), + pass_threshold: 60.0, + pass_threshold_base: Some(PassThresholdBase::VotesCast { + exclude_abstentions: None, + }), + ..Default::default() + }; + + // Render template and check the output + let output = VoteCreated::new(&input, &cfg).render().unwrap(); + check_golden_file("vote-created-votes-cast", &output); +} + +#[test] +fn test_vote_created_votes_cast_excluding_abstentions() { + // Setup input and configuration + let input = CreateVoteInput::new(None, &Event::Issue(setup_test_issue_event())); + let cfg = CfgProfile { + duration: std::time::Duration::from_hours(24), + pass_threshold: 50.01, + pass_threshold_base: Some(PassThresholdBase::VotesCast { + exclude_abstentions: Some(true), + }), + ..Default::default() + }; + + // Render template and check the output + let output = VoteCreated::new(&input, &cfg).render().unwrap(); + check_golden_file("vote-created-votes-cast-excluding-abstentions", &output); +} + +#[test] +fn test_vote_created_votes_cast_omitted_exclude_abstentions_equals_false() { + // Setup input and render the template with the flag omitted and set to false + let input = CreateVoteInput::new(None, &Event::Issue(setup_test_issue_event())); + let outputs: Vec = [None, Some(false)] + .into_iter() + .map(|exclude_abstentions| { + let cfg = CfgProfile { + duration: std::time::Duration::from_hours(24), + pass_threshold: 60.0, + pass_threshold_base: Some(PassThresholdBase::VotesCast { exclude_abstentions }), + ..Default::default() + }; + VoteCreated::new(&input, &cfg).render().unwrap() + }) + .collect(); + + // Check both outputs are identical + assert_eq!(outputs[0], outputs[1]); +} + #[test] fn test_vote_created_with_teams_and_users() { // Setup input and configuration diff --git a/templates/vote-closed.md b/templates/vote-closed.md index d38fdec..673a4f1 100644 --- a/templates/vote-closed.md +++ b/templates/vote-closed.md @@ -3,7 +3,7 @@ The vote {% if results.passed %}**passed**! 馃帀{% else %}**did not pass**.{% endif %} -`{{ "{:.2}"|format(results.in_favor_percentage) }}%` of the users with binding vote were in favor and `{{ "{:.2}"|format(results.against_percentage) }}%` were against (passing threshold: `{{ results.pass_threshold }}%`). +In favor: `{{ "{:.2}"|format(results.in_favor_percentage) }}%` 路 Against: `{{ "{:.2}"|format(results.against_percentage) }}%` 路 Passing threshold: `{{ results.pass_threshold }}%` ### Summary diff --git a/templates/vote-created.md b/templates/vote-created.md index ba556ac..911f7db 100644 --- a/templates/vote-created.md +++ b/templates/vote-created.md @@ -39,4 +39,10 @@ You can cast your vote by reacting to `this` comment. The following reactions ar *Please note that voting for multiple options is not allowed and those votes won't be counted.* -The vote will be open for `{{ duration }}`. It will pass if at least `{{ pass_threshold }}%` of the users with binding votes vote `In favor 馃憤`. Once it's closed, results will be published here as a new comment. +The vote will be open for `{{ duration }}`. +{%- if pass_threshold_base_votes_cast %} It will pass if at least `{{ pass_threshold }}%` of the binding votes cast are `In favor 馃憤` + {%- if exclude_abstentions %} (abstentions are recorded but not included in the percentage calculation). + {%- else %} (abstentions are included in the percentage calculation). + {%- endif %} +{%- else %} It will pass if at least `{{ pass_threshold }}%` of the users with binding votes vote `In favor 馃憤`. +{%- endif %} Once it's closed, results will be published here as a new comment. diff --git a/templates/vote-status.md b/templates/vote-status.md index b5bac0b..95c6b51 100644 --- a/templates/vote-status.md +++ b/templates/vote-status.md @@ -1,6 +1,6 @@ ## Vote status -So far `{{ "{:.2}"|format(results.in_favor_percentage) }}%` of the users with binding vote are in favor and `{{ "{:.2}"|format(results.against_percentage) }}%` are against (passing threshold: `{{ results.pass_threshold }}%`). +In favor: `{{ "{:.2}"|format(results.in_favor_percentage) }}%` 路 Against: `{{ "{:.2}"|format(results.against_percentage) }}%` 路 Passing threshold: `{{ results.pass_threshold }}%` ### Summary