Code review: Automatically add/remove Permission Set Assignment (Apex Trigger)

Question

I have created an "After Trigger" to automatically assign/remove a permission set to a user. The trigger works fine, but from what I understand, line 15 of my code is not best practice:

PermissionSetId = tcrmAssignment[0].PermissionSetId;

What the trigger does is anytime a user is created or activated, a permission is automatically assigned to the user. When a user is inactive, the permission is removed.

I first did this requirement using Flow builder. But for some reasons the Flow builder does not work for 2 other System Admin users even though their profiles have the exact same parameters (meaning Flow User checked on their profile, as well as all the appropriate permissions and rights as mine and the rest of Admins), but for the other 3 System Admin including myself the Flow Builder works.

That's the reason I took the trigger route, which is not my preferred choice.

As I said: The code works fine, but I read somewhere that we should not assign an Id to a record in this manner:

PermissionSetId = tcrmAssignment[0].PermissionSetId;

Could someone help reviewing my code to see, if it has been correctly written?

Please see below the Handler class. Thanks!

public class TCRMPermissionSetAssignmentHandler {
    
public static void addTCRMPermissionSetAssignment(List<User> userList) {
        
    List<PermissionSetAssignment> tcrmPermAssignmentList = new List<PermissionSetAssignment>();
    List<PermissionSetAssignment> tcrmAssignment = new List<PermissionSetAssignment>();
    for(PermissionSetAssignment perm : [SELECT PermissionSetId, AssigneeId FROM PermissionSetAssignment WHERE PermissionSet.Name = 'EinsteinAnalyticsUser']) {
        tcrmAssignment.add(perm);
    }
    update tcrmAssignment;
                                
    for(User newUser : userList) {
        if(newUser.IsActive == true) {
            tcrmPermAssignmentList.add(new PermissionSetAssignment(AssigneeId = newUser.Id, 
                                                                   PermissionSetId = tcrmAssignment[0].PermissionSetId));
        }
    }
    insert tcrmPermAssignmentList;
    
}

public static void removeTCRMPermissionSetAssignment(List<User> userList) {
    Set<Id> userIds = new Set<Id>();
    for(User u : userList) {
        if(u.IsActive == false) userIds.add(u.Id);
    }
    List<PermissionSetAssignment> tcrmAssignmentList = [SELECT Id, AssigneeId, PermissionSetId FROM PermissionSetAssignment
                                                        WHERE AssigneeId = :userIds AND PermissionSetId IN 
                                                        (SELECT Id FROM PermissionSet WHERE IsOwnedByProfile = false)];                                                                      
    delete tcrmAssignmentList;
  
} 

}

Answer 1

I'm not sure why you're:

update tcrmAssignment;

Since you're not actually modifying anything here, and there's no triggers that could possibly fire. Further:

tcrmAssignment[0].PermissionSetId

Assumes there will be an existing permission set. You shouldn't assume something that might not be there.

Beyond that, if there are no active users with this permission set, it won't be found, since you're querying PermissionSetAssignment instead of PermissionSet.

A more appropriate implementation would be:

public static void addTCRMPermissionSetAssignment(List<User> userList) {
    try {
        PermissionSetAssignment[] newAssignments = new PermissionSetAssignment[0];
        PermissionSet einsteinAnalyticsUserPS = [SELECT Id FROM PermissionSet WHERE Name = 'EinsteinAnalyticsUser'];
        // Users already assigned this permission set
        Set<Id> existingUsersWithPSA = new Map<Id, AggregateResult>([
            SELECT AssigneeId Id
            FROM PermissionSetAssignment
            WHERE AssigneeId = :userList
            GROUP BY AssigneeId
        ]).keySet();
        for(User userRecord: userList) {
            if(userRecord.IsActive && !existingUsersWithPSA.contains(userRecord.Id)) {
                newAssignments.add(new PermissionSetAssignment(AssigneeId=userRecord.id, PermissionSetId=einsteinAnalyticsUserPS.Id));
            }
        }
        insert newAssignments;
    } catch(SoqlException ex) {
        // There was no permission set to get
    }
}

---

    if(u.IsActive == false) userIds.add(u.Id);

Don't do this. This line of code is just fine, but if you get in to this habit, you'll one day end up with some code that looks like this:

if(conditionA && conditionB && conditionC && conditionD && conditionE && conditionF && conditionG) doSomethingElse();
doSomething();

And you (or someone not you) might misread this code, thinking that doSomething is optional, but is actually always called. Even worse, they might try to fix your code:

if(conditionA && conditionB && conditionC && conditionD && conditionE && conditionF && conditionG) doSomethingElse();
{
    doSomething();
}

Which will compile, but still always be executed.

Coincidentally, this is why I always use the rule that open curly braces must be the last character on a line, rather than a blank line by itself, because I've seen this happen in production code, and it caused a lot of unnecessary debugging.

This is even outlined in ApexPMD Code Style rules:

Avoid using if statements without using braces to surround the code block. If the code formatting or indentation is lost then it becomes difficult to separate the code being controlled from the rest.