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

#5928 Fix secure reply-all issues in Gmail context menu #5934

Merged
merged 14 commits into from
Feb 18, 2025

Conversation

martgil
Copy link
Collaborator

@martgil martgil commented Feb 12, 2025

This PR Fixes FlowCrypt secure reply invocation wrongly defaults to "reply" when accessed in Gmail's context menu.

close #5928


Tests (delete all except exactly one):

  • 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

@martgil martgil requested a review from sosnovsky as a code owner February 12, 2025 07:38
@martgil martgil marked this pull request as draft February 12, 2025 07:39
@martgil martgil changed the title #5928 Fix secure reply issues #5928 Fix secure reply-all issues Feb 12, 2025
@martgil martgil changed the title #5928 Fix secure reply-all issues #5928 Fix secure reply-all issues in Gmail context menu Feb 12, 2025
@sosnovsky
Copy link
Collaborator

Hi @martgil, in this PR the only thing left is to fix failing tests or some additional code fixes are needed too?

@martgil
Copy link
Collaborator Author

martgil commented Feb 16, 2025

Hello @sosnovsky its all good but the tests keep failing. ill rerun the tests when i got home.

@sosnovsky
Copy link
Collaborator

Hi @martgil, tests not pass because of failing Keep original reply message when switching to secure mode live test - maybe some changes affected it?

@martgil
Copy link
Collaborator Author

martgil commented Feb 18, 2025

Sometimes, the test Keep original reply message when switching to secure mode passes when I monitored which tests fails. If it continues to occur, I'll definitely need to fix that.

@martgil
Copy link
Collaborator Author

martgil commented Feb 18, 2025

I can now confirm that Keep original reply message when switching to secure mode is not affected by my changes but is failing too often. I'll check other tests that are often fail and see if there any workaround for them.

@sosnovsky
Copy link
Collaborator

It seems Keep original reply message when switching to secure mode fails most often, let's mark it as test.skip for now and fix in another PR, as tested functionality works well, looks like some implementation issue there.

@martgil
Copy link
Collaborator Author

martgil commented Feb 18, 2025

@sosnovsky - The test just started to work fine in the current test. The other test that fails so often is passed test, finalize for mail.google.com - secure reply btn, reply draft but not it passes

I'll let it finish and if not, then I'll create the non-live test PR for it to be merged.

@martgil
Copy link
Collaborator Author

martgil commented Feb 18, 2025

@sosnovsky - would you like to enable auto-merge on this PR?

@sosnovsky
Copy link
Collaborator

Yes, it seems to work well and fix all reported issues, can be merged once tests are passing.

@martgil
Copy link
Collaborator Author

martgil commented Feb 18, 2025

Let's move on with the new PR @sosnovsky - tests are failing quite too often? Or maybe, a non-live test PR will do?

@sosnovsky
Copy link
Collaborator

Let's move on with the new PR @sosnovsky - tests are failing quite too often?

I think it mostly happens with live tests, as mock tests usually pass without fails

@martgil martgil marked this pull request as ready for review February 18, 2025 12:08
@martgil
Copy link
Collaborator Author

martgil commented Feb 18, 2025

@sosnovsky This one is ready for a review. Thanks!

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.

All good, thanks!

@sosnovsky sosnovsky merged commit ac524c5 into master Feb 18, 2025
12 checks passed
@sosnovsky sosnovsky deleted the issue-5928-fix-secure-reply-invocation-live-test branch February 18, 2025 13:07
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.

Secure Reply to All context menu issues
2 participants