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.

Sign up for GitHub

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

Jump to bottom

Direct to proper screen on address QR code scan #1859

Merged
merged 6 commits into from
Oct 16, 2020
Merged

Direct to proper screen on address QR code scan #1859

merged 6 commits into from
Oct 16, 2020

Conversation

EtDu
Copy link
Contributor

@EtDu EtDu commented Sep 29, 2020

@EtDu EtDu requested a review from a team as a code owner September 29, 2020 05:12
@EtDu EtDu added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 29, 2020
@EtDu EtDu added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Sep 30, 2020
@omnat omnat added this to the v1.0.4 milestone Oct 3, 2020
ibrahimtaveras00
ibrahimtaveras00 suggested changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Issue 1:

When I scan the QR Code from the Receive view, I am correctly sent to to the Send to view, however, upon continuing I see collectible pre-selected with Balance: undefined, with 2 warnings appearing and when I do switch to ETH the Next CTA is disabled and I cannot continue.

Seen here = https://recordit.co/HULaqjSnMe

Screen Shot 2020-10-05 at 7 49 49 PM

Issue 2:

When I go through the request payment flow and I set how much ETH I want, I'm still taken to the Send to view and the same bug as above happens. I'm under the assumption that when a user goes to the Add funds/receive > request payment > and selects the asset with the value wanted we should take them to the confirm view like usual right cc: @omnat @cjeria?

seen here = https://recordit.co/TZac6VURZ8

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 5, 2020
@omnat
Copy link
Contributor

omnat commented Oct 6, 2020

Case 1 >> This behavior is as intended in production - shouldn't change with this (or other) PRs.
When user scans a QR code which is a payment request from someone - we should direct user directly to the confirm screen, with all values correctly pre-filled (asset type, amount, recipient address, network).
Question: This is happening as expected in the prod version. Looks like this PR is breaking this behavior. @EtDu Was this intentional?

Case 2 >> This behavior needed to change with this PR. The intended flow should be the following:
When user scans a QR code which is a public address - we should direct user directly to the 'Send to' screen, correctly pre-filled with recipient address.
Question: Why is collectible the default selected asset on the 'Amount' screen. No reason to change that behavior from how it is today in production - @EtDu was this intentional?

@EtDu
Copy link
Contributor Author

EtDu commented Oct 6, 2020

@omnat
https://trello.com/c/V51d5FO6/204-public-address-qr-code-scanning-opens-confirm-screen-should-be-just-the-prefilled-recipient-address-screen
Case 2 is correct, although collectible shouldn't be the default in the amount screen.

@EtDu
Copy link
Contributor Author

EtDu commented Oct 7, 2020

@ibrahimtaveras00
For issue 2 that seems logical. If the payment request QR contains an amount the behavior should function as normal

@EtDu EtDu force-pushed the address-qr-landing branch from 17beb92 to 647c4df Compare October 8, 2020 04:10
@EtDu EtDu added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Oct 8, 2020
@EtDu
Copy link
Contributor Author

EtDu commented Oct 8, 2020

@ibrahimtaveras00 should behave properly now :)

ibrahimtaveras00
ibrahimtaveras00 suggested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Issue 1 seems resolved 👍

Issue 2 seems to only work when requesting ETH but fails when requesting ERC 20 tokens
Fails when I request KNC (goes to send to view rather than confirm view) seen here = https://recordit.co/18qexARqIT

This works on prod; seen here = https://recordit.co/PbuklCc8YM

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 13, 2020
@EtDu EtDu added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Oct 14, 2020
@EtDu
Copy link
Contributor Author

EtDu commented Oct 14, 2020

@ibrahimtaveras00 Fixed

ibrahimtaveras00
ibrahimtaveras00 approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

QA Passed 👍

@ibrahimtaveras00 ibrahimtaveras00 added next release QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 14, 2020
@EtDu EtDu force-pushed the address-qr-landing branch from 6b04d2d to f8c20af Compare October 15, 2020 03:56
@EtDu EtDu merged commit 70be92a into develop Oct 16, 2020
@EtDu EtDu deleted the address-qr-landing branch October 16, 2020 03:53
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
@EtDu
Direct to proper screen on address QR code scan (#1859)
f3de4f1 
* Direct to adress input screen after scanning address QR code, fill in to address with data from QR code

* Direct to confirm screen if qr code has an amount

* fix balance undefined and displaying collectible when scanning from qr code

* Navbar: fix payment request QR code not working for ERC20 tokens

* add content to function param
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@rickycodes rickycodes rickycodes approved these changes

Assignees

@EtDu EtDu

@ibrahimtaveras00 ibrahimtaveras00

Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Milestone
v1.0.4
Development

Successfully merging this pull request may close these issues.
4 participants
@EtDu @omnat @rickycodes @ibrahimtaveras00