Skip to content

issue 404 remove Promises - #985

Merged
tomholub merged 10 commits into
masterfrom
issue-404-remove-promise
Nov 11, 2021
Merged

issue 404 remove Promises#985
tomholub merged 10 commits into
masterfrom
issue-404-remove-promise

Conversation

@tomholub

@tomholubtomholub commented Nov 10, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR removes remaining Promise usage, particularly in IMAP class.

The SMTP/IMAP functionality is likely broken after this change. IMAP/SMTP is currently low-value feature with very few (if any) users using it. I'll file another issue to test and fix it, and after that, we'll write some UI tests for it to ensure it remains stable.

close#404
close#981

Also stabilized opening menu in appium, and used a shorter pass phrase for appium tests so that less time is wasted typing long pass phrase during tests.

Tests:

  • Tests added or updated

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
tomholub commented Nov 10, 2021
View reviewed changes
Comment threadFlowCrypt/Functionality/Mail Provider/Backup Provider/Imap+Backup.swift
Comment threadFlowCrypt/Functionality/Mail Provider/Imap/Imap+retry.swift
Comment threadFlowCrypt/Functionality/Mail Provider/Imap/Imap+retry.swift
Comment threadFlowCrypt/Functionality/Mail Provider/Imap/Imap+retry.swift
Comment on lines +111 to 119
case .connection:
// the connection has dropped, so it's probably ok to not officially "close" it. but maybe there could be a cleaner way to dispose of the connection?
imapSess = nil
smtpSess = nil
// this is a mess, neads a real refactor. use DI
// todo - for now renewing of session disabled, will probably break retries
// setupSession()
// await connectImap(session: imapSessNotNil)
return true

@tomholubtomholubNov 10, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Todo - this is likely completely broken and will need a fix. But not in this PR.

Also, setupSession and connectImap and connectSmpt all need complete refactoring.

Comment threadFlowCrypt/Functionality/Services/GoogleUserService.swift
Comment threadFlowCrypt/Functionality/Services/GoogleUserService.swift Outdated
This was referenced Nov 10, 2021
Open
Open
Open
Open
@tomholub

tomholub commented Nov 10, 2021•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

This removes Promises. @ivan-ushakov could you share feedback? I could have overlooked something.

// don't really care about IMAP functionality, more about the parts that touch Gmail or general functioning of the app.

tomholub
tomholub commented Nov 11, 2021
View reviewed changes

@tomholubtomholub left a comment •
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

app works again now. checking tests
// swift tests are happy, at least locally

@tomholub
tomholub marked this pull request as ready for review November 11, 2021 11:41
@sosnovsky

sosnovsky commented Nov 11, 2021

Copy link
Copy Markdown
Collaborator

Looks massive :)

Screenshot 2021-11-11 at 13 53 08

@tomholub

tomholub commented Nov 11, 2021

Copy link
Copy Markdown
CollaboratorAuthor

got one failing appium test:

[iPhone 13 iOS 15.0 #0-1] Spec: /Users/semaphore/git/flowcrypt-ios/appium/tests/specs/composeEmail/SelectRecipientByName.spec.ts 18:04
[iPhone 13 iOS 15.0 #0-1] Running: iPhone 13 on iOS 15.0 executing /Users/semaphore/git/flowcrypt-ios/appium/FlowCrypt.app 18:04
[iPhone 13 iOS 15.0 #0-1] Session ID: 4da8bec1-0319-446b-b268-383fe7471ee6 18:04
[iPhone 13 iOS 15.0 #0-1] 18:04
[iPhone 13 iOS 15.0 #0-1] COMPOSE EMAIL: 18:04
[iPhone 13 iOS 15.0 #0-1] ✖ user is able to select recipient from contact list using contact name 18:04
[iPhone 13 iOS 15.0 #0-1] 18:04
[iPhone 13 iOS 15.0 #0-1] 1 failing (1m 18.4s) 18:04
[iPhone 13 iOS 15.0 #0-1] 18:04
[iPhone 13 iOS 15.0 #0-1] 1) COMPOSE EMAIL: user is able to select recipient from contact list using contact name 18:04
[iPhone 13 iOS 15.0 #0-1] Error: element ("~Inbox") still not displayed after 15000ms 18:04
[iPhone 13 iOS 15.0 #0-1] Error: element ("~Inbox") still not displayed after 15000ms 18:04
[iPhone 13 iOS 15.0 #0-1] at processTicksAndRejections (internal/process/task_queues.js:97:5) 18:04
[iPhone 13 iOS 15.0 #0-1] at Function.waitAndClick (/Users/semaphore/git/flowcrypt-ios/appium/tests/helpers/ElementHelper.ts:49:17) 18:04
[iPhone 13 iOS 15.0 #0-1] at MenuBarScreen.clickInboxButton (/Users/semaphore/git/flowcrypt-ios/appium/tests/screenobjects/menu-bar.screen.ts:56:23) 18:04
[iPhone 13 iOS 15.0 #0-1] at UserContext.<anonymous> (/Users/semaphore/git/flowcrypt-ios/appium/tests/specs/composeEmail/SelectRecipientByName.spec.ts:42:23)

will check

@tomholubtomholub mentioned this pull request Nov 11, 2021
Closed
sosnovsky
sosnovsky previously approved these changes Nov 11, 2021
View reviewed changes
Comment threadFlowCrypt/Controllers/Inbox/Container/InboxViewContainerController.swift
Comment threadFlowCrypt/Functionality/Mail Provider/Imap/Imap+Other.swift Outdated
Comment threadFlowCrypt/Functionality/Services/GoogleUserService.swift
Comment threadFlowCrypt/Functionality/Services/GoogleUserService.swift Outdated
Comment threadFlowCrypt/Functionality/Services/GoogleUserService.swift Outdated
@tomholub

tomholub commented Nov 11, 2021•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

(small) regression:

  • menu items (folders) now take visibly longer to load
  • it says INBOX instead of Inbox etc (had to adjust appium test for it)

Will file another issue.

This was referenced Nov 11, 2021
Closed
Closed
@tomholub

tomholub commented Nov 11, 2021

Copy link
Copy Markdown
CollaboratorAuthor

@sosnovsky you're good at reviews - the PR isn't small and you found clear ways to improve it. Updated, hopefully appium tests should pass now (they pass locally)

@tomholub
tomholub enabled auto-merge (squash) November 11, 2021 13:46
@sosnovsky

sosnovsky commented Nov 11, 2021

Copy link
Copy Markdown
Collaborator

@sosnovsky you're good at reviews - the PR isn't small and you found clear ways to improve it. Updated, hopefully appium tests should pass now (they pass locally)

Thanks Tom :) It's easier to find possible improvements when there are 50 files with changes than in some small PRs

Also for @MainActor I left comment only in 1 file, but we can create separate issue for it, as there are other places where it can help us to avoid running some functions on wrong thread. For example we can wrap showToast, handleError with @MainActor so we won't need to check if it's on main thread or not.

@tomholubtomholub mentioned this pull request Nov 11, 2021
Closed
tomholub
tomholub commented Nov 11, 2021
View reviewed changes

@tomholubtomholub left a comment

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

.

@tomholub
tomholub merged commit f20d956 into masterNov 11, 2021
@tomholub
tomholub deleted the issue-404-remove-promise branch November 11, 2021 14:34
ivan-ushakov
ivan-ushakov reviewed Nov 11, 2021
View reviewed changes
Comment threadFlowCrypt/Controllers/Msg/MessageActionsHandler.swift
Task {
do {
let path = try await trashFolderProvider.getTrashFolderPath()
DispatchQueue.main.async {

@ivan-ushakovivan-ushakovNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Better to avoid having to different concurrency approaches in one place

@tomholubtomholubNov 11, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Agree - is it the same issue as #997 ?

@ivan-ushakovivan-ushakovNov 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes

@tomholubtomholub mentioned this pull request Nov 12, 2021
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sosnovskysosnovskysosnovsky approved these changes
+1 more reviewer
@ivan-ushakovivan-ushakovivan-ushakov left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MenuBarScreen.clickMenuIcon not always open menu Replace Promise with async/await

4 participants

@tomholub@sosnovsky@ivan-ushakov@flowcrypt-machine-user