-
Notifications
You must be signed in to change notification settings - Fork 19
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 Permission Modal bugs #2421
Conversation
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.
Besides the things I've pointed out, you also need to filter out the extensions addresses
(if you look at my screenshot you'll see that the address that has the minimal profile, is actually the OneTxPayment
extension address to which we've added, by default, the architecture permission)
To get the installed extensions addresses you need to call this query: SubgraphColony
and that you can get all the installed extensions addresses from the extensions
prop (or you can use the processed colony query as well)
Also. God f***ing damn apollo. I have no idea why it doesn't like the username
name :(
37083c5
to
a1ae752
Compare
0f897b7
to
d90d68d
Compare
a4c9e6c
to
e2933c4
Compare
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.
Just one question about dependencies, other than that looks good.
src/modules/dashboard/components/PermissionManagementDialog/PermissionManagementForm.tsx
Outdated
Show resolved
Hide resolved
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.
Seems to be working fine 👍
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.
Very nice work! 🥇
Thank you untangling and making sense of all these edge cases.
src/modules/dashboard/components/PermissionManagementDialog/PermissionManagementForm.tsx
Outdated
Show resolved
Hide resolved
const canSetPermissionsInRoot = | ||
domainId === ROOT_DOMAIN_ID && | ||
currentUserRoles.includes(ColonyRole.Root) && | ||
(!userDirectRoles.includes(ColonyRole.Root) || rootAccounts.length > 1); |
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.
Shouldn't this be >= 1
?
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.
Im actually not sure about this part of logic 🤔 This logic exist before, but now im not sure if its correct honestly...\
- Should we block possibility of removing root permission by person which also holding root? 🤔 (probably question to product)
- Should we block possibility of removing root from last root? (also probably question to product)
e4535c5
to
6830fee
Compare
Description
Changes 🏗
Notes 🏗
Resolves DEV-259