Saving attachments by ekievsky · Pull Request #352 · FlowCrypt/flowcrypt-ios

ekievsky · 2021-06-16T17:19:43Z

This PR adds Files Manager that allows app to save attachments to user's phone

close #194

Tests:

Added test for FilesManager to make sure that file was saved

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

tomholub · 2021-06-16T18:02:09Z

Does not need tests

It 100% needs tests but we don't have a good mechanism for writing tests yet, blocked on #307 in part

tomholub

I presume this is still a draft? If so, you can mark it as a draft:

Kharchevskyi · 2021-06-16T18:06:15Z

Does not need tests
It 100% needs tests but we don't have a good mechanism for writing tests yet, blocked on #307 in part

#307 will unblock only possibility to write UI tests

tomholub · 2021-06-16T19:42:01Z

Does not need tests
It 100% needs tests but we don't have a good mechanism for writing tests yet, blocked on #307 in part
#307 will unblock only possibility to write UI tests

Yup - that's the type of tests that's the most valuable to me especially initially.

tomholub · 2021-06-16T19:59:40Z

Alternatively, how could this functionality be tested? (rendering a message with an attachment, then downloading the attachment)

ekievsky · 2021-06-17T08:56:37Z

I presume this is still a draft? If so, you can mark it as a draft:

as I understood from issue description, on scope of a task I need to add downloading attachments to user's phone. Since the attachments come along with message I just implemented saving attachment to device.

tomholub · 2021-06-17T09:13:58Z

From looking at the code, it's not clear where does it integrate with the UI and how do I download an attachment to my device as a user. Where is the handling of the tap on the attachment to download it?

ekievsky · 2021-06-17T09:16:20Z

so the UI handling is also in this scope, I'll add. Do I need to implement the details of attachment? I mean if it's PDF should I show in separate view that PDF file?

tomholub · 2021-06-17T09:22:12Z

Do I need to implement the details of attachment? I mean if it's PDF should I show in separate view that PDF file?

No, I'd just like to tap the attachment name to download the file, whatever the file is. No need a separate view.

ekievsky · 2021-06-17T09:24:28Z

should I show a toast that notifies user that the file was saved?

tomholub · 2021-06-17T11:09:07Z

should I show a toast that notifies user that the file was saved?

I think yes - for a user, visual feedback is good to have.

Added tests for files manager;

ekievsky · 2021-06-17T13:06:05Z

FlowCryptUI/Nodes/AttachmentNode.swift

+
+    }
+
+    public override func layoutDidFinish() {


@Kharchevskyi here is the fix that I was talking about

Please check next commit with better approach for this. Please confirm this one is works for you

tomholub

code looks good, will test the app now

tomholub · 2021-06-19T14:32:48Z

I'm having trouble building this, please merge changes from master into here and resolve conflicts. Thanks!

tomholub

After I save the attachment, where can I find it?

I have tried to save an image, but can't find it anywhere on my phone. I would expect it to be in Downloads.

Best would be if after I download the attachment, it would pop up a modal that says

Attachment downloaded
[open now]
[close]

Where I could click open and iOS would figure out which app to open it with (like on a desktop).

But that's not strictly necessary, it would be for now enough to at least tell the user where to find their attachment so that they can open it themselves.

tomholub · 2021-06-21T13:10:01Z

FlowCrypt/Resources/en.lproj/Localizable.strings

 "message_compose_secure" = "Compose Secure Message";
 "message_unknown_sender" = "(unknown sender)";
 "message_missed_subject" = "No subject";
+"message_attachment_saved_successfully" = "Attachemt was saved";


ekievsky · 2021-06-23T14:22:53Z

For now files store in Documents folder which iOS creates for every app. In order to save file to Downloads(you may access it using Files native app) we need to use UIDocumentInteractionController, that's the separate view that allows user to save file to Downloads and can be opened using Files native app in future.

tomholub · 2021-06-23T14:55:29Z

For now files store in Documents folder which iOS creates for every app

As a user, how can I open the file after it downloads?

ekievsky · 2021-06-23T14:56:44Z

For now there is no way, I though we will add files reading inside of app in future.

tomholub · 2021-06-23T18:18:06Z

No, this is not useful as is. I want users to be able to actually get the file they received and do with it what they want, like on a desktop. open with another app, transfer to pc, send through whatsapp, whatever. when not sure, have a look at what other email clients do

…

On Wednesday, June 23, 2021, Evgenii Kievsky ***@***.***> wrote: For now there is no way, I though we will add files reading inside of app in future. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#352 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQDZEKXVBZ55RCL55KJW5LTUHY3PANCNFSM46Z4STYA> .

-- -- Tom James Holub <http://holub.me/>

ekievsky · 2021-06-24T09:38:29Z

Other email clients have theirs own readers. But I got your point I'll add this documents reader. Thanks

tomholub · 2021-06-24T10:30:26Z

that sounds like a lot of work. could you estimate the amount of work to add a document reader vs save to downloads? is there a standardised ios document reader to add or is that something we have to implement?

…

On Thursday, June 24, 2021, Evgenii Kievsky ***@***.***> wrote: Other email clients have theirs own readers. But I got your point I'll add this documents reader. Thanks — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#352 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQDZEPP4RJKOBG6AW57AGLTUL4J5ANCNFSM46Z4STYA> .

-- -- Tom James Holub <http://holub.me/>

ekievsky · 2021-06-24T10:32:03Z

It won't take a lot of time, around an hour, it comes from native SDK.

tomholub · 2021-06-24T10:46:22Z

ok - that sounds good. thanks!

…

On Thursday, June 24, 2021, Evgenii Kievsky ***@***.***> wrote: It won't take a lot of time, around an hour, it comes from native SDK. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#352 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABQDZEOJ3ZSSYXGKTKY64CLTUMCS3ANCNFSM46Z4STYA> .

-- -- Tom James Holub <http://holub.me/>

Opened saved files using Files app;

ekievsky · 2021-06-24T13:58:05Z

Had some merging side effects.

So, I added UIDocumentInteractionController. The flow is following:

User taps on download button
App opens native files browser inside of application.

success: user gets the alert asking him if he want to open Files app to open a file or dismiss alert
failure: user gets the toast saying that there was an error while saving a file

tomholub

Much better! There is a bug though:

click download icon on a small text file
when choosing location, I only saw various app folders, but not Downloads folder. So I made a new folder called test folder
click done
get prompted to open the file, said yes
the app took me to Downloads folder. But I saved it elsewhere

I'll merge this, and then you can fix this in a followup PR. Thanks!

Added files manager to save downloaded files to user's phone;

d8c859c

tomholub reviewed Jun 16, 2021

View reviewed changes

Added action to attachment node to save file;

59c2a32

Added tests for files manager;

ekievsky commented Jun 17, 2021

View reviewed changes

Kharchevskyi and others added 3 commits June 17, 2021 17:52

Fix for download attachment tap

d5c38e9

Increased expectation timeout to FilesManager tests

0267bab

Update Localizable.strings

757a80d

tomholub reviewed Jun 19, 2021

View reviewed changes

Merged master

67d16ff

tomholub reviewed Jun 21, 2021

View reviewed changes

tomholub marked this pull request as draft June 21, 2021 13:33

ykyivskyi-gd added 5 commits June 24, 2021 13:59

Merged master

d7e52da

Saving files using Files app;

655f0d5

Opened saved files using Files app;

Merged master;

718e44c

Fixed pbxproj file;

85f7f31

Fixed merging effects

a997a7e

ekievsky marked this pull request as ready for review June 24, 2021 17:27

tomholub approved these changes Jun 24, 2021

View reviewed changes

tomholub merged commit a352197 into master Jun 24, 2021

tomholub deleted the feature/issue-194-ability-to-download-plain-attachment branch June 24, 2021 19:51

tomholub mentioned this pull request Jun 24, 2021

after downloading file, take user to correct folder #370

Closed

Conversation

ekievsky commented Jun 16, 2021 • edited by tomholub Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

close #194

To be filled by reviewers

Uh oh!

tomholub commented Jun 16, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Kharchevskyi commented Jun 16, 2021

Uh oh!

tomholub commented Jun 16, 2021

Uh oh!

tomholub commented Jun 16, 2021

Uh oh!

ekievsky commented Jun 17, 2021

Uh oh!

tomholub commented Jun 17, 2021

Uh oh!

ekievsky commented Jun 17, 2021

Uh oh!

tomholub commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekievsky commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomholub commented Jun 17, 2021

Uh oh!

ekievsky Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

Kharchevskyi Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub commented Jun 19, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

ekievsky commented Jun 23, 2021

Uh oh!

tomholub commented Jun 23, 2021

Uh oh!

ekievsky commented Jun 23, 2021

Uh oh!

tomholub commented Jun 23, 2021 via email

Uh oh!

ekievsky commented Jun 24, 2021

Uh oh!

tomholub commented Jun 24, 2021 via email

Uh oh!

ekievsky commented Jun 24, 2021

Uh oh!

tomholub commented Jun 24, 2021 via email

Uh oh!

ekievsky commented Jun 24, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ekievsky commented Jun 16, 2021 •

edited by tomholub

Loading

tomholub commented Jun 17, 2021 •

edited

Loading

ekievsky commented Jun 17, 2021 •

edited

Loading