-
-
Notifications
You must be signed in to change notification settings - Fork 37.5k
Support multiple attachments in signal messenger integration #31141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e709d24
5463b25
0a15af1
a181e14
79d321c
236dc56
1d22862
74bdee6
067a3b6
e4430d6
816886c
bfab393
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| bandit==1.6.2 | ||
| black==19.10b0 | ||
| codespell==v1.16.0 | ||
| flake8-docstrings==1.5.0 | ||
| flake8==3.7.9 | ||
| isort==v4.3.21 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Tests for the signal_messenger component.""" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| """The tests for the signal_messenger platform.""" | ||
|
|
||
| import os | ||
| import tempfile | ||
| import unittest | ||
| from unittest.mock import patch | ||
|
|
||
| from pysignalclirestapi import SignalCliRestApi | ||
| import requests_mock | ||
|
|
||
| import homeassistant.components.signal_messenger.notify as signalmessenger | ||
| from homeassistant.setup import async_setup_component | ||
|
|
||
| BASE_COMPONENT = "notify" | ||
|
|
||
|
|
||
| async def test_signal_messenger_init(hass): | ||
| """Test that service loads successfully.""" | ||
|
|
||
| config = { | ||
| BASE_COMPONENT: { | ||
| "name": "test", | ||
| "platform": "signal_messenger", | ||
| "url": "http://127.0.0.1:8080", | ||
| "number": "+43443434343", | ||
| "recipients": ["+435565656565"], | ||
| } | ||
| } | ||
|
|
||
| with patch("pysignalclirestapi.SignalCliRestApi.send_message", return_value=None): | ||
| assert await async_setup_component(hass, BASE_COMPONENT, config) | ||
| await hass.async_block_till_done() | ||
|
|
||
| # Test that service loads successfully | ||
| assert hass.services.has_service(BASE_COMPONENT, "test") | ||
|
|
||
|
|
||
| class TestSignalMesssenger(unittest.TestCase): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New tests must be async standalone pytest test functions. |
||
| """Test the signal_messenger notify.""" | ||
|
|
||
| def setUp(self): | ||
| """Set up things to be run when tests are started.""" | ||
| recipients = ["+435565656565"] | ||
| number = "+43443434343" | ||
| client = SignalCliRestApi("http://127.0.0.1:8080", number) | ||
| self._signalmessenger = signalmessenger.SignalNotificationService( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should never set up a notify service or create an entity directly. We should always use the core to set up the component and platform. That will make the tests robust. |
||
| recipients, client | ||
| ) | ||
|
|
||
| @requests_mock.Mocker() | ||
| def test_send_message(self, mock): | ||
| """Test send message.""" | ||
| message = "Testing Signal Messenger platform :)" | ||
| mock.register_uri( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to test the library requests. We should patch the library and make sure that we are interfacing with the library correctly. The library is responsible for testing that it makes the correct requests. |
||
| "POST", "http://127.0.0.1:8080/v2/send", status_code=201, | ||
| ) | ||
| mock.register_uri( | ||
| "GET", | ||
| "http://127.0.0.1:8080/v1/about", | ||
| status_code=200, | ||
| json={"versions": ["v1", "v2"]}, | ||
| ) | ||
| with self.assertLogs( | ||
| "homeassistant.components.signal_messenger.notify", level="DEBUG" | ||
| ) as context: | ||
| self._signalmessenger.send_message(message) | ||
| self.assertIn("Sending signal message", context.output[0]) | ||
| self.assertTrue(mock.called) | ||
| self.assertEqual(mock.call_count, 2) | ||
|
|
||
| @requests_mock.Mocker() | ||
| def test_send_message_should_show_deprecation_warning(self, mock): | ||
| """Test send message.""" | ||
| message = "Testing Signal Messenger platform with attachment :)" | ||
| mock.register_uri( | ||
| "POST", "http://127.0.0.1:8080/v2/send", status_code=201, | ||
| ) | ||
| mock.register_uri( | ||
| "GET", | ||
| "http://127.0.0.1:8080/v1/about", | ||
| status_code=200, | ||
| json={"versions": ["v1", "v2"]}, | ||
| ) | ||
| with self.assertLogs( | ||
| "homeassistant.components.signal_messenger.notify", level="WARNING" | ||
| ) as context: | ||
| with tempfile.NamedTemporaryFile( | ||
| suffix=".png", prefix=os.path.basename(__file__) | ||
| ) as tf: | ||
| data = {"data": {"attachment": tf.name}} | ||
| self._signalmessenger.send_message(message, **data) | ||
| self.assertIn( | ||
| "The 'attachment' option is deprecated, please replace it with 'attachments'. This option will become invalid in version 0.108.", | ||
| context.output[0], | ||
| ) | ||
| self.assertTrue(mock.called) | ||
| self.assertEqual(mock.call_count, 2) | ||
|
|
||
| @requests_mock.Mocker() | ||
| def test_send_message_with_attachment(self, mock): | ||
| """Test send message.""" | ||
| message = "Testing Signal Messenger platform :)" | ||
| mock.register_uri( | ||
| "POST", "http://127.0.0.1:8080/v2/send", status_code=201, | ||
| ) | ||
| mock.register_uri( | ||
| "GET", | ||
| "http://127.0.0.1:8080/v1/about", | ||
| status_code=200, | ||
| json={"versions": ["v1", "v2"]}, | ||
| ) | ||
| with self.assertLogs( | ||
| "homeassistant.components.signal_messenger.notify", level="DEBUG" | ||
| ) as context: | ||
| with tempfile.NamedTemporaryFile( | ||
| suffix=".png", prefix=os.path.basename(__file__) | ||
| ) as tf: | ||
| data = {"data": {"attachments": [tf.name]}} | ||
| self._signalmessenger.send_message(message, **data) | ||
| self.assertIn("Sending signal message", context.output[0]) | ||
| self.assertTrue(mock.called) | ||
| self.assertEqual(mock.call_count, 2) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that we avoid the breaking change unnecessarily here.
This could be done by allowing both the attribute
filenameas well as the attributefilenamesto be provided and then internally adding thefilenameto the list offilenames.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me 👍 I'll adapt the PR, thanks!
Should the
attachmentoption also be mentioned in the documentation then, or would you see that primarily as a way to stay backwards compatible and as something that's not worth mentioning in the documentation?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would mark it as deprecated and log a warning that it's deprecated and will be removed in version x.y.z (pick a version that's like 3 versions out).
If you are using vol to validate the schema of the service call, we have the deprecated validation helper in
config_validationAlternatively we could keep using attachment as the key and accept both a singular string or a list of strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your suggestions, very much appreciated!
I've updated the PR, please let me know if there's anything else that needs to be changed.
(I've also removed the
Breaking changessection from the PR description)