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.

Sign up for GitHub

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

Jump to bottom

Fixed plugin failure on Issue with no priority. #821

Merged
merged 2 commits into from
Feb 28, 2023
Merged

Fixed plugin failure on Issue with no priority. #821

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff7cc7d
Fixed plugin failure on Issue with no priority.
Nov 16, 2021
b193a35
Merge branch 'master' into nil-priority-field-fix
mattermost-build Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion server/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,9 @@ func getIssueFieldValue(issue *jira.Issue, key string) StringSet {
case labelsField:
return NewStringSet(issue.Fields.Labels...)
case priorityField:
return NewStringSet(issue.Fields.Priority.ID)
if issue.Fields.Priority != nil {
return NewStringSet(issue.Fields.Priority.ID)
}
Comment on lines +784 to +786
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should apply the same fix to anything that can possibly be nil in this function, such as issue.Fields.Status. These may be the only two to apply the fix for.

Can you please check the values being accessed in this function, and which ones are pointers that could produce the same problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @DummyThatMatters, thanks for catching this and providing the fix 🎉

Just so I can understand the issue better, do you know if this is occurring in one of these two cases:

* the issue scheme does not contain the `priority` field

* the issue scheme does have a `priority`, but the priority is not assigned for a given issue somehow

As far as the fix you've provided, I have one request to provide the same fix to other fields potentially having this issue in the same function. Thanks for this great contribution @DummyThatMatters!

Frankly, I don't really know. Does this thing matter? We debugged this a long time ago and I have implemented my patch on our prod server, so it no longer fail and I don't know which exact tickets caused the issue.
I can try to find out if this info is essential in order to investigate the bug and fix potential problems. And we do not have any problems with other fields for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DummyThatMatters Fair enough, I'll approve at its current state. Note that you'll need to sign our CLA before getting this merged, as explained here #821 (comment)

Thanks again for this fix 😄

case "fixversions":
result := NewStringSet()
for _, v := range issue.Fields.FixVersions {
Expand Down