-
Notifications
You must be signed in to change notification settings - Fork 132
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
Fixed issue: Comment notification not working after implementing OAuth2 and Added PKCE code for OAuth2 #953
Changes from 1 commit
f2f48ee
4fb10f4
6c72ae3
ee1820c
7bf8615
46a259c
9005b9f
9972277
c47d12c
6b5a62b
74ca215
04085d1
fdbdc0d
0186841
081df4a
1d39d12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,7 +140,7 @@ func (wh *webhook) PostNotifications(p *Plugin, instanceID types.ID) ([]*model.P | |
|
||
isCommentEvent := wh.Events().Intersection(commentEvents).Len() > 0 | ||
if isCommentEvent { | ||
err = client.RESTGet(notification.commentSelf, nil, &struct{}{}) | ||
err = client.RESTGet(fmt.Sprintf("/2/issue/%s/comment/%s", wh.Issue.ID, wh.Comment.ID), nil, &struct{}{}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wondering if this works for all 3 Jira instance types. This is my main remaining concern here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister I have tested this code on Jira cloud-oauth and server instances but since the Jira JWT is not working for us, we were not able to test it there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I'm hesitant to introduce this change for Jira server. We need to make sure it works on Jira server v7 before release. If it works for Jira 9, it likely works for Jira 7, but we should avoid a regression on Jira server for something related to Jira cloud. Maybe we can wrap this in a conditional block "if cloud" etc. Please let me know your thoughts on risks of both options if you can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister Added the conditional block |
||
} else { | ||
_, err = client.GetIssue(wh.Issue.ID, nil) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this function exist somewhere else earlier? Was it deleted at some point? I'm asking because there is no red diff related to this function in this commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister Yes, it was removed earlier.