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: Dont show failed expenditures in actions list #4217

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

davecreaser
Copy link
Contributor

@davecreaser davecreaser commented Jan 30, 2025

Description

  • So this was a bit of a tricky bug (original ticket: [Staged payment] Invalid staged payment data is displayed on the dashboard when a Metamask transaction is canceled #4033) which is basically caused by the action being created and put in the actions list a little prematurely. If a user cancels the final transaction in metamask, or if the final transaction just fails, then the action is not properly created and gets into a weird state where it is technically created but shouldn't really be seen by the user or show in the lists.

  • This PR aims to fix this by not setting showInActionsList by default to true for createExpenditure actions. The saga will instead set it to true once it has finished all of the necessary transactions. There's also a couple of very small cleanup things tacked on.

There's also a couple of changes in PRs in other repos so please also review them:

Testing

⚠️ You'll need to restart your dev env to use the correct block ingestor and auth proxy ⚠️

  • Install the staged expenditure extension.
  • Log in as Leela using metamask (if you run into issues with this give me a shout and I can help you).
  • Create a staged payment, and complete it as usual, accepting all the metamask transactions. You should see it show up in the actions list.
Screenshot 2025-01-30 at 17 27 41 Screenshot 2025-01-30 at 17 28 02 * Create another staged payment, and this time accept the first metamask transaction, but cancel the second one. The expenditure creation should show as failed, and the action shouldn't show in the actions list. Screenshot 2025-01-30 at 17 28 52 Screenshot 2025-01-30 at 17 30 42 * Repeat the previous step, but this time accept the first two transactions before cancelling the third one. Again, it should fail and not show in the actions list. Screenshot 2025-01-30 at 17 29 11 * To be sure, it's probably worth checking some other advanced payment types to make sure they still work and show as expected.

Diffs

Changes 🏗

  • Changes to the way expenditure actions are created and shown in the actions list

Resolves #4033

@davecreaser davecreaser force-pushed the fix/#4033-dont-show-failed-expenditures branch from 8f6aa74 to 15fca70 Compare January 30, 2025 17:46
@davecreaser davecreaser marked this pull request as ready for review January 30, 2025 17:57
@davecreaser davecreaser requested a review from a team as a code owner January 30, 2025 17:57
@davecreaser davecreaser self-assigned this Jan 30, 2025
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

I'm not rejecting this for this branch's changes necessarily, but when testing this, the user dropdown menu (hamburger) seems to be broken.

I don't know if this is a issue on master that was pulled here, or it was a side-effect of some change here, but I only noticed it on this PR

Screenshot from 2025-01-31 14-49-08
Screenshot from 2025-01-31 14-49-22
Screenshot from 2025-01-31 14-49-26

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice job unpicking this issue! The solution seems to work well, had no rogue actions making their way to the activity feed.

I tested staged payments, advanced payments and split payments, all working as expected.

Rejecting the last transaction:
https://github.com/user-attachments/assets/83e42dd3-07e3-4953-b782-ae9783b89217

Testing split payments:
https://github.com/user-attachments/assets/f4684148-50e6-4122-962c-f57ee293543e

The UserHub issue appears to be on master so approving as it is unrelated to this issue - also noticed this when no items in the list.

Screenshot 2025-01-31 at 10 20 24

@mmioana
Copy link
Contributor

mmioana commented Feb 3, 2025

I'm not rejecting this for this branch's changes necessarily, but when testing this, the user dropdown menu (hamburger) seems to be broken.

I don't know if this is a issue on master that was pulled here, or it was a side-effect of some change here, but I only noticed it on this PR

Screenshot from 2025-01-31 14-49-08 Screenshot from 2025-01-31 14-49-22 Screenshot from 2025-01-31 14-49-26

@rdig I've added a fix for the user hamburger menu in #4216 as I've noticed this issue too

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Really nice work @davecreaser 🙌

Everything works as expected

Accepted all transactions for a Staged payment

Screen.Recording.2025-02-03.at.12.14.34.mov

Accepted the first transaction and rejected the second

Screen.Recording.2025-02-03.at.12.15.11.mov

Accepted first 2 transactions and rejected the 3rd one

Screen.Recording.2025-02-03.at.12.15.49.mov

Tried also for Advanced payment to accept the 1st transaction and reject the 2nd

Screen.Recording.2025-02-03.at.12.17.12.mov

And tested also for Split payment to accept the 1st transaction and reject the 2nd

Screen.Recording.2025-02-03.at.12.17.45.mov

Nice work!

@davecreaser
Copy link
Contributor Author

@rdig I've added a fix for the user hamburger menu in #4216 as I've noticed this issue too

Thanks @mmioana - I also just double checked master and I see it too. @rdig could I ask for a re-review please?

@davecreaser davecreaser requested a review from rdig February 3, 2025 12:15
Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

All good now!

@davecreaser davecreaser merged commit 503510d into master Feb 4, 2025
2 checks passed
@davecreaser davecreaser deleted the fix/#4033-dont-show-failed-expenditures branch February 4, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants