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

Migrate logging to use RPC Logging methods #102

Merged
merged 11 commits into from
Mar 27, 2020
6 changes: 2 additions & 4 deletions server/autolinkplugin/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"strings"

"github.com/mattermost/mattermost-plugin-autolink/server/autolink"
"github.com/mattermost/mattermost-server/v5/mlog"
"github.com/mattermost/mattermost-server/v5/model"
)

Expand All @@ -33,7 +32,7 @@ func (p *Plugin) OnConfigurationChange() error {

for i := range c.Links {
if err := c.Links[i].Compile(); err != nil {
mlog.Error(fmt.Sprintf("Error creating autolinker: %+v: %v", c.Links[i], err))
p.API.LogError("Error creating autolinker", "link", c.Links[i], "error", err.Error())
}
}

Expand Down Expand Up @@ -138,8 +137,7 @@ func (conf *Config) parsePluginAdminList(p *Plugin) {
// Let's verify that the given user really exists
_, appErr := p.API.GetUser(userId)
if appErr != nil {
mlog.Error(fmt.Sprintf(
"error occured while verifying userId %s: %v", v, appErr))
p.API.LogError(fmt.Sprintf("error occured while verifying userId %s: %v", v, appErr))
} else {
conf.AdminUserIds[userId] = struct{}{}
}
Expand Down
63 changes: 63 additions & 0 deletions server/autolinkplugin/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package autolinkplugin

import (
"errors"
"github.com/mattermost/mattermost-plugin-autolink/server/autolink"
"github.com/mattermost/mattermost-server/v5/model"
"github.com/mattermost/mattermost-server/v5/plugin/plugintest"
"github.com/mattermost/mattermost-server/v5/plugin/plugintest/mock"
"github.com/stretchr/testify/assert"
"testing"
)

func TestOnConfigurationChange(t *testing.T) {

t.Run("Invalid Configuration", func(t *testing.T) {
api := &plugintest.API{}
api.On("LoadPluginConfiguration",
mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
return errors.New("LoadPluginConfiguration Error")
})

p := Plugin{}
p.SetAPI(api)

err := p.OnConfigurationChange()
assert.Error(t, err)
})

t.Run("Invalid Autolink", func(t *testing.T) {
conf := Config{
Links: []autolink.Autolink{
autolink.Autolink{
Name: "existing",
Pattern: ")",
Template: "otherthing",
},
},
}

api := &plugintest.API{}
api.On("LoadPluginConfiguration",
mock.AnythingOfType("*autolinkplugin.Config")).Return(func(dest interface{}) error {
*dest.(*Config) = conf
return nil
})

api.On("LogError",
mock.AnythingOfType("string"),
mock.AnythingOfType("string"),
mock.AnythingOfType("autolink.Autolink"),
mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return(nil)

api.On("UnregisterCommand", mock.AnythingOfType("string"),
mock.AnythingOfType("string")).Return((*model.AppError)(nil))

p := New()
p.SetAPI(api)
p.OnConfigurationChange()

api.AssertNumberOfCalls(t, "LogError", 1)
})
}
14 changes: 8 additions & 6 deletions server/autolinkplugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@ func (p *Plugin) IsAuthorizedAdmin(userId string) (bool, error) {
"failed to obtain information about user `%s`: %w", userId, err)
}
if strings.Contains(user.Roles, "system_admin") {
mlog.Info(
p.API.LogInfo(
fmt.Sprintf("UserId `%s` is authorized basing on the sysadmin role membership", userId))
return true, nil
}

conf := p.getConfig()
if _, ok := conf.AdminUserIds[userId]; ok {
mlog.Info(
p.API.LogInfo(
fmt.Sprintf("UserId `%s` is authorized basing on the list of plugin admins list", userId))
return true, nil
}
Expand Down Expand Up @@ -103,14 +103,16 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,
startPos, endPos = autolinkNode.RawDestination.Position+offset, autolinkNode.RawDestination.End+offset
origText = postText[startPos:endPos]
if autolinkNode.Destination() != origText {
mlog.Error(fmt.Sprintf("Markdown autolink did not match range text, '%s' != '%s'", autolinkNode.Destination(), origText))
p.API.LogError(fmt.Sprintf("Markdown autolink did not match range text, '%s' != '%s'",
autolinkNode.Destination(), origText))
return true
}
} else if textNode, ok := node.(*markdown.Text); ok {
startPos, endPos = textNode.Range.Position+offset, textNode.Range.End+offset
origText = postText[startPos:endPos]
if textNode.Text != origText {
mlog.Error(fmt.Sprintf("Markdown text did not match range text, '%s' != '%s'", textNode.Text, origText))
p.API.LogError(fmt.Sprintf("Markdown text did not match range text, '%s' != '%s'", textNode.Text,
origText))
return true
}
}
Expand All @@ -120,14 +122,14 @@ func (p *Plugin) ProcessPost(c *plugin.Context, post *model.Post) (*model.Post,

channel, cErr := p.API.GetChannel(post.ChannelId)
if cErr != nil {
mlog.Error(cErr.Error())
p.API.LogError("Failed to get Channel", "error", cErr.Error())
return false
}
teamName := ""
if channel.TeamId != "" {
team, tErr := p.API.GetTeam(channel.TeamId)
if tErr != nil {
mlog.Error(tErr.Error())
p.API.LogError("Failed to get Team", "error", tErr.Error())
return false
}
teamName = team.Name
Expand Down
11 changes: 11 additions & 0 deletions server/autolinkplugin/plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ func (suite *SuiteAuthorization) TestSysadminIsAuthorized() {
Roles: "smurf,system_admin,reaper",
}

suite.api.On("LogInfo", mock.AnythingOfType("string")).Return(nil)

p := New()
p.SetAPI(suite.api)

Expand Down Expand Up @@ -152,6 +154,8 @@ func (suite *SuiteAuthorization) TestAdminUserIsAuthorized() {
}
suite.adminUsernames = "marynaId"

suite.api.On("LogInfo", mock.AnythingOfType("string")).Return(nil)

p := New()
p.SetAPI(suite.api)

Expand Down Expand Up @@ -181,6 +185,8 @@ func (suite *SuiteAuthorization) TestMultipleUsersAreAuthorized() {
}
suite.adminUsernames = "marynaId,karynaId"

suite.api.On("LogInfo", mock.AnythingOfType("string")).Return(nil)

p := New()
p.SetAPI(suite.api)

Expand Down Expand Up @@ -218,6 +224,8 @@ func (suite *SuiteAuthorization) TestWhitespaceIsIgnored() {
}
suite.adminUsernames = "marynaId , karynaId, borynaId "

suite.api.On("LogInfo", mock.AnythingOfType("string")).Return(nil)

p := New()
p.SetAPI(suite.api)

Expand Down Expand Up @@ -245,6 +253,9 @@ func (suite *SuiteAuthorization) TestNonExistantUsersAreIgnored() {
}
suite.adminUsernames = "marynaId,karynaId"

suite.api.On("LogError", mock.AnythingOfType("string")).Return(nil)
suite.api.On("LogInfo", mock.AnythingOfType("string")).Return(nil)

p := New()
p.SetAPI(suite.api)

Expand Down