-
Notifications
You must be signed in to change notification settings - Fork 22
check if user to send message to is empty #94
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
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks @gerald2545 for taking the time to create this pull request ❤️
err := m.PostMattermost(p, mail) | ||
if err != nil { | ||
m.Error("Mattermost Error", map[string]interface{}{ | ||
m.Error("Right after PostMattermost, Mattermost Error. Email not set as synced in mattermost.", map[string]interface{}{ |
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.
should be more like
m.Error("Mattermost Error", map[string]interface{}{
"call": "PostMattermost",
"error": err
})
I think the "Email not set as synced" part can be removed this its now the correct default behavior.
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 for reference: putting additional information into the additional information part (map[string]interface
) allows the logger to seperate this information and depending on the logging type (text/json) to seperate the fields so they can be read and parsed by a logging backend correctly
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 you fix it pls
m.Error("File Error", map[string]interface{}{ | ||
"Error": err, | ||
}) | ||
m.Debug("In mail2most Run, Before writeToFile on " + m.Config.General.File,nil) |
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.
put the additional logging information into map[string]interface{}
m.Debug("writing to file", map[string]interface{
"filename": m.Config.General.File,
}
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.
same here
func (m Mail2Most) mlogin(profile int) (*model.Client4, error) { | ||
c := model.NewAPIv4Client(m.Config.Profiles[profile].Mattermost.URL) | ||
|
||
m.Info("in mlogin", nil) |
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.
m.Debug("login", map[string]interface{
"profile": profile,
})
no need to have this on Info logging
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.
same here
user, resp := c.GetUserByEmail(email, "") | ||
if resp.Error != nil { | ||
m.Debug("user not found in system", map[string]interface{}{"error": resp.Error}) | ||
m.Debug("user not found in system for email " + email, map[string]interface{}{"error": resp.Error}) |
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.
m.Debug("user not found", map[string]interface{}{
"error": resp.Error,
"email": email,
"call": "GetUserByEmail",
})
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.
should be more precise I think
email sender not defined as mattermost user
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.
how about
m.Debug("user not found", map[string]interface{}{
"error": resp.Error,
"email": email,
"call": "GetUserByEmail",
"note": "email sender not defined as mattermost user",
})
} | ||
} | ||
|
||
//m.Debug("Before SubjectOnly/BodyOnly " + err.Error,nil) |
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 be removed i guess?
} | ||
} | ||
|
||
m.Debug("In PostMattermost, Before m.Config.Profiles[profile].Mattermost.Users treatment",nil) |
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.
same as the others, adding additional information as seperate
var ( | ||
u *model.User | ||
) | ||
m.Debug("In PostMattermost, In m.Config.Profiles[profile].Mattermost.Users treatment ", map[string]interface{}{"length of user name": len(user)}) |
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.
not sure if this is debug line is really needed since it's now catched with a proper error instead
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.
map[string]interface{}{"length of user name": len(user)}
since it is used to allow the logger to create parsable key value pairs the key should follow usual naming standards e.g: username_length
. Also what benefit is created by this debug line providing user length as information for debugging?
for the mage part magefile you can use output should be sth. like this:
relevant commands are |
OK, I'll test mage command, I guess I didn't use the right ones "mage test" instead of "mage test:run" and "mage build" instead of "mage service:build" I used the "mage build" command as found in README > build it yourself, if it's not the right one you chould update it ;) |
with the right command, it works better ;) |
README updated in this PR |
check if mattermost user to send message to is empty (Users parameter in conf file)
in case Users is not empty, a call to mattermost was done to get the user id of Mattermost Username/accesstoken. This call was already done on first login. Si I set UserId attribute at login
Sorry, I didn't manage to make mage work (mage test : Unknown target specified: "test")