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 Permission Modal bugs #2421

Merged
merged 8 commits into from
Mar 29, 2021
Merged

Conversation

alicjakujawa
Copy link
Contributor

@alicjakujawa alicjakujawa commented Mar 22, 2021

Description

  • This PR adds fixes bugs related to setting permissions in management modal

Changes 🏗

  • Updated architecture description to be more general
  • Fixed advanced dialog to let user with root (and only root) permission to enter Permission Management dialog
  • Fixed userHasPermissions flag in PermissionManagementDialog
  • Fixed canRoleBeSet callback in PermissionManagementForm
  • Hide root and recovery permission checkboxes from subdomains

Notes 🏗

  • Run npm install before QA
  • To change root domain permissions you need to have ROOT
  • To change any permission in subdomain, you need to have architecture
  • If you have architecture permission in subdomain, you CANNOT change anything in this domain -> only in future sub domains

Resolves DEV-259

@alicjakujawa alicjakujawa self-assigned this Mar 22, 2021
Copy link
Member

@rdig rdig left a 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 :(

src/data/graphql/queries.graphql Outdated Show resolved Hide resolved
src/data/resolvers/role.ts Outdated Show resolved Hide resolved
@alicjakujawa alicjakujawa force-pushed the bug/dev-259-removing-permission-fix branch 2 times, most recently from 37083c5 to a1ae752 Compare March 24, 2021 16:54
@alicjakujawa alicjakujawa changed the title Add possibility to revoke root/architecture permissions Fix Permission Modal bugs Mar 24, 2021
@alicjakujawa alicjakujawa force-pushed the bug/dev-259-removing-permission-fix branch from 0f897b7 to d90d68d Compare March 25, 2021 12:19
@alicjakujawa alicjakujawa requested a review from a team March 25, 2021 12:47
@alicjakujawa alicjakujawa force-pushed the bug/dev-259-removing-permission-fix branch from a4c9e6c to e2933c4 Compare March 25, 2021 12:48
@alicjakujawa alicjakujawa marked this pull request as ready for review March 25, 2021 12:51
Copy link
Contributor

@chinins chinins left a 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.

Copy link
Contributor

@ArmandoGraterol ArmandoGraterol left a 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 👍

Copy link
Member

@rdig rdig left a 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.

const canSetPermissionsInRoot =
domainId === ROOT_DOMAIN_ID &&
currentUserRoles.includes(ColonyRole.Root) &&
(!userDirectRoles.includes(ColonyRole.Root) || rootAccounts.length > 1);
Copy link
Member

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 ?

Copy link
Contributor Author

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...\

  1. Should we block possibility of removing root permission by person which also holding root? 🤔 (probably question to product)
  2. Should we block possibility of removing root from last root? (also probably question to product)

@alicjakujawa alicjakujawa force-pushed the bug/dev-259-removing-permission-fix branch from e4535c5 to 6830fee Compare March 26, 2021 16:52
@alicjakujawa alicjakujawa merged commit a12d324 into master Mar 29, 2021
@alicjakujawa alicjakujawa deleted the bug/dev-259-removing-permission-fix branch March 29, 2021 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants