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

Fix error on clicking "Send" on an empty transfer #1037

Merged
merged 3 commits into from Oct 13, 2022

Conversation

csillag
Copy link
Contributor

@csillag csillag commented Sep 27, 2022

This avoids the crash on an empty amount.

Fixes #1031

@github-actions
Copy link

github-actions bot commented Sep 27, 2022

MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 5 0 0.02s
✅ GIT git_diff yes no 0.01s
✅ JSON eslint-plugin-jsonc 1 0 0 1.24s
✅ JSON jsonlint 1 0 0.32s
✅ JSON prettier 1 0 0 0.74s
✅ JSON v8r 1 0 1.04s
✅ TSX eslint 2 0 0 6.1s
✅ TYPESCRIPT eslint 1 0 0 4.54s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

@csillag csillag force-pushed the csillag/dont-choke-on-empty branch 3 times, most recently from 1df9d9b to 1f2a7a9 Compare September 27, 2022 17:30
@buberdds
Copy link
Contributor

I am not exactly sure what are expectations here. Do we really don't want to catch an error when a function input is invalid? Why we even trigger a submit event when a form is empty?

@csillag
Copy link
Contributor Author

csillag commented Sep 28, 2022

I am not exactly sure what are expectations here. Do we really don't want to catch an error when a function input is invalid? Why we even trigger a submit event when a form is empty?

The form has been implemented in such a way that when nothing is entered for the amount, it displays a "0". So it's not an invalid value, but an implicit 0.

I asked about it, and a 0 seems to be a valid value.

So I came to the conclusion that

  • an empty value should mean 0
  • 0 is a valid value

That's why I modified the conversion function so that we interpret "" as 0.
The other option would have been to implement this conversion at the form level, but I wanted to catch the biggest possible group of errors (ie. we might be using the same conversion function in other forms in the future), hence the implementation.

Copy link
Member

@lukaw3d lukaw3d left a comment

Choose a reason for hiding this comment

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

Looks like onSubmit needs to be moved from <Button type="submit" label={t('account.sendTransaction.send', 'Send')} onClick={onSubmit} to <Form onSubmit={onSubmit}

src/app/lib/helpers.ts Outdated Show resolved Hide resolved
@csillag
Copy link
Contributor Author

csillag commented Sep 29, 2022

Looks like onSubmit needs to be moved from <Button type="submit" label={t('account.sendTransaction.send', 'Send')} onClick={onSubmit} to <Form onSubmit={onSubmit}

OK, but then we have to adjust the fields a little bit, in order to keep the ability to submit the form without touching the amount field. (I guess we want that, since we had a "0" placeholder on the amount field. ) Done that.

@csillag csillag force-pushed the csillag/dont-choke-on-empty branch 2 times, most recently from 1d0eb1f to 7fc0b3f Compare September 29, 2022 11:10
@csillag csillag changed the title When amount is an empty string, interpret it as 0 Set the initial value of the amount field to 0 Sep 29, 2022
@csillag csillag force-pushed the csillag/dont-choke-on-empty branch 3 times, most recently from 4df17ea to d39050a Compare September 29, 2022 12:23
@csillag csillag force-pushed the csillag/dont-choke-on-empty branch 3 times, most recently from 78c48bf to bcfe46f Compare September 30, 2022 06:24
@lukaw3d lukaw3d changed the title Set the initial value of the amount field to 0 Fix error on clicking "Send" on an empty transfer Oct 5, 2022
@csillag csillag force-pushed the csillag/dont-choke-on-empty branch 2 times, most recently from 8ab0310 to ee04d7f Compare October 13, 2022 11:20
@csillag
Copy link
Contributor Author

csillag commented Oct 13, 2022

Meanwhile, part of this has already been covered by #1070.

What remains here:

  • Jest test for the error condition
  • Cypress test for the error condition
  • Better placeholder value for the send form (to make it obvious that an amount must be entered.)

@csillag
Copy link
Contributor Author

csillag commented Oct 13, 2022

Got approval from Don @donouwens about the UI/UX perspective. Technical review concluded earlier. Merging...

@csillag csillag merged commit 3e9c6f6 into master Oct 13, 2022
@csillag csillag deleted the csillag/dont-choke-on-empty branch October 13, 2022 16:35
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.

Clicking "Send" on an empty transfer throws an error
3 participants