Tools4everBV / HelloID-Conn-Prov-Target-Topdesk

TOPdesk - Target
4 stars 4 forks source link

Budgetholder is queried when none is provided in account object #14

Closed rschouten97 closed 2 months ago

rschouten97 commented 1 year ago

When we don't use a field, we don't provide it in the mapping (account) object. However, if I outcomment (or remove) the field budgetHolder, like so:

# Account mapping. See for all possible options the Topdesk 'supporting files' API documentation at
# https://developers.topdesk.com/explorer/?page=supporting-files#/Persons/createPerson
$account = [PSCustomObject]@{
    surName             = (New-TopdeskName -Person $p).surname      # Generate surname according to the naming convention code.
    prefixes            = (New-TopdeskName -Person $p).prefixes
    firstName           = $p.Name.NickName
    firstInitials       = (New-TopdeskName -Person $p).initials     # Generate initials max 10 char
    gender              = New-TopdeskGender -Person $p
    email               = $p.Accounts.MicrosoftActiveDirectory.mail
    employeeNumber      = $p.ExternalId
    networkLoginName    = $p.Accounts.MicrosoftActiveDirectory.UserPrincipalName
    tasLoginName        = $p.Accounts.MicrosoftActiveDirectory.UserPrincipalName    # When setting a username, a (dummy) password could be mandatory
    #password            = (Get-RandomCharacters -length 10 -characters 'ABCDEFGHKLMNOPRSTUVWXYZ1234567890')
    jobTitle            = $p.PrimaryContract.Title.Name
    branch              = @{ lookupValue = $p.PrimaryContract.Location.Name }
    department          = @{ lookupValue = $p.PrimaryContract.Department.DisplayName }
    # budgetHolder        = @{ lookupValue = $p.PrimaryContract.CostCenter.Name }
    isManager           = $false # When the HelloID source provides an isManager boolean: $p.Custom.isManager
    manager             = @{ id = $mRef }
    #showDepartment      = $true
    showAllBranches     = $true
}

The function Get-TopdeskBudgetHolder to query the Topdesk budgetholder ID is still performed and returns the following error: Requested to lookup budgetholder, but budgetholder.lookupValue is missing. This is a scripting issue.

IMO this should not be the case. We should only query and use the provided fields of the mapping (account) object.

This can be easily solved by changing this:

    # Resolve budgetholder id
    $splatParamsBudgetHolder = @{
        Account                   = [ref]$account
        AuditLogs                 = [ref]$auditLogs
        Headers                   = $authHeaders
        BaseUrl                   = $config.baseUrl
        lookupErrorHrBudgetHolder = $config.lookupErrorHrBudgetHolder
        lookupErrorTopdesk        = $config.lookupErrorTopdesk
    }
    Get-TopdeskBudgetholder @splatParamsBudgetHolder

To this:

    if($null -ne $Account.budgetHolder){
        # Resolve budgetholder id
        $splatParamsBudgetHolder = @{
            Account                   = [ref]$account
            AuditLogs                 = [ref]$auditLogs
            Headers                   = $authHeaders
            BaseUrl                   = $config.baseUrl
            lookupErrorHrBudgetHolder = $config.lookupErrorHrBudgetHolder
            lookupErrorTopdesk        = $config.lookupErrorTopdesk
        }
        Get-TopdeskBudgetholder @splatParamsBudgetHolder
    }

The example given is for budgetholder, but this should be for every field (that isn't required).

Rick-Jongbloed commented 1 year ago

Unfortunately, this doesn't work as other features depend on an empty or null department. If we want to implement this, we need to check if the property exists on the account object and this check in the set- functions need to be removed from the functions.

rschouten97 commented 11 months ago

@Rick-Jongbloed , a check if the property exists isn't that complex either right? Could be something like this:

if ("budgetHolder" -in $Account.PsObject.Properties.Name) {
    if ($null -ne $Account.budgetHolder) {
        # Resolve budgetholder id
        $splatParamsBudgetHolder = @{
            Account                   = [ref]$account
            AuditLogs                 = [ref]$auditLogs
            Headers                   = $authHeaders
            BaseUrl                   = $config.baseUrl
            lookupErrorHrBudgetHolder = $config.lookupErrorHrBudgetHolder
            lookupErrorTopdesk        = $config.lookupErrorTopdesk
        }
        Get-TopdeskBudgetholder @splatParamsBudgetHolder
    }
}

However, the main issue is for budgetHolder as this is not a required field, whereas departmet is in that case. So for starters, could we just implement a check for the budgetHolder? If necessary we can always add the checks for other properties as well.

rhouthuijzen commented 7 months ago

This is fixed in the new PSv2. Linked to new branch/pr