1

I am trying to get a script to work that will organize my active directory accounts based off of their display name since all of our accounts have their OU in their name (or a subOU). I am trying to do this with an If statement inside of a ForEach loop in PowerShell. Every time I run it though, it keeps asking me for an identity. Can anyone help me fix this? This is what I have...

Import-Module ActiveDirectory
$OU = "OU=Test, OU=com"
$Test1OU = "OU=Test1, OU=Test, OU=Com"
$Test2OU = "OU=Test2, OU=Test, OU=Com"

$Users = (Get-ADUser -SearchBase $OU -Filter * -Properties samAccountName,DisplayName)
ForEach ($user in $users)
{
If ($($user.DisplayName -like ("*Supply*" -or "*Supplies*"))
{Move-ADObject -Identity $($user.samAccountName -TargetPath $Test1OU}
ElseIf ($($user.DisplayName -like ("*Accounting*" -or "*Accountant*"))
{Move-AdObject -TargetPath $Test2OU}
}
WeeWoo
  • 27
  • 5
  • `Move-ADObject` WHAT? You specify the destination but don't specify the object to move. – Vesper Jun 18 '15 at 10:35
  • Still new to PowerShell tbh, but I thought that the object to move was the object in the if statement that has a displayname -like "*supply*", etc... – WeeWoo Jun 18 '15 at 10:39
  • You have to supply the object to cmdlet anyway, and it should be an AD object, not a select result. You should use `Get-ADUser` raw output to manipulate them instead of doing `select` and working with strings. You can still get strings off a user list if you do `foreach ($user in $users) { $_.displayname}` – Vesper Jun 18 '15 at 10:45
  • So I changed the if statement to... If($_.DisplayName -like ("*Supply*" -or "*Supplies*")) and ForEach to ($User in $Users) and the command runs but doesnt do anything...at least I have one problem down...I also made sure to use Get-ADUser as well, do you see anything else I'm doing wrong? – WeeWoo Jun 18 '15 at 11:06
  • Do not combine `-like` checks, `-like` expects a string at its right, not an expression with `-or` which makes it a boolean. Also, using `-match` would let you drop wrapping asterisks in either string you want it to match. `if ($_.DisplayName -match "Supply" -or $_.displayname -match "Supplies") ...` – Vesper Jun 18 '15 at 11:26
  • You might want to update the question by editing it, then adding current revision of your script (that doesn't work as intended) with symptoms of its "not-working". – Vesper Jun 18 '15 at 11:27

2 Answers2

0

I'm not able to test currently, but this should do the trick:

Import-Module ActiveDirectory

$OU = "OU=Test, OU=com"
$Test1OU = "OU=Test1, OU=Test, OU=Com"
$Test2OU = "OU=Test2, OU=Test, OU=Com"

$users = (Get-ADUser -SearchBase $OU -Filter * -Properties displayName)
foreach ($user in $users)
{
    if ($($user.displayName) -like "*Supply*" -OR $($user.displayName) -like "*Supplies*")){
        Move-ADObject -Identity $user -TargetPath $Test1OU
    }
    elseif ($($user.displayName) -like "*Accounting*" -OR $($user.displayName) -like "*Accountant*")) {
        Move-AdObject -Identity $user -TargetPath $Test2OU
    }
}

I've Added an Identity Parameter to Move-ADObject also i've changed some of the var names to better reflect their content.

AndyMeFul
  • 489
  • 4
  • 13
  • 1
    `"*Supply*" -or "*Supplies*"` would always evaluate to true. Check the if conditions closely then I think the whole thing would be true .... not sure. – Matt Jun 18 '15 at 11:27
  • @Matt Yep, this will be equal to `($($user.displayName) -like "True")`. Hehe. – Vesper Jun 18 '15 at 11:36
  • `Move-ADObject` does not need you to provide a `samAccountName` value if you already have `$user` as the object to be moved. Therefore it'll be better to replace this big structure to `Move-AdObject -Identity $user -targetpath ...` – Vesper Jun 18 '15 at 11:38
  • Yeah the -or would have been wrong, I've modified now so they'll work correctly, also as @Vesper suggested i've used $user as the Identity. – AndyMeFul Jun 18 '15 at 11:45
0

You are running into a few problems here

  1. Like Vesper said you are not passing anything to Move-ADObject hence the error you are getting
  2. $DisplayNames is not a string array of names but an object with a displayname property. That is what -ExpandProperty parameter is for with Select-Object FYI.
  3. You are pulling all the users but only really want to process certain ones. Instead of -Filter * lets use a more targeted approach.
  4. While it is tempting you cant nest -like conditions like that. If you take "*Supply*" -or "*Supplies*" and type that it will evaluate to true. Same as all non zero length strings.

For what we plan on doing we will not have to address all those issues. We should use the pipeline to help with this. Depending on how many variances you have something like a switch statement might be better which is covered below.

$supplyFilter = 'DisplayName -like "*Supply*" -or DisplayName -like "*Supplies*"'
$accountFilter = 'DisplayName -like "*Accounting*" -or DisplayName -like "*Accountant*"'

Get-ADUser -SearchBase $OU -Filter $supplyFilter -Properties displayName | Move-ADObject -TargetPath $Test1OU
Get-ADUser -SearchBase $OU -Filter $accountFilter -Properties displayName | Move-ADObject -TargetPath $Test2OU

You could get freaky with this and make a custom object in a loop with filter and target pairs so that you don't need to repeat the cmdlet call to each Get-ADuser instance.

$moves = @(
    @{
        Filter = 'DisplayName -like "*Supply*" -or DisplayName -like "*Supplies*"'
        OU = "OU=Test1, OU=Test, OU=Com"  
    },
    @{
        Filter = 'DisplayName -like "*Accounting*" -or DisplayName -like "*Accountant*"'
        OU = "OU=Test2, OU=Test, OU=Com"
    }
) | ForEach-Object{New-Object -TypeName PSCustomObject -Property $_}

ForEach($move in $moves){
    Get-ADUser -SearchBase $OU -Filter $move.Filter -Properties displayName | Move-ADObject -TargetPath $move.OU
}

You should be able to scale into this easily by adding new $moves. This would be cleaner with PowerShell v3.0 but I do not know what version you have.

Using a switch

If you want something closer to what your currently have I would suggest something like this instead then.

$Users = Get-ADUser -SearchBase $OU -Filter * -Properties DisplayName
ForEach ($user in $users){
    switch($user.DisplayName)  {
        ($_ -like "*Supply*" -or $_ -like "*Supplies*"){Move-ADObject -Identity $user -TargetPath $Test1OU}
        ($_ -like "*Accounting*" -or $_ -like "*Accountant*"){Move-ADObject -Identity $user -TargetPath $Test1OU}
    }
}
Matt
  • 45,022
  • 8
  • 78
  • 119
  • This is what I should have done in the first place since it's more in my league. However, I was trying to make something a little more like mine mostly for the experience working with ForEach and If's and to run something that is a bit less intensive on the network. Not to mention, I want something the guys in my office can just click and run without messing with my scripts. Thanks for your help, I am working on this one and will most likely stick to these types in the future! – WeeWoo Jun 18 '15 at 11:50
  • @WeeWoo I don't understand the comment about _something that is a bit less intensive on the network_. My version is significantly more efficient. What changes are you anticipating? What you have right now need to change? Do you figure that your script currently would be easier to scale out from? – Matt Jun 18 '15 at 11:54
  • The network has over 3000 users spread out in about 5 OUs. I'm assigned to the task of organizing them in the right OUs and then renaming them all...I just thought it might be rough on the network to run another line for each OU since it will search through every account each time it looks for the filter. – WeeWoo Jun 18 '15 at 12:08
  • I would think its rougher to pull everything if you are not going to use it. Running multiple targeted command I would think it less taxing then pulling everything back. I don't have the user base to benchmark this comparison. @WeeWoo I have provided 3 solutions that _should_ all work. Have a look and see if something suits your needs. – Matt Jun 18 '15 at 12:15
  • It looks like the switch might be easier for me to use and is a lot closer to what I was originally looking for. I'm still really new to PowerShell so forgive my ignorance. I'm not really sure which would be better. I'm going to have to research how a switch works. The array you showed me might actually be better...I will get to testing and let you know. – WeeWoo Jun 18 '15 at 12:39
  • So I tested the Array version and it worked! Mostly...It moved 4 out of the 5 test accounts into the right OUs just as I had hoped. However the 5th that should have went into the same OU as 2 others, did not move. For example, "John Doe 1Supply" went to the "Test1" OU, however "Sally Green 3Supply" did not move at all. Sorry my examples suck but I can't really use the real naming standards or OU structure. – WeeWoo Jun 18 '15 at 12:52
  • @WeeWoo are you sure those are the displaynames for those users and not just name – Matt Jun 18 '15 at 12:57
  • My mistake, usually they are the same, but since I've used these test accounts testing other things, the display name was missing the "Supply" part >.< Will have to wait for it to replicate and try again. – WeeWoo Jun 18 '15 at 12:59