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.

Sign up for GitHub

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

Add sleep between calls to add user to channels #124

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

Add sleep between calls to add user to channels #124

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf161d2
add sleep betwene calls to add user to channels
mickmister Feb 23, 2024
af24fe5
make delay configurable
mickmister Feb 23, 2024
b11fd9e
Revert "make delay configurable"
mickmister Feb 28, 2024
e99fa66
de-duplicate wait in `joinChannel`
mickmister Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/welcomebot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ func (p *Plugin) renderWelcomeMessage(messageTemplate MessageTemplate, configMes

for _, channelName := range configAction.ChannelsAddedTo {
p.joinChannel(action, channelName)
time.Sleep(time.Second)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hanzei 0/5 on if this should be done this way. Lots of cons come with this:

  • Channel actions may not be used on a server, so this sleep would be for naught
  • The sleep may be unnecessarily long, especially when we're adding the user to a lot of channels
  • It may not solve the problem in every case

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favour of doing it this way, as it should fix the issue out-of-the-box. A config setting seems cumbersome and confusing for admins.

This code only runs once per user, i.e. on their creation. Having a delay is justifiable. Even with 60 channels (!), that is just one minute of wait time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though I wonder if the user is pinged with a "ding" every second. I haven't actually seen this in action yet

Copy link

Choose a reason for hiding this comment

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

Dear Michael @mickmister and Ben @hanzei, thanks for providing this fix and your thoughts. I agree with the concerns, though. This wait-time as the only measure is rather unreliable. You can never guess an optimal wait-time for all cases. Sometimes you bother users with unexpected long wait-times, sometimes you bother them with duplicate categories.

Suggestions:

  1. Is there a way to determine if a category needs to be created, but creation has not yet been triggered (this might be the tricky part), check if the category already exists, wait-retry if is it not yet created but triggered, add member if it is already created?

  2. Is there a way to wait only if a category assignment is configured in the channel actions?

What do you think about these suggestions?

That's of course more challenging to implement, but you might have an idea how to achieve that. Thank you very much for taking these considerations into account!

Copy link
Contributor

@Kshitij-Katiyar Kshitij-Katiyar Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mickmister Are you looking into this or we should?

Copy link
Contributor Author

@mickmister mickmister Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@hanzei Note that there is already a property on the WelcomeMessage struct called DelayInSeconds, which allows the admin to set a delay between certain operations, so introducing another part of the config with this pattern would not be out of the ordinary. 0/5 but in this case it may make sense to allow the admin to set this, so we don't have to revisit it if the wait we choose is not high enough.

}
}

Expand Down Expand Up @@ -200,6 +201,7 @@ func (p *Plugin) processWelcomeMessage(messageTemplate MessageTemplate, configMe
func (p *Plugin) processActionMessage(messageTemplate MessageTemplate, action *Action, configMessageAction ConfigMessageAction) {
for _, channelName := range configMessageAction.ChannelsAddedTo {
p.joinChannel(action, channelName)
time.Sleep(time.Second)
}

tmpMsg, _ := template.New("Response").Parse(strings.Join(configMessageAction.ActionSuccessfulMessage, "\n"))
Expand Down