Fix: Do not needlessly show the Change Vote button #4149

rumzledz · 2025-01-21T00:36:56Z

Description

It was a bit tricky to fix this because at first, I thought I could synchronise the rendering based on the loading state of the query responsible for this button. As I was debugging it, I thought that it's a perfect time to refactor the way we make UI updates for our motions.

If you think this piece of work needs to be done separately, please let me know.

Risky changes

Minimise the reliance on action query polling and prefer real-time updates

Speaking for myself, it got a bit difficult to understand when polling starts and when they end. This is even made more complex due to the mixture of query polling and refetch. I have zero doubt that everyone here can understand this pattern if you thoroughly investigate it but the whole point of these changes is to super minimise this mystical approach.

This PR introduces subscription-based updates when it comes to keeping track of action updates. You can find these subscriptions being used in the useGetColonyAction hook.

The only 2 areas left where this polling mechanism is being used in:

The retry button for the Transaction Error Content
For the completed action form when an annotation is attached to a newly created action

Not so risky changes:

ActionContextProvider

We have this pattern where we would first retrieve the transaction hash to render our completed action component. Then we would prop drill the transaction ID down to our motion/multi-sig widgets so that we can retrieve the action again via another call to useGetColonyActionQuery. I thought this was redundant as we can just globally have a single source of truth for the current action we're dealing with. An exception to this rule is for Actions that are can have nested motions i.e. Advanced Payments. The MotionBox component for those are still calling useGetColonyAction because it is what it is!

General cleanup

MotionStep and MultiSigStep

Our motions have steps that share the same name i.e. FinalizeStep. When debugging, of course it's just a matter of CMD + clicking the component or checking where it's being imported from to see which is which. But for the sake of making it easier to know which motion step you're looking at, at a glance, I have introduced namespacing.

// Before
<FinalizeStep />

// After
<MotionStep.Finalize />
<MultiSigStep.Finalize />

Testing

Note

Known motion-related issues that this PR has inherited

1. Voter Scenario (Reputation)

Enable the Reputation Extension
Create a Simple Payment motion
On the Staking step, Oppose and fully stake
Approve and fully stake
Approve and submit your vote
Verify that the Change Vote button does NOT appear ✅

2. Multiple Voters Scenario (Reputation)

Enable the Reputation Extension
Create a Simple Payment motion
On the Staking step, Oppose and fully stake
Copy the transaction's URL
Open an incognito window and connect Amy's wallet (Wallet 2)
Visit the transaction's URL
Approve and fully stake
Oppose and Submit your vote
Verify that the Change Vote button appears ✅
Click the Change Vote button
Verify that your vote is updated accordingly
As leela, approve and submit your vote
Verify that the Change Vote button does NOT appear ✅
As leela, reveal your vote
As amy, reveal your vote
Finalise the motion
Verify that you can finalise the motion

The ANNOYING part and I'M SORRY

Please test motion creation and finalisation

Advanced Payment via Staking behaviour should be representative of what you can expect for the Split & Staged payments via Staking due to shared logic
One reputation motion should be representative of the rest of the actions due to shared logic

Please test multisig creation and finalisation

Advanced Payment via Staking behaviour should be representative of what you can expect for the Split & Staged payments via Staking due to shared logic
One multisig motion should be representative of the rest of the actions due to shared logic

Resolves #3603

rdig

Everything seems fine, however, as I detailed below, I get stuck on the Advanced Payment release with reputation step.

Also:

Voting step

I think you're confusing this with the "Staking" step, which made your testing steps a bit hard to follow

Single voter:

Screencast.from.2025-01-24.13-44-44.mp4

Multiple voters:
Screencast from 2025-01-24 13-53-47.webm

Advanced Payment via Staking:
I am getting stuck on releasing via reputation. It might not be related to this, but I'm rejecting until we figure it out

Screencast.from.2025-01-24.13-56-42.mp4

Tried a couple of times, and still the same outcome:

rumzledz · 2025-01-24T15:38:41Z

Hey @rdig ! The issue you came across seems to be the one that was fixed on master recently #4140 . I have rebased my PR to incorporate that fix. When you get the chance, can you please test it again? 😄

rdig

Very nice! Yeah, you were right, after that rebase everything works as expected.

This was a lot of work and it seems to be working pretty well. Nicely done Romeo! 💯

Single motion:
https://github.com/user-attachments/assets/9074cee8-c836-4273-836b-75a177cd7328

Motion with multiple users:
Screencast from 2025-01-29 20-24-13.webm

Advanced payment:
https://github.com/user-attachments/assets/c4e7b6a7-e741-4cc1-956b-236ed9b8b25a

Nortsova

Wooooow, @rumzledz , you did enormous work! ⭐

Thanks for the investigation and refactoring everything!

All test cases working great, except Advanced Payment bug that we discussed today, but it is unrelated to current PR 🙌

Voter scenario:
Change button is Not appear:

amy change vote:

after clicking "change vote" my vote changed to "support":

leela no change vote:

motion was finalized:

advanced payments:
funding:

release:

Split payments:

staged payments:

Multisig:

Nortsova

Retested after rebase and it still works as expected, nice job @rumzledz ✨

davecreaser

Damn @rumzledz really really good stuff here, this is a huge simplification and a great refactor. Big fan of all the choices you made with subscriptions, the context and the name spacing. Stellar stuff! ⭐ 🌚

I've just left one comment with something that confused me, maybe you could address that (not a problem, just something I didn't fully understand).

Tested the flows and everything is working great too, and the original bug is of course fixed. Nice one!

Screen.Recording.2025-01-30.at.18.40.09.mov

Screen.Recording.2025-01-30.at.18.41.40.mov

Uploading Screen Recording 2025-01-30 at 18.42.10.mov…

davecreaser · 2025-01-30T18:28:42Z

src/components/v5/common/ActionSidebar/hooks/useGetActionFormData.ts

-    expenditure,
-    loadingExpenditure,
-    startPollingForAction,
-    stopPollingForAction,


Well, that's certainly simpler 🤣 🧹

davecreaser · 2025-01-30T18:29:59Z

src/components/v5/common/ActionSidebar/hooks/useGetColonyAction.ts

+
+  useOnDeleteMultiSigUserSignatureSubscription({
+    onData: updateAction,
+  });


❤️ 🧹

Very clean, I like this a lot

davecreaser · 2025-01-30T18:34:22Z

src/components/v5/common/ActionSidebar/partials/Motions/Motions.tsx

 import UninstalledMessage from '~v5/common/UninstalledMessage/index.ts';
 import Stepper from '~v5/shared/Stepper/index.ts';

+import { MotionStep } from './MotionStep/index.ts';


I like this name spacing change a lot too 🐊

davecreaser · 2025-01-30T18:37:39Z

src/components/v5/common/ActionSidebar/partials/forms/CreateDecisionForm/hooks.ts

-            description: safeDescription,
-            walletAddress,
-          });
-


Just a little confused as to why the deletions in this file were made - they seem slightly unrelated to the other changes?

Could you maybe give a brief reason why this has been removed? Thanks!

(for reference, I tested saving drafts of agreements, and they still work 👍 ⚡ )

Thanks so much for reviewing @davecreaser ! I removed this because I found out that it's actually redundant 😅 This code is meant for the Save as draft button for an agreement and it's not meant to be here

Aha okay perfect. Thanks for the explanation!

rdig

Re-approved

davecreaser

Reapproving after rebase ✅

Nortsova

Re reviewed 🙌 Everything still working ✅

Thank you 👍

rumzledz self-assigned this Jan 21, 2025

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch 3 times, most recently from 7510287 to 376a186 Compare January 21, 2025 09:40

rumzledz marked this pull request as ready for review January 21, 2025 10:05

rumzledz requested a review from a team as a code owner January 21, 2025 10:05

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch 9 times, most recently from b264bd2 to 1651332 Compare January 23, 2025 12:48

rdig requested changes Jan 24, 2025

View reviewed changes

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch from 1651332 to a76c768 Compare January 24, 2025 12:45

rumzledz requested a review from rdig January 24, 2025 15:37

rumzledz requested a review from a team January 24, 2025 15:38

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch from a76c768 to 44686c4 Compare January 28, 2025 12:02

rdig previously approved these changes Jan 29, 2025

View reviewed changes

Nortsova previously approved these changes Jan 29, 2025

View reviewed changes

rumzledz dismissed stale reviews from Nortsova and rdig via 52283d1 January 30, 2025 10:49

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch from 44686c4 to 52283d1 Compare January 30, 2025 10:49

rumzledz requested review from Nortsova and rdig January 30, 2025 10:49

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch from 52283d1 to 118f0c4 Compare January 30, 2025 11:15

rumzledz requested a review from a team January 30, 2025 11:16

Nortsova previously approved these changes Jan 30, 2025

View reviewed changes

davecreaser previously approved these changes Jan 30, 2025

View reviewed changes

Nortsova mentioned this pull request Jan 30, 2025

[Payments] Reward isn't shown on the finalise step #4065

Open

rdig previously approved these changes Jan 31, 2025

View reviewed changes

fix: do not needlessly show the Change Vote button

6fff644

rumzledz dismissed stale reviews from rdig, davecreaser, and Nortsova via 6fff644 February 3, 2025 09:42

rumzledz force-pushed the fix/3603-properly-process-change-vote-button branch from 118f0c4 to 6fff644 Compare February 3, 2025 09:42

rumzledz requested review from davecreaser, Nortsova and rdig February 3, 2025 09:48

davecreaser approved these changes Feb 3, 2025

View reviewed changes

Nortsova approved these changes Feb 3, 2025

View reviewed changes

rdig approved these changes Feb 3, 2025

View reviewed changes

rumzledz merged commit 915cb1a into master Feb 3, 2025
2 checks passed

rumzledz deleted the fix/3603-properly-process-change-vote-button branch February 3, 2025 12:59

mmioana mentioned this pull request Feb 7, 2025

Fix: Finalize button stuck in pending upon motion finalization #4260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Do not needlessly show the Change Vote button #4149

Fix: Do not needlessly show the Change Vote button #4149

rumzledz commented Jan 21, 2025 •

edited

Loading

rdig left a comment

rumzledz commented Jan 24, 2025 •

edited

Loading

rdig left a comment

Nortsova left a comment

Nortsova left a comment

davecreaser left a comment

davecreaser Jan 30, 2025

davecreaser Jan 30, 2025

davecreaser Jan 30, 2025

davecreaser Jan 30, 2025

rumzledz Jan 30, 2025

davecreaser Jan 31, 2025

rdig left a comment

davecreaser left a comment

Nortsova left a comment

Fix: Do not needlessly show the Change Vote button #4149

Fix: Do not needlessly show the Change Vote button #4149

Conversation

rumzledz commented Jan 21, 2025 • edited Loading

Description

Risky changes

Minimise the reliance on action query polling and prefer real-time updates

Not so risky changes:

ActionContextProvider

General cleanup

MotionStep and MultiSigStep

Testing

1. Voter Scenario (Reputation)

2. Multiple Voters Scenario (Reputation)

The ANNOYING part and I'M SORRY

rdig left a comment

Choose a reason for hiding this comment

rumzledz commented Jan 24, 2025 • edited Loading

rdig left a comment

Choose a reason for hiding this comment

Nortsova left a comment

Choose a reason for hiding this comment

Nortsova left a comment

Choose a reason for hiding this comment

davecreaser left a comment

Choose a reason for hiding this comment

davecreaser Jan 30, 2025

Choose a reason for hiding this comment

davecreaser Jan 30, 2025

Choose a reason for hiding this comment

davecreaser Jan 30, 2025

Choose a reason for hiding this comment

davecreaser Jan 30, 2025

Choose a reason for hiding this comment

rumzledz Jan 30, 2025

Choose a reason for hiding this comment

davecreaser Jan 31, 2025

Choose a reason for hiding this comment

rdig left a comment

Choose a reason for hiding this comment

davecreaser left a comment

Choose a reason for hiding this comment

Nortsova left a comment

Choose a reason for hiding this comment

rumzledz commented Jan 21, 2025 •

edited

Loading

rumzledz commented Jan 24, 2025 •

edited

Loading