From 0e222cdeeee469bf2109310d27d61a8afb86a118 Mon Sep 17 00:00:00 2001 From: Yasunari Watanabe Date: Tue, 5 Jan 2021 15:23:20 +0800 Subject: [PATCH 1/3] use plaintext for all fallback messages --- lib/slack.ml | 50 +++++++++++++++++---- test/slack_payloads.expected | 85 ++++++++++++++++++------------------ 2 files changed, 83 insertions(+), 52 deletions(-) diff --git a/lib/slack.ml b/lib/slack.ml index 625c28c1..476304a5 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -53,6 +53,9 @@ let generate_pull_request_notification notification channel = (sprintf "<%s|[%s]> Pull request #%d <%s|%s> %s by *%s*" repository.url repository.full_name number html_url title action sender.login) in + let fallback = + Some (sprintf "[%s] Pull request #%d %s %s by %s" repository.full_name number title action sender.login) + in { channel; text = None; @@ -62,7 +65,7 @@ let generate_pull_request_notification notification channel = { empty_attachments with mrkdwn_in = Some [ "text" ]; - fallback = summary; + fallback; color = Some "#ccc"; pretext = summary; text = mrkdwn_of_markdown_opt body; @@ -93,6 +96,7 @@ let generate_pr_review_notification notification channel = (sprintf "<%s|[%s]> *%s* <%s|%s> #%d <%s|%s>" repository.url repository.full_name sender.login review.html_url action_str number html_url title) in + let fallback = Some (sprintf "[%s] %s %s #%d %s" repository.full_name sender.login action_str number title) in { channel; text = None; @@ -102,7 +106,7 @@ let generate_pr_review_notification notification channel = { empty_attachments with mrkdwn_in = Some [ "text" ]; - fallback = summary; + fallback; color = Some "#ccc"; pretext = summary; text = mrkdwn_of_markdown_opt review.body; @@ -127,6 +131,7 @@ let generate_pr_review_comment_notification notification channel = (sprintf "<%s|[%s]> *%s* %s on #%d <%s|%s>" repository.url repository.full_name sender.login action_str number html_url title) in + let fallback = Some (sprintf "[%s] %s %s on #%d %s" repository.full_name sender.login action_str number title) in let file = match comment.path with | None -> None @@ -141,7 +146,7 @@ let generate_pr_review_comment_notification notification channel = { empty_attachments with mrkdwn_in = Some [ "text" ]; - fallback = summary; + fallback; color = Some "#ccc"; pretext = summary; footer = file; @@ -170,6 +175,7 @@ let generate_issue_notification notification channel = (sprintf "<%s|[%s]> Issue #%d <%s|%s> %s by *%s*" repository.url repository.full_name number html_url title action sender.login) in + let fallback = Some (sprintf "[%s] Issue #%d %s %s by %s" repository.full_name number title action sender.login) in { channel; text = None; @@ -179,7 +185,7 @@ let generate_issue_notification notification channel = { empty_attachments with mrkdwn_in = Some [ "text" ]; - fallback = summary; + fallback; color = Some "#ccc"; pretext = summary; text = mrkdwn_of_markdown_opt body; @@ -205,6 +211,7 @@ let generate_issue_comment_notification notification channel = (sprintf "<%s|[%s]> *%s* <%s|%s> on #%d <%s|%s>" repository.url repository.full_name sender.login comment.html_url action_str number issue.html_url title) in + let fallback = Some (sprintf "[%s] %s %s on #%d %s" repository.full_name sender.login action_str number title) in { channel; text = None; @@ -214,7 +221,7 @@ let generate_issue_comment_notification notification channel = { empty_attachments with mrkdwn_in = Some [ "text" ]; - fallback = summary; + fallback; color = Some "#ccc"; pretext = summary; text = Some (mrkdwn_of_markdown comment.body); @@ -243,6 +250,18 @@ let generate_push_notification notification channel = (if created then "to new branch " else "") sender.login in + let fallback = + if deleted then sprintf "[%s] %s deleted branch %s" repository.name sender.login commits_branch + else + sprintf "[%s:%s] %i commit%s %spushed %sby %s" repository.name commits_branch (List.length commits) + ( match commits with + | [ _ ] -> "" + | _ -> "s" + ) + (if forced then "force-" else "") + (if created then "to new branch " else "") + sender.login + in let commits = List.map commits ~f:(fun { url; id; message; author; _ } -> let title = first_line message in @@ -250,14 +269,15 @@ let generate_push_notification notification channel = in { channel; - text = Some title; + text = None; attachments = Some [ { empty_attachments with mrkdwn_in = Some [ "fields" ]; - fallback = Some "Commit pushed notification"; + pretext = Some title; + fallback = Some fallback; color = Some "#ccc"; fields = Some [ { value = String.concat ~sep:"\n" commits; title = None; short = false } ]; }; @@ -315,12 +335,19 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_ (sprintf "<%s|[%s]> CI Build Status notification for <%s|%s>: %s" repository.url repository.full_name t context state_info) in + let fallback = + match target_url with + | None -> + Some (sprintf "[%s] CI Build Status notification: %s" repository.full_name state_info) + (* in case the CI run is not using buildkite *) + | Some _ -> Some (sprintf "[%s] CI Build Status notification for %s: %s" repository.full_name context state_info) + in let msg = String.concat ~sep:"\n" @@ List.concat [ commit_info; branches_info ] in let attachment = { empty_attachments with mrkdwn_in = Some [ "fields"; "text" ]; - fallback = summary; + fallback; pretext = summary; color = Some color_info; text = description_info; @@ -342,6 +369,11 @@ let generate_commit_comment_notification api_commit notification channel = (sprintf "<%s|[%s]> *%s* commented on `<%s|%s>` %s" repository.url repository.full_name sender.login comment.html_url (git_short_sha_hash commit_id) (first_line commit.message)) in + let fallback = + Some + (sprintf "[%s] %s commented on `%s` %s" repository.full_name sender.login (git_short_sha_hash commit_id) + (first_line commit.message)) + in let path = match comment.path with | None -> None @@ -351,7 +383,7 @@ let generate_commit_comment_notification api_commit notification channel = { empty_attachments with mrkdwn_in = Some [ "pretext"; "text" ]; - fallback = summary; + fallback; color = Some "#ccc"; pretext = summary; footer = path; diff --git a/test/slack_payloads.expected b/test/slack_payloads.expected index 4e02646b..4dfc36f3 100644 --- a/test/slack_payloads.expected +++ b/test/slack_payloads.expected @@ -5,7 +5,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` Update README.md", + "[ahrefs/monorepo] xinyuluo commented on `0d95302a` Update README.md", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -23,7 +23,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` add new line at EOF", + "[xinyuluo/monorepo] xinyuluo commented on `cd5b85af` add new line at EOF", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -39,7 +39,7 @@ will notify #a1 "attachments": [ { "fallback": - " *xinyuluo* commented on `` Add files via upload", + "[xinyuluo/monorepo] xinyuluo commented on `1edcdf5e` Add files via upload", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -54,7 +54,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` Add files via upload", + "[xinyuluo/monorepo] xinyuluo commented on `1edcdf5e` Add files via upload", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -70,7 +70,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` add new line at EOF", + "[xinyuluo/monorepo] xinyuluo commented on `cd5b85af` add new line at EOF", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -89,7 +89,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` add new line at EOF", + "[xinyuluo/monorepo] xinyuluo commented on `cd5b85af` add new line at EOF", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -108,7 +108,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` make changes in 2 files", + "[xinyuluo/monorepo] xinyuluo commented on `41dad1d3` make changes in 2 files", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -124,7 +124,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " *xinyuluo* commented on `` add new line at EOF", + "[xinyuluo/monorepo] xinyuluo commented on `cd5b85af` add new line at EOF", "mrkdwn_in": [ "pretext", "text" ], "color": "#ccc", "pretext": @@ -142,7 +142,7 @@ will notify #default "attachments": [ { "fallback": - " *xinyuluo* on #5 ", + "[xinyuluo/monorepo] xinyuluo commented on #5 handle issue event", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -157,8 +157,7 @@ will notify #a1-bot "channel": "a1-bot", "attachments": [ { - "fallback": - " *xinyuluo* on #4 ", + "fallback": "[xinyuluo/monorepo] xinyuluo commented on #4 README update", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -172,8 +171,7 @@ will notify #backend "channel": "backend", "attachments": [ { - "fallback": - " *xinyuluo* on #4 ", + "fallback": "[xinyuluo/monorepo] xinyuluo commented on #4 README update", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -190,7 +188,7 @@ will notify #frontend-bot "attachments": [ { "fallback": - " *yasunariw* on #6168 ", + "[ahrefs/monorepo] yasunariw commented on #6168 Feature 1234", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -208,7 +206,7 @@ will notify #backend "attachments": [ { "fallback": - " Issue #6 closed by *xinyuluo*", + "[xinyuluo/monorepo] Issue #6 create a test for issue_notification closed by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -223,7 +221,7 @@ will notify #backend "attachments": [ { "fallback": - " Issue #6 labeled by *xinyuluo*", + "[xinyuluo/monorepo] Issue #6 create a test for issue_notification labeled by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -239,7 +237,7 @@ will notify #default "attachments": [ { "fallback": - " Issue #6 opened by *xinyuluo*", + "[xinyuluo/monorepo] Issue #6 create a test for issue_notification opened by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -256,7 +254,7 @@ will notify #backend "attachments": [ { "fallback": - " Issue #6 reopened by *xinyuluo*", + "[xinyuluo/monorepo] Issue #6 create a test for issue_notification reopened by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -271,7 +269,7 @@ will notify #a1-bot "attachments": [ { "fallback": - " Pull request #8 closed by *xinyuluo*", + "[xinyuluo/monorepo] Pull request #8 Update README.md closed by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -286,7 +284,7 @@ will notify #default "attachments": [ { "fallback": - " Pull request #2 closed by *xinyuluo*", + "[xinyuluo/monorepo] Pull request #2 Update README.md closed by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -301,7 +299,7 @@ will notify #a1-bot "attachments": [ { "fallback": - " Pull request #4 labeled by *xinyuluo*", + "[xinyuluo/monorepo] Pull request #4 README update labeled by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -316,7 +314,7 @@ will notify #backend "attachments": [ { "fallback": - " Pull request #4 labeled by *xinyuluo*", + "[xinyuluo/monorepo] Pull request #4 README update labeled by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -332,7 +330,7 @@ will notify #frontend-bot "attachments": [ { "fallback": - " Pull request #3 opened by *xinyuluo*", + "[xinyuluo/monorepo] Pull request #3 Update README.md opened by xinyuluo", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -349,7 +347,7 @@ will notify #default "attachments": [ { "fallback": - " *Khady* #6 ", + "[Khady/monorepo] Khady approved #6 create pr_review_test.md", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -365,7 +363,7 @@ will notify #frontend-bot "attachments": [ { "fallback": - " *xinyuluo* #3 ", + "[xinyuluo/monorepo] xinyuluo commented on #3 Update README.md", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -383,7 +381,7 @@ will notify #default "attachments": [ { "fallback": - " *Khady* #6 ", + "[Khady/monorepo] Khady requested changes on #6 create pr_review_test.md", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -400,8 +398,7 @@ will notify #a1-bot "channel": "a1-bot", "attachments": [ { - "fallback": - " *xinyuluo* commented on #4 ", + "fallback": "[xinyuluo/monorepo] xinyuluo commented on #4 README update", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -417,8 +414,7 @@ will notify #backend "channel": "backend", "attachments": [ { - "fallback": - " *xinyuluo* commented on #4 ", + "fallback": "[xinyuluo/monorepo] xinyuluo commented on #4 README update", "mrkdwn_in": [ "text" ], "color": "#ccc", "pretext": @@ -435,13 +431,14 @@ will notify #backend will notify #all-push-events { "channel": "all-push-events", - "text": - " pushed by thatportugueseguy", "attachments": [ { - "fallback": "Commit pushed notification", + "fallback": + "[monorepo:master] 2 commits pushed by thatportugueseguy", "mrkdwn_in": [ "fields" ], "color": "#ccc", + "pretext": + " pushed by thatportugueseguy", "fields": [ { "value": @@ -455,13 +452,14 @@ will notify #all-push-events will notify #all-push-events { "channel": "all-push-events", - "text": - " pushed by thatportugueseguy", "attachments": [ { - "fallback": "Commit pushed notification", + "fallback": + "[monorepo:master] 2 commits pushed by thatportugueseguy", "mrkdwn_in": [ "fields" ], "color": "#ccc", + "pretext": + " pushed by thatportugueseguy", "fields": [ { "value": @@ -474,13 +472,14 @@ will notify #all-push-events will notify #longest-a1 { "channel": "longest-a1", - "text": - " pushed by thatportugueseguy", "attachments": [ { - "fallback": "Commit pushed notification", + "fallback": + "[monorepo:master] 1 commit pushed by thatportugueseguy", "mrkdwn_in": [ "fields" ], "color": "#ccc", + "pretext": + " pushed by thatportugueseguy", "fields": [ { "value": @@ -498,7 +497,7 @@ will notify #default "attachments": [ { "fallback": - " CI Build Status notification for : failure", + "[ahrefs/monorepo] CI Build Status notification for buildkite/pipeline2: failure", "mrkdwn_in": [ "fields", "text" ], "color": "danger", "pretext": @@ -524,7 +523,7 @@ will notify #default "attachments": [ { "fallback": - " CI Build Status notification: success", + "[Codertocat/Hello-World] CI Build Status notification: success", "mrkdwn_in": [ "fields", "text" ], "color": "good", "pretext": @@ -545,7 +544,7 @@ will notify #all-push-events "attachments": [ { "fallback": - " CI Build Status notification for : success", + "[ahrefs/monorepo] CI Build Status notification for buildkite/pipeline2: success", "mrkdwn_in": [ "fields", "text" ], "color": "good", "pretext": @@ -567,7 +566,7 @@ will notify #default "attachments": [ { "fallback": - " CI Build Status notification for : success", + "[ahrefs/monorepo] CI Build Status notification for buildkite/pipeline2: success", "mrkdwn_in": [ "fields", "text" ], "color": "good", "pretext": From 46b04204b085fd1d2b8c15033deaa3be0a0f9936 Mon Sep 17 00:00:00 2001 From: Yasunari Watanabe Date: Tue, 5 Jan 2021 17:15:31 +0800 Subject: [PATCH 2/3] escape all user text and provide pretty-printing utility functions PR titles, comments, etc. should all be escaped. --- lib/slack.ml | 103 ++++++++++++++++++++++++--------------------------- 1 file changed, 48 insertions(+), 55 deletions(-) diff --git a/lib/slack.ml b/lib/slack.ml index 476304a5..c8fb2bc2 100644 --- a/lib/slack.ml +++ b/lib/slack.ml @@ -34,6 +34,14 @@ let show_labels = function let pluralize name num suffix = if num = 1 then sprintf "%s" name else String.concat [ name; suffix ] +let escape = Mrkdwn.escape_mrkdwn + +let pp_link url text = sprintf "<%s|%s>" url (escape text) + +let pp_repo (repo : repository) = sprintf "[%s]" repo.full_name + +let pp_repo_mrkdwn (repo : repository) = pp_link repo.url @@ pp_repo repo + let generate_pull_request_notification notification channel = let { action; number; sender; pull_request; repository } = notification in let ({ body; title; html_url; labels; _ } : pull_request) = pull_request in @@ -50,11 +58,11 @@ let generate_pull_request_notification notification channel = in let summary = Some - (sprintf "<%s|[%s]> Pull request #%d <%s|%s> %s by *%s*" repository.url repository.full_name number html_url title - action sender.login) + (sprintf "%s Pull request #%d %s %s by *%s*" (pp_repo_mrkdwn repository) number (pp_link html_url title) action + sender.login) in let fallback = - Some (sprintf "[%s] Pull request #%d %s %s by %s" repository.full_name number title action sender.login) + Some (sprintf "%s Pull request #%d %s %s by %s" (pp_repo repository) number title action sender.login) in { channel; @@ -93,10 +101,10 @@ let generate_pr_review_notification notification channel = in let summary = Some - (sprintf "<%s|[%s]> *%s* <%s|%s> #%d <%s|%s>" repository.url repository.full_name sender.login review.html_url - action_str number html_url title) + (sprintf "%s *%s* %s #%d %s" (pp_repo_mrkdwn repository) sender.login (pp_link review.html_url action_str) number + (pp_link html_url title)) in - let fallback = Some (sprintf "[%s] %s %s #%d %s" repository.full_name sender.login action_str number title) in + let fallback = Some (sprintf "%s %s %s #%d %s" (pp_repo repository) sender.login action_str number title) in { channel; text = None; @@ -128,14 +136,14 @@ let generate_pr_review_comment_notification notification channel = in let summary = Some - (sprintf "<%s|[%s]> *%s* %s on #%d <%s|%s>" repository.url repository.full_name sender.login action_str number - html_url title) + (sprintf "%s *%s* %s on #%d %s" (pp_repo_mrkdwn repository) sender.login action_str number + (pp_link html_url title)) in - let fallback = Some (sprintf "[%s] %s %s on #%d %s" repository.full_name sender.login action_str number title) in + let fallback = Some (sprintf "%s %s %s on #%d %s" (pp_repo repository) sender.login action_str number title) in let file = match comment.path with | None -> None - | Some a -> Some (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url a) + | Some a -> Some (sprintf "New comment by %s in %s" sender.login (pp_link comment.html_url a)) in { channel; @@ -172,10 +180,10 @@ let generate_issue_notification notification channel = in let summary = Some - (sprintf "<%s|[%s]> Issue #%d <%s|%s> %s by *%s*" repository.url repository.full_name number html_url title action + (sprintf "%s Issue #%d %s %s by *%s*" (pp_repo_mrkdwn repository) number (pp_link html_url title) action sender.login) in - let fallback = Some (sprintf "[%s] Issue #%d %s %s by %s" repository.full_name number title action sender.login) in + let fallback = Some (sprintf "%s Issue #%d %s %s by %s" (pp_repo repository) number title action sender.login) in { channel; text = None; @@ -208,10 +216,10 @@ let generate_issue_comment_notification notification channel = in let summary = Some - (sprintf "<%s|[%s]> *%s* <%s|%s> on #%d <%s|%s>" repository.url repository.full_name sender.login comment.html_url - action_str number issue.html_url title) + (sprintf "%s *%s* %s on #%d %s" (pp_repo_mrkdwn repository) sender.login (pp_link comment.html_url action_str) + number (pp_link issue.html_url title)) in - let fallback = Some (sprintf "[%s] %s %s on #%d %s" repository.full_name sender.login action_str number title) in + let fallback = Some (sprintf "%s %s %s on #%d %s" (pp_repo repository) sender.login action_str number title) in { channel; text = None; @@ -236,36 +244,24 @@ let generate_push_notification notification channel = let { sender; created; deleted; forced; compare; commits; repository; _ } = notification in let commits_branch = Github.commits_branch_of_ref notification.ref in let tree_url = String.concat ~sep:"/" [ repository.url; "tree"; Uri.pct_encode commits_branch ] in + let repo_branch = sprintf "[%s:%s]" repository.name commits_branch in + let num_commits = sprintf "%d %s" (List.length commits) (pluralize "commit" (List.length commits) "s") in + let action = if forced then "force-pushed" else "pushed" in + let dest = if created then "to new branch " else "" in let title = if deleted then - sprintf "<%s|[%s]> %s deleted branch <%s|%s>" tree_url repository.name sender.login compare commits_branch + sprintf "%s %s deleted branch %s" (pp_link tree_url repository.name) sender.login (pp_link compare commits_branch) else - sprintf "<%s|[%s:%s]> <%s|%i commit%s> %spushed %sby %s" tree_url repository.name commits_branch compare - (List.length commits) - ( match commits with - | [ _ ] -> "" - | _ -> "s" - ) - (if forced then "force-" else "") - (if created then "to new branch " else "") - sender.login + sprintf "%s %s %s %sby %s" (pp_link tree_url repo_branch) (pp_link compare num_commits) action dest sender.login in let fallback = - if deleted then sprintf "[%s] %s deleted branch %s" repository.name sender.login commits_branch - else - sprintf "[%s:%s] %i commit%s %spushed %sby %s" repository.name commits_branch (List.length commits) - ( match commits with - | [ _ ] -> "" - | _ -> "s" - ) - (if forced then "force-" else "") - (if created then "to new branch " else "") - sender.login + if deleted then sprintf "%s %s deleted branch %s" (pp_repo repository) sender.login commits_branch + else sprintf "%s %s %s %sby %s" repo_branch num_commits action dest sender.login in let commits = List.map commits ~f:(fun { url; id; message; author; _ } -> - let title = first_line message in - sprintf "`<%s|%s>` %s - %s" url (git_short_sha_hash id) title author.name) + let title = escape @@ first_line message in + sprintf "`%s` %s - %s" (pp_link url (git_short_sha_hash id)) title author.name) in { channel; @@ -310,7 +306,12 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_ | Some s -> Some (sprintf "*Description*: %s." s) in let commit_info = - [ sprintf "*Commit*: `<%s|%s>` %s - %s" html_url (git_short_sha_hash sha) (first_line message) author.login ] + [ + sprintf "*Commit*: `%s` %s - %s" + (pp_link html_url (git_short_sha_hash sha)) + (escape @@ first_line message) + author.login; + ] in let branches_info = match List.map ~f:(fun { name } -> name) notification.branches with @@ -326,21 +327,12 @@ let generate_status_notification (cfg : Config_t.config) (notification : status_ [ sprintf "*%s*: %s" (pluralize "Branch" (List.length branches) "es") (String.concat ~sep:", " branches) ] in let summary = - match target_url with - | None -> - Some (sprintf "<%s|[%s]> CI Build Status notification: %s" repository.url repository.full_name state_info) - (* in case the CI run is not using buildkite *) - | Some t -> - Some - (sprintf "<%s|[%s]> CI Build Status notification for <%s|%s>: %s" repository.url repository.full_name t context - state_info) + let target = Option.value_map target_url ~default:"" ~f:(fun url -> sprintf " for %s" (pp_link url context)) in + Some (sprintf "%s CI Build Status notification%s: %s" (pp_repo_mrkdwn repository) target state_info) in let fallback = - match target_url with - | None -> - Some (sprintf "[%s] CI Build Status notification: %s" repository.full_name state_info) - (* in case the CI run is not using buildkite *) - | Some _ -> Some (sprintf "[%s] CI Build Status notification for %s: %s" repository.full_name context state_info) + let target = Option.value_map target_url ~default:"" ~f:(fun _ -> sprintf " for %s" context) in + Some (sprintf "%s CI Build Status notification%s: %s" (pp_repo repository) target state_info) in let msg = String.concat ~sep:"\n" @@ List.concat [ commit_info; branches_info ] in let attachment = @@ -366,18 +358,19 @@ let generate_commit_comment_notification api_commit notification channel = in let summary = Some - (sprintf "<%s|[%s]> *%s* commented on `<%s|%s>` %s" repository.url repository.full_name sender.login - comment.html_url (git_short_sha_hash commit_id) (first_line commit.message)) + (sprintf "%s *%s* commented on `%s` %s" (pp_repo_mrkdwn repository) sender.login + (pp_link comment.html_url (git_short_sha_hash commit_id)) + (escape @@ first_line commit.message)) in let fallback = Some - (sprintf "[%s] %s commented on `%s` %s" repository.full_name sender.login (git_short_sha_hash commit_id) + (sprintf "%s %s commented on `%s` %s" (pp_repo repository) sender.login (git_short_sha_hash commit_id) (first_line commit.message)) in let path = match comment.path with | None -> None - | Some p -> Some (sprintf "New comment by %s in <%s|%s>" sender.login comment.html_url p) + | Some p -> Some (sprintf "New comment by %s in %s" sender.login (pp_link comment.html_url p)) in let attachment = { From ee52fb2df818b3b46d81a3f5fa5de2c926987462 Mon Sep 17 00:00:00 2001 From: Yasunari Watanabe Date: Tue, 5 Jan 2021 17:25:19 +0800 Subject: [PATCH 3/3] tests: promote with simple char escape check --- mock_payloads/pull_request_review_comment.created.json | 2 +- test/slack_payloads.expected | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/mock_payloads/pull_request_review_comment.created.json b/mock_payloads/pull_request_review_comment.created.json index acb36d36..c8d6e3ec 100644 --- a/mock_payloads/pull_request_review_comment.created.json +++ b/mock_payloads/pull_request_review_comment.created.json @@ -31,7 +31,7 @@ "type": "User", "site_admin": false }, - "body": "PR review comment example", + "body": "PR review comment example with ", "created_at": "2020-05-25T07:37:52Z", "updated_at": "2020-05-25T07:38:26Z", "html_url": "https://github.com/xinyuluo/monorepo/pull/4#discussion_r0", diff --git a/test/slack_payloads.expected b/test/slack_payloads.expected index 4dfc36f3..3fc0574e 100644 --- a/test/slack_payloads.expected +++ b/test/slack_payloads.expected @@ -403,7 +403,8 @@ will notify #a1-bot "color": "#ccc", "pretext": " *xinyuluo* commented on #4 ", - "text": "PR review comment example", + "text": + "PR review comment example with <escaped & characters>", "footer": "New comment by xinyuluo in " } @@ -419,7 +420,8 @@ will notify #backend "color": "#ccc", "pretext": " *xinyuluo* commented on #4 ", - "text": "PR review comment example", + "text": + "PR review comment example with <escaped & characters>", "footer": "New comment by xinyuluo in " }