Support updating corresponding comment message in Slack #132

sewenthy · 2023-03-13T08:57:59Z

Description of the task

Make monorobot remember each comment it sends to slack and then update the comment if the comment was edited in github.

Supports edits in:

PR review comment
PR review
Issue comment

Commit comment edits do not send an edited event.

All PRs are numbered uniquely with issue numbers so tracking these events as issues. Clean up from state when PR is closed (not supporting comment edits after PR is closed/merged).

How to test

monorobot $ make test

References

existing issue: Update comments once edited #39, Support stateful updates of PR/Issues statuses #129

yasunariw

Please check the unexpected test output.

yasunariw · 2023-04-07T04:36:27Z

lib/slack.ml

+    ( match State.get_review_msg ctx.state repository.url ~issue_num:number 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 review %d in %s for PR #%n" review.id repository.url number)


I prefer this function stays pure; the caller can do the stateful lookup and pass only an optional ts value (and not the entire ctx) to this function. Same for the others below.

The channel is overwritten with the value from the cached post_message_res, which I think is the cause of the bug in the new tests.

I think we should log, not fail, when message ts is not found. Because such a case is not unexpected, according to the PR description.

Unrelated to this PR, but observing that that these functions are called once for each channel, and most of it can be staged, as only the last part depends on channel value.

yasunariw · 2023-04-07T04:40:57Z

lib/api_remote.ml

+  (** [update_notification ctx msg] update a message at [msg.ts] in [msg.channel]
+      with the payload [msg]; uses web API with access token if available, or with
+      webhook otherwise *)
+  let update_notification ~(ctx : Context.t) ~(msg : Slack_t.update_message_req) =


this function is mostly copy pasted from the one above:(

should be able to factor

yasunariw · 2023-04-07T04:46:12Z

lib/slack.atd

+type update_message_req = {
+  channel: string;
+  ts: string;
+  ?text: string nullable;
+  ?attachments: message_attachment list nullable;
+  ?blocks: message_block list nullable;
+}
+
+type update_message_res = {
+  channel: string;
+  ts: string;


I'm not sure we need these additional types (or the corresponding New_message/Update_message). Maybe ts can be optional field in post_message_req, and Api.send_notification can query either chat.postMessage or chat.update depending on its presence/absence?

On second thought, chat.postMessage and chat.update differ in more than the ts field, so separate type seems warranted.

But for now, that is the only difference, so you can do

type update_message_req = { inherit post_message_req; ts: string; }

or have a common base that both inherit from.

yasunariw · 2023-04-07T04:49:23Z

lib/state.atd

+}
+
+(* The runtime state of a given GitHub repository
+   NB: issue number is the same as PR number for github APIs


this detail should be a comment next to the issue_tbl field

yasunariw · 2023-04-07T04:51:30Z

lib/state.ml

@@ -34,6 +38,36 @@ let set_repo_pipeline_status { state; lock } repo_url ~pipeline ~(branches : Git
  repo_state.pipeline_statuses <- Map.update repo_state.pipeline_statuses pipeline ~f:set_branch_status;
  Lwt.return_unit

+let add_comment_msg { state; lock } repo_url ~issue_num comment_id post_message_res =
+  Lwt_mutex.with_lock lock @@ fun () ->


I think this lock is actually unnecessary, not sure why I added it. Anyway, not the concern of this PR.

yasunariw · 2023-04-07T05:15:56Z

lib/action.ml

+      | Github.PR_review_comment n ->
+        State.add_comment_msg ctx.state n.repository.url ~issue_num:n.pull_request.number n.comment.id message
+      | Issue_comment n ->
+        State.add_comment_msg ctx.state n.repository.url ~issue_num:n.issue.number n.comment.id message
+      | PR_review n ->
+        State.add_review_msg ctx.state n.repository.url ~issue_num:n.pull_request.number n.review.id message


maybe the state lookup + update belongs in partition_pr_review_comment etc instead of here

yasunariw · 2023-04-07T05:21:45Z

lib/state.atd

@@ -10,13 +10,23 @@ type branch_statuses = status_state map_as_object
   branch *)
 type pipeline_statuses = branch_statuses map_as_object

-(* The runtime state of a given GitHub repository *)
+type post_message_res <ocaml from="Slack"> = abstract
+type post_msg_map = post_message_res map_as_object


A message can be posted to multiple Slack channels. The value of this map should be the message ts only, not post_message_res.

wait then don't we need post_message_res list to update all the ts for each of the channel?

yasunariw · 2023-04-07T05:30:24Z

lib/github.atd

@@ -287,7 +288,7 @@ type api_commit = {
 }

 type commit_comment_notification = {
-  action: string;
+  action: comment_action;


according to the PR description, commit comment edits do not send an edited event.

yasunariw · 2023-04-07T05:49:25Z

test/slack_payloads.expected

@@ -171,6 +171,34 @@ will notify #frontend-bot
  ]
 }
 ===== file ../mock_payloads/issue_comment.edited.json =====
+will update msg in #some channel
+{
+  "channel": "some channel",


Test output looks wrong, "some channel" is notified twice. If it is the same mock pr as issue_comment.created_in_pr, it should notify #a1-bot and #backend. Same for the other new tests.

o... thanks for the catch!

yasunariw · 2023-04-07T06:08:58Z

mock_states/issue_comment.edited.json

+            "comments": {
+                "0": {
+                    "channel": "some channel",
+                    "ts": "some ts"


Can the mock state be more realistic? The current one should be impossible, since there would never be two different messages posted to the same channel with the same ts. Same for the others

yasunariw · 2023-04-07T06:14:48Z

Curious what's the behavior when the workspace has editing time limit configured and chat.update is called after limit has passed.

I also wonder if there should be periodic cleanup of stale comment timestamps, edit notifications not useful beyond 1 day max?

sewenthy · 2023-04-22T09:48:07Z

lib/action.ml

-      partition_issue_comment cfg n |> List.map ~f:(generate_issue_comment_notification n) |> Lwt.return
+      partition_issue_comment cfg n
+      |> List.map ~f:(fun channel ->
+           (* not sure why but when I remove "fun channel" it evaluates the [generate_issue_comment_notification_content n] before the partition so I got errors but when I add this, it's completely fine, probably some compiler optimization? *)


@yasunariw @Khady any idea why? from what I understand, even without making it an explicit function, the partial evaluation of f should happen after the list generation right?

sewenthy · 2023-04-22T09:57:26Z

edit notifications not useful beyond 1 day max?

we were trying to find the right level of restrictions, I think waiting til a PR is closed is good because we might comment and change after re-visiting so keeping state for the whole time but TBH no strong preference for me 1-day max is also good, if you feel strongly about that then I can change the duration!

sewenthy · 2023-04-22T10:03:25Z

Curious what's the behavior when the workspace has editing time limit configured and chat.update is called after limit has passed.

Just tested, and bot can edit their own message after the workspace editing time passed. Thanks for the catch!

Khady · 2024-03-20T03:18:20Z

what's the status on this one?

sewenthy · 2024-03-21T03:29:39Z

@Khady I lost track of this, will need to rebase and do some more work. If I remember correctly, I needed to fix the mapping for editing the messages since there can be multiple messages sent to different channels for one single PR.

yasunariw requested changes Apr 7, 2023

View reviewed changes

yasunariw force-pushed the master branch from 9b92bf7 to a70c93d Compare April 21, 2023 16:23

sewenthy added 6 commits April 22, 2023 05:58

slack, api: add update message

3ea807b

action, github: support edited comments update + add tests

737863c

github, slack: support pr review updates

fbbe2fd

state, slack: map comments + reviews to its issue

4afb57b

action, state: clean up when receive PR closed event

9d2d6bc

action: also clean issue closed

0bfc75d

sewenthy force-pushed the sewen/update-comments-once-edited branch from ef5e936 to 0bfc75d Compare April 22, 2023 06:00

action, slack: separate content generation + stateful update

e2cfd9c

sewenthy commented Apr 22, 2023

View reviewed changes

sewenthy added 2 commits April 22, 2023 10:22

minor

5df2e8b

slack: refactor api_remote webhook calls into function

817c2c3

sewenthy requested a review from yasunariw April 25, 2023 04:32

sewenthy mentioned this pull request Apr 25, 2023

action: use slack lib implementations sewenthy/monorobot#3

Open

Khady added the Blocked label Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support updating corresponding comment message in Slack #132

Support updating corresponding comment message in Slack #132

sewenthy commented Mar 13, 2023 •

edited

Loading

yasunariw left a comment

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023 •

edited

Loading

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

sewenthy Apr 22, 2023

yasunariw Apr 7, 2023

yasunariw Apr 7, 2023

sewenthy Apr 22, 2023

yasunariw Apr 7, 2023

yasunariw commented Apr 7, 2023 •

edited

Loading

sewenthy Apr 22, 2023

sewenthy commented Apr 22, 2023

sewenthy commented Apr 22, 2023

Khady commented Mar 20, 2024

sewenthy commented Mar 21, 2024

Support updating corresponding comment message in Slack #132

Are you sure you want to change the base?

Support updating corresponding comment message in Slack #132

Conversation

sewenthy commented Mar 13, 2023 • edited Loading

Description of the task

How to test

References

yasunariw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasunariw Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasunariw commented Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

sewenthy commented Apr 22, 2023

sewenthy commented Apr 22, 2023

Khady commented Mar 20, 2024

sewenthy commented Mar 21, 2024

sewenthy commented Mar 13, 2023 •

edited

Loading

yasunariw Apr 7, 2023 •

edited

Loading

yasunariw commented Apr 7, 2023 •

edited

Loading