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

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

Merged
merged 10 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 1 addition & 1 deletion server/client_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@raghavaggarwal2308 raghavaggarwal2308 Dec 7, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion server/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const (

type CreateMetaInfo struct {
*jira.CreateMetaInfo
IssueTypes []*IssueTypeWithStatuses `json:"issue_types"`
IssueTypes []*IssueTypeWithStatuses `json:"issue_types_with_statuses"`
raghavaggarwal2308 marked this conversation as resolved.
Show resolved Hide resolved
}

func makePost(userID, channelID, message string) *model.Post {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ exports[`components/JiraEpicSelector should match snapshot 1`] = `
issueMetadata={
Object {
"expand": "projects",
"issue_types": Array [
"issue_types_with_statuses": Array [
Object {
"statuses": Array [],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ exports[`components/ChannelSubscriptionFilters should match snapshot 1`] = `
instanceID="https://something.atlassian.net"
issueMetadata={
Object {
"issue_types": Array [],
"issue_types_with_statuses": Array [],
"projects": Array [
Object {
"issuetypes": Array [
Expand Down Expand Up @@ -219,7 +219,7 @@ exports[`components/ChannelSubscriptionFilters should match snapshot 1`] = `
}
issueMetadata={
Object {
"issue_types": Array [],
"issue_types_with_statuses": Array [],
"projects": Array [
Object {
"issuetypes": Array [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ exports[`components/EditChannelSubscription should match snapshot after fetching
issueMetadata={
Object {
"expand": "projects",
"issue_types": Array [
"issue_types_with_statuses": Array [
Object {
"statuses": Array [],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -781,7 +781,7 @@ exports[`components/CreateIssue should match snapshot with error 1`] = `
issueMetadata={
Object {
"expand": "projects",
"issue_types": Array [
"issue_types_with_statuses": Array [
Object {
"statuses": Array [],
},
Expand Down Expand Up @@ -4244,7 +4244,7 @@ exports[`components/CreateIssue should match snapshot with form filled 1`] = `
issueMetadata={
Object {
"expand": "projects",
"issue_types": Array [
"issue_types_with_statuses": Array [
Object {
"statuses": Array [],
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"expand": "projects",
"issue_types": [
"issue_types_with_statuses": [
{
"statuses": []
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"expand": "projects",
"issue_types": [
"issue_types_with_statuses": [
{
"statuses": []
}
Expand Down
2 changes: 1 addition & 1 deletion webapp/src/testdata/jira-issue-metadata-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ export const useFieldForIssueMetadata = (field: JiraField, key: string): IssueMe
}],
},
],
issue_types: [],
issue_types_with_statuses: [],
};
};
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"expand": "projects",
"issue_types": [
"issue_types_with_statuses": [
{
"statuses": []
}
Expand Down
2 changes: 1 addition & 1 deletion webapp/src/types/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export type IssueTypeWithStatuses = {

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

export type Status = {
Expand Down
105 changes: 102 additions & 3 deletions webapp/src/utils/jira_issue_metadata.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import createMeta from 'testdata/cloud-get-create-issue-metadata-for-project-man
import {ticketData} from 'testdata/get-ticket-metadata-for-tooltip';
import {useFieldForIssueMetadata} from 'testdata/jira-issue-metadata-helpers';

import {IssueMetadata, JiraField, FilterField, ChannelSubscriptionFilters, FilterFieldInclusion, IssueType, Project} from 'types/model';
import {IssueMetadata, JiraField, FilterField, ChannelSubscriptionFilters, FilterFieldInclusion} from 'types/model';
import {IssueAction, TicketDetails} from 'types/tooltip';

import {getCustomFieldFiltersForProjects, generateJQLStringFromSubscriptionFilters, getConflictingFields, jiraIssueToReducer} from './jira_issue_metadata';
import {getCustomFieldFiltersForProjects, generateJQLStringFromSubscriptionFilters, getConflictingFields, jiraIssueToReducer, getStatusField} from './jira_issue_metadata';

describe('utils/jira_issue_metadata', () => {
const useField = (field: JiraField, key: string): IssueMetadata => {
Expand Down Expand Up @@ -293,6 +293,105 @@ describe('utils/jira_issue_metadata', () => {
expect(actual[0].values).toEqual([{value: '10001', label: 'Admin only'}, {value: '10000', label: 'Everyone'}, {value: '10002', label: 'Staff'}]);
});

test('getStatusField should return options for statuses for selected issue types only', () => {
const metadata: IssueMetadata = {
projects: [
{
key: 'TEST',
issuetypes: [],
},
],
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@mickmister mickmister Dec 8, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister Updated the test files

issue_types_with_statuses: [
{
id: '10001',
name: 'Task',
statuses: [
{
id: '1001',
name: 'TODO',
},
],
},
{
id: '10002',
name: 'Story',
statuses: [
{
id: '1002',
name: 'In Progress',
},
],
},
],
};

const actual = getStatusField(metadata, ['10001']);
expect(actual).not.toBe(null);

if (actual) {
expect(actual.key).toEqual('status');
expect(actual.name).toEqual('Status');
expect(actual.values).toEqual([{value: '1001', label: 'TODO'}]);
}
});

test('getStatusField should return options for statuses for all issue types if no issue type is selected', () => {
mickmister marked this conversation as resolved.
Show resolved Hide resolved
const metadata: IssueMetadata = {
projects: [
{
key: 'TEST',
issuetypes: [],
},
],
issue_types_with_statuses: [
{
id: '10001',
name: 'Task',
statuses: [
{
id: '1001',
name: 'TODO',
},
],
},
{
id: '10002',
name: 'Story',
statuses: [
{
id: '1002',
name: 'In Progress',
},
],
},
],
};

const actual = getStatusField(metadata, []);
expect(actual).not.toBe(null);

if (actual) {
expect(actual.key).toEqual('status');
expect(actual.name).toEqual('Status');
expect(actual.values).toEqual([{value: '1001', label: 'TODO'}, {value: '1002', label: 'In Progress'}]);
}
});

test('getStatusField should return null for statuses if statuses information is empty', () => {
const metadata: IssueMetadata = {
projects: [
{
key: 'TEST',
issuetypes: [],
},
],
issue_types_with_statuses: [],
};

const actual = getStatusField(metadata, []);
expect(actual).toBe(null);
});

test('should return options with a `userDefined` flag for array of strings', () => {
const field = {
autoCompleteUrl: 'https://mmtest.atlassian.net/rest/api/1.0/labels/suggest?customFieldId=10071&query=',
Expand Down Expand Up @@ -388,7 +487,7 @@ describe('utils/jira_issue_metadata', () => {
};

const issueMetadata: IssueMetadata = {
issue_types: [
issue_types_with_statuses: [
{
id: '10001',
name: 'Bug',
Expand Down
80 changes: 44 additions & 36 deletions webapp/src/utils/jira_issue_metadata.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,47 @@ function isValidFieldForFilter(field: JiraField): boolean {
(type === 'array' && allowedArrayTypes.includes(items));
}

export function getStatusField(metadata: IssueMetadata | null, issueTypes: string[]): FilterField | null {
// Filtering out the statuses on the basis of selected issue types
const issueTypesWithStatuses = metadata && metadata.issue_types_with_statuses;
const keys = new Set<string>();
const statuses: Status[] = [];
if (issueTypesWithStatuses) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@raghavaggarwal2308 raghavaggarwal2308 Dec 8, 2023

Choose a reason for hiding this comment

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

I will rename issueTypes to selectedIssueTypes for more clarity.

@mickmister Already changed

}
}
}
}
}

if (!statuses.length) {
return null;
}

return {
key: FIELD_KEY_STATUS,
name: 'Status',
schema: {
type: 'array',
},
values: statuses.map((value) => ({
label: value.name,
value: value.id,
})),
issueTypes: metadata && metadata.issue_types_with_statuses.map((type) => {
return {
id: type.id,
name: type.name,
};
}),
} as FilterField;
}

export function getCustomFieldFiltersForProjects(metadata: IssueMetadata | null, projectKeys: string[], issueTypes: string[]): FilterField[] {
const fields = getCustomFieldsForProjects(metadata, projectKeys).filter(isValidFieldForFilter);
const selectFields = fields.filter((field) => Boolean(field.allowedValues && field.allowedValues.length)) as (SelectField & FieldWithInfo)[];
Expand Down Expand Up @@ -229,42 +270,9 @@ export function getCustomFieldFiltersForProjects(metadata: IssueMetadata | null,
} as FilterField);
}

// Filtering out the statuses on the basis of selected issue types
const issueTypesWithStatuses = metadata && metadata.issue_types;
const keys = new Set<string>();
const statuses: Status[] = [];

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

if (statuses.length) {
result.push({
key: FIELD_KEY_STATUS,
name: 'Status',
schema: {
type: 'array',
},
values: statuses.map((value) => ({
label: value.name,
value: value.id,
})),
issueTypes: metadata && metadata.issue_types.map((type) => {
return {
id: type.id,
name: type.name,
};
}),
} as FilterField);
const statusField = getStatusField(metadata, issueTypes);
if (statusField) {
result.push(statusField);
}

return sortByName(result);
Expand Down
Loading