Skip to content

Сhange title of emails in sent folder#1087

Merged
tomholub merged 4 commits intomasterfrom
feature/issue-1015-sent-screen-recipients
Nov 29, 2021
Merged

Сhange title of emails in sent folder#1087
tomholub merged 4 commits intomasterfrom
feature/issue-1015-sent-screen-recipients

Conversation

@ekievsky
Copy link
Contributor

This PR is about applying gmail style title for SENT tab.
if we need same logic for all folders I can apply it to other tabs as well.

close #1015


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

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

@ekievsky ekievsky requested a review from tomholub November 24, 2021 19:42
@tomholub tomholub requested review from sosnovsky and removed request for tomholub November 25, 2021 14:19
@tomholub
Copy link
Collaborator

@sosnovsky please have a look as it may be related to what you're working on

@sosnovsky
Copy link
Collaborator

I tested app but it still doesn't show recipients on Sent screen.

For example, I sent 3 different emails to 3 different recipients - app will show 3 messages with me label on Sent screen, and I won't be able to quickly find who was email recipient.

If I open Gmail app and go to Sent screen then I'll see To: test1@test.com, To: test2@test.com, To: test3@test.com labels for the these messages.

I checked your discussion #1015 (comment) and it seems you decided to do it this way, but it's opposite to original task description. Comment from link above says that Gmail app shows me label for sent messages, but on my phone Gmail app shows To: *recipient* - maybe it depends on some Gmail setting (as default reply/reply all behavior)

@tomholub do updated Sent screen labels look good for you? As for me, To: *recipient* is better choice and it's the same as Gmail app.

@FlowCrypt FlowCrypt deleted a comment from github-actions bot Nov 25, 2021
@tomholub
Copy link
Collaborator

@sosnovsky my bad. It should be done the same or similar like Gmail app, which means I was wrong in the linked discussion.

Does it make sense to adjust in this PR, or does it make sense to redo this when doing #921 ? It's related.

@tomholub tomholub marked this pull request as draft November 25, 2021 15:31
@sosnovsky
Copy link
Collaborator

I think it'll be better to adjust labels here, as @ekievsky already implemented most part of this functionality, and then I'll be able to reuse his code in #921

@tomholub
Copy link
Collaborator

tomholub commented Nov 25, 2021

ok. I marked it as draft. @ekievsky please follow Gmail functionality here as Roma suggests, it sounds better than what we discussed earlier.

@ekievsky
Copy link
Contributor Author

I will update PR to your requirements @sosnovsky

@sosnovsky
Copy link
Collaborator

I will update PR to your requirements @sosnovsky

Thanks!

@ekievsky ekievsky force-pushed the feature/issue-1015-sent-screen-recipients branch 2 times, most recently from 3617562 to 9afd7a6 Compare November 25, 2021 21:08
Copy link
Collaborator

@sosnovsky sosnovsky left a comment

Choose a reason for hiding this comment

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

Works well now, just some code comments

self.wrappedType = .thread(thread)
}

@MainActor func hideErrorLabel() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it needed?

let identifier: Identifier
let date: Date
let sender: String?
let receiver: String?
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency it'll be better to rename receiver to recipient, as we already have recipient in other parts of the app

emails.remove(at: i)
}
}
let receivers = emails
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be recipients


// for now its not exactly clear how titles on other folders should looks like
// so in scope of this PR we are applying this title presentation only for "sent" folder
if folderPath == sentPath {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can use MessageLabelType.sent.value here instead of just string SENT, as we already have all message labels/folders stored in MessageLabelType.

}
}
let receivers = emails
.compactMap { $0.components(separatedBy: "@").first ?? "" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

?? "" isn't needed here, as compactMap will automatically delete all nil values

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not only not needed, but actively harmful - now we have to also handle empty strings, instead of only nils which get filetered with compactMap.

} else {
return thread.messages
.compactMap(\.sender)
.compactMap { $0.components(separatedBy: "@").first ?? "" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

?? "" isn't needed too

@ekievsky ekievsky force-pushed the feature/issue-1015-sent-screen-recipients branch from 9afd7a6 to d43ae16 Compare November 25, 2021 21:38
@ekievsky ekievsky marked this pull request as ready for review November 25, 2021 21:39
@ekievsky
Copy link
Contributor Author

@sosnovsky please re-review

Copy link
Collaborator

@sosnovsky sosnovsky 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 for me now 👍

@tomholub
Copy link
Collaborator

@ekievsky when you address a PR comment, please click "Resolve conversation" on each of the comments you believe you fixed. That way, it's easier to see at a glance what have you done so far and what is still left to do. Thanks!

@tomholub tomholub enabled auto-merge (squash) November 26, 2021 11:57
@tomholub
Copy link
Collaborator

@ekievsky there's one failed test (at least)

[0-5] RUNNING in /Users/semaphore/git/flowcrypt-ios/appium/FlowCrypt.app - /tests/specs/inbox/CheckReplyAndForwardForEncryptedEmail.spec.ts 17:50
[0-5] Screenshot of failed test was saved to /Users/semaphore/git/flowcrypt-ios/appium/tmp 17:50
[0-5] Error in "INBOX:  user is able to reply or forward email and check info from composed email" 17:50
Error: 17:50
    at <Jasmine> 17:50
    at NewMessageScreen.checkFilledComposeEmailInfo (/Users/semaphore/git/flowcrypt-ios/appium/tests/screenobjects/new-message.screen.ts:92:41) 17:50
    at UserContext.<anonymous> (/Users/semaphore/git/flowcrypt-ios/appium/tests/specs/inbox/CheckReplyAndForwardForEncryptedEmail.spec.ts:31:28) 17:50
    at processTicksAndRejections (internal/process/task_queues.js:97:5) 17:57
[0-5] RETRYING in /Users/semaphore/git/flowcrypt-ios/appium/FlowCrypt.app - /tests/specs/inbox/CheckReplyAndForwardForEncryptedEmail.spec.ts 18:05
[0-5] RUNNING in /Users/semaphore/git/flowcrypt-ios/appium/FlowCrypt.app - /tests/specs/inbox/CheckReplyAndForwardForEncryptedEmail.spec.ts 19:57
[0-5] Screenshot of failed test was saved to /Users/semaphore/git/flowcrypt-ios/appium/tmp 19:57
[0-5] Error in "INBOX:  user is able to reply or forward email and check info from composed email" 19:57
Error: 19:57
    at <Jasmine> 19:57
    at NewMessageScreen.checkFilledComposeEmailInfo (/Users/semaphore/git/flowcrypt-ios/appium/tests/screenobjects/new-message.screen.ts:92:41) 19:57
    at UserContext.<anonymous> (/Users/semaphore/git/flowcrypt-ios/appium/tests/specs/inbox/CheckReplyAndForwardForEncryptedEmail.spec.ts:31:28) 19:57
    at processTicksAndRejections (internal/process/task_queues.js:97:5) 20:04
[0-5] FAILED in /Users/semaphore/git/flowcrypt-ios/appium/FlowCrypt.app - /tests/specs/inbox/CheckReplyAndForwardForEncryptedEmail.spec.ts (1 retries)

please try to run it locally and debug it

@tomholub tomholub merged commit 810b589 into master Nov 29, 2021
@tomholub tomholub deleted the feature/issue-1015-sent-screen-recipients branch November 29, 2021 10:43
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.

"Sent" screen should show recipients

3 participants