Skip to content

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

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

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

coltoneshaw
Copy link
Contributor

Fixes #67.

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

@coltoneshaw coltoneshaw requested a review from marianunez as a code owner July 16, 2021 18:20
@marianunez marianunez requested a review from hanzei July 19, 2021 22:02
@marianunez marianunez added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 19, 2021
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Good catch 👍

@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Aug 11, 2021
@hanzei hanzei requested a review from DHaussermann August 11, 2021 12:33
@hanzei hanzei added this to the v1.4.0 milestone Aug 11, 2021
@DHaussermann
Copy link

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

@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.

Copy link

@dipak-demansol dipak-demansol left a comment

Choose a reason for hiding this comment

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

LGTM

@dipak-demansol dipak-demansol added QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Oct 27, 2021
@hanzei
Copy link
Contributor

hanzei commented Oct 27, 2021

@DHaussermann Can we merge this?

@DHaussermann
Copy link

/update-branch

@DHaussermann
Copy link

@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.

@DHaussermann DHaussermann added 3: QA Review Requires review by a QA tester and removed QA Review Done PR has been approved by QA labels Oct 27, 2021
@coltoneshaw
Copy link
Contributor Author

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

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

@DHaussermann
Copy link

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 Unable to add Team selection to a custom attribute #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.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested ands passed

As per testing above the issue is resolved.

LGTM!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Oct 28, 2021
@hanzei hanzei merged commit e153602 into mattermost-community:master Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank GroupIDs cause a Go error when parsing the config.
6 participants