1698886 - Also return forwarded emails as attachments

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[John Bieling (:TbSync)]

Use allUserAttachments() instead of allAttachments() to return forwarded emails as well.

See:
https://searchfox.org/comm-central/rev/5428d2836831a284a682b6d1268f489ba1502e4c/mailnews/db/gloda/modules/MimeMessage.jsm#368-395

Comment 1

[John Bieling (:TbSync)]

Attachment: Bug 1698886 - Also return forwarded emails as attachments. r=mkmelin

Comment 2

[Mark Banner (:standard8)]

The problem with doing this change is that you will not be able to get details of any attachments contained within the forwarded messages.

This might mean that getFull() has full details, but then getting the attachment details won't be able to, because of the limitation.

See also this thread for some info on how to reproduce: https://github.com/thunderbird-conversations/thunderbird-conversations/issues/1486

Note that Conversations' "I love attachments" mode is effectively toggling between allUserAttachments when it is off and allAttachments together with allUserAttachments when it is on.

Comment 3

[John Bieling (:TbSync)]

Confirmed. I find it somewhat odd to not get eml files returned as attachments, but to have to craw for their bodies through getFull.

Comment 4

[Mark Banner (:standard8)]

We could do both maybe? or provide an option?

Comment 5

[John Bieling (:TbSync)]

I am working on https://phabricator.services.mozilla.com/D148120 which will add support for dummy messages to the API. When a message has an attached eml file, and that is opened, the onMessageDisplayed event will give you a WebExt MessageHeader, which should work with getRaw (patch ok), getFull (patch ok) and also with the attachment methods (patch wip) from them messages API.

Once that fully works, I intend to use allUserAttachments() for the attachments only, but include a MessageHeader, if it is a message.

1. The number of returned attachments will be what Thunderbird shows
2. The nested attachments can be retrieved by looking at the nested messages.

Do you think an extra option for listAttachments, which includes allAttachments again, would still be useful? (inline attachments mode?)

Comment 6

[Mark Banner (:standard8)]

For me, I think having an option would be useful - it would save having to do getFull on the message and then manually having to figure out what's an attachment & what's not.

Comment 7

[John Bieling (:TbSync)]

You would call listAttachment() to get all attachments (including emails) and call listAttachemnt() on those emails again. You would not call getFull() anymore.

Comment 8

[John Bieling (:TbSync)]

This is the next thing on my list.

When you enable "show attachments inline" in the Thunderbird UI, you get all attachments and(!) all emails, which is different from our current API return value, which only returns all attachments, but no emails.

Should we consider our current behaviour broken and instead include the messages just like the UI? I would then introduce an "inlineAttachments" option (defaulting to true) which can be set to false to get only the first level attachments.

However, from the part number one can see, which attachments are inline and which are not. So is the extra option needed at all? Just return everything and add a description about the part number and the concept of inline attachments to the documentation?

Comment 9

[Mark Banner (:standard8)]

I think returning all attachments + documents would be fine. The only thing I can see that affecting is performance, but I think this is more about correctness.

Comment 10

[John Bieling (:TbSync)]

Attachment: Bug 1698886 - Make messages.listAttachments() return a MessageHeader for attached messages. r=mkmelin

Depends on D108666

Comment 11

[John Bieling (:TbSync)]

try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=ca7ac314203bc65b25439c0acaa846792eafc97e

Comment 12

[John Bieling (:TbSync)]

There are a few test fails, but I have seen those before and I think they are not related to this patch, are they?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c353442104cf
Also return forwarded emails as attachments. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5524490ed106
Make messages.listAttachments() return a MessageHeader for attached messages. r=mkmelin

Comment 14

[Magnus Melin [:mkmelin]]

There are test failures so I'll back this out:
https://treeherder.mozilla.org/logviewer?job_id=390045791&repo=comm-central&lineNumber=4407
At least comm/mail/components/extensions/test/xpcshell/test_ext_messages_attachments.js perma fails locally for me

Comment 15

[Magnus Melin [:mkmelin]]

Backout:
https://hg.mozilla.org/comm-central/rev/56875463f616b1b64ef9da46836a26ab9c5f3c48
https://hg.mozilla.org/comm-central/rev/86d016a7723c42f1ef523b9402f22589f55f46db

Comment 16

[John Bieling (:TbSync)]

Argh, my try run did not cover xpcshell tests. I am sorry.

Comment 17

[John Bieling (:TbSync)]

Should work now. The eslint error in the new try run has been fixed as well:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=d366c87a3ce1006a7d18b53dbb53acdf5583dd44

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2aaa8c1e8b25
Also return forwarded emails as attachments. r=mkmelin
https://hg.mozilla.org/comm-central/rev/ca9f804f014c
Make messages.listAttachments() return a MessageHeader for attached messages. r=mkmelin

Comment 19

[Geoff Lankow (:darktrojan)]

Is there a need to hang on to dummy message headers after the message window closes? Because you are, and that's why browser_detachedWindows.js is failing. It looks like holding a reference to an nsDummyMsgHeader keeps the whole window alive, and that's not good.

Comment 20

[Geoff Lankow (:darktrojan)]

I don't know how (because I haven't looked) but revision ca9f804f014c also caused the toolkit/mozapps/extensions/test/xpcshell/rs-blocklist/ failures. I'm going to back it out. 2aaa8c1e8b25 looks okay AFAICT.

Comment 21

[Geoff Lankow (:darktrojan)]

Backed out: https://hg.mozilla.org/comm-central/rev/e9e43627b8c4741f6d82b573f389a7901f5159d8

Comment 22

[Geoff Lankow (:darktrojan)]

Okay, I took the other revision out too: https://hg.mozilla.org/comm-central/rev/2e8a1a3c46a6b6d58e311c05390905a55730e644

Comment 23

[John Bieling (:TbSync)]

@Geoff: Can you say why 2aaa8c1e8b25 was backed out? What test do I have to look out for?

Comment 24

[Geoff Lankow (:darktrojan)]

I should've done that. test_ext_messages_attachments.js doesn't seem to want to work after only the first patch.

Comment 25

[John Bieling (:TbSync)]

Attachment: Bug 1698886 - Fix test fails. r=darktrojan

Depends on D156580

Comment 26

[John Bieling (:TbSync)]

All green for patch D108666: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e04d4bb51b7f6ca5ac5a2db8020d21f073f66e24
All green for patch D156580: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=7c2c92af7677c06fbb18221a62699f7aa92b0101

Comment 27

[Alessandro Castellani [:aleca]]

The build had a problem so pushbot didn't add the landing revision: https://hg.mozilla.org/comm-central/rev/c3572e1fbf0704503e6e6c3fe89aed7b06ee814c

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/10fa3728eda3
Make messages.listAttachments() return a MessageHeader for attached messages. r=mkmelin