Closed Bug 782670 Opened 12 years ago Closed 10 years ago

Allow to save items to calendar, even though they are invitations (override REQUEST with PUBLISH)

Categories

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: MakeMyDay)

References

Details

Attachments

(1 file, 2 obsolete files)

Sometimes you receieve an invitation, but all you want to do is save the item to your calendar, without notifying anyone about it. We should add a button/menuitem that allows overriding the REQUEST method and handling it the same as if it were a PUBLISH.

Our current GSoc student is implementing COUNTER (bug 426532) and delgation and has made the third button a dropdown button. We could easily use this dropdown to provide a such menuitem.

I'm not sure what to call it though, "Save" sounds too general and might make users accidentally do the wrong thing.

Also, we should check if there are any notes in rfc5545 to this end.
The patch provides a solution as proposed by storing a copy of the event spoofing a published event.

Therefore, further processing is determined by the standard behaviour for processing of published items, mainly a manual selection of the target calendar (while on invitations, the app makes a guess to determine it without interaction).

This may lead to weired user expirience in an edge case, if the user first saves a copy selecting a different calendar than that, the app would use for processing the invitation, and thereafter replying on the invitation (then without getting aksed for the target calendar). In this case the user may expect, the invitation was added to the same calendar as choosen previously, what isn't the case. A solution for that may be to use the same guessing function for published events as for invitations generally.

To distinguish the local copy from the original invitation event, the local copy gets a new UID. This enables the user to deal with the invitation independently. Currently, the user can thereby create as much copies as he likes. 

To not loose the information about the original event, a sibling relation with reference to the original event UID is added to the copy. If we have the ability to search for related items once, we can provide the option to update a local copy once the original invitation gets an update. But I would like to leave this to be adressed in a separate bug.

Regarding feature advertisting to the user the patch currently relies on the new button patch of bug 990009.
Assignee: nobody → makemyday
Status: NEW → ASSIGNED
Attachment #8409672 - Flags: review?(philipp)
Updated patch to apply smoothly after last changes in patch to bug 990009.
Attachment #8409672 - Attachment is obsolete: true
Attachment #8409672 - Flags: review?(philipp)
Attachment #8411645 - Flags: review?(philipp)
Comment on attachment 8411645 [details] [diff] [review]
SaveEventCopyFromInvitation-V2.diff

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

I'd like to take another look at the patch when you have the following comments resolved:

::: calendar/base/modules/calItipUtils.jsm
@@ +289,2 @@
>                      }
> +                    data.buttons.push("imipMoreButton");

The more button is auto-hidden when there are no menuitems, i.e in cases where saving to calendar makes no sense, right?

@@ +805,5 @@
> +
> +        let itipItem = Components.classes["@mozilla.org/calendar/itip-item;1"]
> +                                 .createInstance(Components.interfaces.calIItipItem);
> +        let serializedItems = "";
> +        (aItems || {}).forEach(function(item) serializeItems += cal.getSerializedItem(item));

Do you mean [] here?

@@ +815,5 @@
> +        itipItem.isSend = ("isSend" in props) ? props.isSend : aItipItem.isSend;
> +        itipItem.localStatus = ("localStatus" in props) ? props.localStatus : aItipItem.localStatus;
> +        itipItem.receivedMethod = ("receivedMethod" in props) ? props.receivedMethod : aItipItem.receivedMethod;
> +        itipItem.responseMethod = ("responseMethod" in props) ? props.responseMethod : aItipItem.responseMethod;
> +        itipItem.targetCalendar = ("targetCalendar" in props) ? properties.targetCalendar : aItipItem.targetCalendar;

This code would be calling the setters every time, although it might be less nice to read, I'd suggest to use:

if ("autoResponse" in props) itipItem.autoResponse = props.autoResponse;

I would relax the rule for brackets even for one-line ifs here. Another approach thats nice and readable:


function adoptProperty(name) {
  if (name in props) {
    itipItem[name] = props[name];
  }
}

["autoResponse", "identity", "isSend", ... ].forEach(adoptProperty);

::: calendar/base/modules/calUtils.jsm
@@ +198,5 @@
> +     * - has a relation set to the original event
> +     * - has the same organizer but
> +     * - has any attendee removed
> +     * Intended to get a PUBLISHed like copy of an event invitation
> +     * for to store it as published event

nit: this could use some rewording:

Intended to get a copy of a normal event invitation that behaves as if the PUBLISH method was chosen instead.

@@ +203,5 @@
> +     *
> +     * @param aItem         original item
> +     * @param aUid          (optional) UID to use for the new item
> +     */
> +    getPublishLikeItemCopy: function cal_GetPublishLikeItemCopy(aItem, aUid) {

Feel free to drop the extra function name, this comes from times where the function would show up as anonymous in the debugger. This has now been fixed so we can make them anonymous as long as they have a label from the containing object.

@@ +207,5 @@
> +    getPublishLikeItemCopy: function cal_GetPublishLikeItemCopy(aItem, aUid) {
> +        // avoid changing aItem
> +        let item = aItem.clone();
> +        // reset to a new UUID if applicable
> +        item.id = (aUid == undefined) ? cal.getUUID() : aUid;

Since you are doing a simple == compare here, you can just do:

item.id = aUid || cal.getUUID();

@@ +216,5 @@
> +        item.addRelation(relation);
> +        // remove attendees but preserve organizer
> +        let organizer = item.organizer;
> +        item.removeAllAttendees();
> +        item.organizer = organizer;

Does removing all attendees also remove the organizer? If not, you likely don't need to set the organizer again.

Another question while I am here, if the organizer doesn't match the target calendar organizer, I believe this event will show up in the summary event dialog. Did you test that case? Possibly we need to change the organizer?

::: calendar/lightning/content/imip-bar.js
@@ +204,5 @@
> +
> +        function _execAction(aActionFunc, aItipItem, aWindow, aPartStat) {
> +            if (cal.itip.promptCalendar(aActionFunc.method, aItipItem, aWindow)) {
> +                // filter out fake partstats
> +                let partStat = (aPartStat.indexOf("X-") == -1) ? aPartStat : '';

They all start with X-, so you could do aPartStat.startsWith("X-");

@@ +243,5 @@
> +                return true;
> +            }
> +            return false;
> +        }
> +        

A whitespace snuck in here.

@@ +281,5 @@
> +                // we create and adopt copies of the respective events
> +                let items = [];
> +                ltnImipBar.itipItem
> +                          .getItemList({})
> +                          .forEach(function(item) items.push(cal.getPublishLikeItemCopy(item)));

You can use map here instead:

let items = ltnImipBar.itipItem.getItemList({}).map(cal.getPublishLikeItemCopy.bind(cal));

::: calendar/locales/en-US/chrome/lightning/lightning.dtd
@@ +58,5 @@
>  <!ENTITY lightning.imipbar.btnMore.tooltiptext                              "Click to show more options">
>  <!ENTITY lightning.imipbar.btnReconfirm.label                               "Send">
>  <!ENTITY lightning.imipbar.btnReconfirm.tooltiptext                         "Sends a re-confirmation to the organizer">
> +<!ENTITY lightning.imipbar.btnSaveCopy.label                                "Save a copy">
> +<!ENTITY lightning.imipbar.btnSaveCopy.tooltiptext                          "Save a copy of the event to the calendar without replying to the organizer (attendee list will be dropped)">

"The list of attendees will be cleared" sounds less technical. Go ahead and make it a new sentence without parentheses.
Attachment #8411645 - Flags: review?(philipp) → review-
Updated patch with (most) comments considered.

> The more button is auto-hidden when there are no menuitems, i.e in cases
> where saving to calendar makes no sense, right?
The more button shows up only when receiving an initial or updated invitation. In this cases, the contained menuitem to enable saving locally is always available (currently it also keeps enabled after saving the copy, maybe it should be disabled then?).

The patch moves the action to accept tentatively from a separate button to the dropdown of the accept button to keep the 3 button style.

> This code would be calling the setters every time, although it might be less
> nice to read, I'd suggest to use: 
This is intended, as the new itipItem property needs either the value of the original one or that one specified to be overwritten. Therefore I'm leaving this part as it is. 

> Another question while I am here, if the organizer doesn't match the target
> calendar organizer, I believe this event will show up in the summary event
> dialog. Did you test that case? Possibly we need to change the organizer?
I don't get the point here. We're at incoming invitations here, so the organizer is usually a remote party and not one of the registered calendar identities. If you open the saved copy, this is done with the event dialog.

> "The list of attendees will be cleared" sounds less technical. Go ahead and
> make it a new sentence without parentheses.
I made it to "Save a copy of the event to the calendar independently of replying to the organizer. The list of attendees will be cleared." to make it more obvious that this is not related to the reply to the organizer.
Attachment #8416713 - Flags: review?(philipp)
Comment on attachment 8416713 [details] [diff] [review]
SaveEventCopyFromInvitation-V3.diff

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

> > Another question while I am here, if the organizer doesn't match the target
> > calendar organizer, I believe this event will show up in the summary event
> > dialog. Did you test that case? Possibly we need to change the organizer?
> I don't get the point here. We're at incoming invitations here, so the
> organizer is usually a remote party and not one of the registered calendar
> identities. If you open the saved copy, this is done with the event dialog.
I'm sorry I described it wrong, I was thinking about the case where an attendee matches the identity of one of the calendar's organizers. In this case the summary dialog is opened. But you mention the list of attendees is cleared, so this shouldn't be an issue.
Attachment #8416713 - Flags: review?(philipp) → review+
Keywords: checkin-needed
Attachment #8411645 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/b7824bdf5b5d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.5
Regressions: 1751981
No longer regressions: 1751981
See Also: → 1751981
You need to log in before you can comment on or make changes to this bug.