Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

MM-14897: Changes to be able to add and remove groups from channels. #827

Merged
merged 13 commits into from May 15, 2019

Conversation

mkraft
Copy link
Contributor

@mkraft mkraft commented May 6, 2019

Summary

Change to be able to add and remove groups from channels.

This is pretty much the same thing as #821, but for channels.

Relies on: mattermost/mattermost#10794

Ticket Link

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

Checklist

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit tests passed
  • Ran make flow to ensure type checking passed
  • Added or updated unit tests (required for all new features)
  • All new/modified APIs include changes to the drivers

Test Information

Chrome browser, macOS.

@mkraft mkraft added the Work In Progress Not yet ready for review label May 6, 2019
@mkraft mkraft force-pushed the MM-14410 branch 2 times, most recently from ae056bd to 0fd0d3e Compare May 9, 2019 13:41
@mkraft mkraft added this to the v5.12.0 milestone May 9, 2019
@mkraft mkraft changed the base branch from MM-14410 to master May 10, 2019 15:59
@mkraft mkraft added 2: Dev Review Requires review by a core commiter and removed Work In Progress Not yet ready for review labels May 10, 2019
@mkraft mkraft added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label May 10, 2019
@@ -51,6 +51,11 @@ function getTeamGroupIDSet(state, teamID) {
return new Set(arr);
}

function getChannelGroupIDSet(state, channelID) {
Copy link
Member

Choose a reason for hiding this comment

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

this selector will every single time generate a new object, we could create a function for the array, and a selector (based on that function) for the set, that way, if the underneath array hasn't change, the set will be cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Looks good in general, but a selector needs an small adjustment :)

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM

@jespino jespino added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels May 14, 2019
@mkraft mkraft merged commit f8fa3de into master May 15, 2019
@mkraft mkraft deleted the MM-14897 branch May 15, 2019 16:03
@lindalumitchell lindalumitchell added the Tests/Done Required tests have been written label May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Tests/Done Required tests have been written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants