Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1221 UI for message passwords #1249

Merged
merged 25 commits into from
Dec 27, 2021
Merged

Conversation

sosnovsky
Copy link
Collaborator

@sosnovsky sosnovsky commented Dec 23, 2021

This PR adds UI for setting message passwords

close #1221


Tests (delete all except exactly one):

  • Tests added or updated - updated SendEmailToRecipientWithoutPublicKey test with new password functionality

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
Copy link
Collaborator Author

@tomholub password UI is ready, but I think we should merge it into the master only after finishing other tasks from #955, as currently it's only UI without functionality.

@tomholub
Copy link
Collaborator

tomholub commented Dec 23, 2021

@tomholub password UI is ready, but I think we should merge it into the master only after finishing other tasks from #955, as currently it's only UI without functionality.

We can merge it, this will take care of it:

let domainsWithPasswordSupport = ["flowcrypt.com"]

There are no customers on flowcrypt.com domain 😃 so we can safely merge it without affecting anyone.

// edit - I just saw that the code decides on domain of the recipient, but we should be deciding based on domain of sender. So that the functionality is only available to our team until finalized. It will take more than one PR, and I'd rather merge them gradually instead of waiting.

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.

Didn't run app yet - please see code comments.

@sosnovsky sosnovsky marked this pull request as ready for review December 24, 2021 15:22
@tomholub
Copy link
Collaborator

Please make sure to also resolve the hidden conversations (github hides them by default when too many)

image

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.

Almost perfect, except:

  • setting empty string on this modal should not be allowed. I could use cancel for that. So either I enter an actual string, or it should tell me to enter one. Soon there will also be more complex evaluation of password strength, so some way to indicate the input is invalid is needed anyway.

image

  • crash when trying to remove recipient

image

@sosnovsky
Copy link
Collaborator Author

  • crash when trying to remove recipient

@tomholub are you able to reproduce it with the latest code? As recipients deletion works well for me - no crash occurred

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.

Changes look good. Will retry the deletion

@tomholub
Copy link
Collaborator

Still reproduce, it's actually when tapping the recipient. This is on simulator.

  1. start typing email of someone who doesn't have pubkey in recipient field. Choose suggestion from offered contacts.
  2. now the contact is gray and it prompts me to add password
  3. I tap the gray recipient bubble
  4. crash:

image

@sosnovsky sosnovsky requested a review from tomholub December 27, 2021 13:07
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.

This fixed the issue 👍

@tomholub tomholub merged commit 77de981 into master Dec 27, 2021
@tomholub tomholub deleted the feature/issue-1221-message-password branch December 27, 2021 20:04
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.

UI for password encrypted message functionality
2 participants