-
Notifications
You must be signed in to change notification settings - Fork 9
Add basic Calendar support/tests to JMAP::TestSuite #27
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
base: master
Are you sure you want to change the base?
Conversation
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 added @rsto to review, since this MR is old and may no longer reflect state of calendars. He's AFK for a while, so this will likely wait until Dec/Jan.
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 code looks sane to me but I barely know anything about JMAP-TestSuite internals. Is there a specific area where you prefer my feedback?
@rsto Only "are any of the properties we're creating, or values we're putting in them, not actual specified properties". In the past, we've done stuff (by accident) like test for Cyrus-only properties, or dead properties that Cyrus ignored. |
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 looks to be based on a fairly old spec. The most recent definition of the Calendar type is at https://www.ietf.org/archive/id/draft-ietf-jmap-calendars-22.html#section-4
color => jstr, | ||
sortOrder => jnum, | ||
isVisible => jbool, | ||
mayReadFreeBusy => jbool, |
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 mayXxx
items aren't top-level properties in the Calendar object type. Instead, the top-level property is myRights
. Also, these rights look outdated, e.g. the mayRVSP or the mayUpdatePrivate right is missing.
See the definition of the CalendarRights object type in https://www.ietf.org/archive/id/draft-ietf-jmap-calendars-22.html#section-4
my %required = ( | ||
id => jstr, | ||
name => jstr, | ||
color => jstr, |
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.
color is optional
name => jstr, | ||
color => jstr, | ||
sortOrder => jnum, | ||
isVisible => jbool, |
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 following non-null, non-default properties are missing:
- isSubscribed
- isDefault
- includeInAvailability
id => jstr, | ||
name => jstr, | ||
color => jstr, | ||
sortOrder => jnum, |
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.
sortOrder isn't strictly required, it has a default value (0
).
use Carp (); | ||
with 'JMAP::TestSuite::Entity' => { | ||
singular_noun => 'calendar', | ||
properties => [ qw( |
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.
Same as in Comparator/Calendar.pm.
No description provided.