Add Call Data Log platform. Mailboxes no longer require media #16579

PhracturedBlue · 2018-09-12T13:00:06Z

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/frontend#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:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

New files were added to .coveragerc.

…initializing CDR

houndci-bot · 2018-09-12T13:00:22Z

homeassistant/components/mailbox/asterisk_mbox.py

@@ -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):


line too long (83 > 79 characters)

houndci-bot · 2018-09-12T13:00:22Z

homeassistant/components/mailbox/asterisk_cdr.py

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


line too long (82 > 79 characters)

balloob · 2018-09-17T08:40:05Z

homeassistant/components/mailbox/asterisk_cdr.py

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


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

balloob

Ok to merge when variable renamed.

PhracturedBlue · 2018-09-17T12:16:52Z

Thanks, should be better now

MartinHjelmare · 2018-09-17T17:32:27Z

homeassistant/components/asterisk_mbox.py


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


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.

MartinHjelmare · 2018-09-17T17:36:22Z

homeassistant/components/mailbox/__init__.py

                    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(


Use hass.async_add_executor_job.

MartinHjelmare · 2018-09-17T17:38:31Z

homeassistant/components/mailbox/asterisk_cdr.py

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


The url is wrong.

MartinHjelmare · 2018-09-17T17:39:18Z

homeassistant/components/mailbox/asterisk_cdr.py

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


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 · 2018-09-17T21:42:53Z

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

MartinHjelmare · 2018-09-17T22:22:41Z

homeassistant/components/mailbox/asterisk_mbox.py

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


Please import this as another name too.

MartinHjelmare · 2018-09-17T22:24:33Z

homeassistant/components/mailbox/demo.py

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


Please rename the constant. The string is ok.

MartinHjelmare · 2018-09-17T22:33:19Z

Not sure how to debug that.

Maybe by timing callbacks?

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

PhracturedBlue · 2018-09-18T00:53:03Z

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

MartinHjelmare · 2018-09-18T02:39:34Z

Yes, that was what I was answering.

PhracturedBlue · 2018-09-18T13:55:23Z

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.

PhracturedBlue added 6 commits September 11, 2018 05:56

Add multiple mailbox support

559c59b

Fix extraneous debugging

8b65450

Add cdr support

baa28fb

liniting errors

47fd776

Mailbox log messages should mostly be debug. Fix race condition with …

52190d8

…initializing CDR

async decorators to async

2bf8742

homeassistant added cla-signed integration: asterisk_mbox integration: mailbox new-platform platform: mailbox.demo labels Sep 12, 2018

ghost added the in progress label Sep 12, 2018

houndci-bot reviewed Sep 12, 2018

View reviewed changes

PhracturedBlue mentioned this pull request Sep 12, 2018

Add support for multiple separate mailboxes home-assistant/frontend#1660

Merged

Lint fixes

b1806a8

homeassistant added the cla-signed label Sep 12, 2018

PhracturedBlue mentioned this pull request Sep 12, 2018

Add asterisk_cdr platform home-assistant/home-assistant.io#6264

Merged

2 tasks

PhracturedBlue added 2 commits September 13, 2018 05:32

Typo

30d75ef

remove unneeded parameter

58f5531

balloob reviewed Sep 17, 2018

View reviewed changes

balloob approved these changes Sep 17, 2018

View reviewed changes

Fix variable names

fc359dd

MartinHjelmare requested changes Sep 17, 2018

View reviewed changes

Fix async calls from worker thread. Other minor cleanups

31b0f97

PhracturedBlue force-pushed the asteriskvm4 branch from 008e69a to 31b0f97 Compare September 17, 2018 20:21

homeassistant added the cla-signed label Sep 17, 2018

MartinHjelmare reviewed Sep 17, 2018

View reviewed changes

homeassistant added the cla-signed label Sep 18, 2018

PhracturedBlue force-pushed the asteriskvm4 branch from e94d67e to 644144c Compare September 18, 2018 01:45

homeassistant added the cla-signed label Sep 18, 2018

PhracturedBlue force-pushed the asteriskvm4 branch from 644144c to 3a2a972 Compare September 18, 2018 09:59

homeassistant added the cla-signed label Sep 18, 2018

more variable renames

2f80578

PhracturedBlue force-pushed the asteriskvm4 branch from 3a2a972 to 2f80578 Compare September 18, 2018 10:30

homeassistant added the cla-signed label Sep 18, 2018

MartinHjelmare approved these changes Sep 18, 2018

View reviewed changes

balloob merged commit 98b92c7 into home-assistant:dev Sep 21, 2018

ghost removed the in progress label Sep 21, 2018

balloob removed the platform: mailbox.demo label Sep 28, 2018

balloob mentioned this pull request Sep 28, 2018

0.79.0 #16940

Merged

home-assistant locked and limited conversation to collaborators Feb 5, 2019

ghost added integration: asterisk_cdr and removed platform: mailbox.asterisk_cdr labels Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Call Data Log platform. Mailboxes no longer require media #16579

Add Call Data Log platform. Mailboxes no longer require media #16579

PhracturedBlue commented Sep 12, 2018 •

edited

houndci-bot Sep 12, 2018

houndci-bot Sep 12, 2018

balloob Sep 17, 2018

balloob left a comment

PhracturedBlue commented Sep 17, 2018

MartinHjelmare Sep 17, 2018

MartinHjelmare Sep 17, 2018

MartinHjelmare Sep 17, 2018

MartinHjelmare Sep 17, 2018

PhracturedBlue commented Sep 17, 2018

MartinHjelmare Sep 17, 2018

MartinHjelmare Sep 17, 2018

MartinHjelmare commented Sep 17, 2018

PhracturedBlue commented Sep 18, 2018

MartinHjelmare commented Sep 18, 2018

PhracturedBlue commented Sep 18, 2018

MartinHjelmare left a comment

		DOMAIN = "asterisk_cdr"


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

Add Call Data Log platform. Mailboxes no longer require media #16579

Add Call Data Log platform. Mailboxes no longer require media #16579

Conversation

PhracturedBlue commented Sep 12, 2018 • edited

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

PhracturedBlue commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhracturedBlue commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinHjelmare commented Sep 17, 2018

PhracturedBlue commented Sep 18, 2018

MartinHjelmare commented Sep 18, 2018

PhracturedBlue commented Sep 18, 2018

MartinHjelmare left a comment

Choose a reason for hiding this comment

PhracturedBlue commented Sep 12, 2018 •

edited