1225784 - Deal properly with incoming counter proposals

Status: RESOLVED FIXED

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

Description

[[:MakeMyDay]]

This bug is to deal with incoming invitation COUNTER proposals, while I creating counter proposals for received invitations is left for bug 426532.

Comment 1

[[:MakeMyDay]]

Attachment: Illustration adaptation of event dialog in case of counter proposal

Comment 2

[[:MakeMyDay]]

Attachment: rescheduling-alternateUIproposal.png

Comment 3

[[:MakeMyDay]]

After debitrotting my patch for this bug after landing the eslint stuff, I'm currently only lacking the UI implemenation in case the organizer wants to reschedule the event after receiving a counter proposal, so I need some feedback here.

The overall workflow will be:
- send out an invitation as an organizer by email
- receive a counter proposal by email
- the imipbar offers either to decline the proposal directly or to reschule the event
- for the latter, the usual event dialog/tab is opened (the organizer needs all options for rescheduling, including changing the description, checking FB status of all attendees, changing location / datetimes,...)

There are three approaches on the table:

1. just preload the event with the proposed modifications (most probably start/end time or location)
2. display both, the values from the sent invitation and the counter proposal (and be able to toogle between both)
   a) by using the notificationbox for the counter proposal
   b) displaying the counter proposal inline along with the respective control (don't mind the duplication of the datetime widget in the other screenshot, I assume it was probably just easier when I did the mockup)

I currently tend towards 2a because it's more flexible. It would also allow to structure and format the message to an extend by using a DocumentFragment.

Do you have any preference which way to go here?

Comment 4

[Richard Marti (:Paenglab)]

I'm for the notificationbox design. Maybe don't use a notificationbox because what happens to the counter proposal when the close button of the notification is pressed?

Comment 5

[[:MakeMyDay]]

Attachment: rescheduling-alternateUIproposal2.png

Picking up Paenglab's comment on the notificationbox, I've created another mockup using a separate container. The existing notificationbox would be displayed below the blue bar if needed. This still needs some fine tuning for edge cases, but this is for later.

If I don't here any objections, I would go ahead with that approach.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

You could also consider not using the notification bar but using a simple xul box instead, that acts the same. This way you have a bit more flexibility w.r.t. the layout and style. Aside from that I am fine with the notification box.

I'm not quite sure when the user would be getting this though. How does the event dialog know that a COUNTER proposal has been made? I would have though the only place this needs UI would be in the message viewer.

As this is a new feature, any chance you could implement the same for the html dialog?

Can you change the order of the words a bit?

> A counter proposal has been made by Example User <[e-mail]>
> -or-
> A counter proposal has been made for this event by Example User <[e-mail]>

Regarding the buttons, does "apply proposal" apply the values to the fields in the event dialog? If yes, then please rename the reset button to something like "Ignore proposal" or "Keep current values". Using "reset" suggests that something is or will be changed.

Also, maybe you can make more clear what the attendee state means. I don't think it is quite obvious that this means the user had declined before (but now accepts with this proposal?).

Comment 7

[[:MakeMyDay]]

Thanks for your feedback.

(In reply to Philipp Kewisch [:Fallen] from comment #6)
> You could also consider not using the notification bar but using a simple
> xul box instead, that acts the same. This way you have a bit more
> flexibility w.r.t. the layout and style. Aside from that I am fine with the
> notification box.

The mockup with the blue bar is already using a simple box instead of the notificationbox, which is my currently preferred approach.

> 
> I'm not quite sure when the user would be getting this though. How does the
> event dialog know that a COUNTER proposal has been made? I would have though
> the only place this needs UI would be in the message viewer.

This gets displayed only if you open the event dialog from the imip bar of the respective email by pressing a "reschedule" button. Displaying this also if opened from e.g. the calendar view would require to persist the fact that there was a counter proposal at some time within the ics (or elsewhere) regardless whether the organizer has dealt with it or not when receiving the mail - this not how the imip bar currently works and is not intended in the first place, but we can be change that later if required.
 
> As this is a new feature, any chance you could implement the same for the
> html dialog?

If the html implemenation would be already in shape to adopt it, I would include it directly. But as it currently stands, I would file a follow up bug for this and take care later, but porting this will straight forward and a minimal effort.

> Can you change the order of the words a bit?
> 
> > A counter proposal has been made by Example User <[e-mail]>
> > -or-
> > A counter proposal has been made for this event by Example User <[e-mail]>

Suer, this was not yet meant to be final ;-)

> Regarding the buttons, does "apply proposal" apply the values to the fields
> in the event dialog? If yes, then please rename the reset button to
> something like "Ignore proposal" or "Keep current values". Using "reset"
> suggests that something is or will be changed.

The intention is that only one of the buttons is enabled and the state of them will be toogled when either populating the proposal values or resetting to the state before the proposal. Using two separate buttons instead of one that is dynamically changed seems to be better from accessibillity standpoint to me, but maybe I'm wrong. The nameing of the buttons must make clear that not pressing the button accepts or declines the counter proposal but just populates the respective dialog fields.

> Also, maybe you can make more clear what the attendee state means. I don't
> think it is quite obvious that this means the user had declined before (but
> now accepts with this proposal?).

A countering attendee may have either declined or accepted tentatively. I think this is a relevant information when deciding about rescheduling for the organizer. From the workflow perspective, the organizer will send out a new REQUEST (maybe based on the proposal or even by making a third proposal). The countering attendee (and any other) can then send a REPLY on it to finally accept or decline or counter once again. As mentioned before, the wording here can still be adopted.

Comment 8

[Decathlon]

Not sure what is the best solution.
I think that some kind of information, a descriptive text, is needed here so I would discard the first screenshot that is a bit cryptic. I don't know if using the notificationbox is right though, it seems a better place for information or warnings about wrong data, data that can cause problems, or to inform the user when Lightning can't display correctly all the parameters of an event (for example warn the user when a wrong end/until date is entered in the datepickers).

At the beginning I thought it was important the presence of elements that highlight this part of UI because the user has to give an answer to a direct question coming from another user (it's a slightly different use of the dialog, different than e.g. to confirm an invite), so that the notificationbox could be useful, but actually it shouldn't be so important because the user opens the dialog from the imipbar just with the intention to give an answer to the COUNTER question.
In my opinion could be right just adding inline the information and the buttons for rescheduling/keep the dates in the same section where are the start/end datepickers. The difference in the UI compared to the standard case should be sufficient to attract user attention but I'm not too sure about that.

What if the user closes the dialog without pressing any button about the COUNTER question? Should the UI display the notification and the buttons every time the dialog is opened? What if more users ask to reschedule the dates? Put together an inline information with the buttons and a warning in the notificationbox that invites the user user to give an answer sounds a bit exaggerated.

By the way, is this feature implemented in other software/app? How do they handle it?

Comment 9

[[:MakeMyDay]]

Thank you for your feedback.

(In reply to Decathlon from comment #8)
> Not sure what is the best solution.
> I think that some kind of information, a descriptive text, is needed here so
> I would discard the first screenshot that is a bit cryptic. I don't know if
> using the notificationbox is right though, it seems a better place for
> information or warnings about wrong data, data that can cause problems, or
> to inform the user when Lightning can't display correctly all the parameters
> of an event (for example warn the user when a wrong end/until date is
> entered in the datepickers).

Ok, so we all agree that using the notificationbox for this purpose is not the way to go, which makes https://bugzilla.mozilla.org/attachment.cgi?id=8793722 obsolete.

> In my opinion could be right just adding inline the information and the
> buttons for rescheduling/keep the dates in the same section where are the
> start/end datepickers. The difference in the UI compared to the standard
> case should be sufficient to attract user attention but I'm not too sure
> about that.

That would tend to go in the direction of https://bugzilla.mozilla.org/attachment.cgi?id=8697755 (in terms of having the buttons next to the respective control, not in terms of duplicating the control), which you have mentioned above you would discard. So I'm a little confused. The downside of this approach that the UI may get cluttered, if the attendee not just proposed changes to datetime but also to the location and maybe other proprties.

Meanwhile, I prefer https://bugzilla.mozilla.org/attachment.cgi?id=8794129 (which is not a notificationbox - see comment 7).

> What if the user closes the dialog without pressing any button about the
> COUNTER question? Should the UI display the notification and the buttons
> every time the dialog is opened?

If the organizer just closes the dialog without changing anything, the imipbar would keep offering the reschedule or decline counter options. If the organizer has changed something in the dialog, but not according to the attendees proposal (which is a valid use case), it would trigger an updated invitation to be send out regradless. If the original countering attendee is not satisfied with the update, he's free to counter once again or respond in any other way.

> What if more users ask to reschedule the
> dates? Put together an inline information with the buttons and a warning in
> the notificationbox that invites the user user to give an answer sounds a
> bit exaggerated.
 
As mentioned in comment 7, the additional information about the counter proposal is only available when opening from the repsective counter email and contextwise restricted to that reply. That said, there can only be one proposal per view of the dialog concurrently. If there are multiple proposals for the same event, it's up to the user which to pick up for sending an updated request. I will take care that after sending out an update referring to one proposal, the remaining proposals still can be opened, but then maybe with an additional notification (a good use case for the notificationbox, s.i.c.) that the proposal was made for a previous version of the event.

> By the way, is this feature implemented in other software/app? How do they
> handle it?

Dealing with counter is a standard feature of all major apps like Outlook or Notes. Outlook e.g. deals with it in a similar way (with the difference that their invitation preview looks more like their edit dialog and offeres more options directly due to the ribbon bar), but just preloads the proposed data and offeres no option to toogle between the proposal and the original values, iirc.

Comment 10

[[:MakeMyDay]]

Attachment: rescheduling-dialog.png

This is the event dialog in reschedule mode based on the current patch. The buttons in the blue box still need some alignment.

Comment 11

[[:MakeMyDay]]

Attachment: HandleIncomingCounter-V2.diff

Work in progress patch. This is already working, but some xpcshell tests still need completion and the patch overall some polishing.

Comment 12

[[:MakeMyDay]]

Attachment: HandleIncomingCounter-V2-Strings-V1.diff

String part of the patch for checkin.

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/40eadd4a11bd430d0ca17d52ee5db2f65defc187
Bug 1225784 - Deal properly with incoming counter proposals - Strings only. rs=pre-merge-strings-only

Comment 14

[aleth [:aleth]]

Note I didn't review these strings, as I don't know enough about the context. 

However: isn't counterproposal one word (not: counter proposal)? (This could be easily fixed without requiring new strings, as it doesn't change the meaning.)

Comment 15

[[:MakeMyDay]]

(In reply to aleth [:aleth] from comment #14)
> Note I didn't review these strings, as I don't know enough about the
> context. 
> 
> However: isn't counterproposal one word (not: counter proposal)? (This could
> be easily fixed without requiring new strings, as it doesn't change the
> meaning.)

afaik, you can do it either way. But nevertheless, Philipp, I would appreciate a post landing review for the strings. As we will have late-l10n changes anyway, we could still correct if something is wrong or misleading.

Comment 16

[[:MakeMyDay]]

Comment on attachment 8810122 [details] [diff] [review]
HandleIncomingCounter-V2.diff

Removing the feedback request for the main patch as I have an updated patch that will be almost ready for review.

Comment 17

[[:MakeMyDay]]

Attachment: HandleIncomingCounter-V3.diff

Ok, I think this is ready for review now. There is still one question left (moving some code from imip-bar to calItipUtils), but the patch also could land as-is.

The xpcshell test all pass locally, but I couldn't trigger a try build due to current bustage. The tests requires also bug 1317064 to pass.

On top of this, it passes my local wip mozmill test for invitations for bug 1003207 (not yet uploaded).

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8812449 [details] [diff] [review]
HandleIncomingCounter-V3.diff

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

I'm generally ok taking this, but as you know our itip tests are not so great. We might get some regressions for this, and if they are only caught in release we might need to act fast on patches and new releases. I'd appreciate if you could keep an eye on this.

Here are the mach eslint errors I get, ignoring two hunks that failed to apply on my current tree: https://pastebin.mozilla.org/8932143

Instructions to get mach eslint working:
cd comm-central
rm -r testing # Yes, you'll need to revert this before creating a patch :-(
ln -s mozilla/testing
ln -s mozilla/tools
# edit calendar/.eslintrc.js and add a line "space-infix-ops": 0,
# edit tools/lint/eslint/node_modules/eslint-plugin-mozilla/lib/processors/xbl-bindings.js to change INDENT_LEVEL to 4

I hope to make this easier in the future, I need to correct the path issue somewhere in m-c's python code, and I have a wip patch for the indent level.

::: calendar/base/content/calendar-item-editing.js
@@ +325,5 @@
>   *                                           modification.
>   * @param aPromptOccurrence     If the user should be prompted to select if the
>   *                                parent item or occurrence should be modified.
>   * @param initialDate           (optional) The initial date for new task datepickers
> + * @param aCounterProposal      (optional) An object representing the counter proposal

Would probably be a good idea to describe the required object keys here. You can either use something like this (without the type) http://usejsdoc.org/tags-param.html#parameters-with-properties, or just describe it in the first part of the comment, e.g.

/**
 * Creates a task with the calendar event dialog. The aCounterProposal
 * parameter expects an object like this:
 *
 *     {
 *         ...
 *     }
 *
 * @param ...
 */

@@ +362,5 @@
>   * @param mode              The operation the dialog should do ("new", "modify")
>   * @param callback          The callback to call when the dialog has completed.
>   * @param job               (optional) The job object for the modification.
>   * @param initialDate       (optional) The initial date for new task datepickers
> + * @param counterProposal   (optional) An object representing the counter proposal

You'd have to re-describe it here, but I guess you could do it once and then reference to the other function.

::: calendar/base/modules/calItipUtils.jsm
@@ +279,5 @@
>  
> +        let disallowedCounter = false;
> +        if (foundItems && foundItems.length) {
> +             let disallow = foundItems[0].getProperty("X-MICROSOFT-DISALLOW-COUNTER");
> +             disallowCounter = disallow && disallow == "TRUE";

just disallowCounter = disallow == "TRUE"; should do it here, since you are not accessing properties of disallow.

@@ +296,5 @@
> +                        data.label = _gs("imipBarDisallowedCounterText");
> +                    } else {
> +                        let items = itipItem.getItemList({});
> +                        let comparison = [];
> +                        items.forEach((aItem) => {

Any specific reason to use forEach here instead of for..of ? Doing that you could also check for specific values and use flag variables instead of creating an array and using includes().

@@ +414,5 @@
> +        let compFields = Components.classes["@mozilla.org/messengercompose/composefields;1"]
> +                                   .createInstance(Components.interfaces.nsIMsgCompFields);
> +        let addresses = compFields.splitRecipients(author, true, {});
> +        if (addresses.length != 1) {
> +            cal.WARN("No unique email address for lookup!\r\n" + cal.STACK(20));

If we are going to warn about this in the console, maybe it makes sense to include more information on the msghdr this is about.

@@ +958,5 @@
> +     * @param aMethod iTIP method
> +     * @param aRecipientsList an array of calIAttendee objects the message should be sent to
> +     * @param aAutoResponse an inout object whether the transport should ask before sending
> +     */
> +    sendImipMessage: function(aItem, aMethod, aRecipientsList, aAutoResponse) {

If this is only for DECLINECOUNTER, I'd suggest renaming the method sendDeclineCounterMessage, in which case you could drop the else block (especially since you are returning a value only in one branch)

::: calendar/base/src/calItipItem.js
@@ +33,5 @@
> +        return this.mSender;
> +    },
> +    set sender(aValue) {
> +        return (this.mSender = aValue);
> +    },

I never really liked this style of getter/setter, since it doesn't do anything. Just |sender: null| would also do it. I get that its done for consistency though, so I won't insist.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +36,5 @@
>  
> +/*--------------------------------------------------------------------
> + *   Event dialog counter box section
> + *-------------------------------------------------------------------*/
> + 

Whitespace

@@ +51,5 @@
> +    font-weight: bold;
> +}
> +
> +.counter-buttons {
> +    max-height: 25px;

May make sense to convert this into em units

::: calendar/lightning/content/imip-bar.js
@@ +396,5 @@
> +                            attendee: proposingAttendee,
> +                            proposal: parsedProposal.differences,
> +                            oldVersion: parsedProposal.result == "OLDVERSION" ||
> +                                        parsedProposal.result == "NOTLATESTUPDATE",
> +                            onReschedule: onReschedule

I'd suggest to inline the function into the object, I don't think this needs to be a separate value.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +3043,5 @@
>                      eventDialogCalendarObserver.observe(window.calendarItem.calendar);
>                  }
>              }
> +            // this triggers the update of the imipbar in case this is a rescheduling case
> +            if ("onReschedule" in counterProposal) {

Isn't this always in the object? Probably better to require this, and then check if (counterProposal.onReschedule).

@@ +3903,5 @@
> +    let propValues = document.getElementById("counter-proposal-property-values");
> +    let idCounter = 0;
> +    let comment;
> +
> +    window.counterProposal.proposal.forEach(aProposal => {

Any specific reason to use forEach? I'd prefer for..of

@@ +4004,5 @@
> +        // as label control assignment should be unique, we can just take the first result
> +        labelValue = labels[0].value;
> +    } else {
> +        cal.LOG("Unsupported property " + aProperty.property + " detected when setting up counter " +
> +                "box labels. Please file a bug for this.");

Be careful with "Please file a bug for this". We had a string like that, and people started filing the same bug without searching for duplicates :-)

@@ +4032,5 @@
> +    } else if (stringProps.includes(aProperty.property)) {
> +        val = aProperty.proposed;
> +    } else {
> +        cal.LOG(`
> +            Unsupported property ${aProperty.property} detected when setting up counter box values.

Using a template string with newlines will also display the newlines in the console, with the same indent you are using here.

::: calendar/lightning/modules/ltnInvitationUtils.jsm
@@ +559,5 @@
> +                    status.descr = "This is a counter proposal not based on the latest event update.";
> +                    status.type = "NOTLATESTUPDATE"
> +                }
> +                for (let prop of properties) {
> +                    let newValue = aProposedItem[prop] || aProposedItem.getProperty(prop) || null;

I believe getProperty("DTSTART") will also give you the right thing, so maybe use getProperty exclusively and always use the property names. Please check though.

::: calendar/test/unit/test_calitiputils.js
@@ +14,5 @@
> +    compareStamp_test();
> +    compare_test();
> +}
> +
> +// tests for calItipUtils.jsm

Maybe put this (with spacing) above run_test.

@@ +17,5 @@
> +
> +// tests for calItipUtils.jsm
> +/*
> + * Helper function to get an ics for testing sequence and stamp comparison
> + * 

Whitespace.

@@ +146,5 @@
> +        input: { author: "Sender 1 <[e-mail]>" },
> +        expected: "[e-mail]"
> +    }];
> +    let i = 0;
> +    for (let test of data) {

If you need a sequence loop anyway, maybe use for (let i = 0; i < data.length; i++) { let test = data[i]; ... } and use 0-based test numbers. Same with other tests.

::: calendar/test/unit/test_calutils.js
@@ +112,5 @@
> +        let detected = [];
> +        cal.getAttendeesBySender(attendees, test.input.sender).forEach(att => {
> +            detected.push(att.id);
> +        });
> +        ok(detected.every(aId => test.expected.includes(aId)) &&

Maybe split this into two ok()'s in case one but not the other is true?

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8810123 [details] [diff] [review]
HandleIncomingCounter-V2-Strings-V1.diff

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

::: calendar/locales/en-US/chrome/calendar/calendar-event-dialog.dtd
@@ +166,5 @@
>  
> +<!-- Counter box -->
> +<!ENTITY counter.button.proposal.label                    "Apply proposal">
> +<!ENTITY counter.button.proposal.accesskey                "p">
> +<!ENTITY counter.button.proposal.tooltip                  "Apply the proposal data to the form.">

Please change the strings as discussed on IRC, and also add localization notes here.

::: calendar/locales/en-US/chrome/lightning/lightning.properties
@@ +117,5 @@
>  imipAddedItemToCal2=The event has been added to your calendar.
>  imipCanceledItem2=The event has been deleted from your calendar.
>  imipUpdatedItem2=The event has been updated.
>  imipBarCancelText=This message contains an event cancellation.
> +imipBarCounterErrorText=This message contains a counter proposal to an invitation that cannot be processed.

http://www.merriam-webster.com/dictionary/counterproposal suggests this is one word, I think we should stick with that.

@@ +155,5 @@
>  itipRequestUpdatedSubject=Updated Event Invitation: %1$S
>  itipRequestBody=%1$S has invited you to %2$S
>  itipCancelSubject=Event Canceled: %1$S
>  itipCancelBody=%1$S has canceled this event: « %2$S »
> +itipCounterBody=%1$S has made a counter proposal for « %2$S »:

Interesting use of the arrow quotes. I think we should switch to something different since this is not commonly used in the English language as far as I read https://en.wikipedia.org/wiki/Guillemet

You could change this for other strings without an entity change, since this doesn't change the meaning of the string.

Comment 20

[[:MakeMyDay]]

Attachment: HandleIncomingCounter-V4.diff

Final patch for the remaining bits with all comment from the bug and irc considered (except the getter/setter thing and the px/em conversion) and xpcshell tests passing on automation.

a+ via irc

Comment 21

[[:MakeMyDay]]

Remaining part:

https://hg.mozilla.org/comm-central/rev/1e17d317033e80964dc488ad238f829aaeb2b1f1

Comment 22

[Jorg K (CEST = GMT+2)]

Aurora (Thunderbird 52 / Lightning 5.4)
https://hg.mozilla.org/releases/comm-aurora/rev/f73933c09b84