-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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(invite-members): enable member role in dropdown #77871
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #77871 +/- ##
===========================================
- Coverage 88.40% 78.12% -10.28%
===========================================
Files 3000 6994 +3994
Lines 186662 310204 +123542
Branches 30663 50752 +20089
===========================================
+ Hits 165012 242343 +77331
- Misses 15621 56136 +40515
- Partials 6029 11725 +5696 |
@@ -31,7 +38,10 @@ function RoleSelectControl({roles, disableUnallowed, ...props}: Props) { | |||
({ | |||
value: r.id, | |||
label: r.name, | |||
disabled: disableUnallowed && !r.isAllowed, | |||
disabled: | |||
disableUnallowed && |
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.
Should these be && or || 🤔
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.
i believe && is fine, we want to disable if its not allowed and if its not a member invite or the inviting roll is member. demorgan's law makes the && valid i think.
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.
1 main question about the component being reused couple places
organization.features.includes('members-invite-teammates') && | ||
organization.allowMemberInvite && | ||
organization.access?.includes('member:invite'); | ||
|
||
return ( | ||
<SelectControl |
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.
fwiw this component is used in a couple places in our code, we are ok with this logic to change in all of the places right?
@@ -31,7 +38,10 @@ function RoleSelectControl({roles, disableUnallowed, ...props}: Props) { | |||
({ | |||
value: r.id, | |||
label: r.name, | |||
disabled: disableUnallowed && !r.isAllowed, | |||
disabled: | |||
disableUnallowed && |
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.
i believe && is fine, we want to disable if its not allowed and if its not a member invite or the inviting roll is member. demorgan's law makes the && valid i think.
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.
the other places the component is being used should also have support for member invite
fix for the "Member" role being disabled for members sending invites <img width="886" alt="Screenshot 2024-09-20 at 1 43 16 PM" src="https://github.com/user-attachments/assets/c6735fd4-9e06-4c1d-9321-e4ae1b041f80"> chose to fix this on the frontend rather than modifying the GET request for this specific page so that the backend continues to returns the same result for all pages
fix for the "Member" role being disabled for members sending invites
chose to fix this on the frontend rather than modifying the GET request for this specific page so that the backend continues to returns the same result for all pages