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 Call Data Log platform. Mailboxes no longer require media #16579

Merged
merged 12 commits into from Sep 21, 2018

Conversation

@PhracturedBlue
Contributor

PhracturedBlue commented Sep 12, 2018

Description:

This patch adds the asterisk_cdr mailbox platform to view call activity. It also makes it possible to dynamically create mailbox platforms (needed in case CDR isn't supplied by the server). Additionally, mailboxes without media are now supported, as are read-only messages, making the mailbox platform more generic. Lastly all asycio.coroutines have been migrated to async/await.
This pull request requires a matching frontend change

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6264
Pull request in home-assistant-polymer for frontend updates: home-assistant/home-assistant-polymer#1660

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 files were added to .coveragerc.

PhracturedBlue added some commits Aug 30, 2018

@@ -21,8 +20,7 @@
SIGNAL_MESSAGE_UPDATE = 'asterisk_mbox.message_updated'
@asyncio.coroutine
def async_get_handler(hass, config, async_add_entities, discovery_info=None):
async def async_get_handler(hass, config, async_add_entities, discovery_info=None):

This comment has been minimized.

@houndci-bot

houndci-bot Sep 12, 2018

line too long (83 > 79 characters)

DOMAIN = "asterisk_cdr"
async def async_get_handler(hass, config, async_add_devices, discovery_info=None):

This comment has been minimized.

@houndci-bot

houndci-bot Sep 12, 2018

line too long (82 > 79 characters)

@PhracturedBlue PhracturedBlue referenced this pull request Sep 12, 2018

Merged

Add asterisk_cdr platform #6264

2 of 2 tasks complete

PhracturedBlue added some commits Sep 13, 2018

DEPENDENCIES = ['asterisk_mbox']
_LOGGER = logging.getLogger(__name__)
DOMAIN = "asterisk_cdr"

This comment has been minimized.

@balloob

balloob Sep 17, 2018

Member

Please don't call it DOMAIN as platforms don't have domains.

@balloob

Ok to merge when variable renamed.

@PhracturedBlue

This comment has been minimized.

Contributor

PhracturedBlue commented Sep 17, 2018

Thanks, should be better now

async_dispatcher_connect(
self.hass, SIGNAL_MESSAGE_REQUEST, self._request_messages)
async_dispatcher_connect(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

This must be called from within the event loop but it's called from setup ie from a worker thread. Change to use the sync version.

platform.async_get_handler(hass, p_config, discovery_info)
elif hasattr(platform, 'get_handler'):
mailbox = yield from hass.async_add_job(
mailbox = await hass.async_add_job(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Use hass.async_add_executor_job.

Asterisk CDR interface.
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/mailbox.asteriskvm/

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

The url is wrong.

from homeassistant.core import callback
from homeassistant.components.asterisk_mbox import SIGNAL_CDR_UPDATE
from homeassistant.components.asterisk_mbox import DOMAIN

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

This platform belongs to the mailbox domain. Import the asterix_mbox domain as another name.

from homeassistant.components.asterisk_mbox import DOMAIN as ASTERIX_MBOX_DOMAIN

@PhracturedBlue PhracturedBlue force-pushed the PhracturedBlue:asteriskvm4 branch from 008e69a to 31b0f97 Sep 17, 2018

@PhracturedBlue

This comment has been minimized.

Contributor

PhracturedBlue commented Sep 17, 2018

@MartinHjelmare Thanks for the review. Moving those async calls to sync uncovered a real race condition. Is there any way to detect async calls made from the wrong thread to help prevent these types of errors?

I believe I have resolved all of your concerns.

@@ -4,7 +4,6 @@
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/mailbox.asteriskvm/
"""
import asyncio
import logging
from homeassistant.components.asterisk_mbox import DOMAIN

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Please import this as another name too.

@@ -18,8 +17,7 @@
DOMAIN = "DemoMailbox"

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Please rename the constant. The string is ok.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 17, 2018

Not sure how to debug that.

Maybe by timing callbacks?

https://pymotw.com/3/asyncio/debugging.html

@PhracturedBlue

This comment has been minimized.

Contributor

PhracturedBlue commented Sep 18, 2018

Not sure how to debug that.

Maybe by timing callbacks?

https://pymotw.com/3/asyncio/debugging.html

Sorry, I wasn't clear. I meant that changing to using the sync versions uncovered a timing race which I have already fixed. I was just wondering in general about checkers to help find these types of issues before I submit patches

@PhracturedBlue PhracturedBlue force-pushed the PhracturedBlue:asteriskvm4 branch from e94d67e to 644144c Sep 18, 2018

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Sep 18, 2018

Yes, that was what I was answering.

@PhracturedBlue PhracturedBlue force-pushed the PhracturedBlue:asteriskvm4 branch from 644144c to 3a2a972 Sep 18, 2018

@PhracturedBlue PhracturedBlue force-pushed the PhracturedBlue:asteriskvm4 branch from 3a2a972 to 2f80578 Sep 18, 2018

@PhracturedBlue

This comment has been minimized.

Contributor

PhracturedBlue commented Sep 18, 2018

Thanks, hopefully I've addressed your concerns now. Please let me know if you see anything else

@MartinHjelmare

Looks good! I'll let @balloob merge since this depends on a frontend PR.

@balloob balloob merged commit 98b92c7 into home-assistant:dev Sep 21, 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 decreased (-0.009%) to 93.687%
Details

@wafflebot wafflebot bot removed the in progress label Sep 21, 2018

@balloob balloob referenced this pull request Sep 28, 2018

Merged

0.79.0 #16940

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