Skip to content

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

Merged
balloob merged 7 commits intohome-assistant:devfrom
liaanvdm:alarm-manual-restore-state
Oct 25, 2018
Merged

Restore manual alarm-control-panel state using async_get_last_state#17521
balloob merged 7 commits intohome-assistant:devfrom
liaanvdm:alarm-manual-restore-state

Conversation

@liaanvdm
Copy link
Copy Markdown
Contributor

@liaanvdm 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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

balloob commented Oct 17, 2018

Makes sense. please add a test

@liaanvdm
Copy link
Copy Markdown
Contributor Author

Makes sense. please add a test

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

self.assertEqual(STATE_ALARM_TRIGGERED, state.state)


@asyncio.coroutine
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ghost ghost assigned balloob Oct 25, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 25, 2018

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

@balloob
Copy link
Copy Markdown
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
@ghost ghost removed the in progress label Oct 25, 2018
@liaanvdm
Copy link
Copy Markdown
Contributor Author

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 mentioned this pull request Nov 9, 2018
@edif30
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Manual Alarm doesn't restore previous state after reboot

6 participants