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: Handle uninstalled Staged Expenditure extension #3402

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jakubcolony
Copy link
Collaborator

@jakubcolony jakubcolony commented Oct 20, 2024

Description

This PR fixes an issue where users could interact with staged payments created using a different installation of the Staged Expenditure extension. In order for the checks to work properly, there is a block-ingestor PR that facilitates storing the relevant extension address on the expenditure object.

In addition, it fixes an issue with the flow where you could not see previous funding actions after you uninstalled the extension. That meant if there was an unclaimed motion, there was no way for you to claim it.

Testing

  1. Install Staged Expenditure extension.
  2. Create 4 staged payments - you might find the Redo action option useful with that:
image
  1. Leave the first payment at the review step. Advance each of the remaining payments to further steps:
  • Review
  • Funding
  • Release
  • Payment
  1. Uninstall Staged Expenditure extension.

  2. Verify that the uninstalled extension warning is shown in each payment you created. You should also be able to go back to any of the previous steps and see the details of the action related to that step (ie. the creator, date, etc.).

  3. Now install Staged Expenditure extension again.

  4. Verify the warning is still shown for payments created using the uninstalled extension.

  5. Create another Staged Expenditure payment and move it through all stages to check for regressions.

image image image Screenshot 2024-10-20 at 22 49 19

Resolves #3304

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.

Long way to test this one :)

But ran through all the stages and it seems to be behaving properly now. Nicely done! 💯

Screenshot from 2024-10-22 15-47-34
Screenshot from 2024-10-22 15-50-58
Screenshot from 2024-10-22 15-51-08
Screenshot from 2024-10-22 15-51-49
Screenshot from 2024-10-22 15-52-06
Screenshot from 2024-10-22 15-52-10
Screenshot from 2024-10-22 15-52-13
Screenshot from 2024-10-22 15-52-17
Screenshot from 2024-10-22 15-52-48
Screenshot from 2024-10-22 15-52-57
Screenshot from 2024-10-22 15-53-00
Screenshot from 2024-10-22 15-53-03
Screenshot from 2024-10-22 15-53-06
Screenshot from 2024-10-22 15-54-16
Screenshot from 2024-10-22 15-54-21
Screenshot from 2024-10-22 15-54-29
Screenshot from 2024-10-22 15-54-35
Screenshot from 2024-10-22 15-54-44

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.

Very nicely done Jakub! All the test cases work as expected.

Screenshot 2024-10-24 at 11 42 06 Screenshot 2024-10-24 at 11 42 12 Screenshot 2024-10-24 at 11 42 17 Screenshot 2024-10-24 at 11 42 21

I'm not sure whether these pills should change to the "Uninstalled" pill for extensions which have been uninstalled - I think this is how the reputation extension behaves as far as I remember. But this can be it's own PR.

Screenshot 2024-10-24 at 11 40 59

Tested making another staged payment on the new extension installation and all worked without issue. Great job!

@mmioana mmioana self-requested a review October 25, 2024 11:06
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.

Nice job on this one @jakubcolony 👏 💯

Installed the extension
Screenshot 2024-10-25 at 13 43 48

Created the 4 staged payments and advanced them to a different step
Screenshot 2024-10-25 at 13 49 04
Screenshot 2024-10-25 at 13 49 28
Screenshot 2024-10-25 at 13 49 51
Screenshot 2024-10-25 at 13 50 14
Screenshot 2024-10-25 at 13 51 24

Uninstalled the extension
Screenshot 2024-10-25 at 13 51 44
Screenshot 2024-10-25 at 13 51 49

The errors are showing up
Screenshot 2024-10-25 at 13 52 02
Screenshot 2024-10-25 at 13 52 06
Screenshot 2024-10-25 at 13 52 10
Screenshot 2024-10-25 at 13 52 16

Re-installed the extension
Screenshot 2024-10-25 at 13 52 49

The errors are still showing up for the previous staged payments
Screenshot 2024-10-25 at 13 53 01
Screenshot 2024-10-25 at 13 53 05
Screenshot 2024-10-25 at 13 53 09
Screenshot 2024-10-25 at 13 53 13

And for regression I created a new staged payment and managed to finalise it
Screenshot 2024-10-25 at 13 53 49
Screenshot 2024-10-25 at 13 54 25

Awesome job 🥇

Just one question that I have @jakubcolony is if we don't want to actually show up the card in the Grouped actions list?

Without applying this diff, I didn't manage to see it yet there

diff --git a/src/components/v5/common/ActionSidebar/partials/PaymentGroup/GroupList.ts b/src/components/v5/common/ActionSidebar/partials/PaymentGroup/GroupList.ts
index e2c060dff..fcd52f47d 100644
--- a/src/components/v5/common/ActionSidebar/partials/PaymentGroup/GroupList.ts
+++ b/src/components/v5/common/ActionSidebar/partials/PaymentGroup/GroupList.ts
@@ -1,11 +1,11 @@
 import {
   HandCoins,
   Money,
+  Steps,
 
   // @TODO: uncomment when streaming payment,split payment and staged payment will be ready
   // Waves,
   // ArrowsOutLineHorizontal,
-  // Steps,
 } from '@phosphor-icons/react';
 
 import { Action } from '~constants/actions.ts';
@@ -43,13 +43,13 @@ export const GROUP_LIST = [
   //   Icon: ArrowsOutLineHorizontal,
   //   action: Action.SplitPayment,
   // },
-  // {
-  //   title: formatText({ id: 'actions.stagedPayment' }),
-  //   description: formatText({
-  //     id: 'actions.description.stagedPayment',
-  //   }),
-  //   Icon: Steps,
-  //   action: Action.StagedPayment,
-  //   isNew: true,
-  // },
+  {
+    title: formatText({ id: 'actions.stagedPayment' }),
+    description: formatText({
+      id: 'actions.description.stagedPayment',
+    }),
+    Icon: Steps,
+    action: Action.StagedPayment,
+    isNew: true,
+  },
 ];

@iamsamgibbs
Copy link
Contributor

Just one question that I have @jakubcolony is if we don't want to actually show up the card in the Grouped actions list?

There is an issue for this @mmioana :) #3441

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.

Staged payments no longer handling uninstalled extension
4 participants