Fixes #67. Replaced empty string with empty array

mattermost-community / mattermost-plugin-custom-attributes

Mattermost plugin for adding custom attributes to users!

Apache License 2.0

46 stars 23 forks source link

Fixes #67. Replaced empty string with empty array #70

Closed coltoneshaw closed 3 years ago

coltoneshaw commented 3 years ago

Fixes #67.

Just replaced the empty string with an empty array. Tested and the config.json file saves / parses properly on restarting mattermost.

DHaussermann commented 3 years ago

@coltoneshaw strangely, I still see this behavior occurring.

Build Custom Attributes from this branch and deploy
Enable Custom Attributes
Add a record that has no Group ID and save
Disable the plugin
Enable the plugin Observed Server logs still show {"timestamp":"2021-10-14 10:33:43.063 -04:00","level":"debug","msg":"2021/10/14 10:33:43 LoadPluginConfiguration API failed to unmarshal: json: cannot unmarshal string into Go struct field CustomAttribute.CustomAttributes.GroupIDs of type []string","caller":"plugin/hclog_adapter.go:54","plugin_id":"com.mattermost.custom-attributes"}

Maybe I missed something can you please see if you can reproduce this on the PR branch as well?

dipak-demansol commented 3 years ago

@coltoneshaw strangely, I still see this behavior occurring.

Build Custom Attributes from this branch and deploy

Enable Custom Attributes

Add a record that has no Group ID and save

Disable the plugin

Enable the plugin Observed Server logs still show {"timestamp":"2021-10-14 10:33:43.063 -04:00","level":"debug","msg":"2021/10/14 10:33:43 LoadPluginConfiguration API failed to unmarshal: json: cannot unmarshal string into Go struct field CustomAttribute.CustomAttributes.GroupIDs of type []string","caller":"plugin/hclog_adapter.go:54","plugin_id":"com.mattermost.custom-attributes"}

Maybe I missed something can you please see if you can reproduce this on the PR branch as well?

Hi Dylan, i tested with Master +patch1 branch & master + patch + coltoneshaw:groupIdfixes and the error was not generated, LGTM.

hanzei commented 3 years ago

@DHaussermann Can we merge this?

DHaussermann commented 3 years ago

/update-branch

DHaussermann commented 3 years ago

@hanzei can we please leave this open for a couple more days. @dipak-demansol and I are seeing different results on this and are working to find the cause of the deployment issue.

coltoneshaw commented 3 years ago

@DHaussermann , are you testing this with a fresh plugin, or did you have the old version. IIRC you may need to update the config.json to fix the original issue and then test. Or remove the plugin along with its config and test with the new version.

DHaussermann commented 3 years ago

Thanks @coltoneshaw I can rip all pre-existing data out of my config or try this on a fresh server.

DHaussermann commented 3 years ago

After doing another round of testing this PR is good to merge.

After testing this with the config in a fresh state - I see that the error with .json unmarshall is resolved which is the scope of this PR. This can now be merged.
The issue I was seeing was that Team names cannot be retrieved and is unrelated. Furthermore, the issue also occurs in the master branch. I have created https://github.com/mattermost/mattermost-plugin-custom-attributes/issues/76 to address it.
FWIW the deployment issue @dipak-demansol and were seeing seems to have been caused by differences in the main.js in the webapp folder of the plugin. Having the same mattermost/mattermost-webapp on the same more current build hash resolved this discrepancy and confirmed the issue is valid.