Skip to content

#903 Include attachments in forwarded messages#1090

Merged
tomholub merged 7 commits intomasterfrom
feature/issue-903-forward-attachments
Nov 26, 2021
Merged

#903 Include attachments in forwarded messages#1090
tomholub merged 7 commits intomasterfrom
feature/issue-903-forward-attachments

Conversation

@sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Nov 25, 2021

This PR adds attachments support for forwarded messages

close #903


Tests (delete all except exactly one):

  • Tests added or updated - updated UI test to check attachment presence

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@sosnovsky sosnovsky requested a review from tomholub November 25, 2021 11:57
@sosnovsky sosnovsky marked this pull request as ready for review November 25, 2021 11:57
@sosnovsky
Copy link
Collaborator Author

@tomholub I currently have only 2 assigned tasks left - #953 and #840, can you please check which tasks should I work after these ones?

@tomholub
Copy link
Collaborator

Very gladly :-)

Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Will test. Comments below.

@tomholub
Copy link
Collaborator

tomholub commented Nov 25, 2021

I got this when forwarding a 9+ mb video. But maybe that's for another issue?

image

This was after it already spent some time uploading. So if we're uplaoding a large message, we should have higher timeout? Or the timeout should be based on "how long we didn't send any bytes for" vs "how long it took overall"

@tomholub
Copy link
Collaborator

I've encountered the following issues

xlsx files malformed when forwarding

  1. open a plain message that has a xlsx file in it
  2. forward it encrypted
  3. open on a laptop with FlowCrypt
  4. downloaded file doesn't get recognized as a xlsx file. It does have the right extension, and it does seem to have the right contents. But maybe the mime-type is wrong / not preserved. Compare to forwarding with browser extension, which works. (issue also present on a jpeg file, and likely most others)

changed subject not recognized when forwarding

  1. open a message (tested with encrypted but probably applies to both)
  2. forward
  3. change subject
  4. send
  5. inspect headers of sent message. It will wrongly have the subject of the original conversation, not the one I edited.

@tomholub
Copy link
Collaborator

I suppose the xlsx files malformed when forwarding issue should be coupled with some unit test as it seems an easy thing to break again in the future.

@sosnovsky sosnovsky marked this pull request as draft November 25, 2021 14:22
@sosnovsky
Copy link
Collaborator Author

4. downloaded file doesn't get recognized as a xlsx file. It does have the right extension, and it does seem to have the right contents. But maybe the mime-type is wrong / not preserved. Compare to forwarding with browser extension, which works. (issue also present on a jpeg file, and likely most others)

I tried to reproduce this issue, but .xlsx file opens well after forwarding from iOS. Maybe it happens only with some specific files, I forwarded the same message to your email, can you please check if it works or not for you?

@tomholub
Copy link
Collaborator

tomholub commented Nov 25, 2021

  1. downloaded file doesn't get recognized as a xlsx file. It does have the right extension, and it does seem to have the right contents. But maybe the mime-type is wrong / not preserved. Compare to forwarding with browser extension, which works. (issue also present on a jpeg file, and likely most others)

I tried to reproduce this issue, but .xlsx file opens well after forwarding from iOS. Maybe it happens only with some specific files, I forwarded the same message to your email, can you please check if it woraks or not for you?

The issue (missing mime type inside the encrypted enclosure) is present on all files. But the user-facing problem (can't open as a result) only manifests on some host OS, and possibly only for some file types.

@sosnovsky
Copy link
Collaborator Author

I tested sending and forwarding message with 2 attachments (image and xls file) - in both cases app sends right content-types (image/jpeg and application/vnd.openxmlformats-officedocument.spreadsheetml.sheet) to Core.composeEmail function.
Then I downloaded these attachments from browser on macOS and checked mime types - they were the same as sent by the app.
So I think we should create another issue for finding why attachments mime-type is wrong on Linux OS, as it's not only for forwarded messages. May be some difference in decrypting .pgp files.

@tomholub
Copy link
Collaborator

Ok. I'll let this merge and we'll investigate it separately.

@tomholub tomholub merged commit f8f1eff into master Nov 26, 2021
@tomholub tomholub deleted the feature/issue-903-forward-attachments branch November 26, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

include attachments in forwarded message

2 participants