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

[Bug]: Native Send - Clicking Edit/Back arrow doesn't work on the Dialog window if the tx is initiated in the full window #28316

Closed
seaona opened this issue Nov 6, 2024 · 1 comment · Fixed by #28382
Closed

[Bug]: Native Send - Clicking Edit/Back arrow doesn't work on the Dialog window if the tx is initiated in the full window #28316

seaona opened this issue Nov 6, 2024 · 1 comment · Fixed by #28382
Assignees
@pedronfigueiredo
Labels
regression-RC-12.7.0 release-12.8.0 Issue or pull request that will be included in release 12.8.0 release-blocker This bug is blocking the next release Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug Something isn't working

Comments

@seaona
Copy link
Contributor

seaona commented Nov 6, 2024

Describe the bug

Whenever I am sending a native token, once I land into the last confirmation screen, if I use the MM dialog and click the Back/Edit arrow, it doesn't work and I remain in the same page.

This happens if I initiate the Send from the full view, and then try to click the Back/Edit on the Dialog view

Expected behavior

No response

Screenshots/Recordings

send-back-edit-not-work-dialog.mp4

Steps to reproduce

  1. Start a native send from the MM full view
  2. Once on the last confirmation screen, open the popup (ie click on the MM icon, or trigger a tx from a dapp)
  3. Try to click the Back/Edit arrow from the dialog
  4. See nothing happens

Error messages or log output

No response

Detection stage

During release testing

Version

12.7.0

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response
@seaona seaona added type-bug Something isn't working Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team labels Nov 6, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Nov 6, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Nov 6, 2024
@seaona seaona changed the title [Bug]: Native Send - Clicking Edit/Back arrow doesn't work on the Dialog window [Bug]: Native Send - Clicking Edit/Back arrow doesn't work on the Dialog window if the tx is initiated in the full window Nov 6, 2024
@seaona seaona added the release-blocker This bug is blocking the next release label Nov 6, 2024
@seaona
Copy link
Contributor Author

seaona commented Nov 6, 2024 •
edited
Loading

ℹ️ : @bschorchit adding the release-blocker tag, as all regressions are considered release-blockers by default. Feel free to remove the label if you want to re-assess the priority

@pedronfigueiredo pedronfigueiredo mentioned this issue Nov 8, 2024
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this issue Nov 8, 2024
@pedronfigueiredo
fix: Return to send page with different asset types (#28382)
4ba7d89 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This bug was reproducible by opening a new wallet initiated send
confirmation with a Native token ("simple send") from the extension full
screen view, and then triggering a dApp initiated confirmation, and
trying to return back to the send flow stepper.

The bug was provoked due to having the `editExistingTransaction` action
dispatched on back button click hardcoded for asset of type token. The
fix involves dynamically determining the asset type.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28382?quickstart=1)

## **Related issues**

Fixes: #28316

## **Manual testing steps**

See above or check video on the bug report ticket.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Nov 8, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 8, 2024
pedronfigueiredo added a commit that referenced this issue Nov 8, 2024
@pedronfigueiredo
fix: Return to send page with different asset types (#28382)
395919d 
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This bug was reproducible by opening a new wallet initiated send
confirmation with a Native token ("simple send") from the extension full
screen view, and then triggering a dApp initiated confirmation, and
trying to return back to the send flow stepper.

The bug was provoked due to having the `editExistingTransaction` action
dispatched on back button click hardcoded for asset of type token. The
fix involves dynamically determining the asset type.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28382?quickstart=1)

## **Related issues**

Fixes: #28316

## **Manual testing steps**

See above or check video on the bug report ticket.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@pedronfigueiredo pedronfigueiredo mentioned this issue Nov 8, 2024
Merged
14 tasks
danjm pushed a commit that referenced this issue Nov 11, 2024
@pedronfigueiredo
fix: cherry-pick: Return to send page with different asset types (#28384
54588f9 
)

Cherry-pick: #28382
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution? -->

This bug was reproducible by opening a new wallet initiated send
confirmation with a Native token ("simple send") from the extension full
screen view, and then triggering a dApp initiated confirmation, and
trying to return back to the send flow stepper.

The bug was provoked due to having the `editExistingTransaction` action
dispatched on back button click hardcoded for asset of type token. The
fix involves dynamically determining the asset type.

[![Open in GitHub

Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28382?quickstart=1)

## **Related issues**

Fixes: #28316

## **Manual testing steps**

See above or check video on the bug report ticket.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding

Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.


<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28384?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pedronfigueiredo pedronfigueiredo

Labels
regression-RC-12.7.0 release-12.8.0 Issue or pull request that will be included in release 12.8.0 release-blocker This bug is blocking the next release Sev2-normal Normal severity; minor loss of service or inconvenience. team-confirmations Push issues to confirmations team type-bug Something isn't working
Projects
Bugs by severity
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Return to send page with different asset types MetaMask/metamask-extension
3 participants
@pedronfigueiredo @metamaskbot @seaona