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

[DDW-595] Warn user when pasting an address of the same wallet #2506

Conversation

DeeJayElly
Copy link
Contributor

@DeeJayElly DeeJayElly commented Apr 5, 2021

This PR adds warning message as tooltip to the user when pasting an address of the same wallet in the send form.

POTENTIALLY BREAKING CHANGES 💥

This PR includes an improved Input component in react-polymorph to make it more configurable and support use cases like in this PR: input-output-hk/react-polymorph#173

Input and FormField components from now on support:

  • Showing form field error on hover
  • Showing form field error on focus
  • Styling the form field errors differently via injected css variables

The breaking change is that ALL form fields in the whole application now show their errors on focus/hover by default - this needs to be checked carefully before merging this PR.

TODO ✅

  • Update copy for tooltip + translations

Screenshots

Screen Shot 2021-04-04 at 11 38 13 PM

Screen Shot 2021-04-04 at 11 38 35 PM


Testing Checklist 👀

  • Slack QA thread

  • ❗we need to test out all form fields across the whole app, so we could be sure that nothing is broken.


Test Scenarios

Scenario 1 - Use Address from Wallet A on Wallet A

Given that Daedalus is synced
And I have synced at least one Shelley wallet with funds
And I am on the "Send" screen from Wallet A
When I paste an address from Wallet A in the "Receiver" input
And I add a valid amount on the "Ada" input
Then I can see the "Send" button is enabled
And the "Receiver" input is outlined in orange
And the following alert is displayed on a tooltip

ENGLISH: This address belongs to your [Wallet name] wallet. No assets will be transferred, but fee will be charged.
JAPANESE: このアドレスはあなたのものです[Wallet name]財布。資産は譲渡されませんが、手数料がかかります

Scenario 2 - Use Address from Wallet B on Wallet A

Given that Daedalus is synced
And I have synced at least two Shelley wallet with funds
And I am on the "Send" screen from Wallet A
When I paste an address from Wallet B in the "Receiver" input
And I add a valid amount on the "Ada" input
Then I can see the "Send" button is enabled
And the "Receiver" input shows no alerts

Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

There is a UX issue:
The warning briefly shows up but disappears after a few seconds (I guess this has something to do with the wallets data refresh every 5 seconds - maybe this is overriding the boolean flag again?).

The length of how long it is shown varies (again, probably because of the wallets data refresh interval kicking in at different times)

CleanShot.2021-04-05.at.11.28.56.mp4

source/renderer/app/components/wallet/WalletSendForm.js Outdated Show resolved Hide resolved
@gabriela-ponce gabriela-ponce self-requested a review April 5, 2021 12:47
@gabriela-ponce
Copy link

@aleksandardjordjeviciohk I agree with @DominikGuzei. The outline on the field sometimes is displayed for a split second and you can't see the tooltip because it hides very quickly. Maybe it can be displayed all the time as the red outline when the address is incorrect? This way, every time the user hovers the field, they should be able to see the tooltip. Check this video.

@gabriela-ponce
Copy link

@aleksandardjordjeviciohk The previous issue is fixed, nice work 👍

I found other issues in this instance:

  • When you paste an address from the same wallet and then change the value to make it not valid, the yellow outline and tooltip are still displayed. Check this video.

  • Also, if you clear the field by clicking on "x", you'll see the same message after inserting any value. Check this video.

@DeeJayElly
Copy link
Contributor Author

@aleksandardjordjeviciohk The previous issue is fixed, nice work 👍

I found other issues in this instance:

  • When you paste an address from the same wallet and then change the value to make it not valid, the yellow outline and tooltip are still displayed. Check this video.
  • Also, if you clear the field by clicking on "x", you'll see the same message after inserting any value. Check this video.

These issues are fixed, pls recheck @gabriela-ponce

@DeeJayElly DeeJayElly removed the WIP label Apr 7, 2021
@gabriela-ponce
Copy link

@aleksandardjordjeviciohk The fixes look good on the build 17524.
There is a small issue now with the "please insert a valid address" tooltip. In production, this tooltip is displayed if you're writing on the input or when it is in focus. On build 17524, you can't see the tooltip until you click on the field (even if it's in focus). Check videos 1 and 2.

…s for receiver input field focus with tooltip
…asting-an-address-of-the-same-wallet' into feature/ddw-595-warn-user-when-pasting-an-address-of-the-same-wallet
@DeeJayElly
Copy link
Contributor Author

@aleksandardjordjeviciohk The fixes look good on the build 17524.
There is a small issue now with the "please insert a valid address" tooltip. In production, this tooltip is displayed if you're writing on the input or when it is in focus. On build 17524, you can't see the tooltip until you click on the field (even if it's in focus). Check videos 1 and 2.

@gabriela-ponce @DominikGuzei can you guys review again

@DominikGuzei
Copy link
Member

@aleksandardjordjeviciohk the yellow warning does not show (only the borders are yellow) when pasting in an address of the same wallet:

CleanShot.2021-04-08.at.20.19.40.mp4

I guess that's caused by the instant switch to the ADA field … but I would expect to see the warning test immediately and not switch to the ADA field right away (since the user probably doesn't want to finish this operation anyway)

@gabriela-ponce
Copy link

@aleksandardjordjeviciohk The issue on this comment is fixed, nice job 👍

There is a small issue now with the "please insert a valid address" tooltip. In production, this tooltip is displayed if you're writing on the input or when it is in focus. On build 17524, you can't see the tooltip until you click on the field (even if it's in focus). Check videos 1 and 2.

I could replicate the issue @DominikGuzei mentioned in the previous comment. I agree that the tooltip should be displayed in that scenario.

package.json Outdated Show resolved Hide resolved
…asting-an-address-of-the-same-wallet' into feature/ddw-595-warn-user-when-pasting-an-address-of-the-same-wallet
@gabriela-ponce
Copy link

@aleksandardjordjeviciohk I checked the behavior for all inputs and most of them are ok, but found a minor inconsistency:

  • For the "Smash server URL" the tooltip is shown only on focus at first. Then, if you insert an incorrect URL and submit, all the tooltips would be displayed, even if the input is not in focus or hovered. Check this video.

Also, I have a few blockers to complete the testing of this task:

  • On build 17794, there are no installers for macOS.
  • Voting is not enabled on Testnet to check the corresponding inputs.

@nikolaglumac
Copy link
Contributor

On build 17794, there are no installers for macOS.

They will show up eventually. Please stand by 🙏

Voting is not enabled on Testnet to check the corresponding inputs.

You can skip that part as it is not easy to enable it at the moment...

@DominikGuzei
Copy link
Member

@input-output-hk/daedalus-review I updated this PR to point to the 1.0.0-rc.1 release candidate 🎉

@nikolaglumac
Copy link
Contributor

For the "Smash server URL" the tooltip is shown only on focus at first. Then, if you insert an incorrect URL and submit, all the tooltips would be displayed, even if the input is not in focus or hovered. Check this video.

@gabriela-ponce this is the same behavior as in production or develop builds. The input remains focused (even if you click outside of it) after you submit an incorrect URL.

Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

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

Works good!

@nikolaglumac nikolaglumac merged commit e103a2f into develop Apr 29, 2021
@iohk-bors iohk-bors bot deleted the feature/ddw-595-warn-user-when-pasting-an-address-of-the-same-wallet branch April 29, 2021 07:27
@nikolaglumac nikolaglumac added release-4.1.0 Daedalus Mainnet release-4.1.0-FC1 Daedalus Flight and removed ✈️ release-4.1.0-FC1 labels Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature release-4.1.0-FC1 Daedalus Flight release-4.1.0 Daedalus Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants