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

feat(#577): recurring tasks #574

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(#577): recurring tasks #574

wants to merge 3 commits into from

Conversation

kennsippell
Copy link
Member

@kennsippell kennsippell commented Oct 3, 2023

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

events: {
  recurringStartDate: '2023-01-01',
  recurringEndDate: new Date('2024-01-01'),
  period: 7,
  periodUnit: 'days',
  start: 2,
  end: 1,
},

Here is another very different interface with the same info:

const { DateTime, Duration } = require(‘luxon’);
recurring: {
  schedule: {
    start: DateTime.fromISO('2023-01-01'),
    end: new Date('2024-01-01'),
  },
  every: Duration.fromObject({ weeks: 1 }),
  visibility: {
    before: 2,
    after: Duration.fromObject({ days: 1 }),
  },
},

Some interface questions for reviewer:

  1. Do you prefer the use of polymorphism of the 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).
  2. I find it awkward that there are two things called “start” and “end” in the first interface I suggested. The latter attempts to really differentiate them into start/end/before/after. Thoughts or alternatives? How important do you think it is to be consistent with the existing events interface (eg. start, end as integers)?
  3. I am going to add a new interface so users can specify a function and make calculations for the start date. This would support scenarios like “monthly for 3 months after this form is submitted”.
  4. Do you prefer the language of “recurring start” or a “scheduled start”?
  5. Currently, all information is optional. As written, this would make the 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:

recurringEvents: {
  period: 1,
  start: 1,
},

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

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@freddieptf
Copy link
Contributor

I find it awkward that there are two things called “start” and “end” in the first interface I suggested. The latter attempts to really differentiate them into start/end/before/after. Thoughts or alternatives? How important do you think it is to be consistent with the existing events interface (eg. start, end as integers)?

I'd prefer a new interface, it's very clear what this config does

recurring: {
  schedule: {
    start: DateTime.fromISO('2023-01-01'),
    end: new Date('2024-01-01'),
  },
  every: Duration.fromObject({ weeks: 1 }),
  visibility: {
    before: 2,
    after: Duration.fromObject({ days: 1 }),
  },
}

and I'd even go as far as changing before/after to before_due/after_due so it's completely self documenting

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

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

@kennsippell
Copy link
Member Author

kennsippell commented Oct 6, 2023

I'd prefer a new interface

Thanks for the feedback!

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

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?

@dianabarsan dianabarsan self-assigned this Oct 11, 2023
@dianabarsan
Copy link
Member

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.
I particularly would not encourage events to be polymorphic.
I also like the keywords that are used in the new interface: "every", "visibility" and "schedule" are unambiguous.

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?

@kennsippell
Copy link
Member Author

kennsippell commented Oct 12, 2023

Thanks for the great feedback! Appreciate it.

a new interface would be more straightforward

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?

I'm inclined to keep the limit in a single place. How do you see the limit being implemented in cht-conf?

This is implemented already here via timelyInterval. An emission must intersect the timely interval to be emitted.

how would one create a task that shows up every day and has the default resolvedIf?

This should work for that. I don't see a challenge there.

recurring: {
  every: 1,
}

@kennsippell kennsippell changed the title feat(#cht-core/6608) - Recurring tasks feat(#577): recurring tasks Oct 12, 2023
@dianabarsan
Copy link
Member

dianabarsan commented Oct 12, 2023

Can you confirm you're alright with accepting luxon inputs?

I'm reluctant to standardizing a library as part of config. Can't confirm this. How does it look with Date?

@kennsippell
Copy link
Member Author

kennsippell commented Oct 12, 2023

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: int, Date, function without DateTime and Duration.

DateTime is no big deal, you can just use DateTime.fromISO('2023-01-01').toJSDate(). Not a lot of value there.

But Duration has a more value. I really feel strongly we should support things like 1 month so we can have recurring tasks on the first of each month (or 10th of each month like Muso's reporting period) instead of every 30 days (etc). So instead of after: Duration.fromObject({ months: 1 }) I'll need to do something akin to { after: 1, afterUnit: 'months' } or after: '1 month' or after: { amount: 1, units: 'month' }. I suspect we only need to support day and month units to get the others (weeks, quarters, years, etc).

@dianabarsan
Copy link
Member

Definitely agree with calendar durations.
We use later in cht-core to parse duration expressions, and this has been part of config for purging, reminders and a bunch of other configurable recurring tasks. It has good documentation and can parse a large variety of expressions: https://bunkat.github.io/later/parsers.html#text
What do you think about this?

@kennsippell
Copy link
Member Author

kennsippell commented Oct 13, 2023

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 a task on the 10th day of every 2nd month but unclear on how to use it for a visibility like the task is visible for one month or until the end of the month. But that's maybe not that important and we can constrain visibility windows to be expressed in days only.

@freddieptf
Copy link
Contributor

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?

From the projects I've had a hand in, the most common recurring ones are usually monthly tasks

@garethbowen
Copy link
Member

@kennsippell How are you getting on with this change?

@kennsippell
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants