Closed Bug 1341433 Opened 4 years ago Closed 4 years ago

Make events with non-lowercase mime type, like text/Calendar show in IMIP bar as events. Also make it IMIP bar work for .eml files

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed)

RESOLVED FIXED
Thunderbird 54.0
Tracking Status
thunderbird_esr52 53+ fixed
thunderbird53 --- fixed
thunderbird54 --- fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch cal-text-debug.patch (obsolete) — Splinter Review
Receive an event with text/Calendar - wouldn't work.

So not using lowercase confused mime in two places, making it hard to find what was wrong.
To test it with lightning and a file, I made a very simple mozmill test, but had to fix a few cases where lightning assumed msg.folder was set. All the test does is check the bar is really showing.

make -C calendar/test/mozmill SOLO_TEST=invitations/test-imip-bar.js mozmill-one
Attachment #8839671 - Flags: review?(makemyday)
Attachment #8839671 - Flags: review?(jorgk)
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
For manual testing
Comment on attachment 8839671 [details] [diff] [review]
cal-text-debug.patch

Gah, accidentially removed a needed change to imip-bar.js
Attachment #8839671 - Attachment is obsolete: true
Attachment #8839671 - Flags: review?(makemyday)
Attachment #8839671 - Flags: review?(jorgk)
Correct, working version.
Attachment #8839675 - Attachment is obsolete: true
Attachment #8839677 - Flags: review?(makemyday)
Attachment #8839677 - Flags: review?(jorgk)
Attachment #8839675 - Attachment is obsolete: false
Comment on attachment 8839677 [details] [diff] [review]
bug13454379_text_Calendar_imip.patch

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

r+ for the mailnews changes.

::: calendar/test/mozmill/invitations/message-containing-event.eml
@@ +15,5 @@
> +X-Spam-Status: LOW ; 31%
> +X-PMX-Spam: Gauge=XXXI, Probability=31%, Report='
> + AALTO_ENVELOPE+ 1, AALTO_FROM_01+ 0.75, AALTO_KLIKKAA_02+ 0.5, AALTO_SUSP_URL_PARTS+ 0.5, IMGSPAM_BODY 0.5, HTML_70_90 0.1, FROM_NAME_ONE_WORD 0.05, BODYTEXTH_SIZE_10000_LESS 0, BODYTEXTH_SIZE_3000_MORE 0, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_6000_6999 0, BODY_SIZE_7000_LESS 0, DATE_TZ_NA 0, DKIM_SIGNATURE 0, LINK_TO_IMAGE 0, __ANY_URI 0, __C230066_P5 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __CTYPE_MULTIPART_MIXED 0, __FRAUD_MONEY_CURRENCY 0, __FRAUD_MONEY_CURRENCY_EURO 0, __HAS_ATTACHMENT 0, __HAS_ATTACHMENT1 0, __HAS_FROM 0, __HAS_HTML 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HIGHBITS 0, __HTML_AHREF_TAG 0, __HTTPS_URI 0, __HTTP_IMAGE_TAG 0, __IMGSPAM_BODY 0, __MIME_HTML 0, __MIME_VERSION 0, __MULTIPLE_URI_HTML 0, __MULTIPLE_URI_TEXT 0, __SANE_MSGID 0, __TAG_EXISTS_HTML 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_IN_BODY 0, __URI_NO_MAILTO 0,
> + __URI_NS , __URI_WITH_PATH 0'
> +X-Spam-Flag: No

Can you please strip some of those unnecessary headers.

::: calendar/test/mozmill/invitations/test-imip-bar-eml.js
@@ +37,5 @@
> +
> +  msgc.waitFor(function() {
> +    let nb = msgc.window.document.getElementById("imip-bar");
> +    if (!nb) {
> +      throw new Error("Could'f find imip-bar in DOM.");

typo.
Attachment #8839677 - Flags: review?(jorgk) → review+
Comment on attachment 8839671 [details] [diff] [review]
cal-text-debug.patch

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

Thanks. r+ for the calendar part with the style nits considered and an at least one additional variant of the mime type like text/Calendar being tested. Please kick of a try run before landing to make sure this test passes on all platforms.

::: calendar/base/modules/calItipUtils.jsm
@@ +471,5 @@
>          }
>  
>          // First check the recipient list
> +        let toList = MailServices.headerParser.makeFromDisplayAddress(
> +          aMsgHdr.recipients || "");

Please keep that in one line.

@@ +481,5 @@
>          }
>  
>          // Maybe we are in the CC list?
> +        let ccList = MailServices.headerParser.makeFromDisplayAddress(
> +          aMsgHdr.ccList || "");

Same here.

::: calendar/test/mozmill/invitations/message-containing-event.eml
@@ +15,5 @@
> +X-Spam-Status: LOW ; 31%
> +X-PMX-Spam: Gauge=XXXI, Probability=31%, Report='
> + AALTO_ENVELOPE+ 1, AALTO_FROM_01+ 0.75, AALTO_KLIKKAA_02+ 0.5, AALTO_SUSP_URL_PARTS+ 0.5, IMGSPAM_BODY 0.5, HTML_70_90 0.1, FROM_NAME_ONE_WORD 0.05, BODYTEXTH_SIZE_10000_LESS 0, BODYTEXTH_SIZE_3000_MORE 0, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_6000_6999 0, BODY_SIZE_7000_LESS 0, DATE_TZ_NA 0, DKIM_SIGNATURE 0, LINK_TO_IMAGE 0, __ANY_URI 0, __C230066_P5 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __CTYPE_MULTIPART_MIXED 0, __FRAUD_MONEY_CURRENCY 0, __FRAUD_MONEY_CURRENCY_EURO 0, __HAS_ATTACHMENT 0, __HAS_ATTACHMENT1 0, __HAS_FROM 0, __HAS_HTML 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HIGHBITS 0, __HTML_AHREF_TAG 0, __HTTPS_URI 0, __HTTP_IMAGE_TAG 0, __IMGSPAM_BODY 0, __MIME_HTML 0, __MIME_VERSION 0, __MULTIPLE_URI_HTML 0, __MULTIPLE_URI_TEXT 0, __SANE_MSGID 0, __TAG_EXISTS_HTML 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_IN_BODY 0, __URI_NO_MAILTO 0,
> + __URI_NS , __URI_WITH_PATH 0'
> +X-Spam-Flag: No

I think you can spare the X- headers here.

@@ +39,5 @@
> +
> +--_=ALT_=aspNetEmail=_51bed191ceac49f7a22392ea84b6ef35--
> +
> +--_=aspNetEmail=_51bed191ceac49f7a22392ea84b6ef35
> +Content-Type: text/calendar; name="booking.ics"

As the bug is about being able to deal also with not all lowercase mime type, you probably want to additionally test another variant like text/Calendar

Apart from that, this is not an RfC (6047) compliant specification of Content-Type - this should be

Content-Type: text/calendar; method=PUBLISH; charset=UTF-8

(with method being mandatory and charset mandatory if non UTF-8 characters would occur - specifying a name is not needed here)

::: calendar/test/mozmill/invitations/test-imip-bar-eml.js
@@ +12,5 @@
> +
> +var RELATIVE_ROOT = "../shared-modules";
> +var MODULE_REQUIRES = [
> +  "folder-display-helpers",
> +  "window-helpers",

Unlike TB, Calendar uses 4 whitespace indentation, so please adjust this everywhere in the new test.

@@ +34,5 @@
> +  let file = os.getFileForPath(os.abspath("./message-containing-event.eml", thisFilePath));
> +
> +  let msgc = open_message_from_file(file);
> +
> +  msgc.waitFor(function() {

Please make this an arrow function:
msgc.waitFor(() => {

@@ +37,5 @@
> +
> +  msgc.waitFor(function() {
> +    let nb = msgc.window.document.getElementById("imip-bar");
> +    if (!nb) {
> +      throw new Error("Could'f find imip-bar in DOM.");

Typo.

@@ +38,5 @@
> +  msgc.waitFor(function() {
> +    let nb = msgc.window.document.getElementById("imip-bar");
> +    if (!nb) {
> +      throw new Error("Could'f find imip-bar in DOM.");
> +    }

We usually use some helpers from calendar-utils for looking up the DOM element. If you want to get the bonus points, look at the other calendar tests an pick up the approach. But as this test will get touched anyway when landing my WIP patch for mozmill invitation tests, you also can leave it with your current approach for now.
Attachment #8839671 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bbeb17bcfd6ef5651d91c444d29601e1d883c18a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
Version: unspecified → Trunk
Comment on attachment 8840072 [details] [diff] [review]
bug13454379_text_Calendar_imip.patch

[Approval Request Comment]
User impact if declined: events with non-lowercase mime type can't be added to calendar (in any easy way)
Testing completed (on c-c, etc.): mozmill test
Risk to taking this patch (and alternatives if risky): not risky
Attachment #8840072 - Flags: approval-comm-release?
Attachment #8840072 - Flags: approval-comm-beta?
Attachment #8840072 - Flags: approval-comm-aurora?
Comment on attachment 8840072 [details] [diff] [review]
bug13454379_text_Calendar_imip.patch

I can't do Aurora, since it's already *in* Aurora, TB 54.
We don't uplift to c-r. I can do c-ers52 after a beta run, gotta stick to the rules.
Attachment #8840072 - Flags: approval-comm-release?
Attachment #8840072 - Flags: approval-comm-esr52?
Attachment #8840072 - Flags: approval-comm-beta?
Attachment #8840072 - Flags: approval-comm-beta+
Attachment #8840072 - Flags: approval-comm-aurora?
TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/72a7fd03d6be7f26f453e3fe76e6209fbc12d4c1

I've just noticed that the commit message for all branches has non-existent bug 13454379 :-(
Attachment #8840072 - Flags: approval-comm-esr52? → approval-comm-esr52+
I backed that out and landed it again, at least on C-C so we stand the chance to track the changes back to bug:
https://hg.mozilla.org/comm-central/rev/19dc916b9af963c59a54b11d7208dbb89e819a25
https://hg.mozilla.org/comm-central/rev/7480da62b6bfede9e4a5fe401da22989865ec73b

While doing so I've looked at it again and noticed:
https://hg.mozilla.org/comm-central/rev/7480da62b6bfede9e4a5fe401da22989865ec73b#l2.1
copy from calendar/lightning/content/imip-bar.js
copy to   calendar/lightning/content/imip-bar-eml.js

Why was that file copied? Both files then received identical edits. And imip-bar-eml.js doesn't appear to be used anywhere.
Flags: needinfo?(mkmelin+mozilla)
That was not supposed to be there. I had accidentally renamed/moved one file, but apparently didn't get it cleared up properly.

https://hg.mozilla.org/comm-central/rev/d68d335c34a7f42fadda05c06db127b1f264ff5c
Flags: needinfo?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.