Fix the 'Reply' button behavior, align it with Gmail by limonte · Pull Request #4107 · FlowCrypt/flowcrypt-browser

limonte · 2021-11-09T22:28:57Z

This PR aligns the behavior of the 'Reply' button with Gmail:

if from !== acctEmail => reply to the from email address only
if from === acctEmail then reply to all to email addresses (excluding cc and bcc)

Also, it fixes the reply icon button which is injected in Gmail UI, so it now acts the same way as the Secure Reply button.

close #4077

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

rrrooommmaaa

There are some strangeness in behaviour, I would like to hear your comments.
Also, it would be more straightforward to name properties myEmail and from instead of from and orifinalFrom. Please see the suggested update #4111

rrrooommmaaa · 2021-11-11T06:55:57Z

test/source/tests/compose.ts

+      const appendUrl = 'threadId=17d02296bccd4c5c&skipClickPrompt=___cu_false___&ignoreDraft=___cu_false___&replyMsgId=17d02296bccd4c5c';
+      const composePage = await ComposePageRecipe.openStandalone(t, browser, 'compatibility', { appendUrl, hasReplyPrompt: true });
+      await composePage.waitAndClick('@action-accept-reply-all-prompt', { delay: 1 });
+      await expectRecipientElements(composePage, { to: ['vladimir@flowcrypt.com'], cc: ['limon.monte@gmail.com'], bcc: ['sweetalert2@gmail.com'] });


Strange that flowcrypt.compatibility@gmail.com is not included in to with "reply all", whereas it was included with "reply". Is there a purpose of such behaviour?

In this PR I was focused on fixing the "Reply" button functionality, but you're correct here. There should be flowcrypt.compatibility@gmail.com when the user clicks "reply all":

I will fix this in scope of this PR 👍

rrrooommmaaa · 2021-11-11T07:01:13Z

test/source/tests/compose.ts

+      const appendUrl = 'threadId=17d02268f01c7e40&skipClickPrompt=___cu_false___&ignoreDraft=___cu_false___&replyMsgId=17d02268f01c7e40';
+      const composePage = await ComposePageRecipe.openStandalone(t, browser, 'compatibility', { appendUrl, hasReplyPrompt: true });
+      await composePage.waitAndClick('@action-accept-reply-all-prompt', { delay: 1 });
+      await expectRecipientElements(composePage, { to: ['limon.monte@gmail.com', 'vladimir@flowcrypt.com'], cc: ['limon.monte@gmail.com'], bcc: [] });


do we really need limon.monte@gmail.com twice?
However, if this mimicks gmail behaviour, then it's ok.

Yup, this is aligned with Gmail:

rrrooommmaaa · 2021-11-11T07:02:07Z

test/source/tests/compose.ts

+      const appendUrl = 'threadId=17d02296bccd4c5c&skipClickPrompt=___cu_false___&ignoreDraft=___cu_false___&replyMsgId=17d02296bccd4c5c';
+      const composePage = await ComposePageRecipe.openStandalone(t, browser, 'compatibility', { appendUrl, hasReplyPrompt: true });
+      await composePage.waitAndClick('@encrypted-reply', { delay: 1 });
+      await expectRecipientElements(composePage, { to: ['flowcrypt.compatibility@gmail.com', 'vladimir@flowcrypt.com'], cc: [], bcc: [] });


Strange that flowcrypt.compatibility@gmail.com is included in to with "reply", whereas it's not included with "reply all". Is there a purpose of such behaviour?

My goal here was to align the behavior with Gmail so there will be no surprises for our users.

In this particular case flowcrypt.compatibility@gmail.com sends the message

to

flowcrypt.compatibility@gmail.com (itself, from === acctEmail)

vladimir@flowcrypt.com

cc

limon.monte@gmail.com

bcc

sweetalert2@gmail.com

Here's the Gmail's behavior which is aligned with the test's expectations:

You can test it by yourself by logging into flowcrypt.compatibility@gmail.com and opening this thread https://mail.google.com/mail/u/4/#inbox/WhctKKXHCWWrrFvSmnhdCjDHBtHBbzrqmFPQWKBVqftJdVQFxGtTTPDWZrvZKPXlJpckmNL

* Fix the 'Reply' button behavior, align it with Gmail * copy before iterating * Add tests * add clearRecipientsForReply() to initComposeBox() * Revert * fix the reply button behavior * await promise * Add live gmail test for the reply icon button * simpler namings (#4111) * align 'reply all' behavior with Gmail Co-authored-by: Roman <rrrooommmaaa@mail.ru>

…4109) * test cancelling passphrase dialog in compose * Change the 'keyup' events handlers to 'input' because text content can be changed with the mouse (#4100) * Change the 'keyup' handlers to 'input' because text content can be changed with the mouse * trigger 'input' events * Upgrade to Puppeteer 11 (#4093) * Upgrade to Puppeteer 11 * unsuccessful attempt to refactor createSecureDraft, add todo for the future * Refactor pageHasSecureDraft() to use getFrame() instead of opening new tab * use getFrame() in Thunderbird tests * workaround sending in the 'secure reply btn, reply draft' test * Simplify openGmailPage() * fail faster - add timeout param to waitAndClick() * wip * handle 'Node is either not clickable' error * wait longer for @action-step0 and @action-step1 * wait longer for @input-compatibility-fix-expire-years * wip * always delete local draft after sending * cleanup * rename mock live test * log * fix 'secure reply btn, reply draft' test * cleanup * do not rely on sleep timeouts * typo * timeout in seconds * let composeBox: Controllable | undefined * #4052 passphrase dialog for non-primary S/MIME signing * Do not show post-it reminder for EKM (#4103) * Do not show post-it reminder for EKM * upd tests * enterPp.expectPostitNoteReminder * Remove expectPostitNoteReminder and related check * Fix the 'Reply' button behavior, align it with Gmail (#4107) * Fix the 'Reply' button behavior, align it with Gmail * copy before iterating * Add tests * add clearRecipientsForReply() to initComposeBox() * Revert * fix the reply button behavior * await promise * Add live gmail test for the reply icon button * simpler namings (#4111) * align 'reply all' behavior with Gmail Co-authored-by: Roman <rrrooommmaaa@mail.ru> * issue #3885 add checkbox per email for attester key submission (#3907) * add checkbox per email for attester key submission * fixes tslint error * fixes tslint error * add xss-escaped comment to pass pattern checks * Added test for issue-3885 selectable email aliases to submit on attester * Added a private key with two UIDs * refactor data-test naming and add multi email alias account to google-endpoint.ts * fix tslint error * fix failing test on setup-page-recipe * Added test for importing key with multiple email alias (incomplete) * fixes tslint formatting error * complete test for importing key with multiple email alias * rename data-test and class container * rename data-test and class container * separate the test to CONSUMER-MOCK test variant * move the render display function to key-import-ui.ts * emails for checkboxes are default to 'unchecked' state * remove accidental console.log * Added automatic check/uncheck when an email is present. * fixes tslint by adding interface property * add event of keyup paste and change to manipulate checkboxes * change button color from gray to green when valid private key * remove checkEmailAliasIfPresent and uses fillOnly * use fillOnly completely * bring back checkEmailAliasIfPresent and wrap it in fillOnly * rename css class name to avoid interfering with any className based checking. * added key with multiple aliases * simplified code and move data-test to label input * corrected any type to string * remove spagetti code and better checking for submitkeyforaddrs * exclude email (uncheck checkbox) before submitting * attester pubkey for multi alias user (failing) * added test "setup - imported key from a file with multiple alias" * added test if excluded email was submitted from the attester * collect submitted keys from attester * patch data-test (selector) trasnformer to match/replace any provided selector * move data-test directly to the input * fix inconsistency in checking detected email alias * manipulate test key and added 1 UID * complete neccessary tests by checking default detected key states * refactor email alias process [floating-promise-error] in constructor setup.ts (key-import-ui initPrvImportSrcForm) * remove comment * parse aliases via already rendered input checkbox * patched 'saveKeysAndPassPhrase' on setup.ts * fix conflict * fix conflict * fixes eslint * added callback when performing tests * remove unnecessary undefined initialization * fix pubkey definition * uncheck the checkbox when submit pubkey was set to false * better flow for pubkey submission * check for checkbox state in the first run * fix test title typo * reverted back changes [proposed solution] * clean up setup procedure Co-authored-by: Roman Shevchenko <rrrooommmaaa@mail.ru> Co-authored-by: Mart Gil Robles <mart@Marts-MacBook-Air.local> Co-authored-by: Tom J <6306961+tomholub@users.noreply.github.com> Co-authored-by: Tom <tom@flowcrypt.com> Co-authored-by: Tom J <tom@holub.me> * issue #4097 add warning when manually importing public keys (#4110) * issue #4097 add warning when manually importing public keys * Improve wording, make import buttons orange * more flexible google mock * allow opening a draft compose box based from inbox.ts in debug mode * fix * added filePath option for addKeyTest * test loading draft with a forgotten non-primary passphrase * merge fix * merge fix * test fix * remark Co-authored-by: Limon Monte <limon.monte@gmail.com> Co-authored-by: martgil <46025304+martgil@users.noreply.github.com> Co-authored-by: Mart Gil Robles <mart@Marts-MacBook-Air.local> Co-authored-by: Tom J <6306961+tomholub@users.noreply.github.com> Co-authored-by: Tom <tom@flowcrypt.com> Co-authored-by: Tom J <tom@holub.me>

Limon Monte added 7 commits November 9, 2021 23:13

Fix the 'Reply' button behavior, align it with Gmail

e61e170

copy before iterating

43f92ce

Add tests

e1ce0b4

add clearRecipientsForReply() to initComposeBox()

df6a27d

Revert

3b27ba9

fix the reply button behavior

28af7ff

await promise

77d78fb

limonte requested a review from rrrooommmaaa as a code owner November 9, 2021 22:28

limonte mentioned this pull request Nov 9, 2021

[WIP] Fix the 'Reply' button behavior, align it with Gmail #4101

Closed

5 tasks

Add live gmail test for the reply icon button

c4e9b8b

limonte force-pushed the issue-4077-reply-button-run-gmail-tests branch from aa510b7 to c4e9b8b Compare November 9, 2021 23:17

limonte marked this pull request as draft November 9, 2021 23:37

limonte marked this pull request as ready for review November 9, 2021 23:53

rrrooommmaaa mentioned this pull request Nov 11, 2021

simpler namings #4111

Merged

5 tasks

rrrooommmaaa suggested changes Nov 11, 2021

View reviewed changes

rrrooommmaaa and others added 2 commits November 11, 2021 11:02

simpler namings (#4111)

2a5a4f1

align 'reply all' behavior with Gmail

e5592d1

rrrooommmaaa approved these changes Nov 11, 2021

View reviewed changes

rrrooommmaaa merged commit fc8db5b into master Nov 11, 2021

rrrooommmaaa deleted the issue-4077-reply-button-run-gmail-tests branch November 11, 2021 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the 'Reply' button behavior, align it with Gmail#4107

Fix the 'Reply' button behavior, align it with Gmail#4107
rrrooommmaaa merged 10 commits intomasterfrom
issue-4077-reply-button-run-gmail-tests

limonte commented Nov 9, 2021

Uh oh!

rrrooommmaaa left a comment •

edited

Loading

Uh oh!

rrrooommmaaa Nov 11, 2021

Uh oh!

limonte Nov 11, 2021

Uh oh!

limonte Nov 11, 2021

Uh oh!

rrrooommmaaa Nov 11, 2021

Uh oh!

limonte Nov 11, 2021

Uh oh!

rrrooommmaaa Nov 11, 2021

Uh oh!

limonte Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

limonte commented Nov 9, 2021

To be filled by reviewers

Uh oh!

rrrooommmaaa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

limonte Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

limonte Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

limonte Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

limonte Nov 11, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rrrooommmaaa left a comment •

edited

Loading