Skip to content

Mqtt alarm added value_template and code_arm_required #19558

Merged
emontnemery merged 11 commits into
home-assistant:devfrom
ToRvaLDz:dev
Feb 28, 2019
Merged

Mqtt alarm added value_template and code_arm_required #19558
emontnemery merged 11 commits into
home-assistant:devfrom
ToRvaLDz:dev

Conversation

@ToRvaLDz
Copy link
Copy Markdown
Contributor

@ToRvaLDz ToRvaLDz commented Dec 24, 2018

Description:

Added value_template config option to parse json incoming message.
Added code_arm_required config option to avoid code enter on arm.

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

Checklist:

  • The code change is tested and works locally.
  • 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.

Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
Comment thread homeassistant/components/alarm_control_panel/mqtt.py Outdated
@emontnemery
Copy link
Copy Markdown
Contributor

It looks pretty OK, please add tests and fix the lint error (move the docstring).

@ToRvaLDz
Copy link
Copy Markdown
Contributor Author

ToRvaLDz commented Jan 5, 2019

It looks pretty OK, please add tests and fix the lint error (move the docstring).

I fixed the lint error but I'm not a python developer and I'm not sure I can add tests :(

@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented Jan 6, 2019

I fixed the lint error but I'm not a python developer and I'm not sure I can add tests :(

The changes are OK, but you need to add new tests before merge.

Have a look here:
https://developers.home-assistant.io/docs/en/development_testing.html

Note that if you don't want to wait for Travis/tox to do the full suite of tests you can do:
script/lint to just lint your modifications
py.test tests/components/alarm_control_panel/test_mqtt.py to just test the alarm stuff
https://developers.home-assistant.io/docs/en/development_testing.html#testing-outside-of-tox

Inspiration for the test cases:
https://github.com/home-assistant/home-assistant/blob/76c30aca38d9c386ab4825439694646083a9bc59/tests/components/switch/test_mqtt.py#L98
https://github.com/home-assistant/home-assistant/blob/76c30aca38d9c386ab4825439694646083a9bc59/tests/components/alarm_control_panel/test_mqtt.py#L117
https://github.com/home-assistant/home-assistant/blob/76c30aca38d9c386ab4825439694646083a9bc59/tests/components/alarm_control_panel/test_mqtt.py#L58

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for new functionality

@ToRvaLDz
Copy link
Copy Markdown
Contributor Author

Please add tests for new functionality

I'm sorry for late reply, can you please check if tests are ok? Tnx

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 26, 2019

Hey @ToRvaLDz, the tests are great! Ok to merge when merge conflicts resolved.

@ToRvaLDz ToRvaLDz requested a review from a team as a code owner February 27, 2019 14:31
@ToRvaLDz
Copy link
Copy Markdown
Contributor Author

@balloob conflicts should be fixed

@emontnemery emontnemery dismissed balloob’s stale review February 28, 2019 16:39

Tests have been added and approved.

@emontnemery emontnemery merged commit c3d4738 into home-assistant:dev Feb 28, 2019
@ghost ghost removed the in progress label Feb 28, 2019
@JumpMaster
Copy link
Copy Markdown

Sorry to jump in after this has been merged. But shouldn't this also be applied to async_alarm_arm_night?

@ToRvaLDz
Copy link
Copy Markdown
Contributor Author

ToRvaLDz commented Mar 8, 2019

@JumpMaster your're right,tnx. @balloob how can I fix it, need to open another PR?

@MartinHjelmare
Copy link
Copy Markdown
Member

Please open a new PR.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Mar 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants