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: Clickable transactions #4007

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

Conversation

davecreaser
Copy link
Contributor

Description

  • Refactor the way clickable transactions works, and ensure that it works for all transaction types.

An issue was raised (#3765) where motion related transactions were not opening the correct action when clicked. After investigating the issue, it turned out that most transactions were not opening the correct action, and in fact were not even opening the correct colony. This PR aims to refactor the way clicking on a transaction in the user hub works, and introduces a new 'associatedActionId' property on the transaction type. This will hold the action id (which is itself a transaction hash) of the action which should be opened. If it is not present, it can be assumed that the transaction hash contained in the transaction itself is the correct one.

This PR fixes the following navigation bugs when a transaction is clicked:

  • Opening the correct colony for all transactions
  • Going to the correct action for motion related transactions
  • Going to the correct action for expenditure related transactions
  • Going to the correct action for multi sig related transactions
  • Going to the correct action for expenditure funding related transactions (permissions, multi sig, and repuation)
  • Going to the correct colony dashboard for create colony transactions
  • Going to the extensions page for extension related transactions
  • Going to the incoming funds page for funds claimed transactions

Testing

Theoretically, every transaction type can be tested, so if you really really want to spend two days testing this then feel free to just trigger every possible type of transaction method listed here: src/types/transactions.ts and make sure when clicking them in the user hub that you navigate where you would expect!

In reality of course, you probably don't want to do that, so here's some good places to test. Feel free to test either all of them or just some of them:

  • After running the create data script and before doing anything, just check that all the transactions generated by that script running navigate correctly:
Screen.Recording.2024-12-24.at.11.17.01.mov
  • Install some extensions (I used reputation and multisig, and changed the multisig threshold since this will help with testing the motions further down the line). Now check the transactions for them:
Screen.Recording.2024-12-24.at.11.20.21.mov
  • Create an agreement, and follow the entire flow until it has succeeded (stake for and against to trigger a vote, and then vote for it, reveal and finalize) and then check that these transactions navigate properly:

In my video some other motion related bugs are shown, but you can ignore these (claiming twice at the end, seeing a flash of the change vote button, and seeing oppose wins for a second even though the motion passed)

Screen.Recording.2024-12-24.at.11.31.20.mov
  • Create a multisig motion (don't forget to first give permissions!), and again follow the flow until it has succeeded. In my testing, I just manipulated the multisig threshold instead of using two users, but feel free to test however you see fit. Check the transactions for these:
Screen.Recording.2024-12-24.at.11.39.54.mov
  • Create an expenditure (an advanced payment or a streaming payment, or a split payment) and again follow the flow until it has succeeded. It would be good to test funding with all different types of funding methods. In my video I've used reputation. Check the transactions for these:
Screen.Recording.2024-12-24.at.11.50.18.mov

If you can think of any other transactions to test, feel free! Some other things which I haven't mentioned are cancelling transactions, and doing actions using permissions for example.

Diffs

New stuff

  • New optional property associatedActionId on the transaction model

Changes 🏗

  • Ensured that associatedActionId is added to transactions where necessary.
  • Changed the way clickable transactions are handled.

Resolves #3765

@davecreaser davecreaser requested a review from a team as a code owner December 24, 2024 11:54
mmioana
mmioana previously approved these changes Dec 27, 2024
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.

Thanks for solving this issue @davecreaser 🙌

Checked transactions after running the create-data script

Screen.Recording.2024-12-27.at.14.36.16.mov

Then after installing the Multi-sig and Reputation extensions

Screen.Recording.2024-12-27.at.14.39.45.mov

Then for creating an Agreement

Screen.Recording.2024-12-27.at.14.43.45.mov

Then for a simple payment using Multi-sig

Screen.Recording.2024-12-27.at.14.45.12.mov

Then an Advanced payment where I used Permissions, Reputation and Multi-sig to fund the payment

Screen.Recording.2024-12-27.at.14.46.54.mov
Screen.Recording.2024-12-27.at.14.47.44.mov
Screen.Recording.2024-12-27.at.14.49.07.mov

And it all works like a charm ✨ really great work!

I am just wondering if it would make sense to create a helper for getting the associatedActionId based on the expenditure or txHash, something like

const getTransactionAssociatedActionId = (
  expenditure?: ExpenditureFragment,
  actionTxHash?: string,
) => expenditure?.creatingActions?.items[0]?.transactionHash || actionTxHash;

Either way this should not be a blocker for merging this PR 💯

@davecreaser davecreaser self-assigned this Dec 30, 2024
@davecreaser
Copy link
Contributor Author

Thanks @mmioana, nice idea with the helper util, I've updated the PR based on that feedback!

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.

Userhub Transactions tab: Motion related transactions should open the correct motion
2 participants