-
-
Notifications
You must be signed in to change notification settings - Fork 383
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: Enable Org Admins to View and Modify Member Admin Status #8840
feat: Enable Org Admins to View and Modify Member Admin Status #8840
Conversation
Looks good :) |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Codecov Report
@@ Coverage Diff @@
## main #8840 +/- ##
=======================================
Coverage 47.88% 47.88%
=======================================
Files 65 65
Lines 20266 20266
Branches 4916 4916
=======================================
Hits 9704 9704
Misses 9307 9307
Partials 1255 1255 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
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.
@MonalikaPatnaik first thanks for the PR and you demonstrate to be quite smart at tackling the issue.
That said it seems to me we can simplify it (the most important is on the template part).
cgi/org.pl
Outdated
remove_user_from_org($org_ref, $user_id, ["admins"]); | ||
@admin_status = grep {$_ != $user_id} @admin_status; | ||
} | ||
store_org($org_ref); |
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.
store_org() should be outside the for loop, otherwise we will save the org as many times as there are users
Co-authored-by: Stéphane Gigandet <[email protected]>
@MonalikaPatnaik I did a commit to propose changes to simplify form (avoid JS). Now I let you see if it's working as desired and fix lint and critic. |
sorry @MonalikaPatnaik I didn't have time, yet to look into this but I will ! |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Let's merge :-)
Fixes #8780 --------- Co-authored-by: Pierre Slamich <[email protected]> Co-authored-by: Stéphane Gigandet <[email protected]> Co-authored-by: Alex Garel <[email protected]>
What
Video Demo
https://www.loom.com/share/bf155cf22ffe4fd3a6d3a75dd0b4429a?sid=3961c777-2553-4f23-bfad-e3e23d7913e7
Related issue(s) and discussion