-
Notifications
You must be signed in to change notification settings - Fork 1
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
Schedule validation #365
Comments
Task list:
|
I've been looking at the Do we want to raise any warnings or failures in addition to the these requirements, for fields such as |
@Lathrisk so we need enough data to generate at least one occurrence when ignoring If the schedule does not generate any occurrences it should be a FAILURE All data used to generate a |
Also, to help with:
Potential logic as follows for the
Here's the schedule generation code that imin uses, complete with a working timezone implementation: https://gist.github.com/lukehesluke/b00a20caf22abf26b38c0c5e3238e150 Note specifically that You won't need all of the code above, but it should give you a good head start here :) |
Thanks, @nickevansuk. I had started using rrule with luxon so will take a look at this as a more predictable alternative. |
@nickevansuk (cc @thill-odi) I've been working through the timezone code, but I have been trying to understand the reasoning for implementing this as part of this rule. Surely the primary requirement is ensuring that the time, date, and timezone are provided, rather than the specific implementation of the RRule generation that we apply here? The generated recurrence rule is a byproduct of the recommendations in the developer docs, rather than concrete values in the OpenActive data that we need to validate? Is there something important that I am missing? |
Not sure I'm following @Lathrisk - the only reason for the timezone's need to be accurate is for the So perhaps I've misunderstood? Also should have mentioned previously that if using luxon is something that gets around needing to set |
Thanks @nickevansuk, that does make sense wrt to the Is this always in UTC? I notice that the examples provide the closing If I understand correctly then we are generating the RRule with the 'correct' UTC dates so that we can compare this to the exceptDates which are in UTC? |
I attempted to use the tzid/Luxon solution specified in their docs here 👉 https://github.com/jakubroztocil/rrule#timezone-support but it's not machine-independent. In their example, they show that the For example, it failed the "British Summer Time test", presumably because my local machine was already set to > var myRule = new RRule({
freq: RRule.WEEKLY,
interval: 1,
byweekday: [RRule.MO, RRule.FR],
dtstart: new Date(Date.UTC(2021, 2, 18, 7, 30)),
until: new Date(Date.UTC(2021, 3, 6)),
tzid: 'Europe/London'
});
> myRule.all();
[ 2021-03-19T07:30:00.000Z,
2021-03-22T07:30:00.000Z,
2021-03-26T07:30:00.000Z,
// this should be 2021-03-29T06:30:00.000Z (i.e. the UTC version of 2021-03-29T07:30:00.000+01:00)
// as BST occurs on the 28th March. Same for the subsequent ones:
2021-03-29T07:30:00.000Z,
2021-04-02T07:30:00.000Z,
2021-04-05T07:30:00.000Z ]
Agreed! You might be able to use the tzid/luxon solution without requiring that e.g. something like (NOTE: I haven't actually tested this): return datesWithWeirdTimezones.map(date =>
DateTime.fromJSDate(date)
.toUTC()
.setZone(process.env.TZ, { keepLocalTime: true })); This could work if EDIT: I can confirm that |
This issue actually likely contains a number of rules, and the various requirements below could be split into different rules in a number of ways. Generally it's better to keep rule files as focussed as possible, with a good separation of concerns between the validation being conducted by each file.
Validate recurrence rule from
Schedule
The validator should include a rule that validates that a
Schedule
contains a valid iCal recurrence rule.There's already schedule generation logic in the conformance services, so it should be easy to convert this into some validation logic for the same fields, creating an rrule.
Note the logic above does not include
scheduleTimezone
(which it definitely should), so this will need to be added.If there are not enough properties to create an rrule (e.g. the rrule library might return an error saying this?), display #292
If the properties do not create a valid rrule, return an error from the rrule library that explains what’s wrong, as a FAILURE
Using the rrule, validate that all
exceptDate
values are actually part of the recurrence defined by the rrule, and produce a WARNING if any are outside of the rrule.Also note this other rrule library that has better support for timezones, if the library used above is problematic https://www.npmjs.com/package/@rschedule/core - however it has much lower usage so the rrule library is preferred if possible.
The Bookwhen (http://data.bookwhen.com/) and Open Sessions (https://opensessions.io/openactive) feeds should contain good test data for this.
Validate specific properties within
PartialSchedule
(target onlyPartialSchedule
)PartialSchedule
includesstartTime
orendTime
, it must also includescheduleTimezone
, producing a FAILURE otherwiseValidate specific properties within
Schedule
(target onlySchedule
)validate that
idTemplate
andurlTemplate
include a{startDate}
placeholder, and are valid .idTemplate
andurlTemplate
and checks that they both include{startDate}
, producing a FAILURE otherwisevalidate that
scheduledEventType
is a validEvent
subclass.Validate specific properties within
Schedule
andPartialSchedule
(target both)validate that repeatCount is a positive integer greater than zero
Schedule
andPartialSchedule
via the data-modelsvalidate that
scheduleTimezone
matches a value from the IANA Time Zone Database.scheduleTimezone
and uses e.g.moment.tz.names()
(which requires adding moment-timezone) to validate the entries, producing a FAILURE if the value does not match. Note this is a separate rule from the generic rrule validation as it also applies toPartialSchedule
.Also consider whether extending the below could be an approach to implementing any of the above:
See further documentation:
The text was updated successfully, but these errors were encountered: