Closed Bug 1251484 Opened 4 years ago Closed 4 years ago

Prefixed event title in email subject causes problems with some calendar providers

Categories

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

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: fdv+mozbug, Assigned: fdv+mozbug)

Details

Attachments

(1 file, 4 obsolete files)

This is not a bug, but a feature request.

When sending out event invitations or updates to these, Lightning will add a prefix to the email subject, before the event title as such:

 "Event Invitation: <event title>"

or

 "Updated Event Invitation: <event title>"

The event title will of course also be part of the VEVENT, where it is written to the SUMMARY field (without prefix).

The issue is that some calendar providers, among these Microsoft Office 365, will use the email subject as the event title, leading to the only thing being visible in some views or for some events is "Event invitation:" as the event title will have wrapped to the next line (which may or may not be visible).

I have little doubt that the proper place from which to fetch the event title is the VEVENT, so I think Lightning's behavior here is correct, but given that the result still causes problems for many users, being able to work around it would be very nice. I suggest that it can be made configurable whether to prefix the subject with "Event Invitation: " and "Updated Event Invitation: ".
Suggestion for a patch for making it optional whether to prefix email subjects for new and updated event invitations.
Attachment #8723891 - Flags: feedback?(makemyday)
Comment on attachment 8723891 [details] [diff] [review]
Suggested patch that makes prefixed subjects configurable

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

We can make this a non-default hidden preference. However, this is a workaround for a wrong behaviour of such clients, so I would like to encourage you at least also file a bug there, if you don't mind. Once done, please add a reference to this bug.

Thank you for your patch - this is of good quality, especially for a first time contribution - I assume you've found our code style instructions before, as there are none of the usual style nits :-)

Also, it's very much appreciated that you added a test, especially as we currently have no test coverage for calItipEmailTransport.js. Your test will be a good starting point to change this in the near future (but not in this bug). I assume it passed locally?

Please find some comments below. If you have updated the patch, please upload it and request for review by setting the r? flag for the uploaded patch.

::: calendar/itip/calItipEmailTransport.js
@@ +56,1 @@
>  

this will be a not exposed method, so please move that below the sendItems method and prefix it with an underscrore. Better call it _initItems.

You don't need to add a name to the function anymore:

_initItems: function (aItipItem) {

would be enough.

@@ +60,5 @@
>  
> +        let summary = (item.getProperty("SUMMARY") || "");
> +        let aSubject = "";
> +        let aBody = "";
> +        switch (aItipItem.responseMethod) {

Change aSubject and aBody to subject and body, please. The a prefix for a variable name should only be used if this an argument to the respective function/method.

I know you haven't changed this, but whoever touches some code clean it up.

@@ +140,5 @@
> +    sendItems: function cietSI(aCount, aRecipients, aItipItem) {
> +        if (this.mHasXpcomMail) {
> +            cal.LOG("sendItems: Sending Email...");
> +            let items = this.buildItems(aItipItem);
> +            this._sendXpcomMail(aRecipients, items.aSubject, items.aBody, aItipItem);

This must be

return this._sendXpcomMail(aRecipients, items.aSubject, items.aBody, aItipItem);

::: calendar/lightning/content/lightning.js
@@ +80,5 @@
>  
> +// whether to expect that the recipient will use the event title from the email's subject rather
> +// than from the ical SUMMARY field
> +pref("calendar.itip.eventTitleFromSubject", false);
> +

Use a more obvious name for the preference like calendar.itip.useInvitationSubjectPrefixes (in this example, this should default to true, then).

Don't refer to the recipient's client behaviour in the comment - just describe what's the result of the preference when enabled/disabled from Lightning perspective. If you feel more documentation is needed, please create an article in SUMO.

::: calendar/test/unit/test_summary.js
@@ +7,5 @@
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +var calItipEmailTransport = {};
> +Services.scriptloader.loadSubScript("resource://calendar/components/calItipEmailTransport.js", calItipEmailTransport);
> +

Please try to not exceed to 80 character length for a row.

@@ +14,5 @@
> +    cal.getTimezoneService().startup({
> +        onResult: function() {
> +            really_run_test();
> +            do_test_finished();
> +        }

With the mocked ItipItem you don't need the timezone servoce here - but you should keep it when using a real calItipItem (see below)

@@ +23,5 @@
> +    test_title_in_subject();
> +    test_title_in_summary();
> +    test_updated_title_in_subject();
> +    test_updated_title_in_summary();
> +}

Instead of that, please use just add_tasks like in e.g. https://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_ltninvitationutils.js

@@ +26,5 @@
> +    test_updated_title_in_summary();
> +}
> +
> +function aItipItemForTest(title, seq) {
> +    this.seq = seq === undefined ? 0 : seq;

Don't prefix a function name with an a (see explaination above).

You could simply use this.seq = seq || 0; here. However, I would prefer to initialize an calItipItem instead of mocking one. For an example, see https://mxr.mozilla.org/comm-central/source/calendar/test/unit/test_ltninvitationutils.js#114

@@ +52,5 @@
> +function test_title_in_subject() {
> +    Preferences.set("calendar.itip.eventTitleFromSubject", true);
> +    let transport = new calItipEmailTransport.calItipEmailTransport();
> +    let items = transport.buildItems(new aItipItemForTest('foo'));
> +    equal(items.aSubject, 'foo');

Use double quotes instead of single quotes, please. This applies also for the other occurences below.

::: calendar/test/unit/xpcshell-shared.ini
@@ +44,5 @@
>  [test_view_utils.js]
>  [test_webcal.js]
>  [test_weekinfo_service.js]
>  [test_timezone_definition.js]
> +[test_summary.js]

Use a more obvious name for the test, e.g. test_imip.js - we can later extend this test file also to test other parts of calItipEmailTransport.js
Attachment #8723891 - Flags: feedback?(makemyday) → feedback+
Assignee: nobody → fdv+mozbug
Severity: normal → enhancement
One more thing: please add the reviewer to the commit message in the patch header - see [1]. Ususally, you do this already within the patch you upload for review, so that you don't have to upload it again after it passed the review (this is o make it easier for other people to checkin your patch if you don't have the priviledges or oppurtunity to do it by yourself).

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8723891 - Attachment is obsolete: true
Attachment #8727185 - Flags: review?(makemyday)
Assignee: fdv+mozbug → makemyday
I've updated the patch and tried to adhere to the feedback given.

So far, I've yet to find out how to report this to Microsoft, but I've discovered it also affects the Outlook standalone client. I've also noticed that Google Calendar also sends out invites where the subject field is different from the vevent summary field, similar to Lightning. If Microsoft should fix this issue, I still think it would make sense to have this patch in, but only temporarily, as after some years, the number of users left Outlook versions affected by this would be sufficiently small for it not to matter. For now, though, this number is vast, and will undoubtedly be for a long time.
I have now spent close to 2 hours on a Microsoft chat explaining the issue and trying to find out where to report it. The support person claimed it would be escalated and hopefully be looked at by an engineer. If it makes it to a public site, I will post it here.
Comment on attachment 8727185 [details] [diff] [review]
Suggested patch that makes prefixed subjects configurable, v2

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

Thanks for providing the patch. Just a drive by review for now:

- you didn't include the test_imip.js file into the patch
- please move the sendItems method on top of _prepareItems
- you can use a shorter variable name to assign the preference value, like e.g. usePrefix - there's enough context to do so
- please add a Date to the patch header
- it's enough to add the reviewer nick to the commit message, email address is not needed

r- to get the above considered in an updated patch.

One additional remark: you as the patch author stay the assignee of the bug, even when the workflow moves to the reviewer. So, don't change the assignee.
Attachment #8727185 - Flags: review?(makemyday) → review-
Assignee: makemyday → fdv+mozbug
(In reply to MakeMyDay from comment #7)
> Comment on attachment 8727185 [details] [diff] [review]
> Suggested patch that makes prefixed subjects configurable, v2
> 
> Review of attachment 8727185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for providing the patch. Just a drive by review for now:
> 
> - you didn't include the test_imip.js file into the patch

Oops, bummer. Not used to mercurial, I'm afraid.

> - please move the sendItems method on top of _prepareItems

Done.

> - you can use a shorter variable name to assign the preference value, like
> e.g. usePrefix - there's enough context to do so

Done.

> - please add a Date to the patch header

On what format do you want this?

> - it's enough to add the reviewer nick to the commit message, email address
> is not needed

I was unsure of this, it is not stated in the docs. Done.

> r- to get the above considered in an updated patch.
> 
> One additional remark: you as the patch author stay the assignee of the bug,
> even when the workflow moves to the reviewer. So, don't change the assignee.

Also good to know. :-)
(In reply to Fredrik de Vibe from comment #8)
> (In reply to MakeMyDay from comment #7)

[ --8<-- ]

> > - please add a Date to the patch header
> 
> On what format do you want this?

I found the -D switch to hg qnew / qrefresh, I trust this is sufficient.

[ --8<-- ]
Attachment #8727185 - Attachment is obsolete: true
Attachment #8727450 - Flags: review?(makemyday)
Comment on attachment 8727450 [details] [diff] [review]
Suggested patch that makes prefixed subjects configurable, v3

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

r+ with the comments below considered and a successful try run for the test.

If you don't have commit access to throw a try run yourself, you can either needinfo me to do it or ask on irc if somebody else can do this for you once you've uploaded the updated patch.

::: calendar/itip/calItipEmailTransport.js
@@ +54,4 @@
>      sendItems: function cietSI(aCount, aRecipients, aItipItem) {
>          if (this.mHasXpcomMail) {
>              cal.LOG("sendItems: Sending Email...");
> +            let items = this._buildItems(aItipItem);

this must be this._prepareItems(aItipItem) instead of _buildItems

@@ +89,5 @@
> +                } else {
> +                    let seq = item.getProperty("SEQUENCE");
> +                    let subjectKey = (seq && seq > 0)
> +                            ? "itipRequestUpdatedSubject"
> +                            : "itipRequestSubject";

Too much indention here

@@ +125,5 @@
> +                if (!att && aItipItem.identity) {
> +                    att = item.getAttendeeById("mailto:" + aItipItem.identity);
> +                }
> +                if (!att) { // should not happen anymore
> +                    return false;

You must catch this in the calling code (currently your're expecting an object with attributes subject and body). If there items is false you should return false in sendItems, too.

::: calendar/test/unit/test_imip.js
@@ +14,5 @@
> +}
> +
> +function itipItemForTest(title, seq) {
> +    let item = Components.classes["@mozilla.org/calendar/itip-item;1"]
> +            .createInstance(Components.interfaces.calIItipItem);

better call this var itipItem - item is usually used for the inner item (the event itself). Also, please fix the indention to have .classes and .createInstance justified.

@@ +53,5 @@
> +    Preferences.set("calendar.itip.useInvitationSubjectPrefixes", true);
> +    let transport = new calItipEmailTransport.calItipEmailTransport();
> +    let items = transport._prepareItems(itipItemForTest('bar', 2));
> +    equal(items.subject, 'Updated Event Invitation: bar');
> +});

The following applies to all the above add_tasks blocks:
- define transport outside of the task once and reuse it within all of the tasks added - there's no need to recreate it multiple time.
- no need to use function* here, function is sufficient
- use double qutotes, not single quotes

::: calendar/test/unit/xpcshell-shared.ini
@@ +44,5 @@
>  [test_view_utils.js]
>  [test_webcal.js]
>  [test_weekinfo_service.js]
>  [test_timezone_definition.js]
> +[test_imip.js]

Move this up to consider the alphabethic order of the file names, please. (the test_timezone_definition.js has also been moved up meanwhile)
Attachment #8727450 - Flags: review?(makemyday) → review+
Attachment #8727450 - Attachment is obsolete: true
Attachment #8738786 - Flags: review?(makemyday)
I don't have commit access, and on IRC they want a try command. It seems somewhat complex, so some help with this would be great.
Flags: needinfo?(makemyday)
The try run passed, no related errors to this patch:


https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7e484c17181ddf8666c27a9c516a7ab6da9b5bd2

However, I needed to modify the patch slightly to get it applied. It was bitrotted. I'll upload that version later.
Flags: needinfo?(makemyday)
Updated patch which applies on current trunk. There was a conflict for xpcshell-shared.ini.

You can avoid this if you pull the latest changes from Hg and check whether your patches still apply before uploading. Thereby you can make it as easy as possible for others to start a try run or doing a checkin for you.
Attachment #8738786 - Attachment is obsolete: true
Attachment #8738786 - Flags: review?(makemyday)
Attachment #8740072 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a0877b96c8c96620b51670dfa0b762d60e92da08
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 5.0
You need to log in before you can comment on or make changes to this bug.