Closed Bug 370150 Opened 13 years ago Closed 12 years ago

API enhancement: additions for group scheduling

Categories

(Calendar :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

(Whiteboard: [gdata-0.5])

Attachments

(1 file, 4 obsolete files)

Attached patch calIGroupSchedulingCalendar (obsolete) β€” β€” Splinter Review
Proposing calIGroupSchedulingCalendar extending calICalendar:
- isOwnedCalendar, isDefaultCalendar are already used by invitation list prototype
- isInvitation, getInvitedAttendee are used both by the invitation list and event dialog prototype
Attachment #254805 - Flags: first-review?(dmose)
Assignee: nobody → daniel.boelzle
Proposing item filters for reducing the result set when querying for open invitations, used e.g. by invitation list prototype.
Attachment #254806 - Flags: first-review?(dmose)
The semantics of being the default calendar probably need to be nailed down more specifically.  What assumptions is the front-end code allowed to make that are different between default and non-default calendars?  That it should be polling for or expecting invitations to just show up somehow?  If so, how?

> - isInvitation, getInvitedAttendee are used both by the invitation list and
> event dialog prototype

Both of these methods claim to "take the owner of the calendar into account."  What does that mean?  (I'm trying to understand why these methods want to live a calendar object rather than elsewhere).
(In reply to comment #2)
> The semantics of being the default calendar probably need to be nailed down
> more specifically.  What assumptions is the front-end code allowed to make that
> are different between default and non-default calendars?  That it should be
> polling for or expecting invitations to just show up somehow?  If so, how?

Yes, a user's default calendar is the calendar instance where the calendar system places invitations into.
I think polling is generally the worst mechanism; we shouldn't define that. I think group scheduling providers should at least implement a polling (or better a real push) mechanism if e.g. an invitation took place, and fire on calIObserver.

> > - isInvitation, getInvitedAttendee are used both by the invitation list and
> > event dialog prototype
> 
> Both of these methods claim to "take the owner of the calendar into account." 
> What does that mean?  (I'm trying to understand why these methods want to live
> a calendar object rather than elsewhere).

If the owner of a calendar schedules an invitation, she gets the organizer, so
- isInvitation checks whether the owner of the calendar has scheduled the event
- getInvitedAttendee figures out the invited attendee (in case of an invitation)

An alternative would be to shift those members to calIItemBase, but this IMO only bloats that already fat interface.
(In reply to comment #3)
> I think group scheduling providers should at least implement a polling (or
> better a real push) mechanism if e.g. an invitation took place, and fire on
> calIObserver.
 
OK, that sounds reasonable, and I think that's the piece of behavior that needs to be nailed down and documented here (does it also need a callback method added somewhere?)

> If the owner of a calendar schedules an invitation, she gets the organizer, so
> - isInvitation checks whether the owner of the calendar has scheduled the event

This name doesn't seem right to me, as it doesn't really describe what the method does.  That said, I'm not really sure what would be better.   I also wonder if there should simply be an "owner" attribute so that callers can do this comparison themselves.  

> - getInvitedAttendee figures out the invited attendee (in case of an
> invitation)
> 
> An alternative would be to shift those members to calIItemBase, but this IMO
> only bloats that already fat interface.

Maybe a calIInvitation (which could either inherit from or being used in conjunction with calIItemBase) is what's wanted here?

I'd be interested in hearing opinions on this stuff from folks who are working on other providers (e.g. Google, CalDAV) as well.
(In reply to comment #4)

> > If the owner of a calendar schedules an invitation, she gets the organizer, so
> > - isInvitation checks whether the owner of the calendar has scheduled the event
> 
> This name doesn't seem right to me, as it doesn't really describe what the
> method does.  That said, I'm not really sure what would be better.   I also

What about isInvitationCopy?

> wonder if there should simply be an "owner" attribute so that callers can do
> this comparison themselves.  

I think the latter might be a bit complicated as attendee/organizer ids are probably ambiguous when sticking to eMail addresses (which I would prefer).
So comparing those won't necessarily figure out that an item (which is part of a calendar instance) is an invitation copy or not.
E.g. in WCAP provider, I currently let the server return eMail addresses for ORGANIZER/ATTENDEE in the returned ics data, but the real unique calid in an X-param of ORGANIZER/ATTENDEE.
Thus I would like to shift this check to the provider code which knows the details (about X-params, ...), although I admit the API is unfortunate, but pragmatical.

> > - getInvitedAttendee figures out the invited attendee (in case of an
> > invitation)
> > 
> > An alternative would be to shift those members to calIItemBase, but this IMO
> > only bloats that already fat interface.
> 
> Maybe a calIInvitation (which could either inherit from or being used in
> conjunction with calIItemBase) is what's wanted here?

Yes, I like that idea. The implementation in base/src could implement it comparing ORGANIZER/ATTENDEE ids (eMail address), but providers would need the possibility to override it, e.g.
- override attributes e.g. overrideInvitationCopy, overrideInvitedAttendee
- an override bit so the calendar instance is called back on isInvitation/getInvitedAttendee
The (draft) CalDAV scheduling spec defines both Inbox and Outbox calendar-collection types to be used for scheduling, with special properties and content restrictions. They're clearly intended not to be used as 'normal' calendar collections. To the extent that the meaning of 'isDefaultCalendar' is restricted to 'where invitations are normally placed by the system', I don't see a problem with it from the CalDAV perspective. Perhaps 'isInboxCalendar' would be a better name, to make it clear that the semantic includes only that property?

I also like the idea of a calIInvitation, overridable by the provider as described by Daniel. 
I think these are good ideas on the whole.   I wonder how you expect interoperability to work out with the iTIP subsystem?  It uses these calItipItem objects to encapsulate calIItems that are part of an invitation (because more than one Item can be attached to a invitation/response). So, I would like to see some information on how you plan to address these type of out-of-band invitations (i.e. they are not received by the calendar provider).

The iTIP subsystem will need the ability to search for invitations, and having a way to specify what invitations need action will be key for good iTIP and other invitation strategy interoperability. I think that we will also need some kind of tracking mechanism that registers invitations across the product.  I would like to see that addressed in this interface.  For example

readonly attribute nsISimpleEnumerator invitationList

This will be important especially if users do not use one default calendar for their invitations, but perhaps put work invitations in a work calendar and family invitations into a family calendar.

Also, one of the biggest issues in group scheduling is Free/Busy support.  Do you intend that in the future this interface could also be implemented by providers to provide Free/Busy support?   
> What about isInvitationCopy?

Maybe calIInvitation.isOrganizerCopy?

> Perhaps 'isInboxCalendar' would be a better name, to make it clear that
> semantic includes only that property?

Are we really talking about equivalent concepts?  What lives in the inbox and outboxes: if I recall correctly, in CalDAV, it is iTIP invitations, whereas it sounds like with WCAP, the default calendar contains normal VEVENTs.  Or am I confused?

I'm going to remove the r? here, as it sounds like there are a number of issues to be worked through and at least one more iteration is going to be required.
Attachment #254805 - Flags: first-review?(dmose)
Comment on attachment 254806 [details] [diff] [review]
ITEM_FILTER_REQUEST_NEEDS_ACTION, ITEM_FILTER_REQUEST_NEEDSNOACTION

I think we're going to need to get the basic interfaces sorted out before reviewing the ITEM_FILTER patch.  I'd suggest asking ctalbert for review once they're ready, because I think that coherence with the iTIP subsystem is the important thing here.
Attachment #254806 - Flags: first-review?(dmose)
(In reply to comment #8)

> Are we really talking about equivalent concepts?  What lives in the inbox and
> outboxes: if I recall correctly, in CalDAV, it is iTIP invitations, whereas it
> sounds like with WCAP, the default calendar contains normal VEVENTs.  Or am I
> confused?

That's correct wrt CalDAV; perhaps I'm confused about how WCAP handles these things. For what it's worth, other than these scheduling in/out boxes, CalDAV does not really have a concept of a 'default' or 'home' calendar.
(In reply to comment #10)
> (In reply to comment #8)
> 
> > Are we really talking about equivalent concepts?  What lives in the inbox and
> > outboxes: if I recall correctly, in CalDAV, it is iTIP invitations, whereas it
> > sounds like with WCAP, the default calendar contains normal VEVENTs.  Or am I
> > confused?
> 
> That's correct wrt CalDAV; perhaps I'm confused about how WCAP handles these
> things. For what it's worth, other than these scheduling in/out boxes, CalDAV
> does not really have a concept of a 'default' or 'home' calendar.
> 

I think the concepts are similar, BUT
- regarding CalDAV, the client processes processes the inbox and (may automatically) store a copy of an incoming invitation in one of the user's calendar collections, preferably the home-collection (calendar-home-URL) to book it.
- regarding WCAP, the server does that for you, i.e. stores a copy of it into the user's default calendar.
The API somehow has to balance this out.
Attached patch isInvitationCopy, getInvitedAttendee (obsolete) β€” β€” Splinter Review
The invitation copy stuff of the proposed calIGroupSchedulingCalendar still is valid:
The UI needs to figure out whether a specific item corresponds to an invitation or not, presenting different dialogs (e.g. REPLYing to an invitation is limited to only PARTSTAT).

Please mind that this doesn't correlate with incoming (inbox) invitations. Referring to CalDAV, the inbox will be processed/emptied by clients, which then place the (invitation) copy of the item in some calendar.
Attachment #254805 - Attachment is obsolete: true
Attachment #277714 - Flags: review?
Attachment #277714 - Flags: review? → review?(ctalbert)
Comment on attachment 254806 [details] [diff] [review]
ITEM_FILTER_REQUEST_NEEDS_ACTION, ITEM_FILTER_REQUEST_NEEDSNOACTION

Obsolete; As far as I can foresee, this will be covered by calIItipTransport::checkForInvitations.
Attachment #254806 - Attachment is obsolete: true
I don't quite understand the comments describing those two functions. Can you elaborate a bit more?
We need to detect whether an item of a calendar corresponds to an invitation, i.e. the calendar system or the attendee has placed a copy of an item in the calendar to book that time. We want to present different UI (event-summary-dialog) in that case, so the attendee could e.g. only REPLY to the invitation. That's the purpose of those two functions.
If a provider could not distinguish that (e.g. a plain ics file, no multi user calendar system), it should just return false, which leads to the general event dialog. (However I could imagine iTIP code could mark incoming invitation copies using an X-prop.)
Comment on attachment 277714 [details] [diff] [review]
isInvitationCopy, getInvitedAttendee

>Index: calICalendar.idl

>   /**
>+   * Tests whether the passed item corresponds to an invitation copy, i.e.
>+   * whether the calendar owner has been invited.
>+   *
>+   * @param aItem       item to be tested
>+   * @return            whether the passed item corresponds to an invitation
>+   */
>+  boolean isInvitationCopy(in calIItemBase aItem);
>+
I don't really like the word "Copy" in this item.  I think that it connotes an idea that there will be two items, one will be a legitimate event and one will be an invitation to that same event.  That isn't how most of the calendar providers will work for most users. Most of the time, there will only be one item, the invitation.  Why not isItemInvitation?
>+  /**
>+   * Gets the invited attendee (i.e. usually the owner of this calendar)
>+   * if the passed item corresponds to an invitation.
>+   *
>+   * @param aItem       invitation item
>+   * @return            attendee object else null
>+   */
>+  calIAttendee getInvitedAttendee(in calIItemBase aItem);
>+
If this invitation was sent to you (or if you organized the event) then the invited attendee will be yourself (the owner of the calendar).  So, why is it necessary to have an interface to determine that you, as the owner of this calendar are indeed the invited attendee?  Maybe what we really want here is a method that will return the status required from the owner of the calendar w.r.t. this invitation?  

For example, if you are an attendee of an event, the method would indicate that you need to REPLY to this because your PARTSTAT is RSVP-REQUIRED.  Or if you are an Organizer of this event and someone suggested a different time (COUNTER) then you need to decide whether or not to change and re-send the invitation.

It sounds like we are slowly moving toward a calIInvitation interface that providers implement as needed.  I'm happy to keep these few methods on calICalendar for now, but if we keep bloating calICalendar with more of these methods in the future, we might consider splitting the invitation stuff into its own interface just to keep things logically organized.

And my last question, once we have an r+ on this, do we need to also file follow-on bugs against every provider requesting that they evaluate whether or not to implement this interface?

Review is pending these answers...
(In reply to comment #16)
> >+  boolean isInvitationCopy(in calIItemBase aItem);
> >+
> I don't really like the word "Copy" in this item.  I think that it connotes an
> idea that there will be two items, one will be a legitimate event and one will
> be an invitation to that same event.  That isn't how most of the calendar
> providers will work for most users. Most of the time, there will only be one
> item, the invitation.  Why not isItemInvitation?
Ok with me.

> >+  calIAttendee getInvitedAttendee(in calIItemBase aItem);
> >+
> If this invitation was sent to you (or if you organized the event) then the
> invited attendee will be yourself (the owner of the calendar).  So, why is it
> necessary to have an interface to determine that you, as the owner of this
> calendar are indeed the invited attendee?  Maybe what we really want here is a
> method that will return the status required from the owner of the calendar
> w.r.t. this invitation?  

> For example, if you are an attendee of an event, the method would indicate that
> you need to REPLY to this because your PARTSTAT is RSVP-REQUIRED.  Or if you
> are an Organizer of this event and someone suggested a different time (COUNTER)
> then you need to decide whether or not to change and re-send the invitation.

Determining whether it is an invitation is done via isItemInvitation().
If it is an invitation you could get the current user's calIAttendee object from the item. Why should we limit that function to RSVP if calIAttendee gets you all of that: RSVP, PARTSTAT, ...?
I agree that we somehow need to determine the current user's identity w.r.t. invitation scheduling, but regarding this interface/use case, I think still could hide it into the providers.

> It sounds like we are slowly moving toward a calIInvitation interface that
> providers implement as needed.  I'm happy to keep these few methods on
> calICalendar for now, but if we keep bloating calICalendar with more of these
> methods in the future, we might consider splitting the invitation stuff into
> its own interface just to keep things logically organized.
Right, we could do so if necessary. At first step I proposed such a separation (into calIGroupSchedulingCalendar, see attachment), but meanwhile I think it'll just be those two methods. IMO calIItipTransport is the better way to go for scheduling (explicitly sending/replying invitations).

> And my last question, once we have an r+ on this, do we need to also file
> follow-on bugs against every provider requesting that they evaluate whether or
> not to implement this interface?
My plan is to add a default/empty implementation to all providers to make the methods callable in this bug, and write follow-up bugs for providers that could implement it.
I'd vote for a separate interface, because that allows calendar providers to share an implementation of scheduling. I'd think that storage and ics both want the same way to do scheduling, because they are both essentially p2p and lack a central server.
Mvl has a really good point about the /use provider/ or /make new interface/ question.  Daniel, what does having these calls on the provider interface buy us over providing a separate scheduling interface?

Duplicate of this bug: 396866
(In reply to comment #18)
> I'd vote for a separate interface, because that allows calendar providers to
> share an implementation of scheduling. I'd think that storage and ics both want
> the same way to do scheduling, because they are both essentially p2p and lack a
> central server.
> 

Given the existence of Components.utils.import (see <http://developer.mozilla.org/en/docs/Components.utils.import>, it should now be just as easy to share code in JS as it is in C++, even given a single interface.
Flags: wanted-calendar0.8+
Comment on attachment 277714 [details] [diff] [review]
isInvitationCopy, getInvitedAttendee

I'm going to minus on this just to get some movement, since this has rather stalled (largely my own fault, I believe).

We need an updated patch, to address the nits above in comment 16.  

I think this doesn't prevent us from having a shared scheduling interface since scheduling is different than "invitation detection".  And this is trying to solve the "invitation detection" problem.  However, I don't like the idea of using an X prop for this.  But if we did use the X-Prop, the most natural thing to do would be to copy the parent VCalendar's METHOD property value into the X-Prop inside the VEVENT, since this would enable us to also understand the "state" of this invitation (whether it is a reply, a request, update etc).
Attachment #277714 - Flags: review?(ctalbert) → review-
Not going to happen for 0.8
Flags: wanted-calendar0.8+ → wanted-calendar0.8-
Flags: wanted-calendar0.8- → wanted-calendar0.9+
Blocks: 409921
Flags: wanted-calendar0.9+ → blocking-calendar0.9+
Attached patch patch - v1 (obsolete) β€” β€” Splinter Review
This patch is based on the previous ideas the way that:
- it adds calISchedulingSupport (having both isInvitation and getInvitedAttendee)
- it adds a skeleton "base" implementation for storage/memory/caldav that will later be suitable for stored iTIP/iMIP invitations. I think we should tag incoming mail invitation items with a X-MOZ prop, e.g. X-MOZ-IMIP-INCOMING-ACCOUNT. This should finally enable to use the calendar summary dialog for those invitations.
- it reworks the calendar invitation dialog and stuff to pull items with filter bit calICalendar.ITEM_FILTER_REQUEST_NEEDS_ACTION
- it removes various WCAP specific code e.g. from event dialog, ands adds calendar properties organizerId and organozerCN.

I'd like Bruno and Philipp to have a look at the changes.
Attachment #277714 - Attachment is obsolete: true
Attachment #327120 - Flags: review?(philipp)
Comment on attachment 327120 [details] [diff] [review]
patch - v1

>     var isInvitation = false;
>     try {
>-        isInvitation = calendar.isInvitation(calendarItem);
>+        isInvitation = calendar.QueryInterface(Components.interfaces.calISchedulingSupport)
>+                               .isInvitation(calendarItem);
>     }
>     catch(e) {}
I'd prefer
var isInvitation = false;
if (calendar instanceof Components.interfaces.calISchedulingSupport) {
    isInvitation = calendar.isInvitation(calendarItem);
}
 

The same goes for:  


window.readOnly = calendar.readOnly;
if (!window.readOnly && calendar instanceof Components.interfaces.calISchedulingSupport) {
    var attendee = calendar.getInvitedAttendee(item);
    if (attendee) {
        window.attendee = attendee.clone();
        try {
            item.removeAttendee(attendee);
        } catch (exc) {
            // ignore if attendee does not exist, e.g. REPLY on mailing lists
        }
        item.addAttendee(window.attendee);
    }
}
I don't quite understand why we need to remove the old attendee and then add the cloned one. Aren't they the same?


>+[scriptable, uuid(9221E243-C97E-4C5F-9E00-5D7D3521BB44)]
I think mschroeder has proven me wrong on this, I don't quite remember, but I think most of our uuids are lowercase?


>+     * @param aItem item to be tested
>+     * @return whether the passed item corresponds to an invitation
Indent/Align

>+     * @param aItem invitation item
>+     * @return attendee object else null
Indent/Align


>     QueryInterface: function cCC_QueryInterface(aIID) {
>+        if (aIID.equals(Components.interfaces.calISchedulingSupport)) {
>+            // check whether uncached calendar supports it:
>+            if (this.mUncachedCalendar.QueryInterface(aIID)) {
>+                return this;
>+            }
>+        }

>       <method name="getCalendarItemParticipationStatus">
>         <parameter name="aItem"/>
>         <body><![CDATA[

>           try {
try -> if ( instanceof ) here and elsewhere

>               if (att) {
>                   var att_ = att.clone();
>                   att_.participationStatus = aStatus;
>-                  aItem.removeAttendee(att);
>+                  try {
>+                      aItem.removeAttendee(att);
>+                  } catch (exc) {}
Theoretically, removeAttendee shouldn't be throwing just because the attendee doesn't exist? I think it should be that way, since removing an attendee that is not on the item is definitely a success.



>+        for each (var calendar in cals) {
>+            if (calendar.readOnly || calendar.getProperty("disabled")) {
>+                opListener.onOperationComplete();
>+                continue;
>+            }
isCalendarWritable()

>+    itip_getInvitedAttendee: function cPB_itip_getInvitedAttendee(aItem) {
>+        // This is the iTIP specific base implementation for storage and memory,
>+        // it will mind what account has received the incoming message, e.g.
>+//         var account = aItem.getProperty("X-MOZ-IMIP-INCOMING-ACCOUNT");
>+//         if (account) {
>+//             account = ("mailto:" + account);
>+//             var att = aItem.getAttendeeById(account);
>+//             if (att) {
>+//                 // we take the existing attendee
>+//                 return att;
>+//             }
>+//             // else user may have changed mail accounts, or we has been invited via ml
>+//             // in any way, we create a new attendee to be added here, which may be
>+//             // overridden by UI in case that account doesn't exist anymore:
>+//             att = Components.classes["@mozilla.org/calendar/attendee;1"]
>+//                             .createInstance(Components.interfaces.calIAttendee);
>+//             att.participationStatus = "NEEDS-ACTION";
>+//             att.id = account;
>+//         }
>+        // for now not impl
Why?
Attachment #327120 - Flags: review?(philipp) → review+
Attached patch patch - v2 β€” β€” Splinter Review
Patch with review comments addressed, as checked in (since dbo is on vacation)
Attachment #327120 - Attachment is obsolete: true
Attachment #327196 - Flags: review+
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [gdata-cvs]
Target Milestone: --- → 0.9
(In reply to comment #25)
> I don't quite understand why we need to remove the old attendee and then add
> the cloned one. Aren't they the same?
They are the same, but addiong doesn't check for an existing attendee, and we want to handle both the update case (i.e there was an old attendee), and the new case (i.e the attendee wasn't there before, i.e mailing list).

> Theoretically, removeAttendee shouldn't be throwing just because the attendee
> doesn't exist? I think it should be that way, since removing an attendee that
> is not on the item is definitely a success.
Fixed, doesn't throw anymore.


> [commented out code]
> Why?
Not needed for now, example implementation. I didn't quite understand all the details dbo told me here, if anyone is interested maybe dbo can comment when he's back.
Possible Gdata relnote:

With this bug fixed, you should rather have the "show invitations in calendar" option ON.
Whiteboard: [gdata-cvs] → [gdata-0.5]
You need to log in before you can comment on or make changes to this bug.