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

Duplicated fields with same name conflict New-JiraIssues #520

Conversation

jkc-sw
Copy link

@jkc-sw jkc-sw commented Sep 8, 2024

Description

Among multiple fields returned by a field name search, New-JiraIssue will select a field that matches its field name and field id among the candidates.

New-JiraIssue should handle properly when Get-JiraField returns multiple fields given a field name.

Motivation and Context

Before the change, multiple fields would be returned and used as a list together. In this scenario, the field id could be a combination of 2 field ids, which invalidates the Get-JiraIssueCreateMetadata.

For example, when a field 'Reporter' returns 2 fields with ids "Reporter", "customfield_123", it would use the field id "Reporter\ncustomfield_123".

Before the change, multiple fields would be returned and used as a list together.

After the change, the behavior is

  • When there is only 1 field returns, the field is used as is.
  • When there are many fields, the field with its name matched to its ID is used.
  • Otherwise, an exception is thrown.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added Pester Tests that describe what my changes should do.
  • I have updated the documentation accordingly.

@jkc-sw jkc-sw requested a review from a team as a code owner September 8, 2024 17:38
@hmmwhatsthisdo
Copy link
Contributor

hmmwhatsthisdo commented Sep 9, 2024

@jkc-sw #516 changed how we resolve custom fields - AFAICT the way we're selecting fields should be stable under duplicate-name scenarios. Can you try using the tip of branch release/2.15-alpha? (3d2afbe)

The "real" fix here is to use the CreateMeta endpoint (in Jira 8.4+, this is project/issuetype-scoped) - see #519.

@jkc-sw
Copy link
Author

jkc-sw commented Sep 10, 2024

@hmmwhatsthisdo I cherry-pick my test onto release/2.15-alpha and can confirm the issue is addressed properly.

Beside my tests, my changes in New-JiraIssue can be dropped.

Should I close this PR and submit a PR for tests only?

if ($fieldsMatchingName.Count -gt 1) {
$fieldsMatchingId = @($fieldsMatchingName | Where-Object {
$each = $_
$each.ID -eq $name
Copy link
Member

Choose a reason for hiding this comment

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

are you sure you about the -eq?
this would make it case sensitive.

@@ -119,7 +119,27 @@ function New-JiraIssue {
$name = $_key
$value = $Fields.$_key

if ($field = Get-JiraField -Field $name -Credential $Credential -Debug:$false) {
$fieldsMatchingName = Get-JiraField -Field $name -Credential $Credential -Debug:$false
Copy link
Member

Choose a reason for hiding this comment

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

I am not too fond of the way this is coded. Particularly the [0]s.

how about this instead?

     if ($field = Get-JiraField -Field $name -Credential $Credential -Debug:$false) {
        if ($field.Count -gt 1) {
            $field = $field | Where-Object { $_.Id -eq $name } | Select-Object -First 1
        }
     }
    if (-not $field) {
        $exception = ([System.ArgumentException]"Invalid value for Parameter")
                        $errorId = 'ParameterValue.InvalidFields'
                        $errorCategory = 'InvalidArgument'
                        $errorTarget = $Fields
                        $errorItem = New-Object -TypeName System.Management.Automation.ErrorRecord $exception, $errorId, $errorCategory, $errorTarget
                        $errorItem.ErrorDetails = "Field name [$name] matches multiple fields by name or by IDs ($($fieldsMatchingName | Out-String)). Please specify field by ID."
                        $PSCmdlet.ThrowTerminatingError($errorItem)
     )

POC:
image

@lipkau
Copy link
Member

lipkau commented Sep 10, 2024

@hmmwhatsthisdo I cherry-pick my test onto release/2.15-alpha and can confirm the issue is addressed properly.

Beside my tests, my changes in New-JiraIssue can be dropped.

Should I close this PR and submit a PR for tests only?

Damn!
I should read the comments before looking at the code 😆 .

If the problem is fixed in the release/2.15-alpha branch, please submit a PR (or rebase this one and update it) to only add the tests.
I like the test :-)

$each = $_
$each.ID -eq $name
})
if ($fieldsMatchingId.Count -eq 1) {
Copy link
Member

@lipkau lipkau Sep 10, 2024

Choose a reason for hiding this comment

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

Unit test shows that this is not compatible with Windows Powershell (v5).
My best guess (without investigating) is that the Where-Object does not return an array and .Count does not exist.

@hmmwhatsthisdo
Copy link
Contributor

@hmmwhatsthisdo I cherry-pick my test onto release/2.15-alpha and can confirm the issue is addressed properly.
Beside my tests, my changes in New-JiraIssue can be dropped.
Should I close this PR and submit a PR for tests only?

Damn! I should read the comments before looking at the code 😆 .

If the problem is fixed in the release/2.15-alpha branch, please submit a PR (or rebase this one and update it) to only add the tests. I like the test :-)

I'm glad folks are getting eyes on their PRs, at least. 😃

The unspoken "bug" driving this PR is that dupe-name fields aren't handled gracefully at the moment. #516 is "stable" (based on the order we receive CF definitions back from Jira), but if we disambiguate between two CFs and pick one that isn't valid for that issue's context, that wouldn't spark joy for users.

AFAIK, dupe-name fields usually don't touch the same issue type x project combo - unclear if Jira itself enforces this restriction when editing issue schemes/custom field association contexts, or if it's just a matter of convention. In either case, we could ostensibly be doing a better job at name -> ID resolution by looking at the subset of custom fields that are in-scope for a given issue type x project.

CreateMeta/EditMeta are the "right" way to do this long-term, but more work is needed to get JiraPS onto using those endpoints during issue creation/update.

@lipkau
Copy link
Member

lipkau commented Sep 10, 2024

I think the "right" way is to have a AtlassianPS.JiraPS.Field object and only use Id in code.
but we are not there yet :-P

@jkc-sw jkc-sw changed the base branch from develop to release/2.15-alpha September 11, 2024 02:19
@jkc-sw jkc-sw force-pushed the conflict-new-jiraissue-duplicate-fields branch 3 times, most recently from c92cd42 to ba8af65 Compare September 11, 2024 02:28
@jkc-sw
Copy link
Author

jkc-sw commented Sep 11, 2024

I dropped the changes and rebase my tests onto the release/2.15-alpha.

@lipkau lipkau merged commit 3563707 into AtlassianPS:release/2.15-alpha Sep 11, 2024
6 checks passed
@jkc-sw jkc-sw deleted the conflict-new-jiraissue-duplicate-fields branch September 12, 2024 03:13
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 this pull request may close these issues.

3 participants