BASH-18 Add configs to show/hide server management and multiteam settings #600

csduarte · 2017-09-08T17:02:04Z

Description
This PR add values enableMultiTeams and enableServerManagement to show/hide server management and multiteam settings.

read and understood our Contributing Guidelines
completed Mattermost Contributor Agreement
executed npm run lint:js for proper code formatting

Test Cases
Add override values like this to override.json (src/common/config/override.json)
After entering the a fist server no others are accepted

//override.js
{
  "1": {
    "enableMultiTeams": false,
    "enableServerManagement": false
  }
}

Notes
Includes bash-20

jasonblais · 2017-09-15T18:28:42Z

@yuya-oc Same as in #587, can you help share a link for the test build from circle-ci? I'll test this out.

yuya-oc · 2017-09-18T11:40:35Z

https://circleci.com/gh/mattermost/desktop/1020#artifacts

jasonblais · 2017-09-18T13:57:49Z

@csduarte @dmeza What is the purpose of the enableMultiTeams setting?

dmeza · 2017-09-18T15:50:00Z

@jasonblais @yuya-oc enableMultiTeams=false hides the Add new server link if there's already a server added. enableServerManagement=false hides the whole server management, it will not allow you to edit or remove, if there's already a server added.
The image bellow is with both options to false:

jasonblais · 2017-09-18T16:39:36Z

