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

[MM-17061] Support multiple projects for a channel subscription #232

Merged
merged 5 commits into from
Jul 20, 2019
Merged

[MM-17061] Support multiple projects for a channel subscription #232

merged 5 commits into from
Jul 20, 2019

Conversation

mickmister
Copy link
Member

Summary

This PR allows the user to choose multiple projects for a channel subscription.

Ticket Link

https://mattermost.atlassian.net/browse/MM-17061

https://mattermost-plugin-jira-test.test.spinmint.com/test/channels/town-square

jasonblais
jasonblais reviewed Jul 18, 2019
View reviewed changes
Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

Questions:

  1. Can you help share a screenshot, just so I see what it looks like? (I assume it's just a multi-select dropdown like the other two fields)
  2. Confirming we de-duplicate values in the other two fields (events and issue types)
  3. Do we have a limit of how many projects can be subscribed?

@mickmister
Copy link
Member Author

1
2
3

@jasonblais jasonblais added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Jul 18, 2019
@jasonblais jasonblais added this to the v2.1 milestone Jul 18, 2019
@mickmister
Copy link
Member Author

@jasonblais
2. Events and issue type selections are both de-duplicated. Also, when you deselect a project, any event or issue type selections you have made that are specific to only that project are removed from your selections.

  1. I did not put a limit on the number of projects. I have progress on a different branch on limiting the amount of data we give to the browser for this modal. Even though it is scoped to a few projects, there isn't any parsing we're doing to shorten the payload further. This will improve the amount of data we provide the browser for the create issue modal as well. I don't think it needs to go into 2.1
    cc @levb

jasonblais
jasonblais approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

Thanks!

@jasonblais jasonblais removed the 1: PM Review Requires review by a product manager label Jul 18, 2019
crspeller
crspeller approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

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

One non-blocking nit.

webapp/src/components/modals/channel_settings/channel_settings_internal.jsx Outdated Show resolved Hide resolved
@mickmister
Copy link
Member Author

@DHaussermann test server has latest push

@mickmister mickmister added the 3: QA Review Requires review by a QA tester label Jul 19, 2019
@mickmister mickmister requested a review from levb July 19, 2019 06:04
cpoile
cpoile approved these changes Jul 19, 2019
View reviewed changes
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Just minor possible changes, I'll approve for now. You decide on whether to make the changes.
Nice work Michael!

server/http.go
@@ -65,7 +65,7 @@ func handleHTTPRequest(p *Plugin, w http.ResponseWriter, r *http.Request) (int,
case routeAPICreateIssue:
return withInstance(p.currentInstanceStore, w, r, httpAPICreateIssue)
case routeAPIGetCreateIssueMetadata:
return withInstance(p.currentInstanceStore, w, r, httpAPIGetCreateIssueMetadataForProject)
return withInstance(p.currentInstanceStore, w, r, httpAPIGetCreateIssueMetadataForProjects)
Copy link
Member

Choose a reason for hiding this comment

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

👍

webapp/src/components/modals/channel_settings/channel_settings_internal.jsx
@@ -113,21 +115,54 @@ export default class ChannelSettingsModalInner extends PureComponent {
};

handleProjectChange = (id, value) => {
const projectKey = value;
let projects = value;
Copy link
Member

Choose a reason for hiding this comment

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

maybe refactor into ensureArray (reuse for both handle fns)? or maybe not worth it. up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not introduce a library for this PR, I think that's what you mean. However, it does sound like a nice way to remove that ugly type checking code 😜

webapp/src/components/modals/channel_settings/channel_settings_internal.jsx
});

const selectedEventTypes = this.state.filters.events.filter((eventType) => {
if (eventType.includes('customfield')) {
Copy link
Member

Choose a reason for hiding this comment

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

I forget -- does it always start with 'customfield' or it's always somewhere inside the type name? If it starts with, should use startsWith just in case.

Copy link
Member Author

Choose a reason for hiding this comment

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

The event is event_updated_${fieldId} where fieldId would be something like customfield_10001 here.

webapp/src/utils/jira_issue_metadata.jsx
issueTypeHash[issueType.value] = issueType;
});

return Object.values(issueTypeHash);
Copy link
Member

Choose a reason for hiding this comment

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

nice, I like it.

levb
levb approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@levb levb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nice. I am assuming waiting for @DHaussermann to test, before merging?

DHaussermann
DHaussermann approved these changes Jul 19, 2019
View reviewed changes
Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

This change is working. Multiple project subscriptions are functional.
After the merge I will do more testing around the subscribe modal on jira server as there is an unrelated issue reported by Chris P about this.

LGTM

@DHaussermann
Copy link

Also confirmed with @jasonblais we're still okay to include this change in 5.14.

@jasonblais jasonblais 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 Jul 19, 2019
@mickmister mickmister merged commit d864933 into mattermost:master Jul 20, 2019
@mickmister mickmister deleted the multiple-projects-updated branch July 20, 2019 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levb levb levb approved these changes

@cpoile cpoile cpoile approved these changes

@crspeller crspeller crspeller approved these changes

@jasonblais jasonblais jasonblais approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.
6 participants
@mickmister @DHaussermann @levb @cpoile @crspeller @jasonblais