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

[MM-586] Added API token for fetching issue details #1102

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

raghavaggarwal2308
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 commented Jul 15, 2024

Summary

  1. Added API token field in system console.
  2. Added logic to fetch issue details with API token(if set in system console) for comment and issue created events.

Screenshot

image

What to test?

  1. API token set: Notifications are getting delivered for all event, evne if the user triggering the event in not connected.
  2. API token not set: Notification are delivered only if the user triggering the event is connected on mattermost.

How to test:

  1. Connect your Jira account.
  2. Set the API token in settings. (How to create API token)
  3. Trigger the desired event from Jira.
    Note: API token is preferred to be created using an admin Jira account. Otherwise notification will not be delivered for the project the API token user do not have access to.

1. Added API token field in syatem console.
2. Added logic to fetch issue details with API token for comment and issue created events.
@raghavaggarwal2308 raghavaggarwal2308 self-assigned this Jul 15, 2024
@raghavaggarwal2308 raghavaggarwal2308 changed the title [MM-586] Added API token for fetching issue details WIP: [MM-586] Added API token for fetching issue details Jul 15, 2024
@raghavaggarwal2308 raghavaggarwal2308 marked this pull request as draft July 15, 2024 15:26
@raghavaggarwal2308 raghavaggarwal2308 changed the title WIP: [MM-586] Added API token for fetching issue details [MM-586] Added API token for fetching issue details Jul 15, 2024
@raghavaggarwal2308 raghavaggarwal2308 marked this pull request as ready for review July 15, 2024 15:38
plugin.json Outdated
"key": "AdminAPIToken",
"display_name": "Admin API Token",
"type": "text",
"help_text": "This token is used to fetch some extra details for a Jira issue for comment related and 'issue created' subscription events.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"help_text": "This token is used to fetch some extra details for a Jira issue for comment related and 'issue created' subscription events.",
"help_text": "This token is used to fetch some extra details for a Jira issue for 'comment related' and 'issue created' subscription events.",

Copy link
Contributor

@ayusht2810 ayusht2810 Jul 16, 2024

Choose a reason for hiding this comment

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

Also, this isn't matching with the description of what to test? Shouldn't we update it to something like Set this token to view notifications when the user is not connected?

We should add the note here as mentioned in the description: API token is preferred to be created using an admin Jira account. Otherwise, notification will not be delivered for the project the API token user does not have access to.

Also, can we add this somewhere in readme or link here itself, on how to generate this token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ayusht2810 Updated the help test. We will add documentation for this PR once the PR is approved

server/issue.go Outdated Show resolved Hide resolved
server/issue.go Outdated Show resolved Hide resolved
server/issue.go Outdated Show resolved Hide resolved
server/issue.go Show resolved Hide resolved
server/issue.go Show resolved Hide resolved
server/plugin.go Outdated Show resolved Hide resolved
@raghavaggarwal2308 raghavaggarwal2308 added the Docs/Needed Requires documentation label Jul 16, 2024
plugin.json Outdated Show resolved Hide resolved
server/issue.go Outdated Show resolved Hide resolved
server/issue.go Outdated Show resolved Hide resolved
webapp/junit.xml Outdated Show resolved Hide resolved
server/webhook_jira.go Outdated Show resolved Hide resolved
webapp/.gitignore Outdated Show resolved Hide resolved
server/kv.go Outdated Show resolved Hide resolved
@wiggin77 wiggin77 added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 31, 2024
@wiggin77 wiggin77 removed the 2: Dev Review Requires review by a core committer label Jul 31, 2024
@AayushChaudhary0001
Copy link

While testing this PR, I found an issue regarding the notifications. When the user who triggers the event is not connected but the API token is set for the instance, the user receives the notification. But when the API token is not set and the user is not connected, still the notifications are getting delivered on Mattermost instance.
Note:- I have tested this with triggering the event on Jira with the same account which is connected to MM.

@AayushChaudhary0001
Copy link

While testing this PR, I have experienced an issue which was making plugin to crash. While triggering an event to receive a notification on MM, the plugin creates a panic(as visible in the logs) which crashes the plugin. Please refer the logs below:-

image (9)

@raghavaggarwal2308
Copy link
Contributor Author

@AayushChaudhary0001 The above panic issue should be fixed now. Can you please re-test the PR.

Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

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

The PR has been tested for the below conditions:-

  • Notification received if the user is connected.
  • Notifications for issue created and comment fit he user is not connected but the API token is set.

The PR is working fine for the above conditions, LGTM. Approved.

@raghavaggarwal2308 raghavaggarwal2308 added this to the v4.2.0 milestone Oct 18, 2024
@raghavaggarwal2308 raghavaggarwal2308 merged commit 85725f3 into master Oct 18, 2024
9 checks passed
@raghavaggarwal2308 raghavaggarwal2308 deleted the use_api_token branch October 18, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: QA Review Requires review by a QA tester Docs/Needed Requires documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants