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

Fix: use aref when fieldmapping is empty #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rhouthuijzen
Copy link
Contributor

No description provided.

@rhouthuijzen rhouthuijzen added the needs-review Indicates that the PR is ready and waiting for review label Dec 3, 2024
@JeroenBL JeroenBL self-assigned this Dec 23, 2024
@JeroenBL JeroenBL added the current-sprint PRs to be addressed within the current sprint label Dec 23, 2024
Copy link
Member

@Rick-Jongbloed Rick-Jongbloed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting connector... :-) Please have a look at my comments and contact me if you have any questions

@@ -33,6 +33,7 @@ The following lifecycle actions are available:
| Action | Description |
| ------------------ | ------------------------------------ |
| create.ps1 | PowerShell _create_ lifecycle action |
| update.ps1 | PowerShell _update_ lifecycle action |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to describe the actions you're executing in each script. Especially since the create and update scripts only do correlation. I know we have a bit disclaimer describing this, but it wouldn't hurt to repeat that message?

$outputContext.AccountReference = $actionContext.Data.EmailAddress
}
$outputContext.success = $true
$outputContext.AuditLogs.Add([PSCustomObject]@{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to do something with previousData so the audit message is only shown when previousdata != data?

@@ -44,16 +44,22 @@ try {
throw 'The account reference could not be found'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the readme, you say: 'The $actionContext.References.Account is used when $actionContext.Data.EmailAddress is empty in the delete lifecycle action.' and in the mapping the aRef is will empty when there's no emailaddress. If there's a situation where the emailaddress is empty in aRef, the script will fail here, instead of getting the emailaddress from the mapping. The chance that this occurs is very slim, but i do think we should fix this.

I think we can fix this by throwing an error only when aRef and Data.EmailAddress are empty.

}
$outputContext.success = $true
$outputContext.AuditLogs.Add([PSCustomObject]@{
Action = 'CorrelateAccount'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also say we correlate while Data.Emailaddress is empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think these comments apply to the create script too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
current-sprint PRs to be addressed within the current sprint needs-review Indicates that the PR is ready and waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants