user10659686
user10659686

Reputation:

why doesnt my powershell script compare dates correctly?

trying to compare dates if its todate or greater then do not enable, if its less then or equal to today, enable account based on the description

enter image description here

    $newemployees = Get-aduser -Filter * -Properties whencreated, Description, Samaccountname | Where-Object {$_.whenCreated -gt $Datecutoff}  `
| Select-Object Description, enabled, Samaccountname | Where-Object {$_.Enabled -eq $False}
$DateCutOff=(Get-Date).AddDays(-14)

$lastpasstest = Get-ADUser -Identity lastpasstest -Properties Description, Samaccountname | Select-Object Description, enabled, Samaccountname

$today = Get-Date -format MM/dd/yyyy
Write-Output "todays Date: $today"

$newpwd = ConvertTo-SecureString -String "apasswordwemightuse!" -AsPlainText –Force

ForEach ( $user in $newemployees ) {
    if ($user.description -ge $today)
    {
    Write-Output "Start Date: $($user.description)"
    Write-Output "Users will be enabled on their start date: $($user.SamAccountName)"
    } 
    if ($user.description -le $today) {
     Write-Output "Enabling User: $($user.SamAccountName) $($user.description)"
     $user.SamAccountName | Enable-ADAccount 
     Set-ADAccountPassword $user -NewPassword $newpwd -Reset
     Set-ADuser $user -ChangePasswordAtLogon $True
    }
}

Upvotes: 0

Views: 919

Answers (1)

TheMadTechnician
TheMadTechnician

Reputation: 36277

There's a few things here, so let's go over them one at a time, but first for the TLDR folks: The answer to your original question is that you're comparing strings. The $User.description value is a string, and you converted that date to a string for $today when you formatted it. In order to compare dates you would need the value type on the left of the operator to be a [datetime] object.

Now let's get down to the business of really fixing this here.

The first issue I see is that you define $DateCutOff in line 3 after you try to use it in a comparison in line 1. So we move line 3 to the top, simple enough.

Next you get all of the users in AD, then whittle them down with Where-Object statements. That's asking for a lot, it is far better to use the -Filter parameter to only ask AD for the users you want. Let's convert the lines that you query AD to do that with this:

$DateCutOff = (get-date).AddDays(-14)
$newemployees = Get-aduser -Filter {Enabled -eq $false -and whenCreated -gt $DateCutOff} -Properties whencreated, Description, Samaccountname | 
    Select-Object Description, enabled, Samaccountname 

That should only get the users that you want from AD, which should result in fewer timeouts and faster responses in general.

Then we have a line that appears to serve no purpose so I'm skipping it. Moving down to when you define $today. You use the -f parameter, which is used to format the date as a string. In order to keep it a date you would need to leave that part off. If you want it formatted you can always do that later.

$today = get-date

Personally, and this is my preference, I skip defining things like that and just use [datetime]::today in their place. That's just me, but I thought I'd put it in here in case you wanted to do so as well.

Now, because we changed that to a date, your write-output line won't look the same. For that matter, using Write-Output is redundant. Anything not otherwise explicitly redirected is sent to the output. So we can just change that to this:

"todays Date: {0}" -f $today.ToString("MM/dd/yyyy")

After that you define the password, which is in plain text, and in general is a no-no, but it's functional so I'll leave it alone.

$newpwd = ConvertTo-SecureString -String "apasswordwemightuse!" -AsPlainText –Force

Now we get to the loop where you check if users should be enabled. I mentioned above that you are comparing strings, but it is worth noting that when it comes to comparisons PowerShell looks at the value on the left's type and tries to convert the value on the right to match. So really the simplest way to fix your original script is to type cast the description as a [datetime] object like If([datetime]($user.description) -gt $today){, but that seems dirty to me since it would then have to convert $today to a datetime each time as well as the description. Better to have at least one be a datetime already and work from there. So what I would do is put today's datetime object first, and compare the description to that, like this:

ForEach ( $user in $newemployees ) {
    if ([datetime]::today -lt $user.description)

Beyond that you run users through If($user.description -ge $today) and If($user.description -le $today), but really it's going to be one or the other, so you could just do the second part as an else clause. That said, here's what I'd have done:

ForEach ( $user in $newemployees ) {
    if ([datetime]::today -lt $user.description)
    {
        "Start Date: $($user.description)"
        "Users will be enabled on their start date: $($user.SamAccountName)"
    } 
    else
    {
        "Enabling User: $($user.SamAccountName) $($user.description)"
        $user.SamAccountName | Enable-ADAccount 
        Set-ADAccountPassword $user -NewPassword $newpwd -Reset
        Set-ADuser $user -ChangePasswordAtLogon $True
    }
}

Lastly, you write a fair bit to screen. You might want to convert that to write-verbose so that it quietly runs, but if you need to keep a closer eye on things you can run it with the -verbose switch, or change your $VerbosePreference for the session.

Upvotes: 2

Related Questions