Tests and bugfix

contract/src/lib.rs

@@ -15,6 +15,8 @@ use shared_types::{
 use types::{Account, Organization, VersionedAccount, VersionedOrganization};
 
 pub mod storage;
+#[cfg(test)]
+mod tests;
 pub mod types;
 pub mod views;
 
@@ -118,7 +120,6 @@ impl Contract {
         pr_number: u64,
         started_at: Timestamp,
         override_exclude: bool,
-        comment_id: u64,
     ) {
         self.assert_sloth();
         self.assert_organization_allowed(&organization, &repo);
@@ -140,7 +141,7 @@ impl Contract {
         }
 
         // Create user if it doesn't exist
-        let pr = PR::new(organization, repo, pr_number, user, started_at, comment_id);
+        let pr = PR::new(organization, repo, pr_number, user, started_at);
 
         self.apply_to_periods(&pr.author, started_at, |data| data.pr_opened());
         self.prs.insert(pr_id, VersionedPR::V1(pr));
@@ -291,20 +292,22 @@ impl Contract {
                 .unwrap_or_default();
 
             let streak = self.verify_previous_streak(&streak, &streak_data, user, current_time);
+            env::log_str(&format!("Streak: {achieved:?} {streak} {streak_data:?}"));
 
             match (
                 streak_data.latest_time_string == current_time_string,
                 achieved,
             ) {
                 // We haven't counted current period yet
                 (false, true) => {
-                    streak_data.amount += streak + 1;
+                    streak_data.amount = streak + 1;
                     streak_data.latest_time_string = current_time_string;
+                    streak_data.best = streak_data.best.max(streak_data.amount);
                 }
                 // We have counted current period, but now user is losing the streak
                 (true, false) => {
                     // We have update the streak data previously with success, so we need to revert it
-                    streak_data.amount = streak - 1;
+                    streak_data.amount = streak.max(1) - 1;
                 }
                 // If both are false, then user hasn't achieved the streak and we don't need to do anything
                 // If both are true, then user has already achieved the streak and we don't need to do anything

contract/src/tests.rs

@@ -0,0 +1,205 @@
+use near_sdk::{test_utils::VMContextBuilder, testing_env, AccountId, VMContext};
+use shared_types::SCORE_TIMEOUT_IN_NANOSECONDS;
+
+use super::*;
+
+pub fn github_handle(id: u8) -> GithubHandle {
+    format!("name-{id}")
+}
+
+pub fn admin() -> AccountId {
+    "admin.near".parse().unwrap()
+}
+
+pub fn pr_id_str(pr_id: u64) -> String {
+    format!("NEAR-DevHub/devbot/{pr_id}")
+}
+
+pub struct ContractExt {
+    pub contract: Contract,
+    pub context: VMContext,
+}
+
+impl ContractExt {
+    pub fn new() -> Self {
+        let mut context = VMContextBuilder::new().build();
+        context.predecessor_account_id = admin();
+        testing_env!(context.clone());
+
+        let contract = Contract::new(admin());
+
+        Self { contract, context }
+    }
+
+    pub fn include_sloth_common_repo(&mut self, id: u8, pr_id: u64, started_at: u64) {
+        self.include_sloth_with_org("NEAR-DevHub", id, pr_id, started_at);
+    }
+
+    pub fn include_sloth_with_org(&mut self, org: &str, id: u8, pr_id: u64, started_at: u64) {
+        let handle = github_handle(id);
+        self.contract.sloth_include(
+            org.to_owned(),
+            "devbot".to_string(),
+            handle,
+            pr_id,
+            started_at,
+            true,
+        );
+    }
+
+    pub fn score(&mut self, pr_id: u64, id: u8, score: u32) {
+        self.contract
+            .sloth_scored(pr_id_str(pr_id), github_handle(id), score);
+    }
+
+    pub fn merge(&mut self, pr_id: u64, merged_at: u64) {
+        self.contract.sloth_merged(pr_id_str(pr_id), merged_at);
+    }
+
+    pub fn exclude(&mut self, pr_id: u64) {
+        self.contract.sloth_exclude(pr_id_str(pr_id));
+    }
+
+    pub fn finalize(&mut self, id: u64) {
+        self.contract.sloth_finalize(pr_id_str(id))
+    }
+}
+
+#[test]
+fn success_flow() {
+    let mut contract = ContractExt::new();
+
+    contract.include_sloth_common_repo(0, 0, 0);
+    assert_eq!(contract.contract.unmerged_prs(0, 50).len(), 1);
+
+    contract.score(0, 0, 13);
+
+    contract.merge(0, 10);
+    assert_eq!(contract.contract.unmerged_prs(0, 50).len(), 0);
+    contract.score(0, 1, 8);
+
+    contract.context.block_timestamp = 11;
+    testing_env!(contract.context.clone());
+    assert_eq!(contract.contract.unfinalized_prs(0, 50).len(), 0);
+
+    contract.context.block_timestamp = SCORE_TIMEOUT_IN_NANOSECONDS + 11;
+    testing_env!(contract.context.clone());
+    assert_eq!(contract.contract.unfinalized_prs(0, 50).len(), 1);
+
+    contract.finalize(0);
+
+    assert_eq!(contract.contract.unfinalized_prs(0, 50).len(), 0);
+    let user = contract.contract.user(&github_handle(0), None).unwrap();
+    assert_eq!(user.period_data.total_score, 10);
+    assert_eq!(user.period_data.executed_prs, 1);
+    assert_eq!(user.period_data.prs_opened, 1);
+    assert_eq!(user.period_data.prs_merged, 1);
+    assert_eq!(user.streaks[0].1.amount, 1);
+    assert_eq!(user.streaks[1].1.amount, 1);
+}
+
+#[test]
+fn streak_calculation() {
+    let mut contract = ContractExt::new();
+
+    let mut current_time = 0;
+    for i in 0..12 {
+        contract.include_sloth_common_repo(0, i, current_time);
+        contract.score(i, 1, 10);
+        contract.merge(i, current_time + 1);
+        current_time += SCORE_TIMEOUT_IN_NANOSECONDS * 7 + 1;
+        contract.context.block_timestamp = current_time;
+        testing_env!(contract.context.clone());
+        contract.finalize(i);
+    }
+
+    let user = contract.contract.user(&github_handle(0), None).unwrap();
+    // 12 weeks streak with opened PR
+    assert_eq!(user.streaks[0].1.amount, 12);
+    // 3 months streak with 8+ scopre
+    assert_eq!(user.streaks[1].1.amount, 3);
+
+    assert_eq!(user.period_data.total_score, 12 * 10);
+    assert_eq!(user.period_data.executed_prs, 12);
+}
+
+#[test]
+fn streak_crashed_in_middle() {
+    let mut contract = ContractExt::new();
+
+    let mut current_time = 0;
+    for i in 0..8 {
+        contract.context.block_timestamp = current_time;
+        testing_env!(contract.context.clone());
+        contract.include_sloth_common_repo(0, i, current_time);
+        contract.score(i, 1, 10);
+        contract.merge(i, current_time + 1);
+        current_time += SCORE_TIMEOUT_IN_NANOSECONDS * 7 + 1;
+        contract.context.block_timestamp = current_time;
+        testing_env!(contract.context.clone());
+        contract.finalize(i);
+    }
+
+    let user = contract.contract.user(&github_handle(0), None).unwrap();
+    assert_eq!(user.streaks[0].1.amount, 8);
+    assert_eq!(user.streaks[1].1.amount, 2);
+    assert_eq!(user.period_data.total_score, 8 * 10);
+    assert_eq!(user.period_data.executed_prs, 8);
+
+    // 5 weeks skipped to crush both streaks
+    current_time += SCORE_TIMEOUT_IN_NANOSECONDS * 7 * 5 + 1;
+
+    // we update streak reactively, it means that we will not update streaks until the next PR
+    // somehow we need to update streaks if user stops doing something
+
+    for i in 8..12 {
+        contract.context.block_timestamp = current_time;
+        testing_env!(contract.context.clone());
+        contract.include_sloth_common_repo(0, i, current_time);
+        contract.score(i, 1, 10);
+        contract.merge(i, current_time + 1);
+        current_time += SCORE_TIMEOUT_IN_NANOSECONDS * 7 + 1;
+        contract.context.block_timestamp = current_time;
+        testing_env!(contract.context.clone());
+        contract.finalize(i);
+    }
+
+    let user = contract.contract.user(&github_handle(0), None).unwrap();
+    assert_eq!(user.streaks[0].1.amount, 4);
+    assert_eq!(user.streaks[0].1.best, 8);
+    assert_eq!(user.streaks[1].1.amount, 1);
+    assert_eq!(user.streaks[1].1.best, 2);
+
+    assert_eq!(user.period_data.total_score, 12 * 10);
+    assert_eq!(user.period_data.executed_prs, 12);
+}
+
+#[test]
+#[should_panic(expected = "PR is not started or already executed")]
+fn exclude_pr() {
+    let mut contract = ContractExt::new();
+
+    contract.include_sloth_common_repo(0, 0, 0);
+
+    contract.exclude(0);
+
+    contract.score(0, 1, 13);
+}
+
+#[test]
+#[should_panic(expected = "Organization is not allowlisted")]
+fn notallowlisted_org() {
+    let mut contract = ContractExt::new();
+
+    contract.include_sloth_with_org("blahh", 0, 0, 0);
+
+    contract.score(0, 1, 13);
+}
+
+#[test]
+#[should_panic(expected = "PR is not started or already executed")]
+fn not_started_pr() {
+    let mut contract = ContractExt::new();
+
+    contract.score(0, 1, 13);
+}

contract/src/views.rs

@@ -24,7 +24,6 @@ impl Contract {
             executed: executed_pr.is_some(),
             excluded: self.excluded_prs.contains(&pr_id),
             votes: pr.as_ref().map(|pr| pr.score.clone()).unwrap_or_default(),
-            comment_id: pr.map(|pr| pr.comment_id).unwrap_or_default(),
         }
     }
 

server/src/api/github/mod.rs

@@ -4,7 +4,7 @@ use octocrab::models::{
 };
 use tracing::{error, info, instrument};
 
-use crate::events::commands::Command;
+use crate::events::{commands::Command, Event, EventType};
 
 mod types;
 pub use types::*;
@@ -29,7 +29,7 @@ impl GithubClient {
     }
 
     #[instrument(skip(self))]
-    pub async fn get_events(&self) -> anyhow::Result<Vec<(Command, NotificationId)>> {
+    pub async fn get_events(&self) -> anyhow::Result<Vec<Event>> {
         let page = self
             .octocrab
             .activity()
@@ -100,6 +100,10 @@ impl GithubClient {
                     return None;
                 }
             };
+            let comment_id = comments
+                .iter()
+                .find(|c| c.user.login == self.user_handle)
+                .map(|c| c.id);
 
             let mut results = Vec::new();
             let mut found_us = false;
@@ -112,14 +116,22 @@ impl GithubClient {
                 if let Some(command) =
                     Command::parse_command(&self.user_handle, &pr_metadata, &comment)
                 {
-                    results.push((command, event.id));
+                    results.push(Event {
+                        event: EventType::Command(command),
+                        notification_id: Some(event.id),
+                        comment_id,
+                    });
                 }
             }
 
             // We haven
             if results.is_empty() && !found_us {
                 if let Some(command) = Command::parse_body(&self.user_handle, &pr_metadata) {
-                    results.push((command, event.id));
+                    results.push(Event {
+                        event: EventType::Command(command),
+                        notification_id: Some(event.id),
+                        comment_id,
+                    });
                 }
             }
 
@@ -248,4 +260,49 @@ impl GithubClient {
             .await?;
         Ok(())
     }
+
+    #[instrument(skip(self, comment_id))]
+    pub async fn delete_comment(
+        &self,
+        owner: &str,
+        repo: &str,
+        comment_id: CommentId,
+    ) -> anyhow::Result<()> {
+        self.octocrab
+            .issues(owner, repo)
+            .delete_comment(comment_id)
+            .await?;
+        Ok(())
+    }
+
+    #[instrument(skip(self,))]
+    pub async fn get_comment_id(
+        &self,
+        owner: &str,
+        repo: &str,
+        pr_number: u64,
+    ) -> anyhow::Result<Option<CommentId>> {
+        let mut page = self
+            .octocrab
+            .issues(owner, repo)
+            .list_comments(pr_number)
+            .per_page(100)
+            .send()
+            .await?;
+
+        loop {
+            let items = page.take_items();
+            for comment in items {
+                if comment.user.login == self.user_handle {
+                    return Ok(Some(comment.id));
+                }
+            }
+
+            if let Some(next) = self.octocrab.get_page(&page.next).await? {
+                page = next;
+            } else {
+                return Ok(None);
+            }
+        }
+    }
 }

server/src/api/near.rs

@@ -25,20 +25,14 @@ impl NearClient {
     }
 
     #[instrument(skip(self, pr), fields(pr = pr.full_id))]
-    pub async fn send_start(
-        &self,
-        pr: &PrMetadata,
-        is_maintainer: bool,
-        comment_id: u64,
-    ) -> anyhow::Result<()> {
+    pub async fn send_start(&self, pr: &PrMetadata, is_maintainer: bool) -> anyhow::Result<()> {
         let args = json!({
             "organization": pr.owner,
             "repo": pr.repo,
             "pr_number": pr.number,
             "user": pr.author.login,
             "started_at": pr.started.timestamp_nanos_opt().unwrap_or(0),
             "override_exclude": is_maintainer,
-            "comment_id": comment_id,
         });
 
         self.contract