-
Notifications
You must be signed in to change notification settings - Fork 15
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
Conversation
c83c474
to
a56ce01
Compare
{ id: 3, setTo: roles.role_3 }, | ||
{ id: 5, setTo: roles.role_5 }, | ||
{ id: 6, setTo: roles.role_6 }, | ||
{ id: ColonyRole.Recovery, setTo: roles.role_0 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was definitely a hard issue to wrap your head around, but you came up with a really cool solution @bassgeta 🥇
Followed your testing steps and it seems to be working as expected 💯
/* | ||
* This probably seems so confusing but dropping a here be dragons comment for future developers | ||
* When you create a motion, there is no historic roles created, that's created when a setUserRoles contract passes | ||
* when you do it via permissions, it 100% reflects the roles the user currently holds | ||
* The roles on the action are just a diff, so if we add just 1 role, only that one is there, the same when we remove it | ||
* Motions however modify the user's current roles in the colony, so we use those | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed a couple of reads to actually understand it, but it totally makes sense 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
leela
the Multi-SigOwner
role inGeneral
amy
thePayer
permission bundle inAndromeda
via Permissions and verify it shows up and you can redo itfry
thePayer
permissions inAndromeda
via Multi-sig. Make sure everything is correct and finalize the action so the roles get applied.amy
theAdmin
permission bundle inAndromeda
via a voting reputation motion. Verify it shows up as the admin bundle in the UI. No need to finalize the motionat all 👍fry
custom roles by removing theAdministration
role via permissions from the custom toggle form. NOTE: this is a bit funky with the titles, onmaster
we show this action asRemoving permissions
but motions showAdd custom permissions
🤷fry
's roles inAndromeda
via Multi-Sig.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 🫠
amy
thePayer
Multi-Sig permission bundle inAscendant
viaPermissions
fry
thePayer
Multi-Sig permissions inAscendant
via Multi-sig. Make sure everything is correct and finalize the action so the roles get applied.amy
custom permissions and toggle theArchitecture
role by hand inAscendant
via a voting reputation motion. Verify it shows up as the admin bundle in the UI. No need to finalize the motionat all 👍Administration
Multi-Sig role inAscendant
fromfry
via permissions and make sure it shows up correctly.fry
's Multi-Sig roles inAscendant
via a voting reputation motion and make sure everything shows up as expectedLast 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 🏗
SetUserRoles
completed action view now contains some more comments on what's being done code-wiseResolves #4003