Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 14, 2017

Conversation

csduarte
Copy link
Contributor

@csduarte csduarte commented Sep 8, 2017

Description
This PR allows for a defaultTeam to be defined in the override.json file (src/common/config/override.json). When this value is defined and the config file does not contain any teams in the team’s array, the team defined in the defaultTeam property will be added to the team's array in the config.

Test Cases
Add override values like this:

// Exampleoverride.js
{
  "1": {
    "defaultTeam": {
       “name”: “Test Team”,
       “url”: “https://test.team.com” 
    }
  }
}

Remove the application support folder by running rm -rf /Users/{yourusername}/Library/Application Support/Mattermost

When the application starts the login for the defaultTeam will be shown.

notes
includes bash-20

@dmeza dmeza force-pushed the bash-14 branch 2 times, most recently from 2b08039 to de3c4cc Compare September 8, 2017 17:32
@jasonblais jasonblais added this to the v3.8.0 milestone Sep 15, 2017
@jasonblais
Copy link
Contributor

@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
Copy link
Contributor

yuya-oc commented Sep 18, 2017

https://circleci.com/gh/mattermost/desktop/1023#artifacts However at this time, you would not see any changes if the app is correctly implemented.

@jasonblais
Copy link
Contributor

Hm, that's correct. The settings would have to be specified before building the app. You enable/disable the display of URL hover before building and dist.

@csduarte @dmeza Wondering your thoughts if this should be a setting controlled by the Mattermost server? I would see a benefit of having a default team at server-level.

@dmeza
Copy link
Contributor

dmeza commented Sep 18, 2017

@jasonblais @csduarte the idea of this setting is to already have a default server and not even have to enter it like in this ticket BASH-18 Add configs to show/hide server management and multiteam settings: #600
I think you need the desktop specific changes because it sends you to add a server if one is not configured. The process to check anything server side would mean that you're already connected to a specific server.

@jasonblais
Copy link
Contributor

@dmeza aah, got it. I was thinking of teams as "Mattermost teams" not "servers".

Can you pre-configure more than one server?

@dmeza
Copy link
Contributor

dmeza commented Sep 18, 2017

@jasonblais @csduarte: Yes, if you add more teams directly via the config.json file you would see multiple tabs. If no teams are configured and if defaultTeam is properly configured then you would connect to that specific server directly by default.

Copy link
Contributor

@yuya-oc yuya-oc left a comment

Choose a reason for hiding this comment

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

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

@jasonblais
Copy link
Contributor

jasonblais commented Oct 11, 2017

@yuya-oc Once rebased, would you be able to help with a test build where defaultTeam is set and I'll give this one a try?

@dmeza
Copy link
Contributor

dmeza commented Oct 11, 2017

@yuya-oc rebased and tested.

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 12, 2017

@jasonblais Added https://example.com as the defaultTeam. https://circleci.com/gh/yuya-oc/desktop/420#artifacts

As described in the first comment, please remove AppData\Roadming\Mattermost\config.json before starting the app.

@@ -71,6 +71,12 @@ try {
const spellCheckerLocale = SpellChecker.getSpellCheckerLocale(app.getLocale());
config = settings.loadDefault(null, spellCheckerLocale);
console.log('Failed to read or upgrade config.json', e);
if (!config.teams.length && config.defaultTeam) {
Copy link
Contributor

@jasonblais jasonblais Oct 13, 2017

Choose a reason for hiding this comment

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

@yuya-oc Should we consider renaming the term "team" to a "server" to reduce confusion? I searched through the GitHub repo and realized we refer to servers as teams in many places. This is probably because of how multi-team support worked before the team sidebar was introduced a few months ago.

I could do a search and replace of the terms across our files after this PR and #600 are merged.

Let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasonblais Yeah, we should do that. However, "team" is already used in config.json. So to avoid confusing, I don't think we need to do in v3.8.

Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

Tested and works great. Thanks all!

@yuya-oc
Copy link
Contributor

yuya-oc commented Oct 14, 2017

@jasonblais Added a issue to rename variables. #623

@yuya-oc yuya-oc merged commit e46920e into mattermost:master Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants