Closed Bug 457203 Opened 11 years ago Closed 11 years ago

iTIP overhaul

Categories

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

enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Since we need to implement a similar set of iTIP functionality for caldav as for iTIP/iMIP, it makes sense to refactor and further fix the existing iTIP code base.
I don't think I will recycle the iTIP processor, but rather offer the new API as a js code module (http://developer.mozilla.org/en/Using_JavaScript_code_modules).
Flags: wanted-calendar1.0+
Blocks: 423861
Blocks: 450565
No longer blocks: 454755
Attached patch WIP patch - v1 (obsolete) — Splinter Review
Blocks: 460413
Depends on: 358498
Blocks: 303663
Blocks: 357399
Attached patch patch - v1Splinter Review
This patch already got some pretesting; thanks to Andreas.

Depends on patch on bug 462393.
Attachment #342581 - Attachment is obsolete: true
Attachment #345756 - Flags: review?(philipp)
Depends on: 462393
Attachment #345756 - Flags: review?(philipp) → review+
Comment on attachment 345756 [details] [diff] [review]
patch - v1

>+            if (!item.alarmOffset && (attendee.participationStatus == "NEEDS-ACTION")) {
>+                cal.setDefaultAlarmValues(item);
...
>             // set default alarm on unresponded items that have not been declined:
>             if (oldStatus == "NEEDS-ACTION" && newStatus != "DECLINED") {
>-                setDefaultAlarmValues(newCalendarItem);
>+                cal.setDefaultAlarmValues(newCalendarItem);
>             }
Doesn't this need a similar fix to also check for !item.alarmOffset ?

>+Components.utils.import("resource://calendar/modules/calUtils.jsm");
>+EXPORTED_SYMBOLS = ["cal"]; // even though it's defined in calUtils.jsm, import needs this
>+cal.itip = {
What happens when calItipUtils.jsm and calUtils.js are both imported? Wouldn't something bork because calItipUtil's "cal" already defines things when importing calUtils?

>+            case "CANCEL":
>+            case "REPLY": {
>+                // Per iTIP spec (new Draft 4), multiple items in an iTIP message MUST have
>+                // same ID, this simplifies our searching, we can just look for Item[0].id
>+                let itemList = itipItem.getItemList({});
>+                if (itemList.length > 0) {
>+                    itipItem.targetCalendar.getItem(itemList[0].id,
>+                                                    new ItipFindItemListener(itipItem, optionsFunc));
>+                } else if (optionsFunc) {
>+                    optionsFunc(itipItem, Components.results.NS_OK);
>+                }
>+                break;
>+            }
>+            default: {
>+                if (optionsFunc) {
>+                    optionsFunc(itipItem, Components.results.NS_ERROR_NOT_IMPLEMENTED);
>+                }
>+                break;
>+            }
Although I understand your reasoning, using block scope in a switch() statement has been quite uncommon in the past. Is this something we generally want to do (i.e also put in the style guide) ?


>+        if (invitedAttendee) { // actually is an invitation copy, fix attendee list to send REPLY
...
>+                    sendMessage(aItem, "REPLY", [aItem.organizer], autoResponse);
>+                }
>+            }
>+
>+            return;
>+        }
>+
>+        if (aItem.getProperty("X-MOZ-SEND-INVITATIONS") != "TRUE") { // Only send invitations/cancellations
>+                                                                     // if the user checked the checkbox
>+            return;
>+        }
You send a message even before you check for X-MOZ-SEND-INVITATIONS, is this really what you want?

>+



