Support multiple attachments in signal messenger integration by bbernhard · Pull Request #31141 · home-assistant/core

bbernhard · 2020-01-24T20:39:43Z

Breaking change

Proposed change

There was a feature request to support multiple attachments in a single Signal message. (bbernhard/signal-cli-rest-api#5) Unfortunately, this change wasn't possible without a breaking backwards compatibility (at least I haven't found a nice way to accomplish that; suggestions are welcome).

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

see https://www.home-assistant.io/integrations/signal_messenger/

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Support multiple attachments in signal messenger integration home-assistant.io#11863

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

rohankapoorcom · 2020-01-26T23:41:38Z

-        filename = None
-        if data is not None and ATTR_FILENAME in data:
-            filename = data[ATTR_FILENAME]
+        filenames = None


I would prefer that we avoid the breaking change unnecessarily here.

This could be done by allowing both the attribute filename as well as the attribute filenames to be provided and then internally adding the filename to the list of filenames.

Sounds good to me 👍 I'll adapt the PR, thanks!

Should the attachment option 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?

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_validation

Alternatively we could keep using attachment as the key and accept both a singular string or a list of strings.

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 changes section from the PR description)

* stay backwards compatible by both allowing the "attachment" and the "attachments" attribute.

* added deprecation warning for 'attachment' attribute

rohankapoorcom

Looks good, thanks for incorporating the feedback. The build is breaking though, that will need to be fixed before merge.

* use 'warning' instead of 'warn' when logging the warning message

bbernhard · 2020-02-01T16:37:33Z

Looks good, thanks for incorporating the feedback. The build is breaking though, that will need to be fixed before merge.

Thanks a lot for your suggestions!

I've updated the PR and everything except the codecov/patch seems to be successful now. Not sure what the codecov/patch test is all about nor how to fix it. Any help is appreciated! :)

springstan · 2020-02-01T16:53:42Z

I've updated the PR and everything except the codecov/patch seems to be successful now. Not sure what the codecov/patch test is all about nor how to fix it. Any help is appreciated! :)

The code coverage shows how much of the code is covered by tests. Therefore, you will have to add another test to hit the target for code coverage. Hope this helps! :)

rohankapoorcom · 2020-02-01T17:34:46Z

To clarify, codecov/patch is measuring the code coverage of the diff (exactly what changed in this pull request)

bbernhard · 2020-02-01T21:05:29Z

I've added now 4 tests, but looks like the code coverage is still less than the check expects. Unfortunately, I do not have any experience with code coverage tools, so it's not really clear to me how they calculate the test coverage and what I need to do in order to hit the target.

rohankapoorcom · 2020-02-01T21:29:24Z

You have to remove your files from the coverage ignore file:

    homeassistant/components/signal_messenger/__init__.py
    homeassistant/components/signal_messenger/notify.py

needs to be removed from https://github.com/home-assistant/home-assistant/blob/dev/.coveragerc#L624

bbernhard · 2020-02-01T21:34:04Z

You have to remove your files from the coverage ignore file:
    homeassistant/components/signal_messenger/__init__.py
    homeassistant/components/signal_messenger/notify.py
needs to be removed from https://github.com/home-assistant/home-assistant/blob/dev/.coveragerc#L624

oh, damn...silly me...totally forgot about that. Many thanks!

Hopefully the coverage will now improve 🤞

MartinHjelmare

Please address the comments in a new PR.

MartinHjelmare · 2020-02-02T12:17:23Z

+        assert hass.services.has_service(BASE_COMPONENT, "test")
+
+
+class TestSignalMesssenger(unittest.TestCase):


New tests must be async standalone pytest test functions.

MartinHjelmare · 2020-02-02T12:19:20Z

+        recipients = ["+435565656565"]
+        number = "+43443434343"
+        client = SignalCliRestApi("http://127.0.0.1:8080", number)
+        self._signalmessenger = signalmessenger.SignalNotificationService(


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.

MartinHjelmare · 2020-02-02T12:22:18Z

+    def test_send_message(self, mock):
+        """Test send message."""
+        message = "Testing Signal Messenger platform :)"
+        mock.register_uri(


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.

Bernhard B added 2 commits January 24, 2020 20:38

added support for multiple attachments to signal_messenger integration

e709d24

updated pysignalclirestapi version in requirements_all.txt

5463b25

homeassistant added the cla-signed label Jan 24, 2020

probot-home-assistant Bot added integration: signal_messenger small-pr PRs with less than 30 lines. by-code-owner labels Jan 24, 2020

bbernhard mentioned this pull request Jan 24, 2020

Support multiple attachments in signal messenger integration home-assistant/home-assistant.io#11863

Merged

7 tasks

bbernhard changed the title ~~Signalmessenger multiple attachments~~ support multiple attachments in signal messenger integration Jan 24, 2020

MartinHjelmare changed the title ~~support multiple attachments in signal messenger integration~~ Support multiple attachments in signal messenger integration Jan 25, 2020

rohankapoorcom requested changes Jan 26, 2020

View reviewed changes

Bernhard B added 3 commits January 27, 2020 18:29

reworked multiple attachments feature in signal_messenger integration

0a15af1

* stay backwards compatible by both allowing the "attachment" and the "attachments" attribute.

reworked multiple attachments feature in signal_messenger integration

a181e14

* stay backwards compatible by both allowing the "attachment" and the "attachments" attribute.

small change in signal_messenger integration

79d321c

* added deprecation warning for 'attachment' attribute

rohankapoorcom approved these changes Jan 30, 2020

View reviewed changes

springstan added the dependency Pull requests marked as a dependency upgrade label Feb 1, 2020

Bernhard B added 3 commits February 1, 2020 16:48

small changes in signal_messenger integration

236dc56

* use 'warning' instead of 'warn' when logging the warning message

Merge branch 'dev' into signalmessenger_multiple_attachments

1d22862

re-generated requirements_test_pre_commit.txt

74bdee6

Bernhard B added 3 commits February 1, 2020 19:59

added tests for signal_messenger integration

067a3b6

regenerated requirements_test_all.txt for signal_messenger integration

e4430d6

added more signal_messenger tests

816886c

remove signal_messenger integration files from .coveragerc

bfab393

rohankapoorcom approved these changes Feb 1, 2020

View reviewed changes

rohankapoorcom merged commit 294c6a7 into home-assistant:dev Feb 1, 2020

MartinHjelmare reviewed Feb 2, 2020

View reviewed changes

lock Bot locked and limited conversation to collaborators Feb 3, 2020

		assert hass.services.has_service(BASE_COMPONENT, "test")


		class TestSignalMesssenger(unittest.TestCase):

Uh oh!

Conversation

bbernhard commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

rohankapoorcom Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

bbernhard Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohankapoorcom Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbernhard Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

rohankapoorcom left a comment

Choose a reason for hiding this comment

Uh oh!

bbernhard commented Feb 1, 2020

Uh oh!

springstan commented Feb 1, 2020

Uh oh!

rohankapoorcom commented Feb 1, 2020

Uh oh!

bbernhard commented Feb 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohankapoorcom commented Feb 1, 2020

Uh oh!

bbernhard commented Feb 1, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bbernhard commented Jan 24, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

bbernhard Jan 27, 2020 •

edited

Loading

rohankapoorcom Jan 27, 2020 •

edited

Loading

bbernhard commented Feb 1, 2020 •

edited

Loading