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

Fix Autolink admin config parsing #184

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Fix Autolink admin config parsing #184

merged 1 commit into from
Mar 29, 2022

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Feb 23, 2022

Summary

The plugin was falsely storing it's config in UperCase names, which lead to the webapp not being able to rea

This PR make use of json.Marshal to convert the plugin config to an map[string]Interface{} and hence json tags are used everywhere now.

There is one migration left. If an admin (after updating to this PR) sees the empty list in the system console and inputs something, the existing admin list will be overwritten. Running /autolink add or /autolink delete brings the configuration in the right state again.

Ticket Link

#180
#181

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Feb 23, 2022
@hanzei hanzei added this to the v1.3.0 milestone Feb 23, 2022
@hanzei hanzei requested a review from mickmister February 23, 2022 12:41
@hanzei hanzei requested a review from levb as a code owner February 23, 2022 12:41
@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Mar 3, 2022
@hanzei
Copy link
Contributor Author

hanzei commented Mar 8, 2022

@dipak-demansol gentle reminder to review this PR

@dipak-demansol
Copy link

@dipak-demansol gentle reminder to review this PR

@hanzei @DHaussermann i'm not able to regenerate the 180 & 181 issue with the master Build. Its working fine with the master build.

@hanzei
Copy link
Contributor Author

hanzei commented Mar 17, 2022

@dipak-demansol Did you test with a clean plugin config i.e. PluginSettings.Plugins.mattermost-autolink is empty?

@dipak-demansol
Copy link

@dipak-demansol Did you test with a clean plugin config i.e. PluginSettings.Plugins.mattermost-autolink is empty?

Yes

@hanzei
Copy link
Contributor Author

hanzei commented Mar 28, 2022

@dipak-demansol Could you please test with the following steps:

  1. Set an autolink admin via the system console
  2. Run /autolink add foo to create an link
  3. Check the list of autolink admins in the system console

Observed behaviour on master:
The list is empty

Expected behaviour:
The list contains the admins set in step 1

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.

@hanzei tested as you told and its LGTM, adding the cases with that i tested, pls let me know if i miss any case to test.

  1. adding two member-Users id in console,those 2 user is able to use autolink slash command.
  2. after performing First case, in the admin system console i refresh the page and still all data is there and those Two users are able to use to autolink slash command. (its a case that ben told me to check) (issue 181)
  3. by adding space between the two id, expected result is slash command should not work for both user.:- as expected.
  4. by adding space before the first id, still both user is able to use the slash command and i don't have any issue with that so it LGTM.
  5. by adding space before the first id and adding space after the last id, both User are able to use the slash command.
  6. added the user-id on the config file and restarted the server then i'm able to see that config value in the system-console. (issue 180)

@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 Mar 29, 2022
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed QA Review Done PR has been approved by QA labels Mar 29, 2022
@hanzei hanzei merged commit d071134 into master Mar 29, 2022
@hanzei hanzei deleted the admin branch March 29, 2022 13:37
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.

5 participants