[GH-862] Added issue status field in subscription modal #976

raghavaggarwal2308 · 2023-09-26T10:12:27Z

Summary

Added issue status field in the subscription modal and created an API to fetch the possible statuses of a project.

Screenshots

What to test?

Note: The below things should be tested for both cloud and server instances of Jira.

The status field is properly displayed in the subscription modal.
The status field can be updated and are being sent in the subscription payload.
The status field is properly prefilled while editing a subscription.
The status field is only visible when a project is selected.
The options in the status fields are filtered properly according to project and issue type.
Webhook notifications are received according to the subscription filters.

Steps to reproduce

Connect your Jira account.
Run the /jira subscribe command and create/edit a subscription.
Save/create the subscription.
Generate a notification from the Jira side according to the subscription.

Environment:

MM version: v7.8.2
Node version: 14.18.0
Go version: 1.19.0

Ticket Link

Fixes #862

* [MI-3483] Added the status field in subscription modal * [MI-3483] Code refactoring * [MI-3483] Fix testcases * [MI-3483] Added error handling and code refactoring * [MI-3483] Review fixes * [MI-3483] Review fixes * [MI-3483] Review fixes

mickmister · 2023-09-26T13:36:00Z

@raghavaggarwal2308 I'm thinking this should go in the field filters part of the modal, rather than have its own place under the issue type selector. This way you can selectively include/exclude certain statuses. What are the pros and cons of this approach versus the current one?

raghavaggarwal2308 · 2023-10-10T08:40:33Z

@raghavaggarwal2308 I'm thinking this should go in the field filters part of the modal, rather than have its own place under the issue type selector. This way you can selectively include/exclude certain statuses. What are the pros and cons of this approach versus the current one?

@mickmister There are four options in the filters section i.e. include, exclude, include all, and empty. If we add the status field in the filters section then the "include all" and "empty" options will be useless as I think the status field can't have multiple values at a time and it can't be empty as well.
We have three solutions for this:

We move the field to the filters section and it will be the user's responsibility to be careful while using the two options for "Status filter".
We conditionally remove the two options from the dropdown.
We leave the field as it is now (i.e. below the issue type selector) and the user can select the statuses he wants to get notified for.

The advantage we will have by moving the field to the filters section is that the user will be able to use the "exclude" option.

mickmister · 2023-10-10T15:20:49Z

If we add the status field in the filters section then the "include all" and "empty" options will be useless as I think the status field can't have multiple values at a time and it can't be empty as well.

We conditionally remove the two options from the dropdown.

This seems like the right solution to me

The field "filters" are meant for "what state is the ticket currently in", which the status certainly applies to that. Thoughts on moving it to the filters?

raghavaggarwal2308 · 2023-10-11T11:04:17Z

The field "filters" are meant for "what state is the ticket currently in", which the status certainly applies to that. Thoughts on moving it to the filters?

@mickmister Sounds good, will make the change and update the PR.

…72) * [MI-3597] Moved the issue status field into filters section of modal * [MI-3597] FIxed a minor issue * [MI-3597] Review fix

codecov-commenter · 2023-10-12T10:26:41Z

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (ea9ab23) 33.27% compared to head (47bc1d5) 33.18%.

Files	Patch %	Lines
server/issue.go	0.00%	13 Missing ⚠️
server/client_cloud.go	0.00%	6 Missing ⚠️
server/client_server.go	0.00%	6 Missing ⚠️
server/webhook_jira.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
- Coverage   33.27%   33.18%   -0.09%     
==========================================
  Files          53       53              
  Lines        7950     7977      +27     
==========================================
+ Hits         2645     2647       +2     
- Misses       5078     5103      +25     
  Partials      227      227

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

raghavaggarwal2308 · 2023-10-12T10:28:48Z

@mickmister Updated:

Moved the issue status field into the filters section of the modal.
Conditionally removed the "Empty" and "Include all" options.

Screenshot:

mickmister

