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

Added custom role option for verified users #494

Closed
wants to merge 3 commits into from

Conversation

Haimantika
Copy link

@Haimantika Haimantika commented Aug 8, 2023

What does this PR do?

Adds a condition for verified users

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

Yes

@vercel
Copy link

vercel bot commented Aug 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
console ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 0:54am
console-cloud ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 0:54am
console-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 0:54am
console-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 0:54am
console-preview-cloud ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 10, 2023 0:54am

@@ -38,6 +38,8 @@
<div>Guests</div>
{:else if role === 'any'}
<div>Any</div>
{:else if !role.startsWith('user') && !role.startsWith('team') }
<div>Users/Verfified</div>
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be {role} instead of Users/Verfified since we want to accept whatever the user sets as custom role

Copy link
Author

Choose a reason for hiding this comment

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

This should probably be {role} instead of Users/Verfified since we want to accept whatever the user sets as custom role

Yeah, makes sense. Made changes

@@ -38,6 +38,8 @@
<div>Guests</div>
{:else if role === 'any'}
<div>Any</div>
{:else if !role.startsWith('user') && !role.startsWith('team') }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right condition 🧐

image

image

Copy link
Member

Choose a reason for hiding this comment

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

True... if the custom role starts with users or teams it doesn't work

Copy link
Author

Choose a reason for hiding this comment

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

Maybe a {:else if role.startsWith('user') || role.startsWith('team') } can solve?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a {:else if role.startsWith('user') || role.startsWith('team') } can solve?

sure if that covers all the cases.

@stnguyen90
Copy link
Contributor

Also, looks like the linter fails.

Comment on lines +41 to +42
{:else if role.startsWith('user') || role.startsWith('team')}
<div>{role}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would change the behavior of if you put a specific team/user, right? This wouldn't work anymore, right?

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: Getting not found when hovering over users/verified custom permission
5 participants