Add support for Todoist platform by Jay2645 · Pull Request #9236 · home-assistant/core

Jay2645 · 2017-08-31T11:10:34Z

Description:

Support for Todoist task management (https://todoist.com).

Todoist breaks task management down into 2 structures:

Tasks, which are "things" you can do and can mark as being done
Projects, which are essentially containers for tasks.

In Home Assistant, each project gets its own calendar. The start date of the calendar is set to now, while the end date is set to the due date -- the logic for this being that most tasks you set on here are tasks you could theoretically do now. There are some exceptions (like taking someone to the airport), but a good percentage of the things on your to-do list could be done right now if you wanted to. Because of that, the calendar takes a simple state -- on if you have a task in it that's not marked as completed, or off if you have no tasks at all in that project.

The calendar keeps track of what your most "important" task in that project is, governed by a number of factors specified in the header of the .py file. You can access all the tasks in a project via a template if you wanted to, and they'd all be sorted from most important to least important.

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

Example entry for `configuration.yaml` (if applicable):

calendar:
  - platform: todoist
    token: !secret todoist_token
    custom_projects:
      - name: 'All Projects'
      - name: 'Due Today'
        due_date_days: 0
      - name: 'Due This Week'
        due_date_days: 7
      - name: 'Math Homework'
        labels:
          - Homework
        include_projects:
          - Mathematical Structures II
          - Calculus II

Checklist:

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:

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

Creating a new platform for Todoist - https://todoist.com

This means you can now specify things such as 'all tasks due today', 'all tasks due this week', etc.

…into add-todoist

MartinHjelmare

I think you should use a pypi package for the todoist specific logic.

MartinHjelmare · 2017-09-07T08:47:05Z

+        return attributes
+
+
+class TodoistProjectData(object):


This should probably be broken out into a python package and published on pypi. Is there no pypi package already for todoist?

There is, but I didn't want to introduce any dependencies. When I first started designing the platform, I had the misconception after looking at their Python API that I would need to do everything via OAuth, which is obviously more of a challenge than just having the user specify a token in their config. Looking back at it now, I see that there is actually a way to do things with a token, but I'm still adverse to introducing extra dependencies.

That being said, I would still need some Todoist logic somewhere. This class has the scoring algorithm, which could be moved to the actual device. But (more importantly), it also supports "custom" projects -- i.e., where the user can ask to grab everything in the next week, or all things tagged "Homework", or what have you. That stuff comes from the config file, and it'll all have to be kept track of locally (otherwise users may wind up accidentally creating a new project as soon as the API syncs, from what I've gathered in a cursory examination).

This class also "converts" the data from Todoist to something that Home Assistant can process easier -- as an example, the due date logic around line 400 or so. Sure, I'd be able to get an actual datetime object from the API instead of relying upon strptime, but I would still need to "translate" that to the end time for a Calendar in Home Assistant. This also ensures that every single task has a start and end date, that there's always a list of comments (even if it's empty), etc. By default, if something is empty, there's just no entry for it in the API. The idea is to do as much logic as makes sense here, in order to cut down on the amount of logic contained within the actual device. I can probably move the logic for determining if something is due today or overdue into the device, but at the time it seemed fitting just to do it here, since I was already working with the data.

The end result is I would need some class which looks vaguely like this, except I would have references to Todoist's PyPI API calls instead of using requests. I'd still need to "score" each task, since Todoist sorts by date added, not by actual priority. I'd still need to "translate" a lot of the Todoist-specific things to what the Calendar device expects, but perhaps this can be done on the device end. Most importantly, though, I'd still need to keep a list of "local" projects for those user-defined devices that can be specified in the config. That'd be where most of the logic would come from.

I can do it, if you'd like me to. Doing so would give me access to Todoist Reminders, so I could probably expose the Reminder functionality built-in to Todoist to Home Assistant (right now, I can't access Reminders via the REST API -- just Tasks and their due dates). It would be a fairly easy change overall, but I'm not sure whether it would cut down on the amount of platform-specific logic unless I got rid of the custom user-defined projects (which is where a lot of the logic comes from currently). It would also introduce a new dependency, of course.

Introducing a new dependency is not a problem, but rather part of the ground rules for home assistant:
https://home-assistant.io/developers/code_review_platform/#6-communication-with-devicesservices
https://home-assistant.io/developers/code_review_platform/#1-requirements

Having an interface class or callback function between the pypi package API and the home assistant entity API is ok, but we try to minimize it as much as possible. Usually this class/function is responsible for fetching/accepting new data into home assistant.

MartinHjelmare · 2017-09-07T08:49:02Z

+CONF_PROJECT_WHITELIST = 'include_projects'
+CONF_PROJECT_LABEL_WHITELIST = 'labels'
+
+CONFIG_SCHEMA = vol.Schema({


Platforms should implement PLATFORM_SCHEMA.

MartinHjelmare · 2017-09-07T08:51:23Z

+                task_data['due_datetime'] = call.data['due_datetime']
+            elif 'due_date' in call.data:
+                task_data['due_date'] = call.data['due_date']
+        except KeyError:


The service schema should validate call.data to make this error impossible.

This is left over from before we validated the inputs using the service schema.

This also removes functionality regarding finding out how many comments a task has. This functionality may be added back in the future.

Jay2645 · 2017-09-08T01:00:42Z

@MartinHjelmare I've made the changes you requested.

I changed from implementing CONFIG_SCHEMA to implementing PLATFORM_SCHEMA; I'm not 100% sure I'm doing things right, but it seems to work in Home Assistant.
call.data was already being validated; the try-catch block you pointed out was from an older version before I implemented validation. I went ahead and just removed it.
In the end, I wasn't able to get rid of the TodoistProjectData interface class; I forgot that the Calendar platform itself requires an interface class with an update function and an event object, both of which get defined in the TodoistProjectData interface class. This pattern is seen in the Demo Calendar component as well as the Google Calendar component. I did, however, move to the Todoist PyPI package and added the dependencies.

Let me know if you need any more changes!

MartinHjelmare

My comments are mostly about three themes:

Use the schema validation as much as possible to get good default values and simplify later code.
Optimize checks and iterations using (list) comprehension.
Only log at debug level.

MartinHjelmare · 2017-09-10T10:41:29Z

+import logging
+from datetime import datetime
+from datetime import timedelta
+import os


Please sort the import lines alphabetically within each group builtin, 3rd party and homeassistant.

MartinHjelmare · 2017-09-10T10:46:38Z

@@ -0,0 +1,595 @@
+"""
+Support for Todoist task management (https://todoist.com).


Put all of this info in the docs PR, if not there already. Look at another platform module for an example how to write the docstring. It should contain a link to the docs page.

MartinHjelmare · 2017-09-10T10:47:39Z

+})
+
+# Your Todoist API token.
+# Find yours at https://todoist.com/Users/viewPrefs?page=authorizations.


I think this belongs in the docs.

MartinHjelmare · 2017-09-10T10:54:26Z

+    """Setup the Todoist platform."""
+    # Check token:
+    token = config.get(CONF_API_TOKEN)
+    if token is None or not isinstance(token, str):


Remove this. It's already checked in the config validation.

MartinHjelmare · 2017-09-10T11:01:58Z

+
+# Look up IDs based on (lowercase) names.
+project_id_lookup = {}  # pylint: disable=C0103
+label_id_lookup = {}  # pylint: disable=C0103


It looks like these two dicts are only used within setup_platform. Move them inside setup_platform.

MartinHjelmare · 2017-09-10T13:37:54Z

+        # All task Labels (optional parameter).
+        task['labels'] = []
+        if len(data['labels']) > 0:
+            for label_id in data['labels']:


We can simplify this using a list comprehension:

task['labels'] = [ label['name'] for label in self._labels if label['id'] in data['labels']]

I suggest you also make the labels lowercase here already, see comment below.

MartinHjelmare · 2017-09-10T13:41:02Z

+        if whitelisted_labels is not None:
+            self._label_whitelist = whitelisted_labels
+        else:
+            self._label_whitelist = None


Default to an empty list.

MartinHjelmare · 2017-09-10T13:54:00Z

+        # on the whitelist.
+        if self._label_whitelist is not None:
+            found_label = False
+            for label in self._label_whitelist:


If you make the task labels lowercase above you don't need to do that here and you can just:

if not any(label in task['labels'] for label in self._label_whitelist): return None

MartinHjelmare · 2017-09-10T14:02:00Z

+            has the same priority as our current event, but is due earlier
+            in the day, select it
+        """
+        if len(project_tasks) > 0:


You already make this check before you call this method. I suggest you assume you get a list and just iterate over that.

MartinHjelmare · 2017-09-10T14:04:03Z

+            # Start at the end of the list, so if tasks don't have a due date
+            # the newest ones are the most important.
+
+            event = project_tasks[len(project_tasks) - 1]


event = project_tasks[-1]

Jay2645 · 2017-09-11T08:35:16Z

Alright, I believe all of your changes should be good to go! Let me know if you have any more!

MartinHjelmare

Good work! Some comments below. An optional improvement would be to define string constants at the top of the module for the magic strings, eg:

PROJECTS = 'projects'
LABELS = 'labels'

and use those where needed in the code. It will make typo risk less, look cleaner, and be easier to change a string in the future.

MartinHjelmare · 2017-09-11T12:41:30Z

+            item.update(due_date_utc=due_date)
+        # Commit changes
+        api.commit()
+        _LOGGER.debug("Created Todoist task: " + call.data['content'])


Don't concatenate strings like this. For logging use the old string formatting syntax:

_LOGGER.debug("Created Todoist task: %s", call.data['content'])

MartinHjelmare · 2017-09-11T12:42:09Z

+        # they have, organized)
+        while len(project_tasks) > 0:
+            best_task = self.select_best_task(project_tasks)
+            _LOGGER.debug("Found Todoist Task: " + best_task['summary'])


String formatting.

MartinHjelmare · 2017-09-11T12:43:17Z

+                        timedelta(days=1)
+                    ).strftime(DATE_STR_FORMAT)
+                }
+        _LOGGER.debug("Updated " + self._name + ".")


String formatting. We don't end logging messages with period.

MartinHjelmare · 2017-09-11T12:46:58Z

+        task['summary'] = data['content']
+        task['completed'] = data['checked'] == 1
+        task['priority'] = data['priority']
+        task_url = 'https://todoist.com/showTask?id={}'.format(data['id'])


Why cache this in the task_url variable if you don't use that anywhere else? Just save the string directly in task['description'].

task['description'] = 'https://todoist.com/showTask?id={}'.format( data['id'])

MartinHjelmare · 2017-09-11T12:48:11Z

+            if label['id'] in data['labels']]
+
+        whitelist = self._label_whitelist
+        if any(label in task['labels'] for label in whitelist):


I think this should be:

if not any( label in task['labels'] for label in self._label_whitelist):

Otherwise it will have the opposite effect of what you had originally I think.

I had tried that (you had suggested that change last time), and for whatever reason everything was inverted. I'm not exactly sure why it works, but it works.

Looking at this again. Shouldn't all labels in task[LABELS] be on the whitelist to be ok? Otherwise we should return None. Then I think it should be:

if not all(label in self._label_whitelist) for label in task[LABELS]: return None

@MartinHjelmare
So, here's the results with if not all:

And here is if any:

if any gives the correct results (with the label whitelist working as an or statement, as documented here).

if not all(label for self._label_whitelist) for label in task[LABELS] causes any task with a label to be rejected for some reason. You can see that in the first screenshot -- the project "Mathematical Structures II only consists of tasks with the label "Homework" or "School" (or both), so calendar.mathematical_structures_ii rejects all of them. This change propagates down to my "custom" calendars specified in my configuration: calendar.math_homework and calendar.due_today are only taking into account tasks which don't have labels. You can see calendar.movies_to_watch is not affected, as none of the tasks there have labels.

The end result is only tasks with exactly 0 labels are accepted, with the whitelist not being taken into account at all.

if any(label in task[LABELS] for label in self._label_whitelist) returns the correct values, but I'm not sure why. I agree with you that there should be a not in there somewhere, but a not gives me inverted inputs.

I'll take a look at what's going on exactly and see if I can fix it. There must be some logic error somewhere. Maybe if any(label in task[LABELS] for label in self._label_whitelist) is just never returning true, and thus never tossing out anything?

MartinHjelmare · 2017-09-11T12:58:21Z

+            label['name'] for label in self._labels
+            if label['id'] in data['labels']]
+
+        whitelist = self._label_whitelist


Not needed caching.

Jay2645 · 2017-09-12T10:16:11Z

@MartinHjelmare Your changes should be live now.

MartinHjelmare · 2017-09-12T18:55:39Z

@Jay2645 see my comment here #9236 (comment). Let me know if I'm misunderstanding the whitelist.

Jay2645 · 2017-09-12T20:40:35Z

Fixed it. The whitelist is an or, not an and, (see here), but that wasn't the issue after all.

The issue is I was comparing a list of user-defined label names to a list of the names of labels Todoist reports as being part of the task. The names Todoist reported were all lowercase; the names the user defined could be uppercase or lowercase. It was comparing 'Homework' to 'homework', which (of course) is always false. To make matters worse, in your implementation if an empty list is compared against an empty list, it was returning true.

I moved to your implementation and simply made the user-defined labels lowercase. I also added a sanity check to make sure we're not comparing an empty list, and it seems to work fine now.

MartinHjelmare

Last clean up, then this should be good to go.

MartinHjelmare · 2017-09-12T21:50:30Z

        if not any(label in task[LABELS]
                   for label in self._label_whitelist) and (
-                   len(self._label_whitelist) > 0):
+                           len(self._label_whitelist) > 0):


Don't check the length explicitly, just:

if ... and self._label_whitelist:

An empty whitelist will evaluate to False.

Even better actually would be to turn the expression around and check the whitelist first (true/false), then compare the whitelist labels with the task labels.

MartinHjelmare

Nice!

Jay2645 added 13 commits August 29, 2017 08:04

Added basic Todoist support

be21c63

Creating a new platform for Todoist - https://todoist.com

Added more robust support for creating new custom projects.

ef22c40

This means you can now specify things such as 'all tasks due today', 'all tasks due this week', etc.

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

4a62584

…into add-todoist

Changed logging from warning to info.

df665ec

Added label and comment support.

9bceacd

Added support for overdue tasks.

9b28089

Changed logging to info instead of warning; fixed labels.

4495b13

Added ability to filter projects by name.

babac34

Rename 'extra_projects' to 'custom_projects'.

a818395

Updated code to follow proper HASS style guidelines.

265f9ed

Got new_task service running.

295eac7

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

bba9f76

…into add-todoist

Update .coveragerc.

9ad6151

homeassistant added new-platform platform: calendar.todoist cla-signed labels Aug 31, 2017

Jay2645 mentioned this pull request Aug 31, 2017

Add documentation for Todoist component home-assistant/home-assistant.io#3283

Merged

MartinHjelmare requested changes Sep 7, 2017

View reviewed changes

Jay2645 added 8 commits September 7, 2017 09:13

Remove old try-catch block.

0f7b327

This is left over from before we validated the inputs using the service schema.

Updated to use PLATFORM_SCHEMA.

8ee9948

Updated component to use Todoist API.

11a4350

Removed commented-out code.

633378b

This also removes functionality regarding finding out how many comments a task has. This functionality may be added back in the future.

Clarified TodoistProjectData, removed fetching comments.

5738067

Fixed bug where projects were grabbing all tasks.

4f64c08

Fixed bug where due dates were being ignored.

2e8d474

Removed debug logging.

5df2bb0

Fixed linter errors.

2f81656

homeassistant added the cla-signed label Sep 8, 2017

MartinHjelmare requested changes Sep 10, 2017

View reviewed changes

Jay2645 added 9 commits September 10, 2017 21:00

Moved ID lookups into setup_platform.

05b4b2c

Cleaned up setup_platform a bit.

6a0d501

Cleaned up Todoist service calls.

a709000

Changed debug logging level.

25afa73

Fixed issue with configuration not validating.

556b5c2

Changed from storing the token to storing an API instance.

eaf252d

Use dict instead of Project object.

bf5d004

Updated to use list comprehension where possible.

d79ff44

Fixed linter errors.

e25f5b7

MartinHjelmare requested changes Sep 11, 2017

View reviewed changes

Jay2645 added 3 commits September 12, 2017 03:08

Use constants instead of literals.

0affc0e

Changed logging to use old-style string formatting.

b775567

Removed unneeded caching.

36a4631

Added comments explaining 'magic' strings.

9e3455e

homeassistant added the cla-signed label Sep 12, 2017

Fixed bug where labels were always on the whitelist.

46941b5

homeassistant added the cla-signed label Sep 12, 2017

Fixed linter error.

f2d8ed6

MartinHjelmare requested changes Sep 12, 2017

View reviewed changes

Stopped checking whitelist length explicitly.

65c10c3

MartinHjelmare approved these changes Sep 12, 2017

View reviewed changes

MartinHjelmare merged commit c94b3a7 into home-assistant:dev Sep 14, 2017

balloob mentioned this pull request Sep 22, 2017

0.54 #9532

Merged

home-assistant locked and limited conversation to collaborators Mar 3, 2018

ghost added integration: todoist and removed platform: calendar.todoist labels Mar 21, 2019

		@@ -0,0 +1,595 @@
		"""
		Support for Todoist task management (https://todoist.com).

Uh oh!

Conversation

Jay2645 commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jay2645 Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jay2645 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jay2645 commented Sep 11, 2017

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jay2645 commented Sep 12, 2017

Uh oh!

MartinHjelmare commented Sep 12, 2017

Uh oh!

Jay2645 commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Jay2645 commented Aug 31, 2017 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

Jay2645 Sep 7, 2017 •

edited

Loading

Jay2645 commented Sep 8, 2017 •

edited

Loading

MartinHjelmare left a comment •

edited

Loading

MartinHjelmare Sep 10, 2017 •

edited

Loading

Jay2645 commented Sep 12, 2017 •

edited

Loading

MartinHjelmare Sep 12, 2017 •

edited

Loading