Closed Bug 463402 Opened 16 years ago Closed 6 years ago

Unwanted and confusing E-Mail notification dialogs ("Send out notification E-Mail now?" "Support Outlook 2000 and Outlook 2002/XP?")

Categories

(Calendar :: Lightning Only, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: MakeMyDay)

References

(Depends on 1 open bug)

Details

(Whiteboard: [has l10n impact]{for checkin see comment 86])

Attachments

(7 files, 14 obsolete files)

25.62 KB, image/png
Details
1.49 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
61.78 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
43.63 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
40.92 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
77.77 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
24.14 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
Attached image The dialog β€”
Dan sent me an invitation this morning (from a zimbra application I think). When I selected accept, I got a prompt:

E-Mail Notification
Would you like to send out notification E-Mail now?
[ ] Support Outlook 2000 and Outlook 2002/XP
       [cancel] [ok]

This raises several thoughts, that may end up covering separate bugs, but I wanted to raise them together first as some may be invalid or elsewhere:

1) I just selected Accept, why wouldn't I want to send out notification now?
2) Ok/cancel are wrong, it should be yes/no
3) How do I know if I need to support Outlook 2000 and Outlook 2002/XP? (this sounds like we're working around a different bug).

I also got the prompt when creating a new event and inviting people. Notify Attendees was checked (although I'd didn't actually look at it before I closed the event), so why do I need to be prompted there?
Flags: tb-integration?
See Bug 377761 for information why the dialog was introduced.
Sad but true, people need to know what client the organizer or the attendees are using to judge on whether to use the compat OL2000 option.
One might tend to always use the OL2000 option, because it covers all later OL versions, too. The drawback is that it lacks a plain text body, and we can't send attachments with it (future).
IMO an alternative to get rid of the dialog would be to move the option to the prefs: I suspect most people will always use one or the other.
Daniel, if the libmime jungle were to be revised, would there be more options?
Flags: tb-integration? → tb-integration+
Two questions on how to proceed here:
1. Should we always send mail without asking? Be aware that e.g. the organizer moving an event will automatically send updates to attendees then.
2. What shall happen to the OL compat mode option? -> Pref pane?
One case I did see, which might want to be a separate bug, is the Lightning wants to send revised invites out if you click on Save and Close, even if nothing has changed. Give the save and close button is much bigger on the mac than the 'x', users are potentially more likely to hit that.
> Should we always send mail without asking?

I'd like to see a distinction between (1) clicking the Accept/Decline buttons (automatically send response) and (2) editing an event (please ask us each time or use the checkbox "Send Attendees Invitations via Email" as Mark suggested).

For the second case, I don't want to spam all the attendees if I change only a couple of words in the Description field.
Mark: I think is a separate bug about being smart when closing an evet that hasn't changed.

When you make changes to an event I think we just need a quick confirmation dialog asking if you want to send out notification of the changes you made to all the attendees.  I don't usually advocate a dialog, however I think it's important in this process because we are going to send mail on your behalf.

Back to this bug... I think I was a little confused by this dialog.  When accepting an invitation does the person accepting normally send out an email notification?  Or was this just triggered by saving changes to the calendar event?  I don't think my accepting an invitation should cause a notification to be sent to the other attendees of the event.

The outlook question is difficult.  I think I would have moved toward defaulting to Outlook.  Not having the plaintext version isn't great, but I think it's easier to transition away from the compatible interface is easier than deciding what the clients your recipients have.  i.e. plaintext only people usually know who they are and complain when you don't send a plaintext mail; where everyone else doesn't notice.  And I suspect the cross section of calendar users and plain text only clients is much, much smaller than outlook + calendar users.  Getting the attachments to work with invitations would be the next bug to fix.
(In reply to comment #3)
> Daniel, if the libmime jungle were to be revised, would there be more options?
Hmm, maybe. IIRC I've seen some newer Outlook versions sending out a top-level multipart/alternative with enclosed text/plain, text/calendar parts, but I am not sure whether older Outlook versions swallow such. But worth a try anyway.

(In reply to comment #7)
> Back to this bug... I think I was a little confused by this dialog.  When
> accepting an invitation does the person accepting normally send out an email
> notification?  Or was this just triggered by saving changes to the calendar
> event?  I don't think my accepting an invitation should cause a notification to
> be sent to the other attendees of the event.
Yes, sending a confirmation is usual (RSVP). Your reply is sent to the organizer, not the other attendees.

> The outlook question is difficult.  I think I would have moved toward
> defaulting to Outlook.  Not having the plaintext version isn't great, but I
> think it's easier to transition away from the compatible interface is easier
> than deciding what the clients your recipients have.  i.e. plaintext only
> people usually know who they are and complain when you don't send a plaintext
> mail; where everyone else doesn't notice.  And I suspect the cross section of
> calendar users and plain text only clients is much, much smaller than outlook +
> calendar users.  Getting the attachments to work with invitations would be the
> next bug to fix.
I agree, though I'd prefer to keep a pref pane option. BTW: Google doesn't seem to send out iMIP that OL2000/2002 understands. Maybe we shouldn't care too much about it?
Assignee: nobody → daniel.boelzle
(In reply to comment #7)
> When you make changes to an event I think we just need a quick confirmation
> dialog asking if you want to send out notification of the changes you made to
> all the attendees.  I don't usually advocate a dialog, however I think it's
> important in this process because we are going to send mail on your behalf.

Reading comment #7 again, I think there's no real agreement yet. Mark and Pete vote for sending without reconfirmation (e.g. when accepting an invitation or having the "Notify attendees" checked).
Blocks: 404050
Component: General → Lightning Only
QA Contact: general → lightning
Note from bug 430619: Users may also *never* want to send invitation mails, this should be taken into account when fixing this bug.
Sorry, but I won't find to drive this in the foreseeable future.
Assignee: dbo.moz → nobody
For a company usage, it could be great to enforce this option.
Why not supporting additional values for calendar.itip.compatSendMode ?
0 : default no but show dialog box
1 : default yes but show dialog box
2 : no and don't show dialog box
3 : yes and don't show dialog box
Regards
Patrick
Summary: Unwanted and confusing E-Mail notification dialogs → Unwanted and confusing E-Mail notification dialogs ("Send out notification E-Mail now?" "Support Outlook 2000 and Outlook 2002/XP?")
who will fix it if it is assign to NOBODY ??
who makes lightning ?????
shame shame since 2008 with the same problem !!
http://support.microsoft.com/lifecycle/?p1=2557

outlook 2000 extended support had ended in 2009. We still use OL2000 at the office but to be honest I always hated the extra dialog too. So, I'm with patrick, let's drop the dialog and sent in outlook 2002/2003 by default.
(side note: Its funny that searching for "outlook 2000 user statistics" gives me Lightning as the second entry ;-)

This would be ok from my POV, if someone wants to whip up a patch go ahead. This is not a priority for the next beta though.
Is it not possible to determine which Outlook formats (if any) to support by looking at the User-Agent header of the e-mail containing the invitation?
Tristan, this is more about sending emails, not receiving them. Before sending an email we have no way of knowing if the receiving user will read it with outlook 2000 or not.


By the way, has anyone tested this with Outlook 2010 ? Do our invitations look ok there?
Well, presumably if the invitation was sent by a certain user agent, then it's a good bet the response will be read by the same user agent.  I don't know about you but I don't use separate e-mail clients for sending and receiving.
When fixing this bug, please also make note of the patch in bug 404050. There it is proposed to use a (hidden) pref to auto send notifications without this dialog.
It's a bit shocking to see the discussion (admittedly of a few issues) has continued since 2008 without resolution and update to the program.  What can we do to resolve the issues expediently?

I arrived at this bug on a Google search to find out why Lightning automatically re-notifies users without asking, each time an appointment is re-saved; so I'll give my point of view about that.

If a diary event belongs just to me, I can annotate it with small details as they arise, maybe correct a spelling mistake or reword a phrase for clarity.  I may have cause to do that a dozen times in preparation for an appointment.

If I add other participants, each small adjustment will send them a notification.  I may save several changes (maybe from invitee feedback) which I save during a day; and I will only want a re-notification to be sent out once, after all feedback is returned.  

It's kind of horrific to think that everybody is being e-mailed with the minutiae of my adjustments - re-notification has to be my judgement call.

Outlook 2000 (the only other program with which I can compare) asks me on each save if I want to send updates to all attendees.  Surely this is the appropriate behaviour, isn't it?
Would love to see this fixed. Like Patrick's proposal:

calendar.itip.compatSendMode ?
0 : default no but show dialog box
1 : default yes but show dialog box
2 : no and don't show dialog box
3 : yes and don't show dialog box
So... where ion the heck can I find documentation on what the available options are for this config option? I googled till my fingers bled...
bug 377761
I noticed communigate mapi connector also has this option in the configuration-window so we aren't the only ones having problems with it.

Last week windows-xp extended support was ended. Outlook 2000 doesn't run on windows 7 so its usage will be declinging. Outlook 2002/xp on windows 7 has problems too. Perhaps it would be wise to make this a config option rather than a dialog?
Anyone?

I have some Thunderbird systems that do not prompt... apparently someone clicked the checkbox to not prompt again, and I can't for the life of me figure out how to bring that prompt back.
I don't know what in particular you want to have a prompt for, but try looking in Options | Advanced | Config Editor | (Confirm "Here by Dragons") | Search for "ask", and see if any of the options fit the bill.
?

I'm talking about the Prompt that is the subject of this bug. Not sure how that was unclear?

I have users who see NO prompt to send out these invite/updates...
Ok, sorry if my suggestion didn't help.
Sorry, maybe that sounded too harsh... but I have searched and searched the config editor for any setting that might control this, and the only one that seems like it would/should is this one, but no one seems to be able to tell me what are the available options...

Is it only 0 or 1?

There *has* to be a way to get this dialog back...
(In reply to Bas van den Bosch from comment #26)
> Last week windows-xp extended support was ended. Outlook 2000 doesn't run on
> windows 7 so its usage will be declinging. Outlook 2002/xp on windows 7 has
> problems too. Perhaps it would be wise to make this a config option rather
> than a dialog?

Do you mean it works on win7 or not?
Either way, personally I doubt those versions are very much used anymore and the dialog pref should probably be flipped.
It seems that this issue is about two different things:

Disabling the pop-up:
"Send out notification E-Mail now?"

Disabling the option in the pop-up:
"Support Outlook 2000 and Outlook 2002/XP?"

Personally, I want to disable the pop-up. As far as I understand, this is not doable with calendar.itip.compatSendMode, calendar.itip.notify or calendar.itip.notify-replies.

How to disable the pop-up?
Attached patch notificatonDialog.patch (obsolete) β€” β€” Splinter Review
Hey there, it seems that after 6 years this might be a possible workaround. :)

Take a look at the attached patch for these files:
- http://mxr.mozilla.org/comm-central/source/calendar/itip/calItipEmailTransport.js
- http://mxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning.js

This patch was inspired by the proposal of "patrick.abiven@apitech.fr". I decided to keep
"calendar.itip.compatSendMode" and imipSendMail dialog options separated. The following 3
options can be set with the newly introduced "calendar.custom.sendReplyNotificationEmail":

  '0' -> Prevent dialog and do not send a notification mail

  '1' -> Prevent dialog and do send a notification mail

  '2' -> Show dialog (default, behavior unchanged)

Note: Case '1' will use the currently configured value of the existing 
"calendar.itip.compatSendMode".

With best regards

Amin Zoubaa
Attachment #8526955 - Flags: review?(philipp)
Assignee: nobody → mozilla-lightning-patch
Status: NEW → ASSIGNED
Comment on attachment 8526955 [details] [diff] [review]
notificatonDialog.patch

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

Hi Amin, sorry for delaying this so long. You've done everything right, the patch is very clean and seems straightforward. I'd like to give the patch a test run before marking it r+, and I have one minor nit.

I probably won't get to testing this before Wednesday, maybe you can upload a new patch with the nits fixed in the meanwhile? If you have any questions, feel free to email me directly or visit me on irc.mozilla.org.

::: calendar/itip/calItipEmailTransport.js
@@ +203,4 @@
>                      break;
> +                }
> +                // show a dialog prompting user for action
> +                if (Preferences.get("calendar.imip.sendReplyNotificationEmail", 2) === 2) {

Instead of calling 	
Preferences.get("calendar.imip.sendReplyNotificationEmail", 2) multiple times, I'd suggest to use an intermediate variable.

Also, I don't think we need a strict compare here, == should be enough.
Attached patch notificatonDialog.v2.patch (obsolete) β€” β€” Splinter Review
Hi Philipp,

thanks for your feedback. I have updated my modifications and attached a new patch without the minor nits. Everything else remains as it was.

I hope the testing goes well and look forward to hearing from you :) 

best regards,

Amin Zoubaa
Attachment #8526955 - Attachment is obsolete: true
Attachment #8526955 - Flags: review?(philipp)
Attachment #8536572 - Flags: review?(philipp)
I have had also a look at this patch. Codewise it looks good, but I would like to challenge the approach:

The option 0 in the patch doesn't really makes sense to me. It applies both on invitations and replies. While I see a valid use case for the latter one, I don't do so for the former. And if you don't want to send out replies, you would be forced to toggle the prefs to still be able to send out invitations. This would lead from one poor UX to another.

Instead of this approach, I suggest to remove the compat mode - if somebody in 2014 is still running Outlook 2000/XP, he/she might have much more problems than not being able to deal with Lightning invitations... That said, it is reasonable to me and would enables us to get rid of the dialog easily.

However, I would like to keep the choice for the user whether or not to send a reply - and not hidden in a pref. We can add this more elegant as different choices to the accept/decline buttons for invitations (now that we have the drowdown buttons) as well as to the event context menu in calendar view and the dropdown of the event summary dialog instead with sending as default option. As a plus, this would enable us to intercept the process earlier to prevent sending not only of email invitations to offer this option for all calendars with clientside scheduling management.

For invitation, I think we don't need any special handling, sending them out on hitting save&close is the relevant option here (optionally, we can it more obvious by turning the label to 'send' if there are attendees in this event).

Amin, Philipp what do you think?

It may sound to be more work than it will be - the implentation would be straight forward. If you needed further support to follow this approach, I'm willing to help. (This has been already on my list but with a low priority).
Comment on attachment 8536572 [details] [diff] [review]
notificatonDialog.v2.patch

MakeMyDay, I'm always for getting rid of old code and I think you have a good grip on it. Do you think you could take over testing/reviews here?

Also looking forward to hear what Amin thinks.
Attachment #8536572 - Flags: review?(philipp) → review?(makemyday)
Sure, I will take care. But let's wait for Amin response.
Flags: needinfo?(mozilla-lightning-patch)
Comment on attachment 8536572 [details] [diff] [review]
notificatonDialog.v2.patch

Codewise, this looks good, but r- for the approach - sorry for this. Amin can you elaborate this patch into the direction of my above comment and rerequest review once done? If you need more guidance inbetween, please leave a comment here.
Attachment #8536572 - Flags: review?(makemyday) → review-
Amin, are still working on this or willing to?
MakeMyDay - I think its safe to say Amin is no longer working on this. Are you able to take over from here?

If not, perhaps we should split this issue in two to try and get some progress. Issue #1: remove legacy Outlook 2000/XP compatibility mode - which should be relative uncontentious, and then Issue #2: allow email confirmations to be send always/never/prompt
> If not, perhaps we should split this issue in two to try and get some progress.
+1
I take this on my list. But please be patient a little bit - the release train for the upcoming major release 4.0 is already gone, so this can only be part of the next.

And please don't add such +1 comments, you can vote for the bug instead.
Assignee: mozilla-lightning-patch → makemyday
Flags: needinfo?(mozilla-lightning-patch)
Blocks: ltn47
MakeMyDay - would you post an update on this old bug?
It is still work in progress. This requires large changes allover the application to be working reliably. Therefor it missed the train fΓΌr 4.7.

I have to reorganize my work queue now that 4.7 was released. But I'm confident to land the first patches for this within the next couple of weeks (depends also whether regression in 4.7 show up with require immediate attention). So stay tuned.
No longer blocks: ltn47
Depends on: ltn54
Whiteboard: [needs strings]
Whiteboard: [needs strings] → [l10n impact]
Blocks: ltn54
No longer depends on: ltn54
Ok, this is the first and most important (WIP) patch of a series of patches to get this done. It covers the processing part. Without any of the other patches making use of it, it will not change the current behaviour.

As this is the foundation for everything else in this bug, I would appreciate some feedback. I'll still upload another patch making use of it for the imipbar to demonstrate the way it works.

This patch requires the patches from bug 1317064 and bug 1225784 to be applied (in that order).
Attachment #8536572 - Attachment is obsolete: true
Attachment #8812546 - Flags: feedback?(philipp)
Attached patch RemoveSendNowDialog-Part2-ImipBar-V1.diff (obsolete) β€” β€” Splinter Review
This patch makes use of part 1 to offer the ability to send or not send a reply for the imipbar.

There would be part 3 and 4 to add the same options to the context menus in calendar views and the (summary) dialog.

This bug will have several string additions, so we will still need to decide whether we really want this to include in 5.4, but given we have an extended Aurora period this time, I think this is at least worth to consider.
Comment on attachment 8812546 [details] [diff] [review]
RemoveSendNowDialog-Part1-Processing-V2.diff

Removing f? for now. This patch still has flaws, I'll upload another version.
Attachment #8812546 - Flags: feedback?(philipp)
This bug missed the cut for string changes, so I'm removing the tracking for 5.4.
No longer blocks: ltn54
Whiteboard: [l10n impact]
Any hope for having "Support Outlook 2000 and Outlook 2002/XP" removed?
Here is a tentative patch for the second part of this bug related to the support of old versions of Outlook ("Support Outlook 2000 and Outlook 2002/XP?")
Attachment #8873995 - Flags: review?(philipp)
Comment on attachment 8873995 [details] [diff] [review]
patch for removing "Support Outlook 2000 and Outlook 2002/XP?"

Sorry I haven't gotten around to this. I may have missed something from previous comments, but maybe it makes sense for MakeMyDay to review this one instead.

From a brief glance, I suspect there is some more work needed if this is the right way forward, e.g. removing strings and either not calling the prompt at all or correcting the wording.
Attachment #8873995 - Flags: review?(philipp) → review?(makemyday)
Comment on attachment 8873995 [details] [diff] [review]
patch for removing "Support Outlook 2000 and Outlook 2002/XP?"

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

Thanks for stepping up. I know this bug lives now for quite a while and had to be descoped for the last ESR, which might have been disappointing for people observing this bug. But I'm sure to get this bug fixed for the next ESR. 

This patch is a subset of the processing part patch for this bug, which is currently work in progress (effectively, I picked this up again two weeks ago). There is more to change also for the OL compatibility aspect of this bug, then your patch covers, as Philipp already suggested. But I prefer to leave this part of the mentioned patch, as I already covered this fully in may local version of the patch, so r- for this reason. Please don't take this a disregard of your participation.

If you're interested especially in supporting the progress of this bug, stay tuned, we will need fairly extentsive manually testing once the patches are ready to land as we don't have a good coverage with our automated tests for email invitations - so we will need you. If you want to work on other issues, look at [1] for good first bugs.

If you provide a patch then, please make sure you have incorparted an appropriate patch header including the author and a commit message mentioning the respective bug, a description of what the patch does. There will be bonus points, if you already add a suffix to the commit message mentioning the reviewer, so this doesn't need to be added by the sheriff when landing a patch for you (e.g. "Bug 123456 - This patch does some coo staff;r=(nick)name of the reviewer" see other bugs for examples). And, as you already learned, ask for review when uploading - otherwise there is a good chance your patch wouldn't be looked at for quite some time.

[1] https://www.joshmatthews.net/bugsahoy/?calendar=1
Attachment #8873995 - Flags: review?(makemyday) → review-
> But I'm sure to get this bug fixed for the next ESR. 

Excellent! I look forward to it. Thanks.
Updated version of the patch, almost ready.
Attachment #8812546 - Attachment is obsolete: true
Attached patch RemoveSendNowDialog-Part2-ImipBar-V2.diff (obsolete) β€” β€” Splinter Review
Updated version of the patch. As discussed on irc, I moved the tentative actions to a separate button, but made this behind a pref - the new 4 button style will now be the default, but can be switched back to the current 3 button style. Background: there will be additional options (like "send with comment" per button in (a hopefully not to far) future.

This patch requires part 1 to be applied before.
Attachment #8812547 - Attachment is obsolete: true
Attachment #8924297 - Flags: ui-review?(richard.marti)
Part 4 of this series, taking care of the context menus (still needs some polishing)

There are a lot of comments in the patch that gives you a good grip, especially since the current behaviour will slightly change.

Philipp, I mostly would appreciate your feedback on the large comment in calendar-ui-utils.js, where I described how this should be working.

Richard, I would like to get your attention mostly on the checkmark icon I used in the context menu and the different colors in case occurrence and master partstat are different. But of cause any other feedback is welcome.

This patch requires part 1 and 2 applied before. Part 3 is still work in progress, so you currently cannot make benefit from the new options from the event and summary dialog / event tab yet, but you can apply part 4 without part 3.
Attachment #8924300 - Flags: ui-review?(richard.marti)
Attachment #8924300 - Flags: feedback?(philipp)
One more thing: since part 1 contains an interface change, you will need a clobber build to get this working.
I'll test this this weekend. MakeMyDay, please could you attach test invitations for the different cases?
Thanks. For displaying the buttons for email ivitations you can simply use any invitation you haven't yet responded. If you haven't any at hand, you can e.g. take attachment 8924320 [details] (by doing the latter, you could also reproduce bug 1414608, btw).

Regarding the context menu, you can simply create a single and a recurring event and invite e.g. nobody@example.org if you have made sure to assign an email identity to the calendar you use for that purpose. After saving & closing the events, the attendance item shows up in the context menu for these events in the calendar views or the today pane (the party, who's partstat you could change in this setup would be the organizer, but it works respectively for received invitations, where you are an attendee).
Blocks: ltn62
Whiteboard: [has l10n impact]
Comment on attachment 8924297 [details] [diff] [review]
RemoveSendNowDialog-Part2-ImipBar-V2.diff

Patch 1 and 2 didn't apply cleanly. I had to rebase them. And with calendar.itip.separateInvitationButtons manually applied because of the extension prefs import bug, I saw the four buttons.

With the future plans, four buttons makes sense. r+
Attachment #8924297 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8924300 [details] [diff] [review]
RemoveSendNowDialog-Part4-ContextMenus-V1.diff

ui-r- because the checkmark is too big in the context menu. Why don't you use menuitem type="checkbox"? With this, a system integrated appearance would be guaranteed. Or had you other things in mind?
Attachment #8924300 - Flags: ui-review?(richard.marti) → ui-review-
Comment on attachment 8924300 [details] [diff] [review]
RemoveSendNowDialog-Part4-ContextMenus-V1.diff

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

Wow, kudos for the elaborate description! I think this makes complete sense :) Minor comment on the remaining code, which you might have taken care of anyway:

::: calendar/base/content/calendar-common-sets.xul
@@ +215,3 @@
>              label="&calendar.context.attendance.menu.label;"
>              accesskey="&calendar.context.attendance.menu.accesskey;"
> +            oncommand="setContextPartstat(event.target.getAttribute('respvalue'), event.target.getAttribute('scope'), currentView().getSelectedItems({}), event.target.hasAttribute('respmode') ? { responseMode: Components.interfaces.calIItipItem[event.target.getAttribute('respmode')] } : null)"

This is starting to get pretty long, maybe we can put it into a function.

@@ +538,3 @@
>              label="&calendar.context.attendance.menu.label;"
>              accesskey="&calendar.context.attendance.menu.accesskey;"
> +            oncommand="setContextPartstat(event.target.getAttribute('respvalue'), event.target.getAttribute('scope'), getSelectedTasks(), event.target.hasAttribute('respmode') ? { responseMode: Components.interfaces.calIItipItem[event.target.getAttribute('respmode')] } : null)"

Same here.
Attachment #8924300 - Flags: feedback?(philipp) → feedback+
Depends on: 719351
Finally, I think the patches are ready for review now. There are 6 patches with different scopes, which need to be applied in ascending order:

#1: processing

- adding support for processing in different response modes (AUTO, USER, NONE)
- removing compatibility mode for OL 2002 and earlier


#2: imipbar

- providing the option to accept/decline an invitation from mail view with or
  without notifying the organizer
- re-introcunding separate buttons for accept and tentative in invitation emails
  (having a pref to switch to 3 button mode, disabled by default)
  

#3: dialogs/tabs (summary and event dialog, event tab)

- added a toolbar to summary dialog if not read-only to trigger status updates with or without notifying
- displaying the calendar name in summary dialog (without an option to change the calendar)
- dynamic replacement of toolbar and menu items depending on existnace of attendees in event dialog/tab

- not included and something to follow-up: use separate icons in event dialog/tab for 'send and close'
  and 'send and save' toolbarbuttons (icons only needed)

#4: context menu (calendar view and today pane)

- providing the option to accept/decline invitations from context menu with/without sending a reply
- normalizing the strings to deal with invitations so they are aligned with what we use at other places,
  e.g. use 'accept' instead of 'I will attend' and reuse send/dontsend strings for all partstats

#5: delete, cut, copy & paste

- adds an option paste with or without notification if the popup to choose the target is prompted,
  otherwise fall back to pre-exsiting popup (=USER mode)
- explicitely falling back to pre-existing popup (=USER mode) when cutting or deleting an item

- not included and something to follow-up: find a way to combine the request for cutting and
  pasting when it comes to sending notifications - currently, there is no concept for a cutting to
  be related to a pasting within the app and only delete the item if pasting was successful


#6: drag 'n drop

- explicitely falling back to pre-existing popup (=USER mode) when droping an item


The patch from bug 1437405 should be applied before to have dnd working and also attachment 8955883 [details] [diff] [review] form bug 393084. Apart from that, the patches build on top of the already landed calUtils changes, but doesn't consider not yet landed changes. Part 1 involves an interface change, so you need to compile.

With the whole set applied, the linter and mozmill tests are passing locally.

Part 1 to 5 include string changes, so we should get them landed before the string freeze.
For the scope of this patch, see comment 66.
Attachment #8924292 - Attachment is obsolete: true
Attachment #8955884 - Flags: review?(philipp)
Attached patch RemoveSendNowDialog-Part2-ImipBar-V2.diff (obsolete) β€” β€” Splinter Review
For the scope of this patch, see comment 66.
Attachment #8924297 - Attachment is obsolete: true
Attachment #8955885 - Flags: review?(philipp)
Attached patch RemoveSendNowDialog-Part3-Dialogs-V1.diff (obsolete) β€” β€” Splinter Review
For the scope of this patch, see comment 66.
Attachment #8955886 - Flags: review?(philipp)
For the scope of this patch, see comment 66.
Attachment #8924300 - Attachment is obsolete: true
Attachment #8955887 - Flags: review?(philipp)
For the scope of this patch, see comment 66.
Attachment #8955888 - Flags: review?(philipp)
For the scope of this patch, see comment 66.
Attachment #8955889 - Flags: review?(philipp)
Attachment #8873995 - Attachment is obsolete: true
Comment on attachment 8955884 [details] [diff] [review]
RemoveSendNowDialog-Part1-Processing-V4.diff

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

::: calendar/base/modules/calItipUtils.jsm
@@ +636,5 @@
>       * deleted and sends out appropriate iTIP messages.
> +     * @param {long}               aOpType        Type of operation - (e.g. ADD, MODIFY or DELETE)
> +     * @param {calIEvent|calITodo} aItem          The updated item
> +     * @param {calIEvent|calITodo} aOriginalItem  The original item
> +     * @param {JSObject}           aExtResponse   [optional] An object to provide additional

Please use js types here as far as possible, i.e. Number,Object

@@ +690,5 @@
>                  }
>              }
>          }
> +        // for backward compatibility, we assume USER mode if not set otherwise
> +        let ciii = Components.interfaces.calIItipItem;

We've been using cIII in other places and similar constructs.

@@ +990,5 @@
>       *
> +     * @param {calIItipItem} aItem            item to be sent
> +     * @param {String}       aMethod          iTIP method
> +     * @param {Array}        aRecipientsList  array of calIAttendee objects the message should be sent to
> +     * @param {JsObject}     aAutoResponse    JS object whether the transport should ask before sending

Object
Attachment #8955884 - Flags: review?(philipp) → review+
Comment on attachment 8955885 [details] [diff] [review]
RemoveSendNowDialog-Part2-ImipBar-V2.diff

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

::: calendar/lightning/content/imip-bar-overlay.xul
@@ +140,5 @@
>                  <menupopup id="imipAcceptDropdown">
>                    <menuitem id="imipAcceptButton_Accept"
>                              tooltiptext="&lightning.imipbar.btnAccept2.tooltiptext;"
>                              label="&lightning.imipbar.btnAccept.label;"
> +                            oncommand="ltnImipBar.executeAction('ACCEPTED', { responseMode: Components.interfaces.calIItipItem.AUTO });"/>

Ci

::: calendar/locales/en-US/chrome/lightning/lightning.dtd
@@ +52,5 @@
>  
>  <!-- iMIP Bar (meeting support) -->
>  <!ENTITY lightning.imipbar.btnAccept.label                                  "Accept">
>  <!ENTITY lightning.imipbar.btnAccept2.tooltiptext                           "Accept event invitation">
> +<!ENTITY lightning.imipbar.btnAcceptDontSend.label                          "Accept without sending">

Is it clear from the context without what is being sent? I can imagine it is hard to get this onto a button label
Attachment #8955885 - Flags: review?(philipp) → review+
Comment on attachment 8955886 [details] [diff] [review]
RemoveSendNowDialog-Part3-Dialogs-V1.diff

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

::: calendar/base/content/dialogs/calendar-summary-dialog.js
@@ +320,5 @@
> +        // we display a notification about the users partstat
> +        let partStat = window.attendee.participationStatus || "NEEDS-ACTION";
> +
> +        let msgStr;
> +        switch (partStat) {

Would be more compact to turn this into a map, i.e.

let strmap = {
  ACCEPTED: "itemAccepted",
  ...
}

::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.properties
@@ +457,5 @@
> +# LOCALIZATION NOTE (itemTentative) - this will be displayed as notification
> +# in the summary dialog if the user has accepted the invitation or the assigned
> +# task tentatively
> +# %1$S - either typeEvent or typeToDo
> +itemTentative=You have accepted this %1$S tentatively

Inserting event/todo here might cause some trouble if the translation is different, e.g.

Du hast diese |Aufgabe| azeptiert
Du hast diesen |Termin| Akzeptiert

(I realize that it is event invitation instead of just event, but I can still imagine it might be different in other locales. Would it be feasible to split these strings into event/task variants?

@@ +492,5 @@
> +# invitation
> +typeEvent=event invitation
> +
> +# LOCALIZATION NOTE (typeToDo) - argument to use in item* in case of a task
> +# assigned to the user by somebody else 

Whitespace
Attachment #8955886 - Flags: review?(philipp) → review+
Comment on attachment 8955887 [details] [diff] [review]
RemoveSendNowDialog-Part4-ContextMenus-V2.diff

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

::: calendar/base/content/calendar-common-sets.js
@@ +802,5 @@
>  /**
>   * Handler function to set up the item context menu, depending on the given
>   * items. Changes the delete menuitem to fit the passed items.
>   *
> + * @param  {nsIDOMEvent} aEvent   The DOM popupshowing event that is  triggered

nsIDOMEvent is going away, but DOMEvent is correct.

@@ +804,5 @@
>   * items. Changes the delete menuitem to fit the passed items.
>   *
> + * @param  {nsIDOMEvent} aEvent   The DOM popupshowing event that is  triggered
> + *                                  by opening the context menu
> + * @param  {Array}       aItems   An array of items (usually the selected items)

Either {Array.<calIItemBase>} or {calIItemBase[]} if you want to adhere to jsdoc :)

@@ +814,5 @@
>          let menuItem = document.getElementById(aMenuItemId);
>          if (menuItem) {
> +            menuItem.setAttribute(
> +                "label",
> +                cal.calGetString("calendar", "delete" + aItemType + "Label")

This is probably a good line to use a template string, but not necessary if you prefer normal strings.

::: calendar/base/content/calendar-item-editing.js
@@ +779,5 @@
>  
> +                let extResponse = null;
> +                if (aTarget.hasAttribute("respmode")) {
> +                    let mode = aTarget.getAttribute("respmode");
> +                    let itipMode = Components.interfaces.calIItipItem[mode];

Ci

::: calendar/base/content/calendar-ui-utils.js
@@ +797,5 @@
> +    /**
> +     * Provides the user's participation status for a provided item
> +     *
> +     * @param   {calEvent|calTodo}  aItem  The calendar item to inspect
> +     * @returns {Mixed}             The participation status string as per

jsdoc says {?String} is a nullable string.
Attachment #8955887 - Flags: review?(philipp) → review+
Comment on attachment 8955888 [details] [diff] [review]
RemoveSendNowDialog-Part5-CopyCutPaste-V1.diff

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

::: calendar/base/content/calendar-clipboard.js
@@ +196,5 @@
> +            // we only will need to ask whether to send notifications, if there
> +            // are attendees at all
> +            let withAttendees = items.filter((aItem) => {
> +                return aItem.getAttendees({}).length > 0;
> +            });

You can probably get this into one line

let withAttendees = items.filter(aItem => aItem.getAttendees({}).length > 0);

@@ +223,5 @@
> +                            destCal = aCal;
> +                            notify = false;
> +                        };
> +                        args.labelOk = cal.calGetString("calendar",
> +                                                        "pasteAndNotifyLabel");

If you can get these into a line of 100 please do. I didn't count though.

::: calendar/locales/en-US/chrome/calendar/calendar.properties
@@ +53,5 @@
>  exportPrompt=Which calendar do you want to export from?
>  pastePrompt=Which of your currently writable calendars do you want to paste into?
>  publishPrompt=Which calendar do you want to publish?
>  
> +notificationPrompt=Do you want to notify the participants of the pasted items?

Would the user know what the participants are being notified of here?
Attachment #8955888 - Flags: review?(philipp) → review+
Attachment #8955889 - Flags: review?(philipp) → review+
Generally, I am on the fence on using jsdoc types. On the one hand I can see how it makes sense, but we haven't been doing this very often and since the types are not checked the additional use is marginal.
Attached patch RemoveSendNowDialog-Part2-ImipBar-V3.diff (obsolete) β€” β€” Splinter Review
I just noticed I uploaded an outdated patch for part 2 - sorry for the inconvenience.
Attachment #8955885 - Attachment is obsolete: true
Attachment #8957791 - Flags: review?(philipp)
Comment on attachment 8957791 [details] [diff] [review]
RemoveSendNowDialog-Part2-ImipBar-V3.diff

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

::: calendar/locales/en-US/chrome/lightning/lightning.dtd
@@ +83,5 @@
>  <!ENTITY lightning.imipbar.btnUpdate.tooltiptext                            "Update event in calendar">
>  <!ENTITY lightning.imipbar.description                                      "This message contains an invitation to an event.">
>  
> +<!ENTITY lightning.imipbar.btnSend.label                                    "Send a response now">
> +<!ENTITY lightning.imipbar.btnSend.tooltiptext                              "Send out a response to the organizer">

"Send out" sounds a bit colloquial. Maybe just "Send a response to the organizer" ?

@@ +84,5 @@
>  <!ENTITY lightning.imipbar.description                                      "This message contains an invitation to an event.">
>  
> +<!ENTITY lightning.imipbar.btnSend.label                                    "Send a response now">
> +<!ENTITY lightning.imipbar.btnSend.tooltiptext                              "Send out a response to the organizer">
> +<!ENTITY lightning.imipbar.btnSendSeries.tooltiptext                        "Send out a reply for the entire series to the organizer">

You use response above, and reply here. Can we unify this? If not, then a localization note may be required, otherwise it may be translated wrong.
Attachment #8957791 - Flags: review?(philipp) → review+
Updated patch with comments considered
Attachment #8955884 - Attachment is obsolete: true
Attachment #8957908 - Flags: review+
Updated patch with comments considered.
Attachment #8957791 - Attachment is obsolete: true
Attachment #8957909 - Flags: review+
Updated patch with comments considered.
Attachment #8955886 - Attachment is obsolete: true
Attachment #8957910 - Flags: review+
Updated patch with comments considered.
Attachment #8955887 - Attachment is obsolete: true
Attachment #8957913 - Flags: review+
Updated patch with comments considered. I have split up the notificationPrompt in a more fine grane approach referring to pasting a meeting or assigned event, so the context should be more clear why we offer to send a notification. However, It's hard to tell the user what exactly changed, we keep it a little vague here.
Attachment #8955888 - Attachment is obsolete: true
Attachment #8957929 - Flags: review+
The patches need to be applied in ascending order (part 1 to 6). Please land this before the merge since these have l10n changes. Thanks.
Keywords: checkin-needed
Whiteboard: [has l10n impact] → [has l10n impact]{for checkin see comment 86]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/044240bd35e0
Remove E-Mail notification dialogs - Part 1: processing;r=philipp
https://hg.mozilla.org/comm-central/rev/5ae749bd1ed8
Remove E-Mail notification dialogs - Part 2: imip bar;r=philipp
https://hg.mozilla.org/comm-central/rev/f5138ae6a728
Remove E-Mail notification dialogs - Part 3: dialogs;r=philipp
https://hg.mozilla.org/comm-central/rev/f4e55986c496
Remove E-Mail notification dialogs - part 4: context menus;r=philipp
https://hg.mozilla.org/comm-central/rev/bdc3de3a59b4
Remove E-Mail notification dialogs - part 5: delete, copy, cut & paste;r=philipp
https://hg.mozilla.org/comm-central/rev/0c44042f9e67
Remove E-Mail notification dialogs - part 6: drag & drop;r=philipp
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 6.2
See Also: → 1447830
Depends on: 1451763
Depends on: 1480827
Depends on: 1479449
Depends on: 1480975
See Also: → 1562896
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: