-
Notifications
You must be signed in to change notification settings - Fork 41
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
[GH-108]: Added the setting in the system console for plugin settings #126
base: master
Are you sure you want to change the base?
Conversation
…omebot into MM-108
@mickmister Regarding the QA comment #92 (review), I tried making the build from the master branch and deploying it on MM v5.37+ which is its minimum supported version. I am getting the error
But the above builds are working fine on MMv7.8.0+, So here we can do 2 things I can each commit in this plugin and check for which we are actually getting the error and try to debug and fix that or we can just update the minimum supported version for this plugin. But in any case, this error is not related to this PR. |
] | ||
``` | ||
|
||
If you see `[Object object]` in the text field, that's because the configuration was configured as JSON directly in your `config.json` instead of as a string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead JSON.stringify the data if it's not originally a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister Can you elaborate a bit on this, I am not really able to understand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the other review comments, I am not able to push the code, as we don't have access to push the code. Will fix it afterwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if it's the case that the setting is an object (and not a JSON string), then the frontend can call JSON.stringify
to make it into a string to put into the text box. This would require the plugin setting to be custom
I think. Otherwise the admin cannot go and edit the current configuration if it just says [Object object]
Ideally we can just keep it as an object in the config file too, and not have a JSON string in the config.json
file. Which would also require it to be a custom
admin console setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments here, otherwise LGTM 👍
|
||
In the UI, go to **System Console -> Plugins -> Welcome Bot** and edit the **Welcome Messages** text field to include the JSON array for messages you want to send. Use the following format: | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``` | |
```json |
{ | ||
"key": "WelcomeMessages", | ||
"type": "longtext", | ||
"display_name": "Welcome Messages:", | ||
"help_text": "JSON formatted configuration for the welcome messages. See [usage here](https://github.com/mattermost/mattermost-plugin-welcomebot#usage)." | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation seems off here
{ | |
"key": "WelcomeMessages", | |
"type": "longtext", | |
"display_name": "Welcome Messages:", | |
"help_text": "JSON formatted configuration for the welcome messages. See [usage here](https://github.com/mattermost/mattermost-plugin-welcomebot#usage)." | |
} | |
] | |
{ | |
"key": "WelcomeMessages", | |
"type": "longtext", | |
"display_name": "Welcome Messages:", | |
"help_text": "JSON formatted configuration for the welcome messages. See [usage here](https://github.com/mattermost/mattermost-plugin-welcomebot#usage)." | |
} | |
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has been tested for the below conditions:-
- User getting a welcome message
- User gets the action button in a post.
- User is added to a channel by completing the action as asked in action button.
The PR works fine for all the conditions above, LGTM. Approved.
Summary
This PR has been created using the PR #92. It adds a long text field for the plugin configuration settings.
Ticket Link
Fixes #108
Fixes #56