-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
base: master
Are you sure you want to change the base?
Conversation
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.
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:
|
@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:
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. |
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. |
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.