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

yuya-oc · 2017-10-23T16:04:50Z

Before submitting, please confirm you've

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

Please provide the following information:

Summary
Simplify and split config.json into defaultPreferences and buildConfig

buildConfig is never overrode by user preferences. And this PR proposes several things.

Write config in JS
It allows to write comments.
Obsolete override.json
Admins need to know its format without any comments. In this PR, what they need to do is just editing existing config files.
Multiple default servers
They are listed on the tab bar prior to user-defined servers. And they are not listed on the settings page.

Side effect: Cache purging would not work by merging this. It would be fixed after merging AppStateManager of #582

Issue link
#618

Test Cases

Additional Notes
https://circleci.com/gh/yuya-oc/desktop/441#artifacts

jasonblais · 2017-10-25T13:33:28Z

Looks great. @yuya-oc Just one question on multiple servers, what do you mean by They are listed on the tab bar prior to user-defined servers. And they are not listed on the settings page.

@dmeza let us know if you have any feedback on this PR. It's a follow-up for #586

yuya-oc · 2017-10-25T14:34:09Z

@jasonblais This is an example. https://circleci.com/gh/yuya-oc/desktop/449#artifacts

// buildConfig.js
defaultTeams: [
  {
    name: 'example',
    url: 'https://example.com'
  },
  {
    name: 'google',
    url: 'https://www.google.com'
  }
],

You would see two tabs at least. And they do not appear on the settings page.

jasonblais · 2017-10-25T14:55:10Z

Ah right, makes sense. Thanks @yuya-oc, looks good to me.

dmeza · 2017-10-25T17:45:19Z

@yuya-oc @jasonblais looks good to me. It allows for the same functionality, thank you.

jasonblais · 2017-10-25T17:58:48Z

@yuya-oc This one is ready to merge

yuya-oc · 2017-10-30T14:58:27Z

To avoid confusing, I'll wait the result of #600.

jasonblais · 2017-11-06T16:54:41Z

@yuya-oc Once #600 is merged, we can also merge this. It'll just need EnableServerManagement setting for the admin config file, so that the end user cannot modify this setting.

jasonblais · 2017-11-06T17:18:18Z

(As part of this PR, we'll also restrict users from modifying servers via config.json when EnableServerManagement is set to false.)

yuya-oc · 2017-11-07T13:40:49Z

Updated: https://circleci.com/gh/yuya-oc/desktop/486#artifacts

enableServerManagement: false: https://circleci.com/gh/yuya-oc/desktop/487#artifacts

Now you can't modify servers by editing config.json. And I added a dialog to show error when the app is unusable.

When "enableServerManagement: false" is specified in buildConfig.js,\n"defaultTeams" must have one team at least.

jasonblais · 2017-11-07T14:46:56Z

@yuya-oc I get the attached dialog when opening the app with enableServerManagement: false or enableServerManagement: true

Application: Mattermost 3.7.1
Platform: Windows_NT 10.0.14393 x64
SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at new PermissionManager (C:\Users\Admin\AppData\Local\mattermost\app-3.7.1\resources\app.asar\main_bundle.js:6559:31)
    at permissionRequestHandler (C:\Users\Admin\AppData\Local\mattermost\app-3.7.1\resources\app.asar\main_bundle.js:6634:29)
    at App.app.on (C:\Users\Admin\AppData\Local\mattermost\app-3.7.1\resources\app.asar\main_bundle.js:8168:54)
    at emitTwo (events.js:111:20)
    at App.emit (events.js:194:7)

jasonblais · 2017-11-07T14:48:00Z

Also, I think there's an extra space in front of internal that I hadn't noticed before

jasonblais · 2017-11-07T14:57:24Z

My permissions.json is empty, not sure if that's related.

yuya-oc · 2017-11-07T14:57:37Z

JSON error: It's weird. It means AppData\Roadming\Mattermost\permission.json is unexpectedly broken or a blank file. Probably the app should ignore the error because the user can allow permissions later again even if the file is broken. For now, please remove AppData\Roadming\Mattermost\permission.json.

I had thought the white space is intended. Should we remove it?

jasonblais · 2017-11-07T15:01:45Z

I removed permission.json, works fine now 👍

For the dialog, does it appear for the end user when they launch the app, enableServerManagement is false and DefaultTeams is empty?

Please also help remove the white space in front of Internal.

jasonblais · 2017-11-07T15:02:19Z

Should I create a ticket for the app to ignore the error related to permissions.json?

yuya-oc · 2017-11-07T15:16:33Z

Yes, so actual wrong config would be below.

{
  defaultTeams: [],
  helpLink: 'https://docs.mattermost.com/help/apps/desktop-guide.html',
  enableServerManagement: false
}

I feel Ticket is not needed. It's easy to fix, so may I fix it on master apart from this PR?

jasonblais · 2017-11-07T15:25:42Z

I feel Ticket is not needed. It's easy to fix, so may I fix it on master apart from this PR?

Yes, totally fine 👍

For the dialog, is there any way to inform the admin they made a mistake? Before they distribute it to their users.

yuya-oc · 2017-11-07T15:31:44Z

We can add a script to confirm buildConfig before build distributions.

jasonblais · 2017-11-07T15:35:42Z

Is that something that would be a lot of work? Could it be done as part of #483?

That way the end user would never be affected

yuya-oc · 2017-11-07T15:37:51Z

Not hard. Just writing a simple script outside of the app code.

jasonblais · 2017-11-07T15:40:07Z

Cool, let's do that. So we'd have the following next steps

Script that confirms buildConfig before distributing the build to end users
Remove whitespace in front of Internal
Ignore error on permissions.json

Final question on 1: when would the script be ran (is it automatic or would the admin run it)

yuya-oc · 2017-11-07T15:42:58Z

Both applicable. Probably automatic is effective to avoid mistake. For example, at the first step of npm run package:*.

mattermost#633 (comment)

jasonblais · 2017-11-07T15:45:30Z

@yuya-oc Sounds great, agree. Would the admin run npm run package:* each time before distributing the app?

yuya-oc · 2017-11-07T15:46:55Z

Yeah, the commands generate .zip, .exe, ... packages. So the admin would always run them before distributing.

jasonblais · 2017-11-07T15:47:19Z

Perfect 👍

yuya-oc · 2017-11-07T16:08:22Z

Updated: https://circleci.com/gh/yuya-oc/desktop/491#artifacts
And https://circleci.com/gh/yuya-oc/desktop/492 should fail before testing.

I fixed 2 and 3 on master branch. 30da348 and 4137df4

jasonblais · 2017-11-07T16:13:47Z

@yuya-oc Awesome, thanks! For some reason there are no artifacts on those links. Looks like the build may have stopped before the artifacts had built

yuya-oc · 2017-11-07T16:19:42Z

I've updated links. Please refer again.

jasonblais · 2017-11-07T16:29:04Z

@yuya-oc I still see this

yuya-oc · 2017-11-07T16:33:39Z

@jasonblais Please reload this issue before open the link.

jasonblais

@yuya-oc Thanks! The first link works, but the second still didn't have the artifacts for some reason https://circleci.com/gh/yuya-oc/desktop/492#artifacts

jasonblais · 2017-11-08T14:08:39Z

src/main.js

+    dialog.showMessageBox({
+      type: 'error',
+      title: 'Mattermost',
+      message: 'When "enableServerManagement: false" is specified in buildConfig.js,\n"defaultTeams" must have one team at least.'


Do you think we need the UI dialog now that we automatically message the admin? Or does it still build the app files in which case there's a chance the app will be distributed to others?

Surely we added the automatic checker to the build script. However by reading the contents of package.json, admin can distribute the app with wrong buildConfig. I think there is no such admin, though.

So I'm okay even if the UI dialog is removed. I could not determine whether the dialog is still needed.

Wondering if npm run package:* fail if the admin is distributing the app with an incorrect buildConfig? Thoughts? (Not sure if that's even feasible)

Yes. When using wrong buildConfig, npm run package:* fails.

Actually, npm run package:* consists from three steps.

Check buildConfig. (this PR)

Compile JavaScript with webpack.

Create distributable packages with electron-builder.

The step 3 is written at package.json, so the admin can create packages without error by manually executing only step 3. However it's intentional. So I think it's okay even if we ignore such case.

Sounds great. If people hit issues, we can always add the dialog later, but sounds like this should be okay for the majority of the admins.

We can remove the dialog 👍

jasonblais · 2017-11-08T14:12:48Z

scripts/check_build_config.js

+  if (config.enableServerManagement === false && config.defaultTeams && config.defaultTeams.length === 0) {
+    return {
+      result: false,
+      message: `When "enableServerManagement: false" is specified in buildConfig.js, "defaultTeams" must have one team at least.\n${JSON.stringify(config, null, 2)}`


Propose

Specify at least one server for "defaultTeams" in buildConfig.js when "enableServerManagement is set to false.\n${JSON.stringify(config, null, 2)}

Would you have an example of what ${JSON.stringify(config, null, 2)} will output in the console?

It would be a formatted text of buildConfig:

{ defaultTeams: [], helpLink: 'https://docs.mattermost.com/help/apps/desktop-guide.html', enableServerManagement: false }

This reverts commit fa578fb.

yuya-oc · 2017-11-08T15:40:21Z

@jasonblais Updated. https://circleci.com/gh/yuya-oc/desktop/492 should not have any artifacts because npm run package:* fails due to wrong buildConfig.

jasonblais · 2017-11-08T15:46:12Z

Great, thanks @yuya-oc! Looks good to me, should be good to merge 👍

https://circleci.com/gh/yuya-oc/desktop/492 should not have any artifacts because npm run package:* fails due to wrong buildConfig

Oh right, that makes sense.. :)

yuya-oc · 2017-11-08T15:48:27Z

Thanks @jasonblais and @dmeza for many feedback!

yuya-oc added All Platforms null Windows (looking for testers) labels Oct 23, 2017

yuya-oc added this to the v3.8.0 milestone Oct 23, 2017

MusikPolice approved these changes Oct 23, 2017

View reviewed changes

jasonblais removed the Windows (looking for testers) label Oct 25, 2017

yuya-oc mentioned this pull request Oct 26, 2017

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

Merged

3 tasks

yuya-oc added 4 commits November 7, 2017 21:42

Simplify and split config.json into defaultPreferences and buildConfig

3613f70

Clean up test userData before each test

65286a6

Add test for splitted config files

baf0022

Remove deepmerge

51e6d5f

yuya-oc force-pushed the simplify-two-config branch from 20d0e95 to 51e6d5f Compare November 7, 2017 12:45

yuya-oc added 3 commits November 7, 2017 22:06

Move enableServerManagement key from defaultPreferences to buildConfig

4abd728

Restrict adding a server even when the number of servers is zero

878b5c1

Show error dialog when wrong buildConfig is used

fa578fb

yuya-oc added a commit to yuya-oc/desktop that referenced this pull request Nov 7, 2017

Ignore JSON.parse expections on PermissionManager constructor

4137df4

mattermost#633 (comment)

Check buildConfig before packaging the application

f7a9f50

yuya-oc force-pushed the simplify-two-config branch from a3966e7 to f7a9f50 Compare November 7, 2017 16:09

jasonblais reviewed Nov 8, 2017

View reviewed changes

yuya-oc added 2 commits November 9, 2017 00:35

Revert "Show error dialog when wrong buildConfig is used"

45963a9

This reverts commit fa578fb.

Update error message for buildConfig

fdf0d51

yuya-oc merged commit 3a865f3 into mattermost:master Nov 8, 2017

yuya-oc deleted the simplify-two-config branch November 8, 2017 15:49

jasonblais mentioned this pull request Nov 8, 2017

Simplify override.json and split config.json into two files #618

Closed

3 tasks

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

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

Conversation

yuya-oc commented Oct 23, 2017

jasonblais commented Oct 25, 2017

yuya-oc commented Oct 25, 2017

jasonblais commented Oct 25, 2017

dmeza commented Oct 25, 2017

jasonblais commented Oct 25, 2017

yuya-oc commented Oct 30, 2017

jasonblais commented Nov 6, 2017

jasonblais commented Nov 6, 2017 • edited Loading

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017 • edited Loading

jasonblais commented Nov 7, 2017 • edited Loading

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017 • edited Loading

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017 • edited Loading

jasonblais commented Nov 7, 2017 • edited Loading

yuya-oc commented Nov 7, 2017

jasonblais commented Nov 7, 2017

yuya-oc commented Nov 7, 2017

jasonblais left a comment

Choose a reason for hiding this comment

jasonblais Nov 8, 2017

Choose a reason for hiding this comment

yuya-oc Nov 8, 2017

Choose a reason for hiding this comment

jasonblais Nov 8, 2017

Choose a reason for hiding this comment

yuya-oc Nov 8, 2017

Choose a reason for hiding this comment

jasonblais Nov 8, 2017

Choose a reason for hiding this comment

jasonblais Nov 8, 2017

Choose a reason for hiding this comment

yuya-oc Nov 8, 2017

Choose a reason for hiding this comment

yuya-oc commented Nov 8, 2017

jasonblais commented Nov 8, 2017

yuya-oc commented Nov 8, 2017

jasonblais commented Nov 6, 2017 •

edited

Loading

jasonblais commented Nov 7, 2017 •

edited

Loading

jasonblais commented Nov 7, 2017 •

edited

Loading

jasonblais commented Nov 7, 2017 •

edited

Loading

yuya-oc commented Nov 7, 2017 •

edited

Loading

jasonblais commented Nov 7, 2017 •

edited

Loading