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: lookup action roles from the params and simplify stuff #4169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bassgeta
Copy link
Contributor

@bassgeta bassgeta commented Jan 23, 2025

Description

This one was quite something to wrap one's head around 😅
Basically I've unified how we get roles from the action. We now display a difference between the user's assigned roles and historic roles (if they don't exist, then their current roles in the colony).
Additionally, we determine that an action is removing roles if all the values in the roles array are false, which is true in regards to how we call the contract.
For example, if the user has the roles [2, 5 ,6] and we want to update their roles to [2, 6], we would call the contract with new roles and those override the old ones.

Testing

I am sorry, this one's quite a doozy, but I'll try to make it as concise as possible.

  1. Run your dev env, the create data script, install the Voting Reputation extension and the Multi-sig extension. Give leela the Multi-Sig Owner role in General
  2. Give amy the Payer permission bundle in Andromeda via Permissions and verify it shows up and you can redo it
    image
    image
    image
  3. Give fry the Payer permissions in Andromeda via Multi-sig. Make sure everything is correct and finalize the action so the roles get applied.
    image
    image
  4. Give amy the Admin permission bundle in Andromeda via a voting reputation motion. Verify it shows up as the admin bundle in the UI. No need to finalize the motionat all 👍
    image
  5. Give fry custom roles by removing the Administration role via permissions from the custom toggle form. NOTE: this is a bit funky with the titles, on master we show this action as Removing permissions but motions show Add custom permissions 🤷
    image
    image
  6. Completely remove fry's roles in Andromeda via Multi-Sig.
    image
    image

Whew alright we made it to the end of testing all of this via Permissions, so now let's just do the same for Multi-Sig roles 🫠

  1. Give amy the Payer Multi-Sig permission bundle in Ascendant via Permissions
    image
    image
  2. Give fry the Payer Multi-Sig permissions in Ascendant via Multi-sig. Make sure everything is correct and finalize the action so the roles get applied.
    image
  3. Give amy custom permissions and toggle the Architecture role by hand in Ascendant via a voting reputation motion. Verify it shows up as the admin bundle in the UI. No need to finalize the motionat all 👍
    image
    image
  4. Remove the Administration Multi-Sig role in Ascendant from fry via permissions and make sure it shows up correctly.
    image
    image
  5. Completely remove fry's Multi-Sig roles in Ascendant via a voting reputation motion and make sure everything shows up as expected
    image

Last but not least, make sure that the Redo action popup in the activity feed shows up for actions which are not removing permissions. My dev env just crashed when doing this step 🫠

Diffs

Changes 🏗

  • The SetUserRoles completed action view now contains some more comments on what's being done code-wise
  • Simplified the logic for displaying a user's roles a bit.
  • Note that the activity feed's title still assumes we are removing roles if the passed roles are all false, which is not necessarily true 🤷

Resolves #4003

@bassgeta bassgeta self-assigned this Jan 23, 2025
@bassgeta bassgeta force-pushed the fix/4003-multisig-payer-assign branch from c83c474 to a56ce01 Compare January 24, 2025 11:00
@bassgeta bassgeta marked this pull request as ready for review January 24, 2025 11:01
@bassgeta bassgeta requested a review from a team as a code owner January 24, 2025 11:01
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.

[Permission] Owner role is shown instead of Payer on the complete action page after assigning the Payer role
1 participant