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

Mqtt alarm added value_template and code_arm_required #19558

Merged
merged 11 commits into from Feb 28, 2019

Conversation

Projects
None yet
7 participants
@ToRvaLDz
Copy link
Contributor

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.

@ToRvaLDz ToRvaLDz referenced this pull request Dec 24, 2018

Closed

Mqtt alarm code_arm_required #7940

2 of 2 tasks complete
@emontnemery

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2019

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

@ToRvaLDz

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor

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:

async def test_controlling_state_via_topic_and_json_message(

def test_arm_home_not_publishes_mqtt_with_invalid_code(self):

def test_update_state_via_state_topic(self):

@balloob
Copy link
Member

left a comment

Please add tests for new functionality

@ToRvaLDz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

Please add tests for new functionality

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

@balloob

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

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

@balloob balloob added the almost-done label Feb 26, 2019

ToRvaLDz added some commits Dec 24, 2018

Added value_template config for parsing json value from state topic
Added arm_code_required to avoid code enter when arming
Merge branch 'dev' of github.com:home-assistant/home-assistant into dev
# Conflicts:
#	homeassistant/components/alarm_control_panel/mqtt.py
#	tests/components/alarm_control_panel/test_mqtt.py

@ToRvaLDz ToRvaLDz requested a review from home-assistant/core as a code owner Feb 27, 2019

@ToRvaLDz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@balloob conflicts should be fixed

Tests have been added and approved.

@emontnemery emontnemery merged commit c3d4738 into home-assistant:dev Feb 28, 2019

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
coverage/coveralls Coverage increased (+0.0004%) to 92.75%
Details

@ghost ghost removed the in progress label Feb 28, 2019

@ToRvaLDz ToRvaLDz referenced this pull request Mar 4, 2019

Merged

Mqtt alarm code_arm_required #8822

2 of 2 tasks complete
@JumpMaster

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

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

@ToRvaLDz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

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

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

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.
You can’t perform that action at this time.