Skip to content

Direct to proper screen on address QR code scan#1859

Merged
EtDu merged 6 commits intodevelopfrom
address-qr-landing
Oct 16, 2020
Merged

Direct to proper screen on address QR code scan#1859
EtDu merged 6 commits intodevelopfrom
address-qr-landing

Conversation

@EtDu
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

EtDu commented Oct 8, 2020

@ibrahimtaveras00 should behave properly now :)

ibrahimtaveras00
ibrahimtaveras00 suggested changes Oct 13, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

EtDu commented Oct 14, 2020

@ibrahimtaveras00 Fixed

ibrahimtaveras00
ibrahimtaveras00 approved these changes Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown
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 QA testing has been completed and passed 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

2 more reviewers

@rickycodes rickycodes rickycodes approved these changes

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@EtDu EtDu

@ibrahimtaveras00 ibrahimtaveras00

Labels

next release QA Passed QA testing has been completed and passed

Projects

None yet

Milestone

v1.0.4

Development

Successfully merging this pull request may close these issues.

4 participants

@EtDu @omnat @rickycodes @ibrahimtaveras00