From fd0b013bcdcd5d40cbfde1d204a33e1a0b2bc4f9 Mon Sep 17 00:00:00 2001 From: Hari Prashant Bhimaraju Date: Wed, 17 Aug 2022 01:23:31 +0530 Subject: [PATCH] slack_incoming: Handle optional attachment fields aptly. This commit checks for null values for keys within "attachment" in the Slack integration's incoming payloads. These keys were expected to exist optionally previously, and the existence of null values for these wasn't anticipated. Due to an issue report for such null values in the payload, their handling is updated appropriately. The checks for these values are truthiness checks since the strategy for these values being null or falsy ("", 0) is the same; we don't process that key-value pair. This is consistent with how Slack handles this scenario. For the case where all the attachment fields have null values, Slack displays this as an empty block with no content, and therefore our strategy for this is a no-op. Tests updated. --- .../fixtures/attachment_fields.json | 15 ++ .../fixtures/attachment_pieces_all_null.json | 13 ++ .../attachment_pieces_footer_null.json | 13 ++ .../attachment_pieces_image_url_null.json | 13 ++ .../attachment_pieces_pretext_null.json | 13 ++ .../fixtures/attachment_pieces_text_null.json | 13 ++ .../attachment_pieces_title_link_null.json | 13 ++ .../attachment_pieces_title_null.json | 13 ++ .../fixtures/attachment_pieces_ts_null.json | 13 ++ zerver/webhooks/slack_incoming/tests.py | 147 ++++++++++++++++++ zerver/webhooks/slack_incoming/view.py | 30 ++-- 11 files changed, 286 insertions(+), 10 deletions(-) create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_all_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_footer_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_image_url_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_pretext_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_text_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_link_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_null.json create mode 100644 zerver/webhooks/slack_incoming/fixtures/attachment_pieces_ts_null.json diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_fields.json b/zerver/webhooks/slack_incoming/fixtures/attachment_fields.json index 173da6bd12..72232d6466 100644 --- a/zerver/webhooks/slack_incoming/fixtures/attachment_fields.json +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_fields.json @@ -17,6 +17,21 @@ "title": "Build pipeline", "value": "ConsumerAddressModule", "short": true + }, + { + "title": "Title without value", + "value": null, + "short": true + }, + { + "title": null, + "value": "Value without title", + "short": true + }, + { + "title": null, + "value": null, + "short": true } ], "pretext": "Build bla bla succeeded", diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_all_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_all_null.json new file mode 100644 index 0000000000..ad1a3e448e --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_all_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": null, + "title_link": null, + "image_url": null, + "ts": null, + "text": null, + "pretext": null, + "footer": null + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_footer_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_footer_null.json new file mode 100644 index 0000000000..01d92ab61e --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_footer_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": "Sample title.", + "title_link": "https://www.google.com", + "image_url": "https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg", + "ts": 1655945306, + "text": "Sample text.", + "pretext": "Sample pretext.", + "footer": null + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_image_url_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_image_url_null.json new file mode 100644 index 0000000000..424f7cea80 --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_image_url_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": "Sample title.", + "title_link": "https://www.google.com", + "image_url": null, + "ts": 1655945306, + "text": "Sample text.", + "pretext": "Sample pretext.", + "footer": "Sample footer." + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_pretext_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_pretext_null.json new file mode 100644 index 0000000000..70c53ef2a1 --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_pretext_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": "Sample title.", + "title_link": "https://www.google.com", + "image_url": "https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg", + "ts": 1655945306, + "text": "Sample text.", + "pretext": null, + "footer": "Sample footer." + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_text_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_text_null.json new file mode 100644 index 0000000000..f9233d3850 --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_text_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": "Sample title.", + "title_link": "https://www.google.com", + "image_url": "https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg", + "ts": 1655945306, + "text": null, + "pretext": "Sample pretext.", + "footer": "Sample footer." + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_link_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_link_null.json new file mode 100644 index 0000000000..9793750b2a --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_link_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": "Sample title.", + "title_link": null, + "image_url": "https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg", + "ts": 1655945306, + "text": "Sample text.", + "pretext": "Sample pretext.", + "footer": "Sample footer." + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_null.json new file mode 100644 index 0000000000..7f7b69d3b6 --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_title_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": null, + "title_link": "https://www.google.com", + "image_url": "https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg", + "ts": 1655945306, + "text": "Sample text.", + "pretext": "Sample pretext.", + "footer": "Sample footer." + } + ] +} diff --git a/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_ts_null.json b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_ts_null.json new file mode 100644 index 0000000000..a045f289e3 --- /dev/null +++ b/zerver/webhooks/slack_incoming/fixtures/attachment_pieces_ts_null.json @@ -0,0 +1,13 @@ +{ + "attachments": [ + { + "title": "Sample title.", + "title_link": "https://www.google.com", + "image_url": "https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg", + "ts": null, + "text": "Sample text.", + "pretext": "Sample pretext.", + "footer": "Sample footer." + } + ] +} diff --git a/zerver/webhooks/slack_incoming/tests.py b/zerver/webhooks/slack_incoming/tests.py index 543cd2c17a..51107764b8 100644 --- a/zerver/webhooks/slack_incoming/tests.py +++ b/zerver/webhooks/slack_incoming/tests.py @@ -167,6 +167,8 @@ Build bla bla succeeded **Requested by**: Some user **Duration**: 00:02:03 **Build pipeline**: ConsumerAddressModule +**Title without value** +Value without title """.strip() self.check_webhook( @@ -197,3 +199,148 @@ Build bla bla succeeded else: file_type = "json" return self.webhook_fixture_data("slack_incoming", fixture_name, file_type=file_type) + + def test_attachment_pieces_title_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +Sample pretext. + +Sample text. + +[](https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg) + +Sample footer. + + + """.strip() + + self.check_webhook( + "attachment_pieces_title_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_image_url_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +## [Sample title.](https://www.google.com) + +Sample pretext. + +Sample text. + +Sample footer. + + + """.strip() + + self.check_webhook( + "attachment_pieces_image_url_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_ts_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +## [Sample title.](https://www.google.com) + +Sample pretext. + +Sample text. + +[](https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg) + +Sample footer. + """.strip() + + self.check_webhook( + "attachment_pieces_ts_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_text_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +## [Sample title.](https://www.google.com) + +Sample pretext. + +[](https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg) + +Sample footer. + + + """.strip() + + self.check_webhook( + "attachment_pieces_text_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_pretext_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +## [Sample title.](https://www.google.com) + +Sample text. + +[](https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg) + +Sample footer. + + + """.strip() + + self.check_webhook( + "attachment_pieces_pretext_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_footer_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +## [Sample title.](https://www.google.com) + +Sample pretext. + +Sample text. + +[](https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg) + + + """.strip() + + self.check_webhook( + "attachment_pieces_footer_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_title_link_null(self) -> None: + expected_topic = "(no topic)" + expected_message = """ +## Sample title. + +Sample pretext. + +Sample text. + +[](https://pbs.twimg.com/profile_images/625633822235693056/lNGUneLX_400x400.jpg) + +Sample footer. + + + """.strip() + + self.check_webhook( + "attachment_pieces_title_link_null", + expected_topic, + expected_message, + ) + + def test_attachment_pieces_all_null(self) -> None: + self.check_webhook("attachment_pieces_all_null", expect_noop=True) diff --git a/zerver/webhooks/slack_incoming/view.py b/zerver/webhooks/slack_incoming/view.py index 10901dc276..a3e42d5c65 100644 --- a/zerver/webhooks/slack_incoming/view.py +++ b/zerver/webhooks/slack_incoming/view.py @@ -175,33 +175,43 @@ def render_block_element(element: WildValue) -> str: def render_attachment(attachment: WildValue) -> str: # https://api.slack.com/reference/messaging/attachments + # Slack recommends the usage of "blocks" even within attachments; the + # rest of the fields we handle here are legacy fields. These fields are + # optional and may contain null values. pieces = [] - if "title" in attachment: + if "title" in attachment and attachment["title"]: title = attachment["title"].tame(check_string) - if "title_link" in attachment: + if "title_link" in attachment and attachment["title_link"]: title_link = attachment["title_link"].tame(check_url) pieces.append(f"## [{title}]({title_link})") else: pieces.append(f"## {title}") - if "pretext" in attachment: + if "pretext" in attachment and attachment["pretext"]: pieces.append(attachment["pretext"].tame(check_string)) - if "text" in attachment: + if "text" in attachment and attachment["text"]: pieces.append(attachment["text"].tame(check_string)) if "fields" in attachment: fields = [] for field in attachment["fields"]: - title = field["title"].tame(check_string) - value = field["value"].tame(check_string) - fields.append(f"*{title}*: {value}") + if field["title"] and field["value"]: + title = field["title"].tame(check_string) + value = field["value"].tame(check_string) + fields.append(f"*{title}*: {value}") + elif field["title"]: + title = field["title"].tame(check_string) + fields.append(f"*{title}*") + elif field["value"]: + value = field["value"].tame(check_string) + fields.append(f"{value}") pieces.append("\n".join(fields)) if "blocks" in attachment and attachment["blocks"]: for block in attachment["blocks"]: pieces.append(render_block(block)) - if "image_url" in attachment: + if "image_url" in attachment and attachment["image_url"]: pieces.append("[]({})".format(attachment["image_url"].tame(check_url))) - if "footer" in attachment: + if "footer" in attachment and attachment["footer"]: pieces.append(attachment["footer"].tame(check_string)) - if "ts" in attachment: + if "ts" in attachment and attachment["ts"]: time = attachment["ts"].tame(check_int) pieces.append(f"")