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

[PR-70]: Added the ability to send DMs from bot accounts #129

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Kshitij-Katiyar
Copy link
Contributor

Summary

I added the ability to send a DM from a bot account. Any channel name that starts with an @ symbol that is also a bot user will have a DM sent with the message configured in the Action's ActionDirectMessagePost.

This PR is the extension of the PR #70.

@Kshitij-Katiyar Kshitij-Katiyar changed the title Pr 70 [PR-70]: Added the ability to send DMs from bot accounts Apr 15, 2024
@mickmister mickmister requested a review from ayusht2810 April 23, 2024 21:33
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Thanks @Kshitij-Katiyar 👍

In general LGTM, just some comments for discussion

README.md Outdated Show resolved Hide resolved
{
"TeamName": "your-team-name",
"DelayInSeconds": 3,
"IncludeGuests": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

We're removing a piece of data here, the IncludeGuests property

if err := p.handleDMs(action, channelName); err != nil {
p.API.LogError("failed to handle DM channel, continuing to next channel. " + err.Error())
}
} else { // Otherwise treat it like a normal channel
Copy link
Contributor

Choose a reason for hiding this comment

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

Can return here instead of having the else?

Comment on lines +259 to +261
if !dmUser.IsBot {
return errors.Wrapf(userErr, "Specified DM user is not a bot for username %s", username)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


func (p *Plugin) handleDMs(action *Action, channelName string) error {
username := channelName[1:]
dmUser, userErr := p.API.GetUserByUsername(username)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use p.client instead of p.API whenever possible

Comment on lines +268 to +271
dmMessage := "Welcome to the team!"
if len(action.Context.DirectMessagePost) != 0 {
dmMessage = action.Context.DirectMessagePost
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this required instead? "Welcome to the team!" seems not too useful

@mickmister mickmister requested a review from hanzei April 23, 2024 21:41
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Apr 24, 2024
@hanzei hanzei removed their request for review April 24, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants