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: Disable payment buttons for staged payments when release is in progress #4237

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

mmioana
Copy link
Contributor

@mmioana mmioana commented Feb 3, 2025

Description

  • This PR addresses an issue in the Staged payment action where the Pay now and Pay all buttons remained enabled until payments were successfully claimed in case of actions.
    To resolve this, I introduced a context dedicated specifically for the Staged payments (and refactored everything related to this type of action that was unnecessarily polluting the PaymentBuilderContext). The context stores the current milestones for release, all the milestones awaiting release, and a flag for the pending release. Based on the last two props, we can easily decide the enabled/disabled state of the buttons, the first prop being used only for the release modal and the saga. In case of using motions to release a payment, once the motion got created, the button will be re-enabled in order to allow using a different decision method.
    Moreover, the context state is scoped by txHash, given we can switch between Staged payment actions and we still want to keep the state until all the payments are released.

Important

If we are using motions (aka Reputation) to release the payments, after discussing with Prod and Network teams, we should still be able to use another method to release those payments. If the payments got released in the meantime using a different decision method, the motions will still pass, but no amount will be transferred.

Note

Even though the context looses its state upon page refresh, it's highly unlikely to refresh the page and the payment to not get in a claimed state, thus making it possible for someone to re-trigger the claim process. However, I'm open to more opinions.

Testing

TODO: Ok, enough talking, now let's make sure we get our desired state.

  • Step 1. First make sure to install the Staged payments and Reputation extensions.

  • Step 2. Now, please create a staged payments with about 4 milestones and go trough all the steps until Payment stage
    Screenshot 2025-02-03 at 20 09 58

  • Step 3. Create another one and reach the Payment stage
    Screenshot 2025-02-03 at 20 10 52

  • Step 4. Now please use Permissions and Payment creator to release different milestones. Switch between the Staged payment actions you previously created and make sure the Pay now/Pay all buttons remain disabled until the Released pill is shown

Screen.Recording.2025-02-03.at.20.11.23.mov
  • Step 5. Create another Staged payments action with 2 milestones and go through all steps until the Payment step
  • Step 6. Now try using Reputation to release the milestone. The button should be disabled until the motion got created.
  • Step 7. Once the button is re-enabled try to release the milestone using Reputation again. This should not be possible.
  • Step 8. Try to use a different decision method to release the milestone. The button should be disabled until the milestone's payment gets claimed.
Screen.Recording.2025-02-04.at.13.43.15.mov
  • Step 9. Try releasing the second milestone using Reputation.
  • Step 10. You should see a single status for the completed action status pill.
    Screenshot 2025-02-04 at 13 53 19
    I haven't done any change for the last step, but it's worth to double-check this issue is no longer happening.

Diffs

New stuff

  • StagedPaymentContext
  • ReleaseButton

Deletions ⚰️

  • All props related to staged payments were removed from PaymentBuilderContext

Resolves #4049

@mmioana mmioana self-assigned this Feb 3, 2025
@mmioana mmioana force-pushed the fix/4049-pay-buttons-enabled-during-release branch from f7f1ad9 to dce96f2 Compare February 4, 2025 12:45
@mmioana mmioana marked this pull request as ready for review February 4, 2025 13:04
@mmioana mmioana requested a review from a team as a code owner February 4, 2025 13:04
…rogress

Fix: Disable payment buttons for staged payments when release is in progress
@mmioana mmioana force-pushed the fix/4049-pay-buttons-enabled-during-release branch from dce96f2 to f17d39d Compare February 4, 2025 13:06
Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Cool fix @mmioana, I think the context makes total sense. I've just left one comment about the placement of the context which maybe we could look into.

Aside from that, it all looks to be working great. I even tried to cheat it at the end by finalizing the reputation payment and then quickly making it with permissions, but everything still worked perfectly.

Nice job!

Screen.Recording.2025-02-04.at.17.10.14.mov
Screen.Recording.2025-02-04.at.17.14.44.mov

<Outlet />
</ColonyLayout>
</ActionContextProvider>
</StagedPaymentContextProvider>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if this has to wrap the entire route, or can it simply wrap the staged payment completed action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davecreaser that was my initial thought, but soon realised the context state will get wiped when the CompletedAction is unmounted, thus changing between actions would have needed to get the current state of the milestones and initialise the context. That seemed a bit too cumbersome 😢

items={items.filter((item) => !item.isClaimed)}
className="w-full text-end"
/>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 🧹

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.

Nice fixes here! Tested most of the steps and everything runs smooth. Nicely done!

Screenshot from 2025-02-05 00-57-05
Screenshot from 2025-02-05 00-58-46

Screencast.from.2025-02-05.00-59-26.mp4

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 tidying this up, it works so much better now!

I managed to find one issue, but it's only really possible to recreate in dev, I don't think you'd be able to click through metamask quick enough in production and it doesn't super break things anyway. Here I have created a motion to release milestone 3, then as I finalize the motion, I release the same milestone with permissions. The result is that the motion box shows both methods of releasing as passed.

Screen.Recording.2025-02-05.at.09.14.572.mov

I don't know if it's easy to disable the "Pay now" button whilst a related motion is being finalized? But it's also such a small issue that we can just leave it.

Similar issue here, the "Pay now" button briefly remains clickable even after a relation motion has finished finalising:

Screen.Recording.2025-02-05.at.09.14.573.mov

Everything else works great.

Screen.Recording.2025-02-05.at.09.14.574.mov

@mmioana mmioana merged commit 4df9967 into master Feb 5, 2025
2 checks passed
@mmioana mmioana deleted the fix/4049-pay-buttons-enabled-during-release branch February 5, 2025 09:54
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 payment] Pay buttons remain enabled during payment processing in Staged Payments
4 participants