search

dsccommunity / SharePointDsc

The SharePointDsc PowerShell module provides DSC resources that can be used to deploy and manage a SharePoint farm
MIT License
247 stars 107 forks source link

SPWebAppPeoplePickerSettings: Adds support for parameter CustomFilter and ShortDomainName #1396

Closed ChristophHannappel closed 2 years ago

ChristophHannappel commented 2 years ago

Pull Request (PR) description

Added

Fixed

This Pull Request (PR) fixes the following issues

Task list

This change is Reviewable

codecov[bot] commented 2 years ago

Codecov Report

Merging #1396 (290d64d) into master (1ee98a0) will increase coverage by 0%. The diff coverage is 76%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1396   +/-   ##
======================================
  Coverage      84%     84%           
======================================
  Files         143     143           
  Lines       22285   22377   +92     
======================================
+ Hits        18817   18901   +84     
- Misses       3468    3476    +8
Impacted Files Coverage Δ
...sources/MSFT_SPShellAdmins/MSFT_SPShellAdmins.psm1 64% <65%> (-1%) :arrow_down:
...kerSettings/MSFT_SPWebAppPeoplePickerSettings.psm1 90% <65%> (-8%) :arrow_down:
...FT_SPSearchServiceApp/MSFT_SPSearchServiceApp.psm1 94% <88%> (-1%) :arrow_down:
...MSFT_SPFarmPropertyBag/MSFT_SPFarmPropertyBag.psm1 88% <91%> (+6%) :arrow_up:
...sc/DSCResources/MSFT_SPInstall/MSFT_SPInstall.psm1 90% <100%> (+6%) :arrow_up:
ChristophHannappel commented 2 years ago

I think the CodeCov has some false postives. https://github.com/dsccommunity/SharePointDsc/pull/1396/files#annotation_2946439513 https://github.com/dsccommunity/SharePointDsc/pull/1396/files#annotation_2946439514 https://github.com/dsccommunity/SharePointDsc/pull/1396/files#annotation_2946439517 https://github.com/dsccommunity/SharePointDsc/pull/1396/files#annotation_2946439518 https://github.com/dsccommunity/SharePointDsc/pull/1396/files#annotation_2946439520

These cases are tested in tests/Unit/SharePointDsc/SharePointDsc.SPWebAppPeoplePickerSettings.Tests.ps1 starting row 201 Does anyone know why codedev thinks it is not covered? Could it be a test against an older commit, How do i restart the tests?

While https://github.com/dsccommunity/SharePointDsc/pull/1396/files#annotation_2946439516 needs a unit test with an empty or $null password. I'll add one tomorrow.

ykuijs commented 2 years ago

@ChristophHannappel Do you have the possibility to update this PR, so we can merge it?

ChristophHannappel commented 2 years ago

@ChristophHannappel Do you have the possibility to update this PR, so we can merge it?

@ykuijs I'll try to do it this afternoon and give you a heads up after it :)

ChristophHannappel commented 2 years ago

Have checked the code coverage locally and it indeed covers the specified lines. I think the code coverage merge has an issue here. I will check separately. We can ignore this issue in this PR.

Just a few minor comments

Reviewed 3 of 6 files at r1, 1 of 1 files at r3, all commit messages. Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @ChristophHannappel and @ykuijs)

_SharePointDsc/DSCResources/MSFT_SPWebAppPeoplePickerSettings/MSFT_SPWebAppPeoplePickerSettings.psm1, line 368 at r4 (raw file):_

                return $false
            }
            if ($searchADDomain.ContainsKey('CustomFilter') -and $searchADDomain.CustomFilter -ne $specifiedDomain.CustomFilter)

You can add these two new parameters to the Test-SPDscParameterState function a few line lower.

Because that function does not support checking credentials, the above check was specifically created.

_SharePointDsc/DSCResources/MSFT_SPWebAppPeoplePickerSettings/MSFT_SPWebAppPeoplePickerSettings.psm1, line 396 at r4 (raw file):_

            "ActiveDirectorySearchTimeout", `
            "OnlySearchWithinSiteCollection",
        "PeopleEditorOnlyResolveWithinSiteCollection")

Please add the two new parameters here.

tests/Unit/SharePointDsc/SharePointDsc.SPWebAppPeoplePickerSettings.Tests.ps1, line 376 at r1 (raw file):

Previously, ChristophHannappel wrote… This does look more readable indeed.

I've moved the properties but now the Pester test fails. Since the properties are for the SearchActiveDirectoryDomains and not the people picker level, do i need to change something at the Test-DSCParameterState Command?

ykuijs commented 2 years ago

Also, make sure you update your branch with the main repo. I merged another PR earlier this week.

ChristophHannappel commented 2 years ago

Also, make sure you update your branch with the main repo. I merged another PR earlier this week.

Thanks for the heads up! I did a rebase and currently a local build is running. After that i'll push the changes.

While working with the Test-SPDscParameterState I saw that the ValuesToCheck Array contained some backticks, but not not on all items. https://github.com/dsccommunity/SharePointDsc/blob/139e5f740be3aac22fff6390fc5c79d1a5613090/SharePointDsc/DSCResources/MSFT_SPWebAppPeoplePickerSettings/MSFT_SPWebAppPeoplePickerSettings.psm1#L324

    $result = Test-SPDscParameterState -CurrentValues $CurrentValues `
        -Source $($MyInvocation.MyCommand.Source) `
        -DesiredValues $PSBoundParameters `
        -ValuesToCheck @("ActiveDirectoryCustomFilter", `
            "ActiveDirectoryCustomQuery", `
            "ActiveDirectorySearchTimeout", `
            "OnlySearchWithinSiteCollection",
        "PeopleEditorOnlyResolveWithinSiteCollection")

To me the reason for backticks seems to be that the Code Formatting looks a bit nicer. Currently I've removed the Backticks from the array. I didn't find any guideline at the DSC Resource Style Guidelines & Best Practices to resolve this. Do you have a preference?

ykuijs commented 2 years ago

Great, have you seen this blog post about running individual unit tests: https://techcommunity.microsoft.com/t5/sharepointdsc-blog/announcement-making-running-unit-tests-easier-for-contributing/ba-p/3149068

Correct, when creating arrays the backticks are not required. I prefer without the backticks, which makes readability better (less unnecessary characters). So you removing them is perfect.