Awesome work on this @raghavaggarwal2308 👍 I have a few suggestions, one being that we include the status information in the create-issue-metadata call to cut down on the frontend code needed to fetch the statuses separately. Please let me know your thoughts thanks

server/client_cloud.go

webapp/src/components/modals/channel_subscriptions/edit_channel_subscription.tsx

server/subscribe.go

webapp/src/components/modals/channel_subscriptions/edit_channel_subscription.tsx

…ata API (#74) * [MI-3628] Updated the code to return the status details in the meta data API instead of making a separate API call. * [MI-3628] Review fixes: Small code optimization

raghavaggarwal2308 · 2023-10-26T11:07:21Z

@mickmister Fixed the review comments mentioned by you. We are now returning the status data from the create meta data API.

mickmister · 2023-10-27T20:23:06Z

@raghavaggarwal2308 Can you please update the screenshot in the PR description to match the current code? Edit: I moved the one from the comment above into the PR description

mickmister

A few more comments for discussion

mickmister · 2023-10-27T20:29:55Z

server/issue.go

 	client, _, _, err := p.getClient(instanceID, mattermostUserID)
 	if err != nil {
 		return nil, err
 	}

-	return client.GetCreateMetaInfo(p.API, &jira.GetQueryOptions{
+	projectStatuses, err := client.ListProjectStatuses(projectKeys)


Have we tested this feature on Jira Cloud and Jira Server?

@mickmister Tested and made the necessary changes

mickmister · 2023-10-27T20:35:39Z

webapp/src/types/model.ts

+export type IssueMetadata = {
+    projects: Project[];
+    issue_types: IssueTypeWithStatuses[];
+}


Is the sole purpose of having a separate issue_types here to have the statuses? Any way we can just have the backend update project.issuetypes to include the statuses? It seems like we may be returning twice the data in this code by duplicating the issue types. If we keep them separate then I think we should at least remove everything from IssueTypeWithStatuses that doesn't have to do with statuses before sending back to the client, if statuses is all it's being used for.

Edit: I think I understood this wrong. The ListProjectStatuses backend JiraClient function doesn't return all of the issue type data. I'll trust your judgment how this should be organized. I think we should be more specific with this name though like issue_types_with_statuses.

@mickmister Yes, ListProjectStatuses returns only the fields that we need in our implementation. It would be very complex to move this inside the project.issueTypes as we have to manually iterate over both the array multiple times and add the field. Also, the type used for issueTypes inside the project data does not have the "statuses" field. So, we have to create a new custom type instead of using the type provided by the package.

webapp/src/utils/jira_issue_metadata.tsx

- Added testcases for status field. - Fixed issue: create meta info API not working for Jira server instance

raghavaggarwal2308 · 2023-11-02T11:21:43Z

@mickmister Fixed the review comments mentioned by you

hanzei · 2023-12-05T11:36:24Z

@raghavaggarwal2308 heads up that there is a merge conflict to resolve

… into fix_issue_862

raghavaggarwal2308 · 2023-12-05T11:42:19Z

@hanzei Resolved conflicts

mickmister

LGTM, just a few comments for discussion

mickmister · 2023-12-06T06:56:16Z

server/client_server.go

@@ -211,7 +211,7 @@ func (client jiraServerClient) GetIssueTypes(projectID string) ([]jira.IssueType

 func (client jiraServerClient) ListProjectStatuses(projectID string) ([]*IssueTypeWithStatuses, error) {
 	var result []*IssueTypeWithStatuses
-	if err := client.RESTGet(fmt.Sprintf("3/project/%s/statuses", projectID), nil, &result); err != nil {
+	if err := client.RESTGet(fmt.Sprintf("2/project/%s/statuses", projectID), nil, &result); err != nil {


A little concerning to me, what causes this to change from 3 to 2?

@mickmister It was a mistake on my side, the latest API version for the Jira server is 2.

Okay we definitely want to test any changes made to Jira Server code, with Jira Server. And mention in the test steps of the PR to test both Jira Cloud/Server depending on the changes

@mickmister Yes, we always test the changes on both instances and it's also mentioned in the description. We missed it initially in this PR, apologies for that.

server/issue.go

mickmister · 2023-12-06T07:03:43Z

webapp/src/utils/jira_issue_metadata.tsx

+        for (const issueType of issueTypesWithStatuses) {
+            if (issueTypes.includes(issueType.id) || !issueTypes.length) {
+                for (const status of issueType.statuses) {
+                    if (!keys.has(status.id)) {
+                        keys.add(status.id);
+                        statuses.push(status);


Will issueTypes ever have length 0? (looking at !issueTypes.length)

Also it's a bit hard to read will all of the variations of:

issueTypes

issueTypesWithStatuses

issueType

issueType.statuses

Not sure there is a better way though. I was going to suggest renaming the function parameter issueTypes to issueTypesWithoutStatuses, but that sounds like "these issue types have no statuses associated with them". Current code seems fine, just a lot of redundancy causing a bit of confusion for me.

Just to clarify, issueTypes are the values that are selected in the modal, and issueTypesWithStatuses contains the data for all the issue types in that project. I will rename issueTypes to selectedIssueTypes for more clarity.

Will issueTypes ever have length 0? (looking at !issueTypes.length)
@mickmister Yes, when we have not selected any value in the modal.

Maybe we can change the name to selectedIssueTypes to convey they are user set values

I will rename issueTypes to selectedIssueTypes for more clarity.

@mickmister Already changed

mickmister · 2023-12-06T07:04:53Z

webapp/src/utils/jira_issue_metadata.test.tsx

+            projects: [
+                {
+                    key: 'TEST',
+                    issuetypes: [],
+                },
+            ],


Is it realistic to have a blank array for issuetypes here?

@mickmister No, I don't think this will ever be blank. This particular field is not useful in the function we are testing here, so we kept it empty. I you want we can add some mock data or maybe just add a comment above it.

We should try to have realistic test data I suppose. I recommend using the saved test data when possible. Or generate new ones / edit existing ones if we need to update them, which I think we do need to update them in this case since we changed the API associated with the testdata files

@mickmister Updated the test files

webapp/src/utils/jira_issue_metadata.test.tsx

raghavaggarwal2308 · 2023-12-07T10:05:32Z

@mickmister Fixed the review comments mentioned by you.

… into fix_issue_862

arush-vashishtha · 2023-12-28T14:27:34Z

Tested the PR and is working fine for the following events,

Issue updated
Issue deleted
Comment created
Comment updated
Comment deleted

Nonetheless in the event issue created when creating a subscription with the filter applied for status, the notifications are being generated for all the status in the subscribed channel or DM/GM on MM .
cc @raghavaggarwal2308

raghavaggarwal2308 · 2023-12-29T10:16:00Z

@arush-vashishtha The issue is fixed now.

@mickmister The issue was caused because of an invalid payload being sent from the Jira side in case of issue created event. The status field was always sending the data of a specific status ("Backlog" in our case) irrespective of the status selected on Jira. I have updated the logic to fetch the proper data of the issue in case of this event. Also added a TODO to update the logic in case we get proper data from Jira in the future.

AayushChaudhary0001 · 2024-01-04T13:15:46Z

The reported issue is fixed and tested now, working fine for this one and all the events which are already mentioned before. Approved.

mickmister · 2024-01-18T22:15:22Z

Heads up that there are conflicts to resolve here

… into fix_issue_862

raghavaggarwal2308 · 2024-01-19T07:27:24Z

@mickmister Resolved

mickmister · 2024-01-30T02:17:34Z

@arush-vashishtha Were you able to test this with both Jira Cloud and Jira Server?

arush-vashishtha · 2024-02-09T09:34:34Z

@arush-vashishtha Were you able to test this with both Jira Cloud and Jira Server?

Yes I have tested this on both Jira Cloud and Jira Server.

raghavaggarwal2308 requested a review from mickmister as a code owner September 26, 2023 10:12

[MI-3597] Moved the issue status field into filters section of modal (#…

3b26284

…72) * [MI-3597] Moved the issue status field into filters section of modal * [MI-3597] FIxed a minor issue * [MI-3597] Review fix

mickmister reviewed Oct 12, 2023

View reviewed changes

raghavaggarwal2308 mentioned this pull request Oct 16, 2023

[MI-3628] Updated the code to return the status details in the meta data API Brightscout/mattermost-plugin-jira#74

Merged

[MI-3628] Updated the code to return the status details in the meta d…

87933d8

…ata API (#74) * [MI-3628] Updated the code to return the status details in the meta data API instead of making a separate API call. * [MI-3628] Review fixes: Small code optimization

raghavaggarwal2308 requested a review from mickmister October 26, 2023 11:07

mickmister reviewed Oct 27, 2023

View reviewed changes

[MI-3690] Review fixes

65f5dbd

- Added testcases for status field. - Fixed issue: create meta info API not working for Jira server instance

raghavaggarwal2308 requested a review from mickmister November 2, 2023 11:21

hanzei added 2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author 3: QA Review Requires review by a QA tester labels Dec 5, 2023

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

a88431c

… into fix_issue_862

mickmister removed the Awaiting Submitter Action Blocked on the author label Dec 6, 2023

mickmister requested review from Kshitij-Katiyar and AayushChaudhary0001 December 6, 2023 07:07

mickmister approved these changes Dec 6, 2023

View reviewed changes

[MM-4] Review fixes: Fixed some variable names

ee39df0

mickmister approved these changes Dec 12, 2023

View reviewed changes

[MM-4] Updated the test data files and test cases

a105d53

mickmister approved these changes Dec 18, 2023

View reviewed changes

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

1f7ce31

… into fix_issue_862

[MM-71] Fix issue: status filter not working with issue created event

926895f

AayushChaudhary0001 approved these changes Jan 4, 2024

View reviewed changes

mickmister approved these changes Jan 18, 2024

View reviewed changes

Merge branch 'master' of github.com:mattermost/mattermost-plugin-jira…

47bc1d5

… into fix_issue_862

mickmister added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 9, 2024

mickmister merged commit 20f948a into mattermost:master Feb 9, 2024
9 checks passed

raghavaggarwal2308 deleted the fix_issue_862 branch February 12, 2024 10:10

[GH-862] Added issue status field in subscription modal #976

[GH-862] Added issue status field in subscription modal #976

Conversation

raghavaggarwal2308 commented Sep 26, 2023 • edited by mickmister Loading

Summary

Screenshots

What to test?

Steps to reproduce

Environment:

Ticket Link

mickmister commented Sep 26, 2023

raghavaggarwal2308 commented Oct 10, 2023

mickmister commented Oct 10, 2023

raghavaggarwal2308 commented Oct 11, 2023

codecov-commenter commented Oct 12, 2023 • edited Loading

Codecov Report

raghavaggarwal2308 commented Oct 12, 2023

mickmister left a comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Oct 26, 2023

mickmister commented Oct 27, 2023 • edited Loading

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Nov 2, 2023

hanzei commented Dec 5, 2023

raghavaggarwal2308 commented Dec 5, 2023

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Dec 7, 2023

arush-vashishtha commented Dec 28, 2023

raghavaggarwal2308 commented Dec 29, 2023

AayushChaudhary0001 commented Jan 4, 2024

mickmister commented Jan 18, 2024 • edited Loading

raghavaggarwal2308 commented Jan 19, 2024

mickmister commented Jan 30, 2024

arush-vashishtha commented Feb 9, 2024

raghavaggarwal2308 commented Sep 26, 2023 •

edited by mickmister

Loading

codecov-commenter commented Oct 12, 2023 •

edited

Loading

mickmister commented Oct 27, 2023 •

edited

Loading

raghavaggarwal2308 Dec 7, 2023 •

edited

Loading

raghavaggarwal2308 Dec 8, 2023 •

edited

Loading

mickmister Dec 8, 2023 •

edited

Loading

mickmister commented Jan 18, 2024 •

edited

Loading