xADGroup: Fix Issue #166

[devopsjesus]

Pull Request (PR) description

This PR fixes issue #166 to have the resource throw the correct error when adding a member object that does not exist in the domain to a group that does exist.

This Pull Request (PR) fixes the following issues

• Fixes ADGroup: Throws incorrect error message when MembersToInclude syntax is wrong #166

Task list

• Added an entry under the Unreleased section in the CHANGELOG.md.
  Entry should say what was changed, and how that affects users (if applicable).
• Resource documentation added/updated in resource directory README.md.
• Resource parameter descriptions added/updated in schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Resource Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
• New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

---

This change is 

[codecov-io]

Codecov Report

Merging #429 into dev will decrease coverage by <1%.
The diff coverage is 87%.

@@         Coverage Diff         @@
##            dev   #429   +/-   ##
===================================
- Coverage    92%    91%   -1%     
===================================
  Files        20     20           
  Lines      2536   2539    +3     
  Branches     10     10           
===================================
- Hits       2347   2335   -12     
- Misses      179    194   +15     
  Partials     10     10

[johlju]

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @devopsjesus)

a discussion (no related file):
Please rename back ActiveDirectory.psm1 to ActiveDirectoryStub.psm1 since the file should only contain stubs to be used when mocking (empty shells so that Pester have something to hook onto)

---

---

CHANGELOG.md, line 31 at r2 (raw file):

    ([issue #408](https://github.com/PowerShell/xActiveDirectory/issues/408)).
- Changes to xADUser
  - Fixes exception when creating a user with an empty string property ([issue #407](https://github.com/PowerShell/xActiveDirectory/pull/407)).

This row should be removed since it is the same as the entry two down (the one tow down is more correct since it has the correct url and row length.

---

CHANGELOG.md, line 32 at r2 (raw file):

- Changes to xADUser
  - Fixes exception when creating a user with an empty string property ([issue #407](https://github.com/PowerShell/xActiveDirectory/pull/407)).
  - Fixes exception when updating `CommonName` and `Path` concurrently ([issue #402](https://github.com/PowerShell/xActiveDirectory/pull/402)).

This row should be removed since it is the same as the entry two down (the one tow down is more correct since it has the correct url and row length.

---

CHANGELOG.md, line 45 at r2 (raw file):

  - Updated tests and remove unnecessary mocks of `Write-Error`.
- Changes to xADGroup
  - Fixes wrong error message when attempting to add a non-existent object to a group ([issue #166](https://github.com/PowerShell/xActiveDirectory/issues/166)).

Can we try to keep the line length to ~80 chars? Split it into multiple rows if needed. See other rows.

---

DSCResources/MSFT_xADGroup/MSFT_xADGroup.psm1, line 565 at r2 (raw file):

$errorTrace = $PSItem.ScriptStackTrace | Select-String -Pattern '(at )(.*)(,)'

This seems to risky to do if there were another localized language running this code. To resolve this we should either wrap each call into it's own Try-catch-block or fix the helper function, or both.

If we add a try-catch-block around Add-ADCommonGroupMember we would still have a problem knowing what caused the problem and catch the correct error? 🤔 We could make sure the helper Add-ADCommonGroupMember handles when a member does not exist. If the Add-ADGroupMember does not report the correct error (missing member), then maybe it could do a Get-ADObject maybe to verify that the member does exist, if not it throws a correct error. Hopefully if we wrap a try-catch block around Add-ADGroupMember in the helper function it will return the correct error? Maybe we need do both of this option to solve this?

---

DSCResources/MSFT_xADGroup/en-US/MSFT_xADGroup.strings.psd1, line 7 at r2 (raw file):

GroupMemberNotFound

This is not used anywhere?

---

Modules/xActiveDirectory.Common/xActiveDirectory.Common.psm1, line 489 at r2 (raw file):

-and (-not (Get-Module -Name $ModuleName)

What is your thought why we need this change? This helper function is to assert that the module exist in a modules path.

With this change it will not throw if the module is loaded into the session, even if the module does not exist in a modules path.

---

Tests/Unit/MSFT_xADGroup.Tests.ps1, line 28 at r2 (raw file):

Import-Module (Join-Path -Path $PSScriptRoot -ChildPath 'Stubs\ActiveDirectory.psm1') -Force

You might need to load these inside the InModuleScope-block, see xADComputer and xADDomainController as an example.

Same for the stubs.

---

Tests/Unit/MSFT_xADGroup.Tests.ps1, line 338 at r2 (raw file):

Context 'Initial tests' {

See next comment. Not sure this brings any value, maybe just remove it?

---

Tests/Unit/MSFT_xADGroup.Tests.ps1, line 407 at r2 (raw file):

Context 'Continued cases' {

Not sure this context block bring any value. Rather see context block for 'When adding members to an existing group', 'When removing members from an existing group' and so on. But I think for now just remove this context block?

---

Tests/Unit/MSFT_xADGroup.Tests.ps1, line 707 at r2 (raw file):

Context 'Error Handling for group membership' {

This should say 'When adding a member that does not exist'

---

Tests/Unit/MSFT_xADGroup.Tests.ps1, line 709 at r2 (raw file):

'Throws the correct message when adding a member that does not exist (#166)'

See previous comment, then this should just say 'Should throws the correct error'

---

Tests/Unit/MSFT_xADGroup.Tests.ps1, line 718 at r2 (raw file):

"ADIdentityNotFoundException"

We should test for the correct error here, see comment in the resource code.

---

Tests/Unit/Stubs/ActiveDirectory.psm1, line 227 at r2 (raw file):

    throw New-Object Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException
} #>

This should be changed back to throw '{0}: StubNotImplemented' -f $MyInvocation.MyCommand, and then in the Mock in the unit test you should add throwing this type instead.

---

Tests/Unit/Stubs/ActiveDirectory.psm1, line 5289 at r2 (raw file):

$Confirm

Instead of adding this, we should add [CmdletBinding(SupportsShouldProcess = $true)] which should add WhatIf and Confirm properties.

Will that work?

---

Tests/Unit/Stubs/ActiveDirectory.psm1, line 5339 at r2 (raw file):

$Confirm

See previous comment.

[johlju]

The repo has been renamed to ActiveDirectoryDsc and all resource names have been renamed removing the 'x'. Please make sure this PR is updated accordingly when you rebase. 🙂 Let me know if you need any help with this.

[johlju]

@devopsjesus Do you have time to work on this? Love to get this in before next release in a week. If you don't have time, I can look at getting this over the finish line if you like. 🙂

[devopsjesus]

hey! sorry for the extended absence, I've been on leave and otherwise pre-occupied 😨 I plan on getting to this within the next couple days

[johlju]

@devopsjesus welcome back! 🙂 Awesome, that would be great!

[johlju]

@devopsjesus do you think you have time to work on this? 🙂

[devopsjesus]

It's a top priority for this week for me. Sorry for the delays...

[johlju]

No worries. Thanks for looking at this. 🙂

[johlju]

@devopsjesus I think this change is covered in PR #497. 🤔