Skip to content
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

Add huawei_lte notify component #19544

Merged
merged 2 commits into from Feb 1, 2019

Conversation

Projects
None yet
5 participants
@scop
Copy link
Contributor

scop commented Dec 23, 2018

Description:

Add notify component using Huawei LTE router SMS.

Related issue (if applicable): https://community.home-assistant.io/t/adding-notification-to-huawei-lte-router/70950

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7934

Example entry for configuration.yaml (if applicable):

notify:
  - platform: huawei_lte
    target: "+15105550123"

Checklist:

  • The code change is tested and works locally. Caveat: tested without an SMS enabled plan, so verified only that the API calls succeed and fail when/like they should, and sent messages end up in the router's SMS outbox.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@andyinno

This comment has been minimized.

Copy link

andyinno commented Dec 28, 2018

Changing

from ..huawei_lte import DATA_KEY

to

from homeassistant.components.huawei_lte import DATA_KEY

Allowed me to test it as custom component.
I tested the sending of the sms 4 times and 2 times it was successful. The other two times I reached line 60 and the error message appeared.

@scop

This comment has been minimized.

Copy link
Contributor Author

scop commented Jan 16, 2019

Anything I can do to get this moving forward? I don't think there's anything to be done for send failures besides logging them here, if they're not about some syntax errors caused by the implementation (which I think they aren't, but can't obviously tell for sure without seeing the error messages).

@andyinno

This comment has been minimized.

Copy link

andyinno commented Jan 16, 2019

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good! Just a small comment.


PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_URL): cv.url,
vol.Required(ATTR_TARGET): vol.All(cv.ensure_list, [cv.string]),

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 31, 2019

Member

We should use CONF_ constants for config keys

This comment has been minimized.

@scop

scop Feb 1, 2019

Author Contributor

Changed to CONF_RECIPIENT as that seems to be used for this purpose in other notify component configs. But note that the similar netgear_lte and tplink_lte components use ATTR_TARGET, that's where I initially picked it up from. Maybe it should somehow be fixed in them too.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 1, 2019

Member

It would be good to fix that, yes.

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

It's target in the notify service itself so I'm not sure why we would use recipient in the platforms?

https://www.home-assistant.io/components/notify/#service

This comment has been minimized.

@scop

scop Feb 1, 2019

Author Contributor

For consistency within platforms? I have no strong opinions either way, but the majority of platforms already use recipient (clickatell, clicksend, clicksend_tts, mailgun, sendgrid, smtp, xmpp, yessssms, and now huawei_lte).

This comment has been minimized.

@amelchio

amelchio Feb 1, 2019

Member

I'm aiming for consistency, period. Switching nine platforms over now might seem like a lot of work, but better to do it before it's twentynine ...

I also have no particular preference for either word but for usability alone, I think it should be the same everywhere.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 1, 2019

Member

Target is easier to spell. 😄

This comment has been minimized.

@scop

scop Feb 2, 2019

Author Contributor

Recipient is more descriptive :)

@MartinHjelmare MartinHjelmare merged commit 3f997ae into home-assistant:dev Feb 1, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 93.044%
Details

@wafflebot wafflebot bot removed the in progress label Feb 1, 2019

@scop scop deleted the scop:huawei-lte-notify branch Feb 1, 2019

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.