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

Provide ability to handle multiple custom fields with the same name. #973

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mammabear123
Copy link
Contributor

In larger Jira instances it is surprisingly common to find two custom fields with the same name. Currently the mapping code naively chooses one of the fields up front and uses it for all items.

The proposal instead collects a list of matching fields during the building of the mappers and then for each item the fields are checked sequentially, and the first field that actually contains a value is used. This will not fix items where more than one of the same-named fields contain a value.

This is a draft request in order to get an opinion of what the team thinks of it. It has not been tested thoroughly enough for pulling yet.

Note: the previous pull request with branch SameNameFields, this PR is for branch SameNameFields2.
This PR includes code to actually make use of this change unlike the previous one. Indeed this is done inside the IfChanged method, and so is applied for nearly every field. It also contains a number of bug fixes, and more extensive unit tests.

Note: this commit only adds the capability, further changes are required to switch usages of TryGetValue() in the field mappers of FieldMapperUtils for FieldMapperUtils.TryGetFirstValue().
New methods GetCustomIdList() and TryGetFirstValue() have been added to handle multiple custom fields that have the same name.   This change modifies IfChanged<T>() to actually start using the new code.
@mammabear123
Copy link
Contributor Author

Most of the comments in #973 also apply here, and I'll not be repeating their explanation.

I would like to ask your help with two scenarios though, as you have far more experience with this stuff than me. In particular, I'l like you to consider the following:

  • If a particular ticket somehow ends up with non-null values in multiple fields, then one will be chosen using the same approach as was done previously - presumably this is still okay?
  • I have zero knowledge of how the APIs return default field values. Specifically, if two custom fields with the same name exist, and one of them has a default value. What would happen in a ticket which is actually referencing the non-defaulted field. Would the default value still be present in the returned from the API and therefore confuse this algorithm?

@Alexander-Hjelm
Copy link
Collaborator

@mammabear123 I'm skeptical to merging this one as it seemingly deals with a symptom and not the underlying cause.

Why not use the ID of the custom field in your mapping instead of the field name?

@mammabear123
Copy link
Contributor Author

@mammabear123 I'm skeptical to merging this one as it seemingly deals with a symptom and not the underlying cause.
Why not use the ID of the custom field in your mapping instead of the field name?

I think this is valid criticism. We started down this road after struggling to get the id thing to work properly for us. However, whatever was broken is working now. Using id is definitely a cleaner solution, although is not nearly as intuitive for a user - so there is still an argument that this feature improves usability a bit. (It is definitely better than "just choosing the first one with the right name").

However, our testing has revealed that my concerns about this scenario were warranted:

"...if two custom fields with the same name exist, and one of them has a default value. What would happen in a ticket which is actually referencing the non-defaulted field. Would the default value still be present in the response from the API and therefore confuse this algorithm?"

I don't know if there is an easy way to tell if the value you see in the response JSON is just a defaulted one, or whether it is a 'real' value. The answer may make this 'nice-to-have' more trouble than it's worth.

As far as we are concerned, I'm happy for you to reject this if you wish to. We will use the direct id values as you suggested.

@Alexander-Hjelm
Copy link
Collaborator

Try the to use the field ID instead and let me know if that gives you any issues. I agree that this solution is nice to have, but I also think that it is somewhat patching a very niche scenario, and future usage of this feature could be unintuitive. I'm not sure here, but I'm leaning towards using the Field ID as our recommended approach. Sometimes you can hover over the field on the issue screen in Jira to see the field ID. Otherwise you can always view this information through the Jira Rest API as well, which is not super intuitive either, but offers a complete index of the fields, and is not prone to ambiguity.

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.

2 participants