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

MM-8622: Improved plugin error reporting #1180

Merged
merged 8 commits into from
May 23, 2018
Merged

MM-8622: Improved plugin error reporting #1180

merged 8 commits into from
May 23, 2018

Conversation

lieut-data
Copy link
Member

@lieut-data lieut-data commented May 7, 2018 •
edited
Loading

Summary

This represents the implementation effort of a design proposed in ~developer-toolkit on pre-release.mattermost.com.

Here's the main take aways from the webapp PR. More details are given in the accompanying server and redux PRs:

  • The Plugins > Management page is now driven by queries / websocket events re: PluginStatuses
  • Updated plugin UI for single server case:
    image
  • Updated plugin UI for multi-server case:
    image

Ticket Link

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

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)
  • Has server changes: MM-8622: Improved plugin error reporting mattermost#8737
  • Has redux changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates

@lieut-data lieut-data added 2: Dev Review Requires review by a core commiter Do Not Merge Should not be merged until this label is removed labels May 7, 2018
@lieut-data lieut-data mentioned this pull request May 7, 2018
Merged
2 tasks
@lieut-data lieut-data added the 1: PM Review Requires review by a product manager label May 7, 2018
@lieut-data lieut-data mentioned this pull request May 7, 2018
Merged
6 tasks
@lieut-data lieut-data force-pushed the mm-8622-plugin-error-reporting branch from 72c93da to fb60761 Compare May 7, 2018 02:13
@jasonblais jasonblais self-assigned this May 7, 2018
@jwilander jwilander added this to the v4.11.0 milestone May 7, 2018
@jwilander
Copy link
Member

This is going to be a great addition to plugins 👍

enahum
enahum approved these changes May 7, 2018
View reviewed changes
Copy link
Contributor

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

@jasonblais
Copy link
Contributor

@asaadmahmood @michaelgamble thoughts on the above UI screenshots?

@asaadmahmood
Copy link
Contributor

@jasonblais Looks good for the most part, shouldn't we have a different icon for the running and off state? Perhaps an x icon for the off state, and a checkmark icon for the running state?

@jasonblais
Copy link
Contributor

@asaadmahmood Makes sense when the plugin is running.

The plugin may also be intentionally deactivated (e.g. someone installed Zoom but decided they didn't want it). So I'm not sure if "X" makes sense in that case, although definitely open to using a different icon.

@asaadmahmood
Copy link
Contributor

@jasonblais Then maybe we can use this icon for the deactivated state. The "x" icon may indicate something is wrong.
https://fontawesome.com/icons/ban?style=solid

@jasonblais
Copy link
Contributor

@lieut-data Is there enough information here for you to make the two updates (use a checkmark when the plugin is running and ban icon when it's deactivated).

@lieut-data
Copy link
Member Author

@jasonblais, yep, I can effect those changes!

@lieut-data lieut-data force-pushed the mm-8622-plugin-error-reporting branch from 5096393 to e5924c9 Compare May 21, 2018 17:34
@lieut-data lieut-data removed the Do Not Merge Should not be merged until this label is removed label May 23, 2018
@lieut-data lieut-data removed the 1: PM Review Requires review by a product manager label May 23, 2018
@lieut-data
Copy link
Member Author

lieut-data commented May 23, 2018 •
edited
Loading

This was already reviewed, but I'm waiting on the server PR before merging.

@lieut-data lieut-data merged commit 790b5f2 into master May 23, 2018
@lieut-data lieut-data deleted the mm-8622-plugin-error-reporting branch May 23, 2018 18:26
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels May 23, 2018
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Jun 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwilander jwilander jwilander approved these changes

@enahum enahum enahum approved these changes

@jasonblais jasonblais Awaiting requested review from jasonblais

Assignees

@jasonblais jasonblais

Labels
2: Dev Review Requires review by a core commiter Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lieut-data @jwilander @jasonblais @asaadmahmood @enahum @lindalumitchell @amyblais