Remove mm_config and mm_license global state from webapp (PR #6) #823

lieut-data · 2018-02-15T21:20:04Z

Summary

This is a subset of the changes in the MM-8589-remove_mm_global_license branch, broken up into multiple PRs to facilitate easier review. See any linked PRs for other changes if you're interested.

These changes, as a whole, move towards removing our dependence on the global mm_config and mm_license objects. These changes will ultimately facilitate resolving https://mattermost.atlassian.net/browse/MM-8604, allowing us to stop hard refreshing the application whenever someone makes a configuration change.

Ticket Link

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

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)

Related PRs

#816
#819
#820
#821
#822

jwilander

LGTM, just one non-blocking comment

jwilander · 2018-02-15T21:27:51Z

components/channel_notifications_modal/index.js

+ const sendPushNotifications = config.SendPushNotifications === 'true';
+
+ return {
+ ...ownProps,


We actually don't need to include ...ownProps in this return statement, the connect function handles it. I know we do it everywhere but it was a mistake that got copied/pasted everywhere before we caught it. @stephenkiers has a a PR #798 to remove them all

I'm 0/5 if you want to go through the effort of removing them here though, I'm trying to avoid being too nitpicky because of the volume of PRs coming through

Ah, fascinating! I'll either rip them out as part of the PRs, or go back and fix things up later depending on the volume of feedback I receive.

jespino

LGTM. Some comments about improvements to do, but they could be done later.

jespino · 2018-02-16T08:19:29Z

components/channel_notifications_modal/channel_notifications_modal.jsx

@@ -247,5 +247,6 @@ ChannelNotificationsModal.propTypes = {
 onHide: PropTypes.func.isRequired,
 channel: PropTypes.object.isRequired,
 channelMember: PropTypes.object.isRequired,
- currentUser: PropTypes.object.isRequired
+ currentUser: PropTypes.object.isRequired,
+ sendPushNotifications: PropTypes.bool.isRequired


We can move the propTypes to the static propType class attribute. Not a stopper.

jespino · 2018-02-16T08:26:24Z

tests/components/navbar/navbar.test.jsx

@@ -55,7 +57,10 @@ describe('components/navbar/Navbar', () => {
 test('should match snapshot, if not licensed', () => {
 global.window.mm_license.IsLicensed = 'false';


I think this line (and similar ones in the following tests) are no longer needed.

Unfortunately, there's some remaining cleanup for higher level components that makes these global assignment still necessary -- for now. Rest assured, it's all going away soon :)

jespino · 2018-02-16T08:33:19Z

@lieut-data feel free to ask me for another review if you apply new changes and you think that changes needs review. 🙂

…ponent

lieut-data · 2018-02-16T15:04:46Z

Thanks, @jespino! I've rebased and removed the unnecessary ownProps and converged on the get(Config|License) selectors. I think we're good to go, so I'll merge as soon as the build completes.

* Migrate a few OAuth actions from webapp * Update src/actions/integrations.js Co-Authored-By: cometkim <[email protected]>

lieut-data requested a review from jwilander February 15, 2018 21:20

jwilander added the 2: Dev Review Requires review by a core commiter label Feb 15, 2018

jwilander approved these changes Feb 15, 2018

View reviewed changes

lieut-data requested a review from jespino February 15, 2018 21:46

jespino approved these changes Feb 16, 2018

View reviewed changes

hmhealey added 4: Reviews Complete All reviewers have approved the pull request Awaiting Submitter Action Blocked on the author and removed 2: Dev Review Requires review by a core commiter labels Feb 16, 2018

lieut-data added 9 commits February 16, 2018 10:00

MM-8589: expunge global mm_config from channel members dropdown

bd8cc90

MM-8589: expunge global mm_config from team sidebar component

dd3904d

MM-8589: expunge global mm_config from invite member modal component

e75e8da

MM-8589: expunge global mm_config from should verify email component

3b5dad4

MM-8589: expunge global mm_config from channel notification modal com…

0bc847f

…ponent

MM-8589: expunge global mm_config from more direct channels component

1b304b0

MM-8589: expunge global mm_config from search results item component

31392aa

MM-8589: expunge unnecessary ownProps

621b891

MM-8589: use the get(Config|License) selector

fc39403

lieut-data force-pushed the MM-8589-remove_mm_global_license_config-pr-6 branch from 63764ad to fc39403 Compare February 16, 2018 15:02

lieut-data merged commit 9c692bd into master Feb 16, 2018

lieut-data deleted the MM-8589-remove_mm_global_license_config-pr-6 branch February 16, 2018 15:31

lieut-data mentioned this pull request Feb 16, 2018

Remove mm_config and mm_license global state from webapp (PR #7) #828

Merged

3 tasks

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Feb 17, 2018

This was referenced Feb 17, 2018

gifv files do not display mattermost/mattermost#8302

Closed

[Help Wanted] [MM-9635] Remove mm_config and mm_license global state from webapp (Phase 2) mattermost/mattermost#8318

Closed

lindalumitchell added the Tests/Not Needed Does not require new release tests label Mar 6, 2018

hmhealey pushed a commit that referenced this pull request Aug 28, 2020

Migrate a few OAuth actions from webapp (#823)

9af2b27

* Migrate a few OAuth actions from webapp * Update src/actions/integrations.js Co-Authored-By: cometkim <[email protected]>

hmhealey pushed a commit that referenced this pull request Mar 17, 2021

Migrate a few OAuth actions from webapp (#823)

936d964

* Migrate a few OAuth actions from webapp * Update src/actions/integrations.js Co-Authored-By: cometkim <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove mm_config and mm_license global state from webapp (PR #6) #823

Remove mm_config and mm_license global state from webapp (PR #6) #823

lieut-data commented Feb 15, 2018

jwilander left a comment

jwilander Feb 15, 2018

lieut-data Feb 15, 2018

jespino left a comment

jespino Feb 16, 2018

jespino Feb 16, 2018

lieut-data Feb 16, 2018

jespino commented Feb 16, 2018 •

edited

Loading

lieut-data commented Feb 16, 2018

		@@ -55,7 +57,10 @@ describe('components/navbar/Navbar', () => {
		test('should match snapshot, if not licensed', () => {
		global.window.mm_license.IsLicensed = 'false';

Remove mm_config and mm_license global state from webapp (PR #6) #823

Remove mm_config and mm_license global state from webapp (PR #6) #823

Conversation

lieut-data commented Feb 15, 2018

Summary

Ticket Link

Checklist

Related PRs

jwilander left a comment

Choose a reason for hiding this comment

jwilander Feb 15, 2018

Choose a reason for hiding this comment

lieut-data Feb 15, 2018

Choose a reason for hiding this comment

jespino left a comment

Choose a reason for hiding this comment

jespino Feb 16, 2018

Choose a reason for hiding this comment

jespino Feb 16, 2018

Choose a reason for hiding this comment

lieut-data Feb 16, 2018

Choose a reason for hiding this comment

jespino commented Feb 16, 2018 • edited Loading

lieut-data commented Feb 16, 2018

jespino commented Feb 16, 2018 •

edited

Loading