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

[MM-22171] Move loadedPlugins into redux-store #5262

Merged
merged 10 commits into from
May 12, 2020
Merged

[MM-22171] Move loadedPlugins into redux-store #5262

merged 10 commits into from
May 12, 2020

Conversation

maciejnems
Copy link
Contributor

@maciejnems maciejnems commented Apr 2, 2020 •
edited by alifarooq0
Loading

Summary

This PR removes loadedPlugins global variable, and switches to using the plugins redux store in mattermost-webapp.

Ticket Link

Fixes: mattermost/mattermost#13928
JIRA: https://mattermost.atlassian.net/browse/MM-22171

@hanzei hanzei added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Apr 3, 2020
@hanzei
Copy link
Contributor

hanzei commented Apr 3, 2020

/update-branch

hmhealey
hmhealey approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @Qovaros! This looks good to me.

@hmhealey
Copy link
Member

hmhealey commented Apr 8, 2020

/update-branch

@alifarooq0
Copy link
Contributor

/update-branch

alifarooq0
alifarooq0 suggested changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@alifarooq0 alifarooq0 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @Qovaros! 🎉 Going through the PR I realized that with this change (my proposal) we'll be introducing another set of plugin state, I hadn't considered that when I wrote the proposal for this ticket, it was an oversight on my part :(. However please see my comment below regarding rejigging a few things that'll help make for a simpler solution.

utils/constants.jsx Outdated Show resolved Hide resolved
reducers/plugins/index.js Outdated Show resolved Hide resolved
alifarooq0
alifarooq0 suggested changes May 7, 2020
View reviewed changes
Copy link
Contributor

@alifarooq0 alifarooq0 left a comment

Choose a reason for hiding this comment

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

Brilliant, looks great @Qovaros 🎉! Just one tiny request to rename the plugins reducer to manifests so we do store.getState().plugins.manifests[manifest.id] instead, after that your changes are good to go :)

plugins/index.js Outdated Show resolved Hide resolved
alifarooq0
alifarooq0 approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@alifarooq0 alifarooq0 left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks @Qovaros!

@alifarooq0 alifarooq0 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter 4: Reviews Complete All reviewers have approved the pull request labels May 7, 2020
@alifarooq0
Copy link
Contributor

Hey @DHaussermann this is ready for QA. Will fill in the details in the jira.

@alifarooq0
Copy link
Contributor

/update-branch

@DHaussermann DHaussermann added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label May 12, 2020
@DHaussermann
Copy link

@Qovaros or @ali-farooq0 I tested the majority of the plugins in the market place.

  • No issues on install
  • Ensured that enable / disable works without needing a refresh
  • No user side console errors on enable / disable. Checked Browser and Desktop client
  • No need for refresh on uninstall

The only issue I found is that with Zoom - when disabled the icon disappears but the /zoom remains in the slash command auto-complete. This did not occur with any other plugin I tested. Any thoughts on if it could be related to this PR?

@alifarooq0
Copy link
Contributor

Thanks for the thorough QA @DHaussermann, can you confirm if same behaviour is observed in master? If so then it's probably isolated to the Zoom plugin and not related to this PR. I suspect it is isolated but better to confirm.

@alifarooq0
Copy link
Contributor

/update-branch

DHaussermann
DHaussermann approved these changes May 12, 2020
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.

@ali-farooq0 the issue did not repro when I tested on my local server with master. When I returned to this PR server, I no longer see the issue. It seems like the autocomplete was maybe caching? Maybe the merge up from master you did changed something?

Regardless - The issue no longer repros on this branch. We're fine to merge this now.

Tested and passed.
Testing noted above.
Thanks @Qovaros for this improvement!
LGTM!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done and removed 3: QA Review Requires review by a QA tester Setup Cloud Test Server Setup a test server using Mattermost Cloud labels May 12, 2020
@mm-cloud-bot
Copy link

Test server destroyed

@alifarooq0
Copy link
Contributor

Thanks @DHaussermann!

Thanks @Qovaros for your contribution! 🎉

merging this now.

@alifarooq0 alifarooq0 merged commit 00d6a96 into mattermost:master May 12, 2020
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 13, 2020
lieut-data added a commit that referenced this pull request May 13, 2020
@lieut-data
Revert "[MM-22171] Move loadedPlugins into redux-store (#5262)"
b802173 
This reverts commit 00d6a96.
lieut-data added a commit that referenced this pull request May 13, 2020
@lieut-data @mattermod
Revert "[MM-22171] Move loadedPlugins into redux-store (#5262)" (#5488
b71676a 
)

This reverts commit 00d6a96.

Co-authored-by: mattermod <[email protected]>
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
@maciejnems @bradjcoughlin
[MM-22171] Move loadedPlugins into redux-store (mattermost#5262)
d5d6658
bradjcoughlin pushed a commit to bradjcoughlin/mattermost-webapp that referenced this pull request May 19, 2020
@lieut-data @mattermod @bradjcoughlin
Revert "[MM-22171] Move loadedPlugins into redux-store (mattermost#…
37b7805 
…5262)" (mattermost#5488)

This reverts commit 00d6a96.

Co-authored-by: mattermod <[email protected]>
@maciejnems maciejnems mentioned this pull request May 25, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hmhealey hmhealey hmhealey approved these changes

@alifarooq0 alifarooq0 alifarooq0 approved these changes

@DHaussermann DHaussermann DHaussermann approved these changes

Assignees

@DHaussermann DHaussermann

Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
Milestone
No milestone
8 participants
@maciejnems @hanzei @hmhealey @alifarooq0 @DHaussermann @mm-cloud-bot @amyblais @mattermod