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

Restore manual alarm-control-panel state using async_get_last_state #17521

Merged
merged 7 commits into from Oct 25, 2018

Conversation

Projects
None yet
6 participants
@liaanvdm
Contributor

liaanvdm commented Oct 16, 2018

Description:

This small change restores the manual alarm control panel component's state should homeassistant be restarted for whatever reason. The change uses async_get_last_state as used by components like input-boolean etc.

Related issue (if applicable): fixes #10793

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

Example entry for configuration.yaml (if applicable):

N/A - No change to configuration required

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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.
@homeassistant

This comment has been minimized.

homeassistant commented Oct 16, 2018

Hi @liaanvdm,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@liaanvdm

This comment has been minimized.

Contributor

liaanvdm commented Oct 17, 2018

This fix will restore the last state logged that was logged by the logger component whenever hass restarts. This would solve the related issue but I'm just wondering if the right thing might not be to also add an initial state configuration option that would overwrite this behaviour. I could add this as well but I honestly can't think of a use case where restoring the last state would not be what you want. As is, this fix is definitely better than the current implementation where the state is hard-coded to be initialised to Disarmed.

As a personal sufferer of this issue, I would certainly be happy with this fix as is. What do you guys think?

@balloob

This comment has been minimized.

Member

balloob commented Oct 17, 2018

Makes sense. please add a test

@liaanvdm

This comment has been minimized.

Contributor

liaanvdm commented Oct 17, 2018

Makes sense. please add a test

I've added two tests, one for restoring armed state and one for restoring disarmed state

@@ -1319,3 +1319,51 @@ def test_arm_away_after_disabled_disarmed(self):

state = self.hass.states.get(entity_id)
self.assertEqual(STATE_ALARM_TRIGGERED, state.state)


@asyncio.coroutine

This comment has been minimized.

@balloob

balloob Oct 18, 2018

Member

We've migrated to async/await syntax. Please drop this decorator and prefix async methods with async like async def test_restore_armed_state . Replace any yield from with await.

This comment has been minimized.

@liaanvdm

liaanvdm Oct 18, 2018

Contributor

I've change the syntax as requested. I based my tests on the tests i found in the input variable components, those are still using the asyncio decorator.

This comment has been minimized.

@balloob

balloob Oct 25, 2018

Member

I know. Those need to be updated too. For now we just make sure we don't add new ones.

@balloob

This comment has been minimized.

Member

balloob commented Oct 25, 2018

Looks good. I just resolved a merge conflict that sneaked in . Will merge when tests pass.

@balloob

This comment has been minimized.

Member

balloob commented Oct 25, 2018

Congratulations on your first PR, great job ! 👍

@balloob balloob merged commit 3c68db3 into home-assistant:dev Oct 25, 2018

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.005%) to 93.271%
Details

@wafflebot wafflebot bot removed the in progress label Oct 25, 2018

@liaanvdm

This comment has been minimized.

Contributor

liaanvdm commented Oct 26, 2018

Congratulations on your first PR, great job !

Thanks balloob! It feels good to be able to give back to this fantastic project, albeit such a tiny contribution. I will certainly be contributing more now that i know the process a bit better.

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

@edif30

This comment has been minimized.

Contributor

edif30 commented Nov 10, 2018

@liaanvdm I know this has been merged but I am wondering if this would be the same fix for when you use alarm control panel with IFTTT. Upon a restart, the state shows "unknown" until you set it again.

See docs. https://www.home-assistant.io/components/alarm_control_panel.ifttt/

I don't know async so I am not sure.

@liaanvdm

This comment has been minimized.

Contributor

liaanvdm commented Nov 11, 2018

@edif30 I'm afraid this fix only applies to the manual alarm control panel. The issue you described will have to be addressed separately. You'll have to log an issue unless one exists already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment