-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(#577): recurring tasks #574
base: main
Are you sure you want to change the base?
Conversation
I'd prefer a new interface, it's very clear what this config does
and I'd even go as far as changing before/after to before_due/after_due so it's completely self documenting
Maybe I'm missing something but can't we remove the default and make it a conscious decision a dev makes during config? The drawbacks can always be included in the docs |
Thanks for the feedback!
We can do this for sure. But its a bit complicated and my guess is that users would prefer to have this detail abstracted away ... All design decisoins can be avoided by making them configurable but good design choices can yield an experience which is easier, smoother, smarter, etc. Public interfaces are costly (compatibility, documentation, cost to learn the system, etc). Exposing this as a public interface and not wanting to break user experiences could perhaps limit changes we'd want to make in cht-core... Ideally, I think we'd be able to find a smart default - but if we can't, let's talk more about asking app devs to make that decision. Have you worked with recurring tasks before for a production app? I'm wondering what you'd set this value to if it could be configured? |
Great write-up and proposal, I'm excited about this! I agree with Fred that a new interface would be more straightforward than bloating the events interface. As for performance, to keep things simple I'm inclined to keep the limit in a single place. How do you see the limit being implemented in cht-conf? Limit number of recurring events (only allow X amount of recurring tasks, regardless of their timeline)? Limit based on event time (similar to what we have in cht-core? About your edge case: how would one create a task that shows up every day and has the default resolvedIf? |
Thanks for the great feedback! Appreciate it.
I'll update with the 2nd interface. I like that one best also. @dianabarsan Can you confirm you're alright with accepting luxon inputs? This will require a dependency on luxon and will probably end up using joi-luxon. Or should I omit that and use only Date?
This is implemented already here via timelyInterval. An emission must intersect the timely interval to be emitted.
This should work for that. I don't see a challenge there.
|
I'm reluctant to standardizing a library as part of config. Can't confirm this. How does it look with Date? |
I think that's not a huge deal, but it was in the 2nd interface so thanks for confirming. Our interface can accept only inputs of type:
But |
Definitely agree with calendar durations. |
Sounds like we already have standardized on a library and it is later. I'll try use that. As compared to the above, later seems like a superior interface for scheduling task due dates. I'm not sure how to use it to specify a visibility window... For example, it is easy to use it to schedule |
From the projects I've had a hand in, the most common recurring ones are usually monthly tasks |
@kennsippell How are you getting on with this change? |
@garethbowen This feedback is a pretty significant change - sort of a rewrite for a lot of this. It is taking me a while to revisit this, but still on my plate. |
Description
#577
Implementing recurring tasks today is complex and limited. In the past, simple mistakes while implementing recurring tasks have caused severe live-site incidents (“task explosions”). It is difficult to achieve reliable reporting on task completion rates for recurring tasks. It is not possible today to have "a task on the first of each month".
This feature adds support for recurring tasks (eg. daily, weekly, quarterly, every 3 days, etc.) through a new interface in
tasks.js
.This PR is a bit rough and isn’t ready for a full review, but I’d like to work with somebody on nailing down a few questions - mostly related to interfaces.
New Interfaces in Tasks.js
The below tasks trigger once per week on Sundays for every day in 2023. The task is visible between Friday - Monday. Here is the proposed interface currently implemented in the PR (it is following the "style" of existing interfaces in tasks.js):
Here is another very different interface with the same info:
Some interface questions for reviewer:
events
interface, or a new interface? Alternatively, this could be a new object schema within the existing events array (so a task could be recurring and have a fixed schedule).event
field optional with a default value of daily recurring forever.Performance
Currently a daily task (default) will result in 91 task documents being created. This simply defers to the design decisions in core. Is this the right approach? 90 task docs is quite a lot, particularly if a user uses this for
appliesTo: ‘reports’
...Disallowing an Edge Case
This is a weird scenario, and I’m thinking of disallowing inputs like this. I think they cause a ton of problems:
The result of this configuration is that on any given day, two tasks show (one due today and one due tomorrow). If the user keeps the default
resolvedIf
then completing either task will complete them both. Generally speaking, this happens any time{ start + end + 1 } is larger than the recurring task’s period in days
. Any objection to erroring for this case?Comments
I found it challenging to write this feature using
var
and with only the Date object. I believe that moving this forward may require resolution of medic/cht-core#573 (proposal within this PR).If we like this approach, I can move #577 into the cht-conf repo since this would require no changes in cht-core.
Code review items
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.