@dmeza Thanks! I can definitely see the use case for enableServerManagement (e.g. use #594 to pre-configure the server URL and don't allow the end user to change it).

Couple of more questions:

can the user configure it via the config.json file? E.g. could they modify it and add more than one server?
would you have an example use case for when enableMultiTeams is false but enableServerManagement is true?

dmeza · 2017-09-18T17:32:22Z

@jasonblais @csduarte:

Yes, the user can configure it via the config.json file. Yes, they could add more than one server.
an example use case for when enableMultiTeams is false but enableServerManagement is true, is when the client can connect to only one server. User could remove or edit the server but never add more than one thru the config screen.

yuya-oc

Due to merging of #586, please rebase the branch.

jasonblais · 2017-10-11T12:51:17Z

@yuya-oc Once rebased, would you be able to help with test builds for the four cases

enableMultiTeams = false / enableServerManagement - false
enableMultiTeams = false / enableServerManagement - true
enableMultiTeams = true / enableServerManagement - false
enableMultiTeams = true / enableServerManagement - true

I'll give this one a try

dmeza · 2017-10-11T17:27:08Z

@yuya-oc rebased and tested.

yuya-oc · 2017-10-12T13:30:49Z

@dmeza Probably you haven't pushed the branch yet.

dmeza · 2017-10-12T15:40:55Z

@yuya-oc I think it was new conflicts from the merge of bash-11. It's rebased and green.

yuya-oc · 2017-10-12T16:21:49Z

@jasonblais

enableMultiTeams = false / enableServerManagement - false
https://circleci.com/gh/yuya-oc/desktop/424#artifacts
enableMultiTeams = false / enableServerManagement - true
https://circleci.com/gh/yuya-oc/desktop/425#artifacts
enableMultiTeams = true / enableServerManagement - false
https://circleci.com/gh/yuya-oc/desktop/427#artifacts
enableMultiTeams = true / enableServerManagement - true
https://circleci.com/gh/yuya-oc/desktop/428#artifacts

jasonblais · 2017-10-18T14:35:35Z

@dmeza Tested all the different cases. Left a few test notes below, but overall it works as expected.

I'm a bit hesitant on how useful enableMultiTeams actually is for admins? Is there a compliance regulation where the user would not be allowed to sign into more than one server at a time?

I can see admins wanting to flip enableServerManagement on/off especially combined with defaultTeam setting added with #594

Test cases:

enableMultiTeams = false / enableServerManagement = false

You can still successfully add a new server via

File > Sign in to Another Server (Mattermost > Sign in to Another Server on MacOS)
Clicking the "+" icon in the server tab bar, if you've added more than one server via config.json (see attached)

If enableServerManagement is set to false, propose

Hiding the menu bar option for Sign in to Another Server
Hiding the "+" icon

enableMultiTeams = false / enableServerManagement = true

Same issues as in the first test case. However, the Sign in to Another Server should only be hidden in this case if you've already added a server.

Another issue I found is that the Server Management section was still missing from the Settings page. So you couldn't actually edit the existing server (or servers if you had added multiple servers directly via config.json).

enableMultiTeams = true / enableServerManagement = false

Same issues as in the first test case, given server management is disabled.

enableMultiTeams = true / enableServerManagement = true

Default case. No issues.

@yuya-oc Once we work on #618, would it be possible to not allow users define teams via config.json if enableServerManagement is set to false?

@yuya-oc @dmeza enableServerManagement should really be an Enterprise setting given it limits user freedom, but I realize we don't have such a thing on the desktop app. I'm not sure if there's much we can do about that?

All the other settings are a benefit for the end user or make the initial administration easier, including settings like defaultTeam.

jasonblais

See above notes

yuya-oc · 2017-10-18T15:01:13Z

@jasonblais Yes. Once we decide, I think enableServerManagement: false can prevent teams which are written at user's config.json.

This feature is never used for us because we must use enableServerManagement: true. I feel the feature would be useful when creating fully-customized enterprise app which uses only specific server though. But we need to continue to maintain unused feature in future, so it might be not suitable for our code base.

jasonblais · 2017-10-24T22:38:42Z

@yuya-oc

Yes. Once we decide, I think enableServerManagement: false can prevent teams which are written at user's config.json.

Would this be easy to implement already for this PR?

This feature is never used for us because we must use enableServerManagement: true.

That's true, but I think that particular setting is fine to support. I can see that some Enterprise admins would find this feature useful. However, the setting I'm less certain about is enableMultiTeams.

dmeza · 2017-10-24T23:20:40Z

@yuya-oc rebased to latest from master. Made changes for comments from @jasonblais #600 (comment):

Hide Sign in to Another Server and "+" icon when enableMultiTeams = false

Jason the problem you saw with:
Another issue I found is that the Server Management section was still missing from the Settings page. So you couldn't actually edit the existing server (or servers if you had added multiple servers directly via config.json).
It's because the values are set in: config.json in the user directory ~/Library/Application Support/Mattermost on OS X

Let me know if you have any other questions.

yuya-oc · 2017-10-25T14:46:50Z

src/browser/components/TabBar.jsx

-      <Glyphicon glyph='plus'/>
-    </NavItem>
-  );
+  if (AppConfig.data.enableMultiTeams === true) {


Probably enableServerManagement should be referred here instead of enableMultiTeams.

And please make components stateless as well as possible. So the value should be passed via props.
For example, add showAddServerButton prop to TabBar.jsx and MainPage.jsx, then pass the parameter at ReactDOM.render() of index.jsx.

@yuya-oc refactored to keep TabBar.jsx stateless. The right condition is to hide the plus is when enableMultiTeams= true since that's when you can add more teams.

yuya-oc · 2017-10-25T14:52:55Z

src/common/config/base.json

+    "helpLink": "https://docs.mattermost.com/help/apps/desktop-guide.html",
+    "enableMultiTeams": true,
+    "enableServerManagement": true,
+    "allowDarwinIconTray": true


Probably unnecessary in this PR.

@yuya-oc that config is related to this condition: https://github.com/mattermost/desktop/pull/600/files#diff-125da514cff642c8a155d6a3e3f118d0R403
I guess it shouldn't have been part of the same commit. How would you like me to proceed? do another PR?

@yuya-oc removed this logic and will add it to a new PR.

yuya-oc · 2017-10-25T14:53:05Z

src/common/config/base.json

+    "helpLink": "https://docs.mattermost.com/help/apps/desktop-guide.html",
+    "enableMultiTeams": true,
+    "enableServerManagement": true,
+    "allowDarwinIconTray": true


Probably unnecessary in this PR.

yuya-oc · 2017-10-26T12:17:04Z

@dmeza To make sure, would you tell us the purpose of enableServerManagement and enableMultiTeams?

I feel enableServerManagement=false is good to prevent adding/editing servers. But I could not imagine the situation: users can edit server settings but it's limited up to one (enableServerManagement=true, enableMultiTeams=false).

When do admins want to limit the number of servers? If the admin can use build-time config, I feel they uses defaultTeams of #633 and enableServerManagement=false.

dmeza · 2017-10-26T16:21:15Z

@yuya-oc @jasonblais how it was originally thought was:

enableMultiTeams=false restricts the user from adding more teams. if one doesn't exist, it allows to add one.
enableServerManagement=false disables the ability to edit or delete teams, so the user can not change them once added.
Is this correct @csduarte?

jasonblais · 2017-10-30T14:12:02Z

@dmeza Thanks for the reply. How are these settings used at Shift?

dmeza · 2017-10-30T16:12:49Z

@jasonblais both would be set to false since there is not option to add new servers or edit/delete the existing one. The changes are not exactly the same right now for shift/desktop since there's burnt logic to have a default Team, to not allow add more Teams and to not allow to edit Teams.

jasonblais · 2017-10-30T17:45:04Z

@dmeza If enableServerManagement is set to false, then enableMultiTeams is obsolete given you can't edit (or add) servers?

I'd lean towards removing enableMultiTeams for now unless it's needed by Shift. I agree with @yuya-oc that the use case for this setting is limited.

dmeza · 2017-11-01T17:53:27Z

@jasonblais @yuya-oc: discussed yesterday with @csduarte that two configs where added in case they needed to be handled as separate options. Shift will just be disabling all options to edit or add servers. I removed config enableMultiTeams and all of it's logic so this ticket can move forward.
Also, rebased to the latest. Let me know if you need any other changes.

jasonblais · 2017-11-01T20:40:01Z

Thanks @dmeza!

@yuya-oc Can you help share a test build with "enableServerManagement": false. I can grab "enableServerManagement": true from the Circle CI.

yuya-oc · 2017-11-02T15:51:53Z

#600: https://circleci.com/gh/mattermost/desktop/1099
"enableServerManagement": false: https://circleci.com/gh/yuya-oc/desktop/468

jasonblais · 2017-11-05T00:43:18Z

@dmeza When enableServerManagement is set to false, I unfortunately still see the following issues

You can still successfully add a new server via

File > Sign in to Another Server (Mattermost > Sign in to Another Server on MacOS)
Clicking the "+" icon in the server tab bar, if you've added more than one server via config.json (see attached)

If enableServerManagement is set to false, propose

Hiding the menu bar option for Sign in to Another Server
Hiding the "+" icon

It would also be good if we prevented users from editing/adding servers via config.json if enableServerManagement is set to false. But this I can create a separate ticket for in order to merge this one and not let it sit much longer.

dmeza · 2017-11-06T16:21:04Z

@jasonblais I believe it's because you have the value "enableServerManagement": true in ~/Library/Application Support/Mattermost/config.json (on OS X). Tested locally and it's working properly. Please help me test again.

jasonblais · 2017-11-06T16:52:29Z

@dmeza hm, I tested again and it seems to be working. Thanks!

@yuya-oc this one is good to merge.

yuya-oc · 2017-11-07T11:56:43Z

I could confirmed, too. Thanks!

jasonblais added this to the v3.8.0 milestone Sep 15, 2017

jasonblais added All Platforms null Windows (looking for testers) labels Sep 15, 2017

dmeza mentioned this pull request Sep 18, 2017

BASH-14 Set default server/team in settings #594

Merged

3 tasks

yuya-oc suggested changes Oct 11, 2017

View reviewed changes

dmeza force-pushed the BASH-18 branch from 62932fa to 5e33705 Compare October 12, 2017 15:37

jasonblais requested changes Oct 18, 2017

View reviewed changes

dmeza force-pushed the BASH-18 branch from 5e33705 to 241839e Compare October 24, 2017 23:16

yuya-oc reviewed Oct 25, 2017

View reviewed changes

yuya-oc mentioned this pull request Oct 30, 2017

Simplify and split config.json into defaultPreferences and buildConfig #633

Merged

3 tasks

add server management and multiteam settings

8c82685

dmeza force-pushed the BASH-18 branch from 4209e58 to 8c82685 Compare November 1, 2017 17:45

yuya-oc approved these changes Nov 7, 2017

View reviewed changes

yuya-oc merged commit 8003c49 into mattermost:master Nov 7, 2017

yuya-oc mentioned this pull request Nov 7, 2017

Dialog to confirm requested permissions #609

Merged

3 tasks

BASH-18 Add configs to show/hide server management and multiteam settings #600

BASH-18 Add configs to show/hide server management and multiteam settings #600

Conversation

csduarte commented Sep 8, 2017 • edited Loading

jasonblais commented Sep 15, 2017

yuya-oc commented Sep 18, 2017

jasonblais commented Sep 18, 2017

dmeza commented Sep 18, 2017 • edited Loading

jasonblais commented Sep 18, 2017

dmeza commented Sep 18, 2017

yuya-oc left a comment

Choose a reason for hiding this comment

jasonblais commented Oct 11, 2017 • edited Loading

dmeza commented Oct 11, 2017

yuya-oc commented Oct 12, 2017

dmeza commented Oct 12, 2017

yuya-oc commented Oct 12, 2017

jasonblais commented Oct 18, 2017

jasonblais left a comment

Choose a reason for hiding this comment

yuya-oc commented Oct 18, 2017

jasonblais commented Oct 24, 2017

dmeza commented Oct 24, 2017 • edited Loading

yuya-oc Oct 25, 2017

Choose a reason for hiding this comment

dmeza Oct 25, 2017

Choose a reason for hiding this comment

yuya-oc Oct 25, 2017

Choose a reason for hiding this comment

dmeza Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

dmeza Oct 25, 2017

Choose a reason for hiding this comment

yuya-oc Oct 25, 2017

Choose a reason for hiding this comment

yuya-oc commented Oct 26, 2017

dmeza commented Oct 26, 2017

jasonblais commented Oct 30, 2017

dmeza commented Oct 30, 2017

jasonblais commented Oct 30, 2017

dmeza commented Nov 1, 2017 • edited Loading

jasonblais commented Nov 1, 2017

yuya-oc commented Nov 2, 2017

jasonblais commented Nov 5, 2017

dmeza commented Nov 6, 2017

jasonblais commented Nov 6, 2017

yuya-oc commented Nov 7, 2017

csduarte commented Sep 8, 2017 •

edited

Loading

dmeza commented Sep 18, 2017 •

edited

Loading

jasonblais commented Oct 11, 2017 •

edited

Loading

dmeza commented Oct 24, 2017 •

edited

Loading

dmeza Oct 25, 2017 •

edited

Loading

dmeza commented Nov 1, 2017 •

edited

Loading