>+// local to this module file
>+function setReceivedInfo(item, itipItemItem) {
...
>+
>+// local to this module file
>+function createOrganizer(aCalendar) {
...
>+
>+// local to this module file
>+function sendMessage(aItem, aMethod, aRecipientsList, autoResponse) {
...
>+
>+// local to this module file
>+function ItipOpListener(opListener, oldItem) {
......

Why not put these to the bottom of the file and add a /* The following functions are local to this module file */ ? Also, some documentation of the functions would be nice.

>+                                                                           aDetail) {
>+        let rc = Components.results.NS_OK;
>+        const method = this.mItipItem.receivedMethod.toUpperCase();
why const and not let/var ? I doubt this is constant?


>+                            let rid = itipItemItem.recurrenceId; //  XXX todo support support
support what?


>+                                    NS_ASSERT(attendees.length == 1, "invalid number of attendees in REFRESH!");
Sometimes you use NS_ASSERT, other times cal.ASSERT, unless there is a difference I haven't noticed, please align.


>+                                    operations.push(
>+                                        function(opListener) {
>+                                            return item.calendar.deleteItem(item, opListener);
>+                                        });
>+                                }
>+                            } else {
>+                                operations.push(
>+                                    function(opListener) {
>+                                        return item.calendar.deleteItem(item, opListener);
>+                                    });
>+                            }
It would probably be nice to unanonymize such functions for better debugging.

> 
>+        // xxx todo: make sure object is properly reset
>         this.setItemBaseFromICS(event);
>         this.mapPropsFromICS(event, this.icsEventPropMap);
Please file a followup bug to clarify


>     ensureNotDirty: function() {
>         if (!this.mDirty)
>             return;
>-
While you are here, please add brackets.

>+                // Bug 348666: When we handle more iTIP methods, we need to create
>+                // more sophisticated error handling.
>+                // TODO L10N localize
>+                imipBar.setAttribute("collapsed", "true");
>+                let msg = "Invitation could not be processed. Status: " + aStatus;
>+                if (aDetail) {
>+                    msg += "\nDetails: " + aDetail;
>+                }
Could we localize this while we are at it?


>+imipBarRefreshText=This message contains an event inquery.
>+imipBarPublishText=This message contains an event.
I doubt the normal user will understand the difference between these two.




r=philipp
(In reply to comment #3)
> Doesn't this need a similar fix to also check for !item.alarmOffset ?
Yes, and should also check against DECLINED.

> >+Components.utils.import("resource://calendar/modules/calUtils.jsm");
> >+EXPORTED_SYMBOLS = ["cal"]; // even though it's defined in calUtils.jsm, import needs this
> >+cal.itip = {
> What happens when calItipUtils.jsm and calUtils.js are both imported? Wouldn't
> something bork because calItipUtil's "cal" already defines things when
> importing calUtils?
I think it doesn't matter since module scripts a set up only once. I did that in imip-bar.js and it seems to work.

> Although I understand your reasoning, using block scope in a switch() statement
> has been quite uncommon in the past. Is this something we generally want to do
> (i.e also put in the style guide) ?
Since we have |let| this IMO really makes sense to further limit scope of variables.

> >+        if (aItem.getProperty("X-MOZ-SEND-INVITATIONS") != "TRUE") { // Only send invitations/cancellations
> >+                                                                     // if the user checked the checkbox
> >+            return;
> >+        }
> You send a message even before you check for X-MOZ-SEND-INVITATIONS, is this
> really what you want?
Yes, X-MOZ-SEND-INVITATIONS is bound to the organizer's event only, thus the check only makes sense when dealing with organizer scope. The code before checks whether it's an attendee's copy resp. if a REPLY needs to be send.

> Why not put these to the bottom of the file and add a /* The following
> functions are local to this module file */ ? Also, some documentation of the
> functions would be nice.
Those are at the bottom (after the exported object)?
I've added documentation.

> >+        let rc = Components.results.NS_OK;
> >+        const method = this.mItipItem.receivedMethod.toUpperCase();
> why const and not let/var ? I doubt this is constant?
You can declare a |const| in function scope, it's just disallowed to reset in the very same function, e.g.

function foo(bar) {
   const c = bar
   c = 5 // fails
}

> >+                            let rid = itipItemItem.recurrenceId; //  XXX todo support support
> support what?
support multiple RECURRENCE-IDs

> >+                                    NS_ASSERT(attendees.length == 1, "invalid number of attendees in REFRESH!");
> Sometimes you use NS_ASSERT, other times cal.ASSERT, unless there is a
> difference I haven't noticed, please align.
I changed all back to cal.ASSERT().

> >+        // xxx todo: make sure object is properly reset
> >         this.setItemBaseFromICS(event);
> >         this.mapPropsFromICS(event, this.icsEventPropMap);
> Please file a followup bug to clarify
filed bug 463195

> >+                let msg = "Invitation could not be processed. Status: " + aStatus;
> >+                if (aDetail) {
> >+                    msg += "\nDetails: " + aDetail;
> >+                }
> Could we localize this while we are at it?
Did that and fixed the showError()'s title from "Error getting calendar" (which sounds weird as a generic error title) to "An error has occurred.".

> >+imipBarRefreshText=This message contains an event inquery.
> >+imipBarPublishText=This message contains an event.
> I doubt the normal user will understand the difference between these two.
IMO in conjunction with a "Send" button (which I've added), it's clearer what it's for. We need to further test/finetune REFRESH messages either way.

Moreover I've removed logError/logWarning/log again. I think we should go with log4moz in the future.


Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/f21bf34efae5>

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
> -errorTitle=Error getting calendar
> +errorTitle=An error has occurred.

> -imipSendMailTitle=Notify Attendees
> -imipSendMail=Would you like to send out notification E-Mails now?
> +imipSendMailTitle=E-Mail Notification
> +imipSendMail=Would you like to send out notification E-Mail now?

Guys, why are you updating strings without providing new keys? How will localizers know that they have update their locales? This is definetely a bad way to go.
(In reply to comment #5)
I will change that shortly, new key names really make sense here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> > >+imipBarRefreshText=This message contains an event inquery.
> > >+imipBarPublishText=This message contains an event.
> > I doubt the normal user will understand the difference between these two.
> IMO in conjunction with a "Send" button (which I've added), it's clearer what
> it's for. We need to further test/finetune REFRESH messages either w

As davida noted on irc, "inquery" is not a word. You mean inquiry, but I still think it might not be clear to users.
Attached patch l10n fixesSplinter Review
Attachment #346457 - Flags: review?(philipp)
Attachment #346457 - Flags: review?(philipp) → review+
Comment on attachment 346457 [details] [diff] [review]
l10n fixes

r=mschroeder
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/b7384fe415d4>

-> FIXED
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
This checkin regressed Bug 463267 and Bug 463282.
Depends on: 463267, 463282
Daniel, your patch has changed localizable entities without changing their names. You should either post about this change to mozilla.dev.l10n, or to change those names. Otherwise, you have no warranty that the localizations will catch up with this change.
Rimas, I've changed the entity names in a second patch yesterday (see comment #8). Which entities do you think of?
(In reply to comment #13)
> Rimas, I've changed the entity names in a second patch yesterday (see comment
> #8). Which entities do you think of?

Sorry, I guess I only looked at the first check-in when commenting...
Checked in lightning build 20090129 -> VERIFIED.
Status: RESOLVED → VERIFIED
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.