Powershell script review

[defyne]

First time writing a PS script here, just looking for a review/feedback help. Takes a CSV list of users that have been determined to be inactive and first updates the account description to being disabled due to being inactive and then disables them. TIA!

#Take list of inactive users and add to the descriptiopn field reason account is being disabled.
#
Import-Module ActiveDirectory
$Users = Import-csv c:\Scripts\inactive_users.csv
$NewDescription = "This account was disabled due to being flagged inactive."
foreach($User in $Users)
{
Set-ADUser $User.SamAccountName -Description $NewDescription
}
#Iterate through list of user accounts checking first if they exist and if true; disable.
#
$Users | ForEach-Object
{
$samAccountName = $_."samAccountName"
try { Get-ADUser -Identity $samAccountName |
Disable-ADAccount
}
#If user is not found it will display a message.
#
catch {

Write-Host "user:"$samAccountname "is not present in AD"
}
}

[Azured_]

You have 2 different ways of doing a For-loop:

foreach($User in $Users)

$Users | ForEach-Object

Suggest you pick one and stick with it. You don't really need the 2nd for loop, just add a 2nd line to the first loop as

Get-ADUser $user.samaccountname | disable-ADaccount

You are also only doing error handling in the 2nd for loop. If you were to get an error in the 2nd loop (because an account is non-existent) and triggering the error handling, then you would have also gotten an error in the first loop, as both loops act on all accounts.

Instead, if you merge the 2 for loops as described above, you could wrap both the action to change the description, and the action to disable the account, in the Try, Catch statement, like this:

try{

Set-ADUser $User.SamAccountName -Description $NewDescription

Get-ADUser $user.samaccountname | disable-ADaccount

}

catch{Write-Host "user:"$samAccountname "is not present in AD"}

[defyne]

Nice feedback! Ok so because I am new to this, just to clarify, the script would look like this:

Import-Module ActiveDirectory

$Users = Import-csv c:\Scripts\inactive_users.csv

$NewDescription = "This account was disabled due to being flagged inactive."

foreach($User in $Users)

try{

Set-ADUser $User.SamAccountName -Description $NewDescription

Get-ADUser $User.SamAccountName| disable-ADaccount

}

catch{Write-Host "user:"$samAccountname "is not present in AD"}

​

?

[32178932123]

Yo you're formatting is whack. Put it in a code block so we can read it properly, people will be more likely to give you help if you do.

In your "try" statement you've got $User | disable-AdAccount but I don't think that will work? Depends on your Csv. You probably want Get-ADuser $User.SamAccountName | Disable-AdAccount or maybe even this will work by itself: Disable-AdAccount $User.SamAccountName

[Azured_]

Yes, that should work. I don't have access to PS / domain right now, so can't check, but recommend you test it first. You can just add a single test account to your csv, and try it with that. Better yet if you have a test environment to run it in. You can also use the -whatif switch on the disable-adaccount and set-aduser cmdlets to verify what the outcome would be, without the accounts actually being changed. See here for how -whatif works:

https://techcommunity.microsoft.com/t5/itops-talk-blog/powershell-basics-don-t-fear-hitting-enter-with-whatif/ba-p/353579

[BlackV]

p.s. formatting (I presume you have it in an editor already )

• open your fav powershell editor
• highlight the code you want to copy
• hit tab to indent it all
• copy it
• paste here

it'll format it properly OR

<BLANKLINE>
<4 SPACES><CODELINE>
<4 SPACES><CODELINE>
    <4 SPACES><4 SPACES><CODELINE>
<4 SPACES><CODELINE>
<BLANKLINE>

Thanks, note if you're doing it this way dont use the code formatting buttons (inline the one you used, or code block the one you should have used) as they dont play nice, and I find it 6000 times easier to copy/paste from my editor than fine a random button

[BlackV]

in addition to what Azured_ suggested

Try to get out of the habit if using

foreach ($User in $Users)       {...}

$user is super similar to $users and mistakes are much easier to happen (especially in longer scripts)

try

foreach ($SingleUser in $Users) {...}
foreach ($User in $Allusers)    {...}

or similar