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

feat: disable options for downgrading the only admin #931

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

LinaYahya
Copy link
Contributor

closes #902

@LinaYahya LinaYahya self-assigned this Jan 5, 2024
@LinaYahya LinaYahya marked this pull request as ready for review January 5, 2024 15:08
@LinaYahya LinaYahya requested a review from spaenleh January 8, 2024 09:33
Copy link
Member

@spaenleh spaenleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR 🧩

Please see my comments. I had a bit of trouble understanding how the disabled permissions are computed. Could we have maybe more comments and examples of input outputs that are expected, i.e. if the user has permission write the admin value should be disabled, if the user is the last admin all permissions should be disabled.

src/components/item/sharing/TableRowPermissionRenderer.tsx Outdated Show resolved Hide resolved
src/components/item/sharing/ItemMembershipSelect.tsx Outdated Show resolved Hide resolved
src/components/item/sharing/ItemMembershipSelect.tsx Outdated Show resolved Hide resolved
src/components/item/sharing/ItemMembershipSelect.tsx Outdated Show resolved Hide resolved
@LinaYahya LinaYahya requested a review from spaenleh January 9, 2024 12:25
@spaenleh
Copy link
Member

spaenleh commented Jan 9, 2024

@LinaYahya I propose some changes.
I think that disabling all the options but allowing to click on the select makes no sens, so I added an option to completely disable the select when editing the only admin.

I also found a bug with the useMemo dependencies that made the select not update its state when adding/removing another admin. This has been fixed.

@spaenleh spaenleh merged commit a7da8b9 into main Jan 15, 2024
3 checks passed
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.

Forbidden permissions should be grayed out
2 participants