Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 calendar platform to Habitica integration #128248
base: dev
Are you sure you want to change the base?
Add calendar platform to Habitica integration #128248
Changes from 13 commits
01daba2
8ceab73
09c79c1
8705b82
1b86dd0
1cd645f
e257737
953a5dd
69f116c
3689edd
a375274
2a413cb
57aa63a
c7d6f45
ecc2da6
978ccb1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should always be
next_recurrence.date() + timedelta(days=1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention is, if there still are unresolved tasks from yesterday, they are still the active tasks, so the enddate is the end of today so the state of the entity stays on. Until these aren't resolved by either completing or skipping them and then starting the new day, the tasks for today are still not the active or next upcoming tasks, they are calculated "future" recurrences. When the daily tasks are reset, the state will advance to the next task that is due.
I know it sounds a bit complicated or I may not be the best in explaining. Maybe this is more helpful https://habitica.fandom.com/wiki/Cron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also is not meeting the get_event spec. get_events should work for any start/end date range. A calendar can render events from the past as well, and triggers work based on past events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Habitica the start of the day can be set to a different time (in my account it is 4:00 am for example. If at that time all dailies that were due have been completed the cron runs at 4 and resets all dailies, marking the start of the day. If there are incompleted dailies from the past day (called yesterdailies), the user gets a last chance to mark tasks as completed (in case the task was done but forgotten to mark as completed) and then the user presses the button 'start my day', which then marks the start of the day. That's what I want to reflect here.
That's also why it doesn't make sense to render events from the past, dailies are reset every day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there is no data about past dailies. I could generate events based on the current recurrence settings but that would be a wild guess, because they can be changed anytime and I don't know if they were possibly different in the past
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, though completed tasks should probably still be returned here, consistent with how other todos on calendar show up. If needed, we can add more rfc5545 attributes to calendar events and render them different in the UI in the future.
I don't really see that we need to bring in the cron stuff here, rather than ignoring it and letting it show up on the calendar how it shows up on the calendar. (t's very hard to reason about given it seems independent of the recurrence rule and not on a single timeline, so maybe i'm missing it). It feels like its trying to implement todo status tracking and i think we should just add support for todo status tracking if we want that.
I'm worried getting fancy here and not following the spec is going to break triggers/automations since it expects this to be confirming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the triggers based on the calendar entities on/off state?
I checked the Todoist integration, it also provides a calendar for tasks with due dates. If a task is completed, it is not shown in the calendar view anymore. That's as far as I can see the only other integration that implements a calendar for tasks. Maybe in the future we could render completed tasks in the calendar in grey, that would be a cool feature, but for now I would prefer to do it like the Todoist integration does it already.
I can't ignore the cron. I need it to determine on what day to render the tasks. That's the only way to know if the task is from yesterday or from today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calendar event Triggers are based on scanning the time range. (discussion)
I see logic in the "select best task" for skipping completed upcoming tasks. But in the calendar view, is that also happening? I saw a difference in the logic that gets the events https://github.com/home-assistant/core/blob/4cbac3a864e0724ad353aa3f4fc159cc8f402ae8/homeassistant/components/todoist/calendar.py#L664C15-L664C31
Regarding the cron are you saying the start date returned from the API is wrong?
If tasks have specific start/end times that are not aligned with date boundaries, then just set start/end times rather than dates and this would all just work how its supposed to naturally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given
between
above, is this greater than check needed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is, to differentiate todays active tasks from the future calculated recurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following why this matters. The start and end date set the range, and so if there are dates within the range they should be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attributes should always be returned, but may have a
None
value. See home-assistant/architecture#680There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change that, no problem. But the default state attributes itself aren't returned either if there is no upcoming task. So Home Assistant seems inconsistent here.
Check warning on line 197 in homeassistant/components/habitica/calendar.py
Codecov / codecov/patch
homeassistant/components/habitica/calendar.py#L197
Check warning on line 102 in homeassistant/components/habitica/util.py
Codecov / codecov/patch
homeassistant/components/habitica/util.py#L101-L102