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

refactor: split Staff Dev role into separate roles, add CM vanity role #2972

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

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Dec 23, 2024

This PR repurposes the currently-unused Staff Developer role, splitting it into the three staff dev roles that have emerged since the original permissions matrix was envisioned. Those roles are:

  • Quality Assurance
  • Developer Compliance
  • Code Reviewer

Each of these three roles has distinct responsibilities, and should ultimately have distinct abilities. There is enough nuance in their day-to-day that this split should occur.

Also, the way Staff Developer was written implied that a user would be promoted from Developer to Staff Developer. However, in practice, QAM/DevC/CR are additive to Developer, giving that Developer additional privileges. A full developer may have 1 to N of these three roles.

Additionally, a new vanity role has been added: Community Manager. This can be attached to users, but it should not have any abilities. A Community Manager should inherit abilities from other roles like Moderator, etc.


SQL commands to run:

-- 1: Detach developer-staff from any users.
DELETE FROM auth_model_roles
WHERE role_id = (SELECT id FROM auth_roles WHERE name = 'developer-staff');

-- 2: Remove developer-staff from the roles table.
DELETE FROM auth_roles WHERE name = 'developer-staff';

-- 3: Insert new roles.
INSERT INTO auth_roles (name, display, guard_name, created_at, updated_at) VALUES
    ('dev-compliance', 4, 'web', NOW(), NOW()),
    ('quality-assurance', 4, 'web', NOW(), NOW()),
    ('code-reviewer', 4, 'web', NOW(), NOW()),
    ('community-manager', 3, 'web', NOW(), NOW());

Things to note regarding this PR:

  • There should not be any behavior changes for current users.
  • When a developer is demoted, any staff developer roles they have should be detached.
  • This is ultimately leading to users being able to select their visible role, which is currently a read-only field on the Settings page.

@wescopeland wescopeland added the deployment/sql Includes SQL that needs to be run before/after deployment label Dec 23, 2024
@wescopeland wescopeland requested a review from a team December 23, 2024 02:06
@wescopeland wescopeland changed the title refactor: split Staff Dev role into separate roles, add CM role refactor: split Staff Dev role into separate roles, add CM vanity role Dec 28, 2024
@@ -145,7 +160,7 @@ public static function boot()
if ($relationName === 'users') {
foreach ($pivotIds as $pivotId) {
$user = User::find($pivotId);
activity()->causedBy(auth()->user())->performedOn($user)
activity()->causedBy($user)->performedOn($user)
Copy link
Member

Choose a reason for hiding this comment

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

$user is overloaded here. it's already defined on line 162

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed in latest. I also added some safeguards to prevent server errors in case these code paths are entered from side effects (ie: a command, Horizon job, etc).

Copy link
Member

Choose a reason for hiding this comment

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

I see you removed the assignment of $user=Auth::user(), but this statement previously was causedBy(auth()->user()) and now it's causedBy(User::find($pivotId)).

The same change occurs a few lines up (151).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see what's happening now. I got confused.

I try to replace those auth()->user() calls with Auth::user() -- for some reason my IDE yells about auth()->user(). I think we should be in a good place now with changes in latest.

app/Policies/CommentPolicy.php Outdated Show resolved Hide resolved
app/Policies/UserPolicy.php Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment/sql Includes SQL that needs to be run before/after deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants