Repository: ansible/ansible
Pull request: Add threading to slack notification module
##### SUMMARY
Fixes #21266. Adds the ability to reply to a thread in a slack message. This follows the slack API documentation for threading from here: https://api.slack.com/docs/message-threading

In addition, the module had no unit tests so I added a few. Its definitley not 100% coverage, but some is better than none.

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request

##### COMPONENT NAME
slack

##### ANSIBLE VERSION
```ansible 2.8.0.dev0 (notification/slack-add-threading-support b3b3487c16) last updated 2018/10/18 23:04:34 (GMT -500)
  config file = None
  configured module search path = [u'/home/aherrington/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/aherrington/code/awx/ansible/lib/ansible
  executable location = /home/aherrington/code/awx/ansible/bin/ansible
  python version = 2.7.15rc1 (default, Apr 15 2018, 21:51:34) [GCC 7.3.0]

```

##### ADDITIONAL INFORMATION



Problem statement:
Support for message threading
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Feature Idea

##### COMPONENT NAME
<!--- Name of the module/plugin/task/feature -->
slack
##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.1.0.0
```

##### CONFIGURATION
<!---
Mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say “N/A” for anything that is not platform-specific.
-->
N/A
##### SUMMARY
<!--- Explain the problem briefly -->
When spamming notifications to Slack channels, it would be nice if we could take advantage of message threading (thread_ts).
##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml

```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes below -->
```

```

Hints from issue discussion:
cc @ramondelafuente
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Fix sanity check

Patch to review:
diff --git a/lib/ansible/modules/notification/slack.py b/lib/ansible/modules/notification/slack.py
index 9c7c2f160c050f..068ce6094bb08c 100644
--- a/lib/ansible/modules/notification/slack.py
+++ b/lib/ansible/modules/notification/slack.py
@@ -55,6 +55,10 @@
   channel:
     description:
       - Channel to send the message to. If absent, the message goes to the channel selected for the I(token).
+  thread_ts:
+    version_added: 2.8
+    description:
+      - Optional. TS of message to thread to as a float
   username:
     description:
       - This is the sender of the message.
@@ -113,6 +117,7 @@
     token: thetoken/generatedby/slack
     msg: '{{ inventory_hostname }} completed'
     channel: '#ansible'
+    thread_ts: 1539917263.000100
     username: 'Ansible on {{ inventory_hostname }}'
     icon_url: http://www.example.com/some-image-file.png
     link_names: 0
@@ -173,7 +178,8 @@ def escape_quotes(text):
     return "".join(escape_table.get(c, c) for c in text)
 
 
-def build_payload_for_slack(module, text, channel, username, icon_url, icon_emoji, link_names, parse, color, attachments):
+def build_payload_for_slack(module, text, channel, thread_ts, username, icon_url, icon_emoji, link_names,
+                            parse, color, attachments):
     payload = {}
     if color == "normal" and text is not None:
         payload = dict(text=escape_quotes(text))
@@ -185,6 +191,8 @@ def build_payload_for_slack(module, text, channel, username, icon_url, icon_emoj
             payload['channel'] = channel
         else:
             payload['channel'] = '#' + channel
+    if thread_ts is not None:
+        payload['thread_ts'] = thread_ts
     if username is not None:
         payload['username'] = username
     if icon_emoji is not None:
@@ -228,7 +236,8 @@ def do_notify_slack(module, domain, token, payload):
         slack_incoming_webhook = SLACK_INCOMING_WEBHOOK % (token)
     else:
         if not domain:
-            module.fail_json(msg="Slack has updated its webhook API.  You need to specify a token of the form XXXX/YYYY/ZZZZ in your playbook")
+            module.fail_json(msg="Slack has updated its webhook API.  You need to specify a token of the form "
+                                 "XXXX/YYYY/ZZZZ in your playbook")
         slack_incoming_webhook = OLD_SLACK_INCOMING_WEBHOOK % (domain, token)
 
     headers = {
@@ -249,6 +258,7 @@ def main():
             token=dict(type='str', required=True, no_log=True),
             msg=dict(type='str', required=False, default=None),
             channel=dict(type='str', default=None),
+            thread_ts=dict(type='float', default=None),
             username=dict(type='str', default='Ansible'),
             icon_url=dict(type='str', default='https://www.ansible.com/favicon.ico'),
             icon_emoji=dict(type='str', default=None),
@@ -264,6 +274,7 @@ def main():
     token = module.params['token']
     text = module.params['msg']
     channel = module.params['channel']
+    thread_ts = module.params['thread_ts']
     username = module.params['username']
     icon_url = module.params['icon_url']
     icon_emoji = module.params['icon_emoji']
@@ -272,7 +283,8 @@ def main():
     color = module.params['color']
     attachments = module.params['attachments']
 
-    payload = build_payload_for_slack(module, text, channel, username, icon_url, icon_emoji, link_names, parse, color, attachments)
+    payload = build_payload_for_slack(module, text, channel, thread_ts, username, icon_url, icon_emoji, link_names,
+                                      parse, color, attachments)
     do_notify_slack(module, domain, token, payload)
 
     module.exit_json(msg="OK")
diff --git a/test/units/modules/notification/test_slack.py b/test/units/modules/notification/test_slack.py
new file mode 100644
index 00000000000000..aba9d21d5567ca
--- /dev/null
+++ b/test/units/modules/notification/test_slack.py
@@ -0,0 +1,85 @@
+import json
+import pytest
+from units.compat.mock import patch
+from ansible.modules.notification import slack
+from units.modules.utils import AnsibleExitJson, AnsibleFailJson, ModuleTestCase, set_module_args
+from ansible import module_utils
+
+
+class TestSlackModule(ModuleTestCase):
+
+    def setUp(self):
+        super(TestSlackModule, self).setUp()
+        self.module = slack
+
+    def tearDown(self):
+        super(TestSlackModule, self).tearDown()
+
+    @pytest.fixture
+    def fetch_url_mock(self, mocker):
+        return mocker.patch('ansible.module_utils.notification.slack.fetch_url')
+
+    def test_without_required_parameters(self):
+        """Failure must occurs when all parameters are missing"""
+        with self.assertRaises(AnsibleFailJson):
+            set_module_args({})
+            self.module.main()
+
+    def test_invalid_old_token(self):
+        """Failure if there is an old style token"""
+        set_module_args({
+            'token': 'test',
+        })
+        with self.assertRaises(AnsibleFailJson):
+            self.module.main()
+
+    def test_sucessful_message(self):
+        """tests sending a message. This is example 1 from the docs"""
+        set_module_args({
+            'token': 'XXXX/YYYY/ZZZZ',
+            'msg': 'test'
+        })
+
+        with patch.object(slack, "fetch_url") as fetch_url_mock:
+            fetch_url_mock.return_value = (None, {"status": 200})
+            with self.assertRaises(AnsibleExitJson):
+                self.module.main()
+
+            self.assertTrue(fetch_url_mock.call_count, 1)
+            call_data = json.loads(fetch_url_mock.call_args[1]['data'])
+            assert call_data['username'] == "Ansible"
+            assert call_data['text'] == "test"
+            assert fetch_url_mock.call_args[1]['url'] == "https://hooks.slack.com/services/XXXX/YYYY/ZZZZ"
+
+    def test_failed_message(self):
+        """tests failing to send a message"""
+
+        set_module_args({
+            'token': 'XXXX/YYYY/ZZZZ',
+            'msg': 'test'
+        })
+
+        with patch.object(slack, "fetch_url") as fetch_url_mock:
+            fetch_url_mock.return_value = (None, {"status": 404, 'msg': 'test'})
+            with self.assertRaises(AnsibleFailJson):
+                self.module.main()
+
+    def test_message_with_thread(self):
+        """tests sending a message with a thread"""
+        set_module_args({
+            'token': 'XXXX/YYYY/ZZZZ',
+            'msg': 'test',
+            'thread_ts': 100.00
+        })
+
+        with patch.object(slack, "fetch_url") as fetch_url_mock:
+            fetch_url_mock.return_value = (None, {"status": 200})
+            with self.assertRaises(AnsibleExitJson):
+                self.module.main()
+
+            self.assertTrue(fetch_url_mock.call_count, 1)
+            call_data = json.loads(fetch_url_mock.call_args[1]['data'])
+            assert call_data['username'] == "Ansible"
+            assert call_data['text'] == "test"
+            assert call_data['thread_ts'] == 100.00
+            assert fetch_url_mock.call_args[1]['url'] == "https://hooks.slack.com/services/XXXX/YYYY/ZZZZ"


Code review comments:
diff_hunk: @@ -55,6 +55,10 @@
   channel:
     description:
       - Channel to send the message to. If absent, the message goes to the channel selected for the I(token).
+  thread_ts:
path: lib/ansible/modules/notification/slack.py
text: @user1:
From a UI perspective, `thread_ts` is an obtuse name. Yes, it's accurate to the API, but maybe `thread_id` would be a better name. It could still be `thread_ts` internally.

@user1:
Also, thanks for adding this. Very nice feature!