Cargo.toml

@@ -3,8 +3,7 @@ resolver = "2"
 members = ["crates/*"]
 
 [workspace.package]
-version = "0.10.0" # Update git-next-* under workspace.dependencies
-
+version = "0.10.0"
 edition = "2021"
 license = "MIT"
 repository = "https://git.kemitix.net/kemitix/git-next"

crates/repo-actor/src/handlers/webhook_notification.rs

@@ -1,27 +1,41 @@
 //
 use actix::prelude::*;
 
-use crate::{self as actor, messages::WebhookNotification, RepoActorLog};
-use git_next_config::WebhookAuth;
-use git_next_config::{
-    webhook::{push::Branch, Push},
-    BranchName,
-};
-use git_next_git::{self as git, Commit, ForgeLike};
+use crate::{self as actor, messages::WebhookNotification};
+use git_next_config as config;
+use git_next_git as git;
 
 use tracing::{info, warn};
 
 impl Handler<WebhookNotification> for actor::RepoActor {
     type Result = ();
 
+    #[allow(clippy::cognitive_complexity)] // TODO: (#49) reduce complexity
     #[tracing::instrument(name = "RepoActor::WebhookMessage", skip_all, fields(token = %self.message_token, repo = %self.repo_details))]
     fn handle(&mut self, msg: WebhookNotification, ctx: &mut Self::Context) -> Self::Result {
+        let Some(expected_authorization) = &self.webhook_auth else {
+            actor::logger(&self.log, "server has no auth token");
+            warn!("Don't know what authorization to expect");
+            return;
+        };
         let Some(config) = &self.repo_details.repo_config else {
             actor::logger(&self.log, "server has no repo config");
             warn!("No repo config");
             return;
         };
-        if validate_notification(&msg, &self.webhook_auth, &*self.forge, &self.log).is_err() {
+        if !self
+            .forge
+            .is_message_authorised(&msg, expected_authorization)
+        {
+            actor::logger(&self.log, "message authorisation is invalid");
+            warn!(
+                "Invalid authorization - expected {}",
+                expected_authorization
+            );
+            return;
+        }
+        if self.forge.should_ignore_message(&msg) {
+            actor::logger(&self.log, "forge sent ignorable message");
             return;
         }
         let body = msg.body();
@@ -40,41 +54,47 @@ impl Handler<WebhookNotification> for actor::RepoActor {
                     );
                     return;
                 }
-                Some(Branch::Main) => {
-                    if handle_push(
-                        push,
-                        config.branches().main(),
-                        &mut self.last_main_commit,
-                        &self.log,
-                    )
-                    .is_err()
-                    {
+                Some(config::webhook::push::Branch::Main) => {
+                    actor::logger(&self.log, "message is for main branch");
+                    let commit = git::Commit::from(push);
+                    if self.last_main_commit.as_ref() == Some(&commit) {
+                        actor::logger(&self.log, "not a new commit on main");
+                        info!(
+                            branch = %config.branches().main(),
+                            %commit,
+                            "Ignoring - already aware of branch at commit",
+                        );
                         return;
-                    };
                     }
-                Some(Branch::Next) => {
-                    if handle_push(
-                        push,
-                        config.branches().next(),
-                        &mut self.last_next_commit,
-                        &self.log,
-                    )
-                    .is_err()
-                    {
-                        return;
-                    };
+                    self.last_main_commit.replace(commit);
                 }
-                Some(Branch::Dev) => {
-                    if handle_push(
-                        push,
-                        config.branches().dev(),
-                        &mut self.last_dev_commit,
-                        &self.log,
-                    )
-                    .is_err()
-                    {
+                Some(config::webhook::push::Branch::Next) => {
+                    actor::logger(&self.log, "message is for next branch");
+                    let commit = git::Commit::from(push);
+                    if self.last_next_commit.as_ref() == Some(&commit) {
+                        actor::logger(&self.log, "not a new commit on next");
+                        info!(
+                            branch = %config.branches().next(),
+                            %commit,
+                            "Ignoring - already aware of branch at commit",
+                        );
                         return;
-                    };
+                    }
+                    self.last_next_commit.replace(commit);
+                }
+                Some(config::webhook::push::Branch::Dev) => {
+                    actor::logger(&self.log, "message is for dev branch");
+                    let commit = git::Commit::from(push);
+                    if self.last_dev_commit.as_ref() == Some(&commit) {
+                        actor::logger(&self.log, "not a new commit on dev");
+                        info!(
+                            branch = %config.branches().dev(),
+                            %commit,
+                            "Ignoring - already aware of branch at commit",
+                        );
+                        return;
+                    }
+                    self.last_dev_commit.replace(commit);
                 }
             },
         }
@@ -90,51 +110,3 @@ impl Handler<WebhookNotification> for actor::RepoActor {
         );
     }
 }
-
-fn validate_notification(
-    msg: &WebhookNotification,
-    webhook_auth: &Option<WebhookAuth>,
-    forge: &dyn ForgeLike,
-    log: &Option<RepoActorLog>,
-) -> Result<(), ()> {
-    let Some(expected_authorization) = webhook_auth else {
-        actor::logger(log, "server has no auth token");
-        warn!("Don't know what authorization to expect");
-        return Err(());
-    };
-
-    if !forge.is_message_authorised(msg, expected_authorization) {
-        actor::logger(log, "message authorisation is invalid");
-        warn!(
-            "Invalid authorization - expected {}",
-            expected_authorization
-        );
-        return Err(());
-    }
-    if forge.should_ignore_message(msg) {
-        actor::logger(log, "forge sent ignorable message");
-        return Err(());
-    }
-    Ok(())
-}
-
-fn handle_push(
-    push: Push,
-    branch: BranchName,
-    last_commit: &mut Option<Commit>,
-    log: &Option<RepoActorLog>,
-) -> Result<(), ()> {
-    actor::logger(log, "message is for dev branch");
-    let commit = git::Commit::from(push);
-    if last_commit.as_ref() == Some(&commit) {
-        actor::logger(log, format!("not a new commit on {branch}"));
-        info!(
-            %branch ,
-            %commit,
-            "Ignoring - already aware of branch at commit",
-        );
-        return Err(());
-    }
-    last_commit.replace(commit);
-    Ok(())
-}

crates/repo-actor/src/tests/handlers/webhook_notification.rs

@@ -252,9 +252,8 @@ async fn when_message_is_push_already_seen_commit_to_main() -> TestResult {
     let forge_notification = ForgeNotification::new(forge_alias, repo_alias, headers, body);
     let repository_factory = MockRepositoryFactory::new();
     let commit = given::a_commit();
-    let main = repo_config.branches().main();
     let push = given::a_push()
-        .with_branch(main.clone())
+        .with_branch(repo_config.branches().main())
         .with_sha(commit.sha().to_string())
         .with_message(commit.message().to_string());
     let mut forge = given::a_forge();
@@ -284,7 +283,7 @@ async fn when_message_is_push_already_seen_commit_to_main() -> TestResult {
 
     //then
     log.no_message_contains("send")?;
-    log.require_message_containing(format!("not a new commit on {main}"))?;
+    log.require_message_containing("not a new commit on main")?;
     Ok(())
 }
 
@@ -301,9 +300,8 @@ async fn when_message_is_push_already_seen_commit_to_next() -> TestResult {
     let forge_notification = ForgeNotification::new(forge_alias, repo_alias, headers, body);
     let repository_factory = MockRepositoryFactory::new();
     let commit = given::a_commit();
-    let next = repo_config.branches().next();
     let push = given::a_push()
-        .with_branch(next.clone())
+        .with_branch(repo_config.branches().next())
         .with_sha(commit.sha().to_string())
         .with_message(commit.message().to_string());
     let mut forge = given::a_forge();
@@ -333,7 +331,7 @@ async fn when_message_is_push_already_seen_commit_to_next() -> TestResult {
 
     //then
     log.no_message_contains("send")?;
-    log.require_message_containing(format!("not a new commit on {next}"))?;
+    log.require_message_containing("not a new commit on next")?;
     Ok(())
 }
 
@@ -350,9 +348,8 @@ async fn when_message_is_push_already_seen_commit_to_dev() -> TestResult {
     let forge_notification = ForgeNotification::new(forge_alias, repo_alias, headers, body);
     let repository_factory = MockRepositoryFactory::new();
     let commit = given::a_commit();
-    let dev = repo_config.branches().dev();
     let push = given::a_push()
-        .with_branch(dev.clone())
+        .with_branch(repo_config.branches().dev())
         .with_sha(commit.sha().to_string())
         .with_message(commit.message().to_string());
     let mut forge = given::a_forge();
@@ -382,7 +379,7 @@ async fn when_message_is_push_already_seen_commit_to_dev() -> TestResult {
 
     //then
     log.no_message_contains("send")?;
-    log.require_message_containing(format!("not a new commit on {dev}"))?;
+    log.require_message_containing("not a new commit on dev")?;
     Ok(())
 }