2
\$\begingroup\$

I wrote this Powershell script years ago for updating AD from a CSV file. I'd like to try to optimize to run quicker and be more efficient... but I'm not sure where to start / what could improve it. Any suggestions are welcome.

EDIT: I've updated my original script to this:

$users = Import-Csv C:\Scripts\Employees.csv

foreach ($user in $users) {

$employeeNumber = $User.employeeNumber
$Department     = $User.department
$Title          = $User.title
$Office         = $User.office
$Address        = $User.address
$City           = $User.city
$State          = $User.state
$PostalCode     = $User.postalCode
$Company        = $User.company
$Telephone      = $User.telephone
$Mobile         = $User.mobile
$Fax            = $User.fax
$Custom1        = $User.custom1
$Custom2        = $User.custom2
$Custom3        = $User.custom3
$Custom4        = $User.custom4

Get-ADUser -Filter "EmployeeID -eq '$($user.EmployeeID)'" `
    -SearchBase "OU=Logins,DC=domain,DC=com" |
    Set-ADUser -Replace @{ `
    employeeNumber=$EmployeeNumber;
    department=$Department;
    title=$Title;
    Office=$office;
    streetAddress=$Address;
    l=$City;
    st=$State;
    postalCode=$PostalCode;
    company=$Company;
    telephoneNumber=$telephone;
    mobile=$cell;
    facsimileTelephoneNumber=$Fax;
    ExtensionAttribute1=($user.custom1);
    ExtensionAttribute2=($user.custom2);
    ExtensionAttribute3=($user.custom3);
    ExtensionAttribute4=($user.custom4)
                    }
}
\$\endgroup\$
  • \$\begingroup\$ Can you add some of the errors you're receiving? \$\endgroup\$ – veefu Sep 6 '18 at 20:29
  • \$\begingroup\$ I know it looks bad to have "set-aduser" twice, but I can't find any way to include the extension attributes in with the first group of attributes. While I can use "Set-ADUser -EmployeeNumber", for the extension attributes, I seem to have to use "Set-ADUser -Replace @{ExtensionAttribute1.....". Is there a better way to do that specifically? \$\endgroup\$ – Jarrod Sep 7 '18 at 15:17
  • \$\begingroup\$ Hrm... I suspect that any attribute may be changed using -Replace [hashtable] syntax, even if a cmdlet parameter is also provided for the attribute. The documentation contains an example that does this with the Title attribute: Set-ADUser -Identity GlenJohn -Replace @{title="director";mail="glenjohn@fabrikam.com"}. \$\endgroup\$ – veefu Sep 7 '18 at 16:13
  • \$\begingroup\$ It doesn't seem possible to include "Replace" for the extension attributes in the same Set-ADUser as the rest of the properties. That's ok though, I can just keep it as two separate ones. My problem now is getting errors due to blanks in the CSV, which I'd like it to skip and keep going. \$\endgroup\$ – Jarrod Sep 7 '18 at 18:25
  • \$\begingroup\$ I was unclear. My suggestion is to remove all the -PropertyName arguments from the Set-ADUser and move them into the hashtable passed to -Remove. \$\endgroup\$ – veefu Sep 7 '18 at 19:53
2
\$\begingroup\$

I agree with what @veefu suggests, and would like to build off that. To start with, you have different columns in your CSV as to what Set-ADUser is expecting. To work around this I would suggest creating a property map hashtable.

$PropMap = @{
    EmployeeNumber='EmployeeNumber'
    Department='Department'
    Title='Title'
    office='Office'
    Address='StreetAddress'
    City='City'
    State='State'
    PostalCode='PostalCode'
    Company='Company'
    telephone='OfficePhone'
    cell='Mobile'
    Fax='Fax'
    custom1='ExtensionAttribute1'
    custom2='ExtensionAttribute2'
    custom3='ExtensionAttribute3'
    custom4='ExtensionAttribute4'
}

Then you want to find out what the valid parameters for Set-ADUser are to determine if a given column should be applied as a parameter, or be fed to the -Replace parameter.

#Find the valid parameters for the Set-ADUser cmdlet (only strings, to avoid complex objects and switches, and no common parameters)
$ValidParams = Get-Command Set-ADUser -ShowCommandInfo |% ParameterSets |% Parameters |?{$_.ParameterType.FullName -match 'String' -and $_.Name -notin [System.Management.Automation.PSCmdlet]::CommonParameters} |Select -Expand Name -Unique

Then you import the CSV and loop through the users. For each user you find what properties they have filled in on the spreadsheet, see if it is a parameter for the Set-ADUser cmdlet, and build a hashtable to splat to the cmdlet.

#Import the list of users and updates
$Users = Import-Csv C:\Scripts\Employees.csv

#Loop through users
ForEach($User in $Users){
    #Set an empty hashtable for what this user needs to have updated
    $UserParams = @{}
    #Find the properties for this user that have values
    $UserProps = $User.PSObject.Properties.Name |?{![string]::IsNullOrWhiteSpace($User.$_)}
    #If it has a parameter in Set-ADUser add it to $UserParams
    $UserProps |?{($ParamName=If($_ -in $PropMap.Keys){$PropMap[$_]}Else{$_}) -in $ValidParams} |%{$UserParams.Add($ParamName,$User.$_)}
    #If it doesn't have a valid parameter build a hashtable for the -Replace parameter, and add that to $UserParams
    $UserProps |?{($ParamName=If($_ -in $PropMap.Keys){$PropMap[$_]}Else{$_}) -notin $ValidParams} |% -Begin {$ToReplace = $_} -Process {$ToReplace.Add($ParamName,$User.$_)} -End {$UserParams.Add('Replace',$ToReplace)}

Here I added a little Try/Catch to let you know if it failed to find or update any of the users, and if it failed to find them, or failed to update them, and gives the error. I used the shortened -ea instead of -ErrorAction out of habit.

    #Try to find the user, and apply the updates
    Try{
        Get-AdUser -Filter "EmployeeID -eq '$($user.EmployeeID)'" -SearchBase "ou=Logins,dc=domain,dc=com" -ea Stop | Set-ADUser @UserParams -ea Stop
    }Catch{
        #If the Get-ADUser or Set-ADUser cmdlets failed throw a warning with the user's EmployeeID for investigation

        #Determine if it was Get-ADUser or Set-ADUser that failed, to see if we couldn't find the user or couldn't update the user
        $WarningMessage = If($_.InvocationInfo.MyCommand -eq 'Get-ADUser'){'find'}else{'update'}
        Write-Warning "Unable to $WarningMessage user with EmployeeID (actual error follows): $($User.EmployeeID)"
        Write-Warning "$_`n"
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thank you both for your input, much appreciated! @TheMadTechnician I'm getting and error on this line: $ValidParams = Get-Command Set-ADUser -ShowCommandInfo |% ParameterSets |% Parameters |?{$_.ParameterType.FullName -match 'String' -and $_.Name -notin [System.Management.Automation.PSCmdlet]::CommonParameters} |Select -Expand Name -Unique The error is: You must provide a value expression on the right-hand side of the '-' operator. I'm not sure where it's looking for a value though. \$\endgroup\$ – Jarrod Sep 7 '18 at 14:13
  • \$\begingroup\$ I am not able to replicate the issue. Try running Update-Help -Module ActiveDirectory -Force or run the line in parts. Do you get anything for [System.Management.Automation.PSCmdlet]::CommonParameters? or Get-Command Set-ADUser -ShowCommandInfo |% ParameterSets |% Parameters? \$\endgroup\$ – TheMadTechnician Sep 8 '18 at 1:07
1
\$\begingroup\$

Here are a few comments:

# -3. No parameters to the script and hard-coded path to the input CSV
#     This makes the script harder for others to reuse
# -2. No error handling
#     if an employee in the CSV doesn't exist in AD, for instance,
#     you might want to know about it.
# -1. Rigid requirements for the CSV file
#     What happens if one of the columns is missing? 
#     Would it be worthwhile to add flexibility logic, so 
#     so the script could handle CSVs containing fewer columns of
#     modification?

$users = Import-Csv -Path C:\Scripts\Employees.csv
# 0. Style - use more vertical space in your code for better readability

foreach ($user in $users) {
# 1. Eliminate duplicate Get-ADUser calls
Get-ADUser -ErrorAction SilentlyContinue -Filter "EmployeeID -eq '$($user.EmployeeID)'" `
        -Properties * -SearchBase "ou=Logins,dc=domain,dc=com" |
# 2. Eliminate '-properties *': your script doesn't read 
#    current user property values, so why retrieve them?

# 3. Consider eliminating multiple calls to Set-ADUser
#    All modifications can probably be done in a single
#    Set-ADUser -Replace $Properties
#    call. You'd need to reorganize the modifications into
#    a single hashtable.

    Set-ADUser -EmployeeNumber $($user."EmployeeNumber") `
        -Department $($user."Department") `
        -Title $($user."Title") `
        -Office $($user."office") `
        -StreetAddress $($user."Address") `
        -City $($user."City") `
        -State $($user."State") `
        -PostalCode $($user."PostalCode") `
        -Company $($user."Company") `
        -OfficePhone $($user."telephone") `
        -Mobile $($user."cell") `
        -Fax $($user."Fax")

Get-ADUser -ErrorAction SilentlyContinue -Filter "EmployeeID -eq '$($user.EmployeeID)'" -Properties * -SearchBase "ou=Logins,dc=domain,dc=com" |
    Set-ADUser -Replace @{
            ExtensionAttribute1=($user.custom1) 
            ExtensionAttribute2=($user.custom2)
            ExtensionAttribute3=($user.custom3)
            ExtensionAttribute4=($user.custom4)
        }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! \$\endgroup\$ – Mast Sep 7 '18 at 7:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.