save drafts during compose or reply by ekievsky · Pull Request #690 · FlowCrypt/flowcrypt-ios

ekievsky · 2021-10-17T18:23:07Z

This PR is adding feature that saves draft encrypted mail during compose new mail or reply

close#514

Tests(delete all except exactly one):

Difficult to test (explain why). there was not tests to this modules ( gmail service etc ). if we need to cover it with tests I would prefer it as separate issue.

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

Didn't run it yet - comments below.

tomholub · 2021-10-17T18:30:45Z


 private var state: State = .main

+ private weak var timer: Timer?


Let's call it saveDraftTimer

tomholub · 2021-10-17T18:38:04Z

+ }
+ }
+
+ private func resolveQueryForDraftAction(raw: String, threadId: String?, draft: GTLRGmail_Draft?) -> GTLRGmailQuery {


This should be called createQueryForDraftAction ?

tomholub · 2021-10-17T18:41:59Z

Difficult to test (explain why). there was not tests to this modules ( gmail service etc ). if we need to cover it with tests I would prefer it as separate issue.

This should be covered by ui tests, I'll file an issue.

tomholub · 2021-10-17T19:52:56Z

Need to make sure that:

the draft is deleted after the message is sent, so unneeded drafts don't accumulate
I can go to drafts folder, open a draft and be able to continue editing it

I'll mark the PR as a draft until then. Thanks.

ekievsky · 2021-10-18T14:31:47Z

@tomholub I thought that it will be separate task.

I will continue working on this feature then, thank you

tomholub · 2021-10-18T14:45:45Z

We're getting close to start releasing early versions to customer, so we can no longer merge PRs that cause degradations (eg creating drafts that don't also get cleared) - degradation need to be addressed in the same PR going forward.

That's also the reason I couldn't merge half-finished #657

On the other hand, when new functionality is unfinished in a way that doesn't degrade the app, that may be ok to merge.

ekievsky · 2021-10-18T15:03:33Z

I understand. I will focus on this both tasks to finish them as soon as possible.
Thank you for bringing clarity.

ekievsky · 2021-10-22T20:41:43Z

Still working on current issue.
There are a lot of things that should be changed/fixed, so it will take more time:

change inbox to fetch drafts instead of messages
open and fill compose controller with message/draft
getting drafts recipients and other properties
etc

tomholub · 2021-10-22T20:45:56Z

Understood. It's not a small issue. Actually, I think this could be in a separate PR after all:

change inbox to fetch drafts instead of messages
open and fill compose controller with message/draft
getting drafts recipients and other properties

As long as drafts are not getting created and then left undeleted after the message is sent.

(just thought that the user could theoretically finish their draft on the desktop, so it's not useless - please test that scenario)

ekievsky · 2021-10-25T19:20:42Z

@tomholub I have done following:

all described in issue
drafts are saving and deleting if user sent an email
inbox view controller now supports fetching drafts for drafts folder instead of fetching messages list ( there is difference with draft_id that is needed )

not finished ( can be scoped as another task )

partially done: opening compose view controller from drafts list.
it is necessary to make it fully working as feature.

there are some things that I should finish, but if we can, I would like to make it right after this PR is merged as a separate task

PS: now I am getting weird crash after "encryptAndSend", I am trying to figure out if my changes caused that or it is known master branch issue. it is only thing that is blocking me of moving this PR from draft status to open.

tomholub · 2021-10-25T20:18:29Z

Sounds good, I'll start reviewing. Thanks!

tomholub · 2021-10-25T20:55:26Z

PS: now I am getting weird crash after "encryptAndSend", I am trying to figure out if my changes caused that or it is known master branch issue. it is only thing that is blocking me of moving this PR from draft status to open.

Now after #773 the sending has changed a lot, you should pull the changes from master and see if by any chance the crash is resolved. If not, then you'll have to investigate further.

ekievsky · 2021-10-26T19:38:47Z

@tomholub crash is fixed, please review.

tomholub · 2021-10-26T21:02:09Z

I'll review functionality, but have a look at the tests - I can't merge until the tests are fixed.

ekievsky · 2021-10-26T21:36:34Z

@tomholub there is the issue with target architecture, will be resolving this issue

.

tomholub

Code looks good. Haven't tested

tomholub · 2021-10-26T22:45:23Z

+ }
+ }
+
+ func getFraft(with identifier: String) async throws -> GTLRGmail_Draft {


tomholub · 2021-10-27T18:09:33Z

@tomholub there is the issue with target architecture, will be resolving this issue .

First make sure to log in under new account #755

ekievsky · 2021-10-27T20:21:21Z

@tomholub didn't work for me.
also - I am facing the same problem on master branch.

@sosnovsky do you have this problem, can you check please?
also - if not, can you share your Xcode version?

sosnovsky · 2021-10-27T20:33:22Z

@ekievsky tests run well for me after changing Build Configuration to Debug (instead of Release). You can change it by going to Product -> Scheme -> Edit Scheme... menu in Xcode. I'm using the latest 13.1 version

ekievsky · 2021-10-27T20:44:54Z

oh. @sosnovsky
I believe I worked with realease one all the way.
was debug added recently?

also, I am wondering, about Xcode version. It looks like you are always updating to latest one, I think we should commit some specific one and work with it. and migrate to new one together, when we need it.

what do you think about it?

sosnovsky · 2021-10-28T09:15:18Z

@ekievsky I think Xcode version doesn't affect running tests, as it doesn't change our project code, this configuration can be changed on any Xcode version.
Also Semaphore is using the latest Xcode version too and unit tests pass there without any issues.

tomholub · 2021-10-28T09:22:41Z

I also support generally updating to latest xcode except if we run into a particular issue that can be traced to some particular new version of xcode. Then we'll let everybody know that some other version is recommended for the time being. But normally, let's all update - it doesn't seem to break stuff.

ekievsky · 2021-10-28T09:49:30Z

@sosnovsky I am not saying that tests failure are connected to Xcode version. its 2 separate points in my message, but thank you for that tips.

I just had not very pleasant experience when developers in big team having different Xcode version, that's why my proposal was to commit some specific version in general.
if you both with @tomholub thinks that you want always update to latest version, let it be.

sosnovsky · 2021-10-28T10:26:40Z

@ekievsky yeah, I got some issues with different Xcode versions too. But it happened in projects which used storyboards for UI, as some Xcodes make unnecessary code updates even when you just open storyboard file, without changing anything. This lead to some merge conflicts.
But as we're not using storyboards in Flowcrypt there shouldn't be any issues between different Xcode versions.
Just let me know if you'll notice any problems with some specific Xcode version.

tomholub · 2021-10-28T12:31:19Z

I also heard of problems between xcode versions before, and good to know that they may have been related to storyboards. Hopefully that could mean we won't run into any for a while 👍

tomholub · 2021-10-28T12:32:29Z

@ekievsky is the PR ready to potentially merge from your point of view? If yes please mark it as ready.

tomholub

code good

tomholub

As implemented, it makes it impossible to type - every other second the phone gets stuck and I can't continue typing for a few hundred ms.

What is it getting stuck on? Seems like some work being done on main thread that should not be done there.

ekievsky · 2021-10-28T17:51:25Z

hmm, I am not getting this kind of problem @tomholub
is it happened when you just typing email?

tomholub · 2021-10-28T18:03:32Z

hmm, I am not getting this kind of problem @tomholub is it happened when you just typing email?

Yes, I'm just typing the message and the keyboard keeps getting frozen for a bit here and there after every few key presses, making it nearly unusable. It's on a 12mini, iOS 15.0.2, Debug FlowCrypt installed directly from Xcode.

tomholub · 2021-10-28T18:40:23Z

@ivan-ushakov could you see if you can reproduce my issue on this PR?

ekievsky · 2021-10-28T20:24:13Z

@tomholub issue is fixed, it was issue of thread as you mentioned above.
please take a look

…ler with message. deleting draft after sending message

… sent

ekievsky · 2021-10-28T20:28:26Z

@tomholub I am fixing conflicts, but it should not block you to try latest commit

tomholub

code change looks good. will test

tomholub

This fixed it, thanks!

ekievsky requested review from Kharchevskyi and tomholub October 17, 2021 18:23

ekievsky force-pushed the feature/issue-514 branch from fa1ae49 to 653aaa3Compare October 17, 2021 18:28

tomholub reviewed Oct 17, 2021
View reviewed changes

tomholub mentioned this pull request Oct 17, 2021
ui tests for saving drafts #691
Closed

ekievsky force-pushed the feature/issue-514 branch from 653aaa3 to 0935ba9Compare October 17, 2021 18:42

tomholub marked this pull request as draft October 17, 2021 19:53

ekievsky force-pushed the feature/issue-514 branch 2 times, most recently from b16618e to 86b3548Compare October 20, 2021 18:19

ekievsky force-pushed the feature/issue-514 branch from 7919170 to 6870a9bCompare October 25, 2021 17:18

ekievsky force-pushed the feature/issue-514 branch 2 times, most recently from cea9282 to a1726c4Compare October 26, 2021 18:11

tomholub reviewed Oct 26, 2021
View reviewed changes

tomholub reviewed Oct 28, 2021
View reviewed changes

tomholub requested changes Oct 28, 2021
View reviewed changes

ekievsky marked this pull request as ready for review October 28, 2021 17:55

ekievsky mentioned this pull request Oct 28, 2021
pass phrase not remembered when sending email #819
Closed

ekievsky force-pushed the feature/issue-514 branch from 275dac4 to 0979af9Compare October 28, 2021 20:25

ekievsky added 7 commits October 28, 2021 23:26

save drafts during compose or reply
47b5ca4

filling drafts list with pure drafts, opening compose message control…
586ee65
…ler with message. deleting draft after sending message

Draft encrypting. Cleaning code, testing draft deletion after message…
b2a4e08
… sent

cleaning code, hide compose message opening from drafts
418deb8

cleaning code before PR
47418ee

removing async from objc func, fixing crash
7f0b317

changing schema to debug, adding mocks for draftgatrway, fixing tests
29c5d98

fixing queue for timer, removing freeze
47c5de6

ekievsky force-pushed the feature/issue-514 branch from 0979af9 to 47c5de6Compare October 28, 2021 20:31

ekievsky requested a review from tomholub October 28, 2021 20:43

tomholub reviewed Oct 28, 2021
View reviewed changes

tomholub approved these changes Oct 28, 2021
View reviewed changes

tomholub merged commit 5830bb3 into masterOct 28, 2021

tomholub deleted the feature/issue-514 branch October 28, 2021 21:19


		private var state: State = .main

		private weak var timer: Timer?

Uh oh!

Conversation

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

Uh oh!

To be filled by reviewers

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholubOct 17, 2021

Choose a reason for hiding this comment

Uh oh!

tomholubOct 17, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub commented Oct 17, 2021

Uh oh!

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

Uh oh!

Uh oh!

ekievsky commented Oct 18, 2021

Uh oh!

tomholub commented Oct 18, 2021

Uh oh!

ekievsky commented Oct 18, 2021

Uh oh!

ekievsky commented Oct 22, 2021

Uh oh!

tomholub commented Oct 22, 2021

Uh oh!

ekievsky commented Oct 25, 2021

Uh oh!

tomholub commented Oct 25, 2021

Uh oh!

tomholub commented Oct 25, 2021

Uh oh!

ekievsky commented Oct 26, 2021

Uh oh!

tomholub commented Oct 26, 2021

Uh oh!

ekievsky commented Oct 26, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholubOct 26, 2021

Choose a reason for hiding this comment

Uh oh!

tomholub commented Oct 27, 2021

Uh oh!

ekievsky commented Oct 27, 2021

Uh oh!

sosnovsky commented Oct 27, 2021

Uh oh!

ekievsky commented Oct 27, 2021

Uh oh!

sosnovsky commented Oct 28, 2021

Uh oh!

tomholub commented Oct 28, 2021

Uh oh!

ekievsky commented Oct 28, 2021

Uh oh!

sosnovsky commented Oct 28, 2021

Uh oh!

tomholub commented Oct 28, 2021

Uh oh!

tomholub commented Oct 28, 2021

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

tomholub left a comment

Choose a reason for hiding this comment

Uh oh!

ekievsky commented Oct 28, 2021

Uh oh!

tomholub commented Oct 28, 2021

Uh oh!

tomholub commented Oct 28, 2021

Uh oh!

ekievsky commented Oct 17, 2021•
edited by tomholub
Loading

tomholub commented Oct 17, 2021•
edited
Loading