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

Google Calendar round 2 #4161

Merged
merged 3 commits into from Nov 19, 2016
Merged

Google Calendar round 2 #4161

merged 3 commits into from Nov 19, 2016

Conversation

mnestor
Copy link
Contributor

@mnestor mnestor commented Oct 31, 2016

Description:
Creating a new pull since #2052 was closed so long ago.

Adds Google Calendar Event Devices and a generic Calendar Device for future calendar integration.
I've set this up after discussions with @Bart274 about doing an iCloud calendar similar to this so it should be easier to add in that support after this.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml:

google:
  client_id: al8e43hyoahjweog8h # required, this comes from Google Api Console
  client_secret: laih34go8ahewo48h # required, comes from Google
  track_new_calendar: yes  # optional, defaults to yes

Checklist:

If user exposed functionality or configuration variables are added/changed:

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New 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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link

@mnestor, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @kfgoode to be potential reviewers.

from homeassistant.helpers.entity import Entity
from homeassistant.const import (STATE_ON, STATE_OFF)
from homeassistant.util import Throttle
from homeassistant.components import (

Choose a reason for hiding this comment

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

farcy v1.1

  • 1: F401 'homeassistant.components.google' imported but unused

"""
import os
import logging
import socket

Choose a reason for hiding this comment

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

farcy v1.1

  • 1: F401 'socket' imported but unused

import os
import logging
import socket
from datetime import timedelta

Choose a reason for hiding this comment

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

farcy v1.1

  • 1: F401 'datetime.timedelta' imported but unused

import homeassistant.helpers.config_validation as cv
import homeassistant.loader as loader
from homeassistant import bootstrap
from homeassistant.const import HTTP_OK

Choose a reason for hiding this comment

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

farcy v1.1

  • 1: F401 'homeassistant.const.HTTP_OK' imported but unused

hass, 'In order to authorize Home-Assistant to view your calendars'
'You must visit: <a href="{}" target="_blank">{}</a> and enter'
'code: {}'.format(devFlow.verification_url,
devFlow.verification_url, devFlow.user_code),

Choose a reason for hiding this comment

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

farcy v1.1

  • 17: E128 continuation line under-indented for visual indent

title=NOTIFICATION_TITLE, notification_id=NOTIFICATION_ID)

listener = track_time_change(hass, step2_exchange,
second=range(0, 60, devFlow.interval))

Choose a reason for hiding this comment

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

farcy v1.1

  • 9: E128 continuation line under-indented for visual indent

for calendar_hash, calendar in calendars.items():
_CALENDARS.update({calendar_hash: calendar})
discovery.load_platform(hass, 'calendar', DOMAIN,
_CALENDARS[calendar_hash])

Choose a reason for hiding this comment

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

farcy v1.1

  • 9: E128 continuation line under-indented for visual indent

_CALENDARS[calendar_hash]
)

discovery.load_platform(hass, 'calendar', DOMAIN, _CALENDARS[calendar_hash])

Choose a reason for hiding this comment

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

farcy v1.1

  • 80: E501 line too long (80 > 79 characters)

@mnestor mnestor force-pushed the google_calendar branch 3 times, most recently from 0a40c39 to 7c20cd5 Compare November 1, 2016 01:03
@lwis
Copy link
Member

lwis commented Nov 1, 2016

Could we have the ability to specify a list of calendars we want to track? Also, thanks for continuing this!

@mnestor
Copy link
Contributor Author

mnestor commented Nov 1, 2016

@lwis It's there, it creates a google_calendars.yaml file similar to known_devices.yaml and also allows you to specify search based.

But I just realized I messed that up when I tried to make it so you could specify the device name. Crap.

a7709b23defc0ebf26e56c9b05b046c2866d6b6fb76223d0522986f8:
  cal_id: 072pj7okgusgb14uqqj0rgf15n9jhmeg@import.calendar.google.com
  entities:
  - name: Mike Nestor (TripIt)
    search: null
    track: true
  name: Mike Nestor (TripIt)
c379a36a72dc93de3bce26b627af0966ec88089abde13045b739613b:
  cal_id: asdfasdf@group.calendar.google.com
  entities:
  - name: Maggie
    search: null
    track: false
  - name: Maggie
    search: Vet
    track: true
  name: Maggie

@lwis
Copy link
Member

lwis commented Nov 1, 2016

@mnestor one comment, if search is null we shouldn't write/display it, just make sure that it's documented on the website.

@mnestor
Copy link
Contributor Author

mnestor commented Nov 1, 2016

@lwis

Changed the google_calendars.yaml around

- cal_id: blahasldfhaiehg@group.calendar.google.com
  entities:
  - device_id: things_to_do
    name: Things to do
    track: false

- cal_id: '#contacts@group.v.calendar.google.com'
  entities:
  - device_id: contacts
    name: Contacts
    track: false

- cal_id: en.usa#holiday@group.v.calendar.google.com
  entities:
  - device_id: holidays_in_united_states
    name: Holidays in United States
    track: false
  - device_id: holidays_chistmas
    name: Christmas
    search: Christmas Day
    track: true

The above would only create one calendar, a search based one in you holiday calendar to turn on only on Christmas Day.

@mnestor mnestor force-pushed the google_calendar branch 2 times, most recently from 7225626 to 2c4038d Compare November 2, 2016 00:22
@Bart274
Copy link
Contributor

Bart274 commented Nov 3, 2016

@mnestor I'll take a look at how I can use your component to add an icloud calendar as well :-)

@mnestor
Copy link
Contributor Author

mnestor commented Nov 4, 2016

@Bart274 Nothing has changed since we last talked. I've just found a new way to do the oauth for the google part that makes it a LOT easier for people to get setup.

@Bart274
Copy link
Contributor

Bart274 commented Nov 4, 2016 via email

@mnestor mnestor force-pushed the google_calendar branch 4 times, most recently from ec6c57f to 4eaf1b8 Compare November 5, 2016 18:54
@mnestor mnestor changed the title WIP: Google Calendar round 2 Google Calendar round 2 Nov 5, 2016
REQUIREMENTS = [
'google-api-python-client==1.5.4',
]
DEPENDENCIES = ['calendar']
Copy link
Member

Choose a reason for hiding this comment

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

The Google component shouldn't depend on calendar.

@property
def track(self):
"""Are we tracking events on this search sensor."""
return self._track
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? If it shouldn't track anything it shouldn't have been loaded to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's dumb. Copy/paste I never thought about.

self._end_listener = track_point_in_time(self.hass, _end,
self._cal_data['end'])

def value_changed(self, state):
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appeared, months ago, that this was called by another process and I didn't have to deal with changing the state. I take it either I'm wrong about that and this was never needed?

Do I need to call update_ha_state when state data changes?

Copy link
Member

Choose a reason for hiding this comment

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

Home Assistant will automatically call update every 60 seconds (as that's what you entered into the EntityComponent constructor).

"""Return the device name."""
return self._name

@property
Copy link
Member

Choose a reason for hiding this comment

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

This is not being used outside of this entity. You can just have other pieces of code directly refer to the internal attribute self._state. I also think that self._state could be renamed to better describe what the value means that it holds (I'm assuming it represents if an event is currently happening?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused. I thought this was needed so I could check it in an automation?

- condition: state
  entity_id: calendar.name
  state: 'on'

I left it as on/off since it made sense to me as these are basically binary switches for being in an event state.

Copy link
Member

Choose a reason for hiding this comment

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

None of the properties that are defined on an entity will be set as state attributes. That's what state_attributes is for.

def _found_calendar(call):
"""Check if we know about a calendar and generate PLATFORM_DISCOVER."""
calendar = get_calendar_info(hass, call.data)
if _CALENDARS.get(calendar[CONF_CAL_ID], None) is not None:
Copy link
Member

Choose a reason for hiding this comment

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

We have added hass.data as a dictionary for components to store data. You will no longer need to use globals. Make sure you make the key unique to your component, ie hass.data['google_calendars']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you

@@ -17,6 +17,7 @@
COMPONENTS_WITH_DEMO_PLATFORM = [
'alarm_control_panel',
'binary_sensor',
'calendar',
Copy link
Member

Choose a reason for hiding this comment

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

A demo platform has not been included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It got lost when I built a new system to work on this on. I'll get it added back.

DEPENDENCIES = ['calendar']

_LOGGER = logging.getLogger(__name__)
_CONFIGURING = {}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be used.

"""
raise NotImplementedError(message)

@Throttle(MIN_TIME_BETWEEN_UPDATES)
Copy link
Member

Choose a reason for hiding this comment

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

We should never throttle at the entity level. It is up to the platforms to decide how much they require throttling. For example the demo platform will never need a throttle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've changed it to how you want it.

@mnestor mnestor force-pushed the google_calendar branch 3 times, most recently from a8dc664 to 22ddc68 Compare November 13, 2016 21:10

TOKEN_FILE = '.{}.token'.format(DOMAIN)

PLATFORM_SCHEMA = vol.Schema({
Copy link
Member

Choose a reason for hiding this comment

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

This is a component, so you should use CONFIG_SCHEMA. See group example

def setup(hass, config):
"""Setup the platform."""
config = config.get(DOMAIN, {})
if isinstance(config, list) and len(config) > 0:
Copy link
Member

Choose a reason for hiding this comment

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

This will no longer ben ecessary when you define CONFIG_SCHEMA instead of PLATFORM_SCHEMA

bootstrap.setup_component(hass, 'calendar', config)

for calendar in hass.data[DATA_INDEX].values():
discovery.load_platform(hass, 'calendar', DOMAIN, calendar)
Copy link
Member

Choose a reason for hiding this comment

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

Is calendar JSON serializable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes


def update_config(path, calendar):
"""Write the google_calendar_devices.yaml."""
with threading.Lock():
Copy link
Member

Choose a reason for hiding this comment

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

This will create a new lock every time you call this, so that means that the lock is never shared -> the locking won't work

@@ -28,6 +28,9 @@ omit =
homeassistant/components/envisalink.py
homeassistant/components/*/envisalink.py

homeassistant/components/google.py
Copy link
Member

Choose a reason for hiding this comment

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

No need to exclude from coverage if you wrote tests ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balloob how would I go about disabling coverage for the google api stuff? Since I'm not failing for those.

@balloob balloob merged commit 6136154 into home-assistant:dev Nov 19, 2016
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants