Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
rschouten97 opened this issue Jul 11, 2023 · 3 comments · Fixed by #21
Closed

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

rschouten97 opened this issue Jul 11, 2023 · 3 comments · Fixed by #21

Comments

@rschouten97
Copy link
Contributor

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
Copy link
Member

Rick-Jongbloed commented Jul 13, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@rhouthuijzen rhouthuijzen linked a pull request Feb 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants