-
Notifications
You must be signed in to change notification settings - Fork 14
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: Add form validation when removing permissions #2749
base: master
Are you sure you want to change the base?
Conversation
91b37c5
to
bb4f647
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.
@rumzledz Great work on this, I appreciate your hard work and for working through the cases.
Everything seems to work as expected, in which the warning and confirmation modal appears in the situation whenever the Root
permission is removed.
My only request would be to close down the modal on clicking the confirmation. That way we return to the action panel, and users can access their Userhub without having to have it float over the top.
b3b6c7d
to
9d8aa99
Compare
c52ec6c
to
c9327f9
Compare
@iamsamgibbs all right after speaking with Arren, here's the new issue as promised #3083. It comes with its own complexities as an edge case so as discussed, it will be dealt with separately 👌 |
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.
Nice job!
One small issue with testing step 6. This might be better to address as a separate PR for the sake of getting this merged. The error message appears as expected, but disappears when changing any other field.
Screen.Recording.2024-09-17.at.14.33.51.mov
Otherwise I think we are maybe good?!
Proof of testing
Step 3:
No screenshot
Step 6:
Has an issue
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.
So codewise this is all good. You deserve praise for just having to deal with this mess of components.
I've ran the setup phase, as well as all the test steps, but cannot replicate test steps: 10, 12, 13 and 15
I suspect their parameters changed through all the subsequent changes you've made to the PR and the description was not updated.
If that's the case, just let me know, and I'll re-run those steps with the new instructions.
Setup phase:
Test 1:
Test 2:
Test 3:
Test 4:
Test 5:
Test 6:
Test 7:
Test 8:
Test 9:
Test 10:
Cannot replicate!: No error message shows up
Test 11:
Test 12:
Cannot replicate!: Getting an error, preventing me from submitting the form
Test 13:
Cannot replicate!: I was able to submit the form / modal
Test 14:
Test 15:
Cannot replicate: Cannot switch off the Architecture toggle
It seems that this PR is also fixing this #3382 and #3344 Here it testing steps: Redo Manage permission action #3344Redo Manage permission action Multi-sig #3382 |
Great job @rumzledz 🌟 Enormous amount of work done ✅ Here is a small bug that I found, for some reason, if I select Multi-sig permission I don't see "Mod" role: Let's rebase and maybe that will fix this |
5066126
to
614b436
Compare
614b436
to
d164a50
Compare
Hey @rdig! I forgot to mention that I retested steps 10, 12, 13 & 15 yesterday before rebasing and I was able to do them. I've rebased yesterday, retested and I'm still able to do them. Hopefully you'll be able to see it working again when you retest 🤞 |
d164a50
to
779d049
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.
Great job dealing with all of this @rumzledz ! Let's get it merged!
The only weird behaviour I've noticed is the validation on the permissions field disappearing when the title is updated, but I think this can be addressed as a separate issue.
weird.behaviour.mov
I did a tiny bit of multi-sig testing and it seems like there are no issues there:
Screen.Recording.2024-10-25.at.13.27.55.mov
I did screenshot and record all of the testing steps, but I won't dump them all here as it'll end up as a huge reply, but all the testing steps work as expected.
Great job! I can't believe this form will finally be fixed forever once this is merged and there will never be another issue with it again! :)
Description
Note
This PR is meant for actions made via the Permissions Decision Method
There are
master
bugs related to the Reputation Decision Method which are present on this PR and they will be dealt with separatelyMulti-Sig will be dealt with separately
Removing Root permissions from someone in a Parent domain
Removing permissions from someone in a subdomain
Testing
Important
Note
1. Selecting Remove Permissions for a member who has permissions for the selected team (Fry)
- Team: General
- Member: Fry
- Permissions: Remove permissions:
2. Selecting Remove Permissions for a member who does not have permissions for the selected team (jasmine-jolt)
- Team: General
- Member: jasmine-jolt
- Permissions: Remove permissions:
3. Checking that the Remove permission option persists when changing the Team & Member fields (Fry)
- Team: General
- Member: Fry
- Permissions: Remove permissions:
4. Removing permissions from someone who has inherited permissions from the Parent domain (Fry)
- Team: Andromeda
- Member: Fry
- Permissions: Remove permissions:
5. Upgrading a user's role, when the user has an inherited role (alex-the-ace)
- Team: Andromeda
- Member: alex-the-ace
- Permissions: Admin
6. Downgrading a user's role, when the user has an inherited role (alex-the-ace)
- Team: Andromeda
- Member: alex-the-ace
- Permissions: Mod:
7. Applying the same inherited role for a user (alex-the-ace)
- Team: Andromeda
- Member: alex-the-ace
- Permissions: Payer:
8. Upgrading a user's role, when the user has inherited Custom permissions (diana-dynamo)
- Team: Andromeda
- Member: diana-dynamo
- Permissions: Admin
9. Downgrading a user's role, when the user has inherited Custom permissions (diana-dynamo)
- Team: Andromeda
- Member: diana-dynamo
- Permissions: Mod:
10. Applying the same inherited permissions for a user (diana-dynamo)
- Team: Andromeda
- Member: diana-dynamo
- Permissions: Custom:
11. Checking if the inherited permission toggles are disabled (diana-dynamo)
- Team: Andromeda
- Member: diana-dynamo
- Permissions: Custom:
12. Removing Root permissions from someone who does not have inherited permissions from a Parent domain (Amy)
- Team: Andromeda
- Member: Amy
- Permissions: Remove permissions:
- it's basically the bullet-pointed list of Colony actions pertinent to the user's Role, in this case it's Admin
- The table body copy should say "Removal of the following Colony actions"
- The table header should say "Remove {role} type"
- The Redo action button should not be available
- You should see the same table UI as you did prior to submission
13. Removing Root permissions from someone in a Parent domain (Fry)
- Team: General
- Member: Fry
- Permissions: Custom permissions:
- You should not be able to click the "Update permissions" button
- You should not be able to click the "Update permissions" button
- You should not be able to click the "Update permissions" button
- Basically, you should see the permissions you will lose, in this case, downgrading to Admin permissions will take away your Root & Recover permissions
14. Upgrading Custom permissions to a Role (diana-dynamo)
- Team: Andromeda
- Member: diana-dynamo
- Permissions: Custom
- Toggle on Funding
This should not say inherited anymore since the Custom Permissions are customised on Team Andromeda
- The Administration & Arbitration toggles are still disabled because these are inherited from the Parent domain
- The Funding toggle is switched on and it should be editable
- The Architecture toggle is switched off but you should be able to edit it
Which makes sense because the Custom Permissions for this domain are left untouched and are the same as the Parent domain
15. Checking the disabled state of Permissions toggles in the Completed Action component (eddy-edge)
- Team: Serenity
- Member: eddy-edge
- Permissions: Custom:
- Even though the Arbitration toggle is disabled, verify that it is not greyed out
Diffs
Changes 🏗
getFormOptions
Resolves #2339, #2241 and #2755