github, slack: support pr review updates

Author: dombong

lib/action.ml

@@ -79,7 +79,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
 
        in both cases, since pull_request_review_comment is already handled by another type of event, information in the pull_request_review payload
        does not provide any insightful information and will thus be ignored. *)
-    | Submitted, _, _ -> partition_label cfg n.pull_request.labels
+    | (Submitted | Edited), _, _ -> partition_label cfg n.pull_request.labels
     | _ -> []
 
   let partition_commit (cfg : Config_t.config) files =
@@ -184,7 +184,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
     | Github.Push n ->
       partition_push cfg n |> List.map ~f:(fun (channel, n) -> generate_push_notification n channel) |> Lwt.return
     | Pull_request n -> partition_pr cfg n |> List.map ~f:(generate_pull_request_notification n) |> Lwt.return
-    | PR_review n -> partition_pr_review cfg n |> List.map ~f:(generate_pr_review_notification n) |> Lwt.return
+    | PR_review n -> partition_pr_review cfg n |> List.map ~f:(generate_pr_review_notification ctx n) |> Lwt.return
     | PR_review_comment n ->
       partition_pr_review_comment cfg n |> List.map ~f:(generate_pr_review_comment_notification ctx n) |> Lwt.return
     | Issue n -> partition_issue cfg n |> List.map ~f:(generate_issue_notification n) |> Lwt.return
@@ -206,6 +206,7 @@ module Action (Github_api : Api.Github) (Slack_api : Api.Slack) = struct
       | Github.PR_review_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message
       | Issue_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message
       | Commit_comment n -> State.add_comment_map ctx.state n.repository.url n.comment.id message
+      | PR_review n -> State.add_review_map ctx.state n.repository.url n.review.id message
       | _ -> Lwt.return_unit
     in
     let notify = function

lib/github.atd

@@ -185,6 +185,7 @@ type pr_notification = {
 
 type review = {
   ?body: string nullable;
+  id: int;
   html_url: string;
   state: string;
 }

lib/slack.ml

@@ -76,12 +76,12 @@ let generate_pull_request_notification notification channel =
       blocks = None;
     }
 
-let generate_pr_review_notification notification channel =
+let generate_pr_review_notification (ctx : Context.t) notification channel =
   let { action; sender; pull_request; review; repository } = notification in
   let ({ number; title; html_url; _ } : pull_request) = pull_request in
   let action_str =
     match action with
-    | Submitted ->
+    | Submitted | Edited ->
       ( match review.state with
       | "commented" -> "commented on"
       | "approved" -> "approved"
@@ -98,22 +98,26 @@ let generate_pr_review_notification notification channel =
     sprintf "<%s|[%s]> *%s* <%s|%s> #%d %s" repository.url repository.full_name sender.login review.html_url action_str
       number (pp_link ~url:html_url title)
   in
-  New_message
-    {
-      channel;
-      text = Some summary;
-      attachments =
-        Some
-          [
-            {
-              empty_attachments with
-              mrkdwn_in = Some [ "text" ];
-              color = Some "#ccc";
-              text = mrkdwn_of_markdown_opt review.body;
-            };
-          ];
-      blocks = None;
-    }
+  let attachments =
+    Some
+      [
+        {
+          empty_attachments with
+          mrkdwn_in = Some [ "text" ];
+          color = Some "#ccc";
+          text = mrkdwn_of_markdown_opt review.body;
+        };
+      ]
+  in
+  match action with
+  | Submitted -> New_message { channel; text = Some summary; attachments; blocks = None }
+  | Edited ->
+    ( match State.get_review_map ctx.state repository.url review.id with
+    | Some ({ channel; ts } : post_message_res) ->
+      Update_message { channel; ts; text = Some summary; attachments; blocks = None }
+    | None -> invalid_arg (sprintf "could not find comment %d in %s" review.id repository.url)
+    )
+  | _ -> invalid_arg "impossible"
 
 let generate_pr_review_comment_notification (ctx : Context.t) notification channel =
   let { action; pull_request; sender; comment; repository } = notification in

lib/state.atd

@@ -11,12 +11,13 @@ type branch_statuses = status_state map_as_object
 type pipeline_statuses = branch_statuses map_as_object
 
 type post_message_res <ocaml from="Slack"> = abstract
-type slack_comment_map = post_message_res map_as_object
+type post_msg_map = post_message_res map_as_object
 
 (* The runtime state of a given GitHub repository *)
 type repo_state = {
   pipeline_statuses <ocaml mutable>: pipeline_statuses;
-  comments_map <ocaml mutable>: slack_comment_map;
+  comments_map <ocaml mutable>: post_msg_map;
+  reviews_map <ocaml mutable>: post_msg_map;
 }
 
 (* The serializable runtime state of the bot *)

lib/state.ml

@@ -7,7 +7,8 @@ type t = {
   lock : Lwt_mutex.t;  (** protect access to mutable string map `pipeline_statuses` *)
 }
 
-let empty_repo_state () : State_t.repo_state = { pipeline_statuses = StringMap.empty; comments_map = StringMap.empty }
+let empty_repo_state () : State_t.repo_state =
+  { pipeline_statuses = StringMap.empty; comments_map = StringMap.empty; reviews_map = StringMap.empty }
 
 let empty () : t =
   let state = State_t.{ repos = Stringtbl.empty (); bot_user_id = None } in
@@ -44,6 +45,16 @@ let get_comment_map { state; _ } repo_url comment_id =
   let repo_state = find_or_add_repo' state repo_url in
   Map.find repo_state.comments_map (Int.to_string comment_id)
 
+let add_review_map { state; lock } repo_url review_id post_message_res =
+  Lwt_mutex.with_lock lock @@ fun () ->
+  let repo_state = find_or_add_repo' state repo_url in
+  repo_state.reviews_map <- Map.set repo_state.reviews_map ~key:(Int.to_string review_id) ~data:post_message_res;
+  Lwt.return_unit
+
+let get_review_map { state; _ } repo_url review_id =
+  let repo_state = find_or_add_repo' state repo_url in
+  Map.find repo_state.reviews_map (Int.to_string review_id)
+
 let set_bot_user_id { state; _ } user_id = state.State_t.bot_user_id <- Some user_id
 let get_bot_user_id { state; _ } = state.State_t.bot_user_id
 let log = Log.from "state"

mock_states/issue_comment.edited.json

@@ -13,5 +13,11 @@
       "channel": "some channel",
       "ts": "some ts"
     }
+  },
+  "reviews_map": {
+    "0": {
+      "channel": "some channel",
+      "ts": "some ts"
+    }
   }
 }

mock_states/pull_request_review.edited.json

@@ -0,0 +1,23 @@
+
+{
+  "pipeline_statuses": {
+    "default": {
+      "master": "failure"
+    },
+    "buildkite/pipeline2": {
+      "master": "success"
+    }
+  },
+  "comments_map": {
+    "0": {
+      "channel": "some channel",
+      "ts": "some ts"
+    }
+  },
+  "reviews_map": {
+    "0": {
+      "channel": "some channel",
+      "ts": "some ts"
+    }
+  }
+}

mock_states/pull_request_review_comment.edited.json

@@ -13,5 +13,11 @@
       "channel": "some channel",
       "ts": "some ts"
     }
+  },
+  "reviews_map": {
+    "0": {
+      "channel": "some channel",
+      "ts": "some ts"
+    }
   }
 }

mock_states/status.state_hide_success_test.json

@@ -13,5 +13,11 @@
       "channel": "some channel",
       "ts": "some ts"
     }
+  },
+  "reviews_map": {
+    "0": {
+      "channel": "some channel",
+      "ts": "some ts"
+    }
   }
 }

mock_states/status.state_hide_success_test_disallowed_pipeline.json

@@ -9,5 +9,11 @@
       "channel": "some channel",
       "ts": "some ts"
     }
+  },
+  "reviews_map": {
+    "0": {
+      "channel": "some channel",
+      "ts": "some ts"
+    }
   }
 }