Closed Bug 980008 Opened 7 years ago Closed 7 years ago

Adjust event status handling in calendar event dialog

Categories

(Calendar :: Dialogs, defect)

Lightning 2.6.4
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: MakeMyDay, Assigned: MakeMyDay)

Details

Attachments

(1 file, 2 obsolete files)

The current implementation of calendar-event-dialog allows to the user to set the status of an event (this bug is not about todo) to any available value (none, confirmed, tentative or cancelled). If not set to none, this results in a respective ical property. If the calendar has scheduling and the event has also attendees, a status change fires a scheduling operation on pressing save&close (currently not delivered in case of event updates, but this is bug 938459).

This may lead to some odd/invalid operations (at least but probably not limited to Itip scheduling):

- a new invitation with status cancelled is sent, if the the event was newly created (it should not be be sent any notification in this case? / RfC5546 allows REQUESTs only to contain status confirmed/tentative)

- an invitation update with status cancelled is sent, if the event already existed with the same attendees (a cancellation would be approriate here instead?).

- the status can be set to none even it had a value before (once set, a status should not be removed but only be set to another value)

For the latter one, a solution similar to the prosal in bug 979996 may be appropriate, while the former onces would need a sophisticated handling in (at least) itip composition if we keep the event status configurable in the current way.

A more straight foreward solution would be to remove the ability to change status from the dilaog menu, set confirmed as default value on event creation and add a checkbox to allow the user to toggle it to tentative instead. If a meeting has to be cancelled, the user can just delete it, which results in appropriate cancellation messages to the attendees.
First, the patch removes the ability to set back the event status to "none" once it had another value when loading the dialog.

Second, it adds special handling on invitations to an event with status cancelled: if the event is newly created, it just skips the invitation, otherwise a cancellation instead of a reqest will be sent.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8386984 - Flags: review?(philipp)
Comment on attachment 8386984 [details] [diff] [review]
ChangeEventStatusHandlingInEventDialog-V1.diff

Review of attachment 8386984 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to r+ and push this with comments taken care of, but there are two open questions I'd like you to answer and/or open a new bug for. The first question is below, the last comment about SEQUENCE:0. Secondly, does this code behave correctly when you first set an event to cancelled, then change it to CONFIRMED? Will it send a normal request, or a (wrong) update?

::: calendar/base/content/dialogs/calendar-event-dialog.js
@@ +384,5 @@
>          gStatus = item.hasProperty("STATUS") ?
>              item.getProperty("STATUS") : "NONE";
> +        if (gStatus == "NONE") {
> +            document.getElementById("options-status-none-menuitem").removeAttribute("hidden");
> +            document.getElementById("event-status-none-menuitem").removeAttribute("hidden");

Instead of setting this on the menuitems, you can just set the attribute on the command that controls it.

@@ +1732,5 @@
>  function rotateStatus() {
>      const states = ["NONE","TENTATIVE","CONFIRMED","CANCELLED"];
>      gStatus = states[(states.indexOf(gStatus) + 1) % states.length];
> +    // if control for status "NONE" is hidden, rotate to next status
> +    if (isEvent(window.arguments[0].calendarEvent) &&

cal.isEvent(), also I think its better to use window.calendarItem.

@@ +1735,5 @@
> +    // if control for status "NONE" is hidden, rotate to next status
> +    if (isEvent(window.arguments[0].calendarEvent) &&
> +        (document.getElementById("options-status-none-menuitem").hasAttribute("hidden") ||
> +         document.getElementById("event-status-none-menuitem").hasAttribute("hidden"))) {
> +        rotateStatus();

This would cause updateStatus to be called twice. I think it might be better to just make states non-const and insert the NONE status if needed.

::: calendar/base/modules/calItipUtils.jsm
@@ +625,5 @@
> +
> +        // special handling for invitation with event status cancelled
> +        if (aItem.getAttendees({}).length > 0 && 
> +            aItem.hasProperty("STATUS") &&
> +            aItem.getProperty("STATUS") == "CANCELLED") {

If the item has no status property it most certainly won't be "CANCELLED". No need for the extra hasProperty call. Also, an extra whitespace.

@@ +628,5 @@
> +            aItem.hasProperty("STATUS") &&
> +            aItem.getProperty("STATUS") == "CANCELLED") {
> +
> +            if(aItem.hasProperty("SEQUENCE") &&
> +               aItem.getProperty("SEQUENCE") != "0") {

Same here.

@@ +633,5 @@
> +                // make sure we send a cancellation and not an request
> +                aOpType = Components.interfaces.calIOperationListener.DELETE;
> +            } else {
> +                // don't send an invitation, if the event was newly created and has status cancelled
> +                return;

Do all newly scheduled items have SEQUENCE:0 or is this provider dependent?
Attachment #8386984 - Flags: review?(philipp) → review+
I'll continue this afternoon. Before you go fixing comments, I have made most changes locally :)
Whether an itip invitation is handled as initial invitation or an update is a receiver side decission based on the the existance of the local copy identified by UID property. There is no specification on sender side (see http://tools.ietf.org/html/rfc5546.html#section-3.2.2 for details). -> no need to care about here

I didn't check each provider, but the RfC makes the sequence property optional for a new request. We can simply use getSequence() from calItipUtils instead to check on the sequence, as this takes already care about the different constellations.

The patch takes care about your comments, the use of getSequence and an alternate approach for the rotateStatus() by just count up gStatus without calling updateStatus twice.
Attachment #8386984 - Attachment is obsolete: true
Attached patch Fix - v3 β€” β€” Splinter Review
Good idea with the getSequence() check, I've taken that into account. I had a slightly different approach with the rotate function which avoids some duplicate code. This also uses the <command> element instead.

As these are just minor modifications to your code, I think I can safely set r=philipp
Attachment #8391714 - Attachment is obsolete: true
Attachment #8391742 - Flags: review+
Pushed to comm-central changeset 1d3f93e06120
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.2
You need to log in before you can comment on or make changes to this bug.