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

Show a clear error message when user has no permission to delete pod #169

Open
wants to merge 161 commits into
base: master
Choose a base branch
from

Conversation

dherault
Copy link
Contributor

Summary

So after investigation I've decided on a simple message Note: ... instead of a full role checking for three reasons:

  • Roles are paginated, it required reading the full list (with fetchMore) before informing the user.
  • Roles are undocumented. I didn't know which roles were needed to perform that action.
  • Roles are repository based. And accessing the repository at this point in the application is heavy.

Labels

Test Plan

Checklist

  • If required, I have updated the Plural documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a meaningful title and summary to convey the impact of this PR to a user.
  • I have added relevant labels to this PR to help with categorization for release notes.

@dherault dherault added the frontend Changes related to the frontend label Dec 12, 2022
@dherault dherault self-assigned this Dec 12, 2022
@@ -63,7 +63,7 @@ export function DeleteJob({ name, namespace, refetch }) {
/>
{confirm && (
<Confirm
description="The pod will be replaced by it's managing controller"
description="The pod will be replaced by it's managing controller. Note: You need the ADMIN/DELETE role to perform this action."
Copy link
Member

Choose a reason for hiding this comment

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

What I'd add here is error handler for mutation above. Because now it's still not clear if any error occurred. Just a simple banner or something like in other places.

Base automatically changed from reskin to master February 7, 2023 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Changes related to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants