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

!free help channel command #255

Merged
merged 29 commits into from Jan 12, 2019

Conversation

7 participants
@fiskenslakt
Copy link
Contributor

fiskenslakt commented Jan 7, 2019

The long overdue !free command which checks for inactivity in all the help channels, and lists them as a suggestion to users to move to a help channel, or switch to one that isn't currently in use.

How it works

  • If invoked outside of a help channel, it will check the timestamp of the last message in each of the help channels.
  • However if invoked in one of the help channels, it will do the same except for the channel it was invoked in.
    • If no user is mentioned, it will check the second to last message to avoid considering the invocation itself as activity in the channel
    • If a user is mentioned, it will check the third to last message by default. The assumption here is that a user asked if the channel was free, or they asked a question in a potentially active channel, and we don't want to consider that message as activity in the channel.
    • Optionally one can override the default of 3 to a number more appropriate for the situation. (eg. if the user who asked if the channel was free or asked their question took up more than one message)

note: I really don't expect that last situation to ever be used, but I included it just in case (for you power users out there).

Cooldown

I've set the cooldown for this to once every minute, on a per channel basis. I felt this was the most appropriate, but I am open to other suggestions.

EDIT - Helpers are now exempt from the cooldown

Time inactive

Currently a channel must be inactive for more than 5 minutes to be considered potentially free. This part I was unsure about, and welcome suggestions if it seems too short, or too long.

EDIT - Activity timeout is 10 minutes

Constraints

seek must be less than or equal to 10. I see no point in checking the channel history any older than that.
seek can't be less than 1 for obvious reasons.

Wording

Finally, any suggestions on my wording for the responses are encouraged.

Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
@SebastiaanZ

This comment has been minimized.

Copy link
Member

SebastiaanZ commented Jan 7, 2019

Sorry, for the messy reviews, seems like I inadvertently deleted some of my comments, so I added them back in later. Also, this is a nice feature to have, I think.

Regarding the cool-down: Should we make an exemption for the time-out from certain staff levels up, like with other commands?

@sco1

This comment has been minimized.

Copy link
Member

sco1 commented Jan 7, 2019

Looks cool!

Yes, definitely want a cooldown exemption for Helpers+

@sco1
Copy link
Member

sco1 left a comment

Pretty much all of the constants should be added to bot/constants.py and configured in /config-default.yml rather than being hardcoded in the cog.

Along with a constants class for this cog, the guild's server categories could probably get their own as well.

Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
@heavysaturn
Copy link
Member

heavysaturn left a comment

Block comments, my dear boy. More block comments. the command should be fairly easy to skim through.

Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py
Show resolved Hide resolved bot/cogs/free.py
Show resolved Hide resolved bot/cogs/free.py
@jos-b

This comment has been minimized.

Copy link
Member

jos-b commented Jan 9, 2019

I'm somewhat concerned about the 5 minute wait before a channel is considered inactive. I have seen a question left for over 10 minutes before but since the channel was considered active and wasn't talked in someone else came along and assisted with it. I'm not sure of a better duration but 5 feels too short, any opinions anyone?

@heavysaturn

This comment has been minimized.

Copy link
Member

heavysaturn commented Jan 9, 2019

@jos-b I think it's important to note that the feature simply lists the channels in order of inactivity and says "hey, these have been inactive for a while so they're probably your best bet but they might be active". the wording isn't very assertive.

that said, I think we could probably set the duration a tiny bit higher, yeah. 5 minutes does seem short, it might even be less than it takes for someone to answer on average. I'd probably go for something like 10. I think we probably need to test this in production and tune it afterwards, though.

@jos-b

This comment has been minimized.

Copy link
Member

jos-b commented Jan 9, 2019

That makes more sense. I was more thinking about peoples questions being moved into the scroll back and I think that we need to be clear on the wording and say something like "make sure there are no pending questions in the channel" or something.

@sco1 sco1 dismissed their stale review Jan 9, 2019

Stale

Show resolved Hide resolved bot/cogs/free.py
Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py
Show resolved Hide resolved bot/cogs/free.py
@sco1

This comment has been minimized.

Copy link
Member

sco1 commented Jan 10, 2019

Other than the outstanding review comments this seems to work great:

image
image
image

@sco1 sco1 added this to Needs review in Bot Tracking Jan 11, 2019

@SebastiaanZ
Copy link
Member

SebastiaanZ left a comment

I like it!

I've attached two points about your way of handling the error to ensure a traceback is printed and the we reinvoke the command with the same arguments to ensure the user will still get mentioned/the seek depth is passed.

Show resolved Hide resolved bot/cogs/free.py Outdated
Show resolved Hide resolved bot/cogs/free.py Outdated
Change ctx.invoke to ctx.reinvoke to conserve passed arguments
Co-Authored-By: fiskenslakt <sjukledighet@gmail.com>

@fiskenslakt fiskenslakt dismissed stale reviews from heavysaturn and heavysaturn via c94189c Jan 12, 2019

Change log.error to log.exception to avoid hiding traceback
Co-Authored-By: fiskenslakt <sjukledighet@gmail.com>

@sco1 sco1 dismissed their stale review Jan 12, 2019

Stale

Bot Tracking automation moved this from Needs review to Reviewer approved Jan 12, 2019

@sco1 sco1 merged commit cc2f484 into python-discord:master Jan 12, 2019

Bot Tracking automation moved this from Reviewer approved to Done Jan 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment