-
-
Notifications
You must be signed in to change notification settings - Fork 95
Semi-Automatic Top Helper Assignment #1303
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: develop
Are you sure you want to change the base?
Conversation
f4d5a20
to
afef4db
Compare
afef4db
to
69bc71b
Compare
application/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersCommand.java
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersCommand.java
Show resolved
Hide resolved
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Show resolved
Hide resolved
Optional<TextChannel> assignmentChannel = guild.getTextChannelCache() | ||
.stream() | ||
.filter(channel -> assignmentChannelNamePredicate.test(channel.getName())) | ||
.findAny(); |
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.
Is it a good idea to use the cache for this? What if the channel isn't in the cache because the bot didn't have to interact with it recently or whatever?
This also kinda leads me to a different question:
Shouldn't the config also be able to match channels by id instead of name/pattern?
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.
Is it a good idea to use the cache for this? What if the channel isn't in the cache because the bot didn't have to interact with it recently or whatever?
I copied this from elsewhere in the existing code. I believe all channels (except maybe threads?) are always in the cache.
Shouldn't the config also be able to match channels by id instead of name/pattern?
its possible to code it like that but it makes the config less readable by humans. names-patterns are just more comfortable to maintain, i guess
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.
I copied this from elsewhere in the existing code. I believe all channels (except maybe threads?) are always in the cache.
Not sure about this one. I quickly checked the docs and could find Guild#getChannels
as an alternative which returns GuildChannel
's though. I don't think it is guaranteed to have all channels in the cache.
its possible to code it like that but it makes the config less readable by humans. names-patterns are just more comfortable to maintain, i guess
Yeah, I guess it isn't really needed to add the complexity for matching against both ids and names/patterns here.
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Outdated
Show resolved
Hide resolved
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Outdated
Show resolved
Hide resolved
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Show resolved
Hide resolved
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Outdated
Show resolved
Hide resolved
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Outdated
Show resolved
Hide resolved
...ion/src/main/java/org/togetherjava/tjbot/features/tophelper/TopHelpersAssignmentRoutine.java
Outdated
Show resolved
Hide resolved
|
Motivation
Currently we do the monthly Top Helpers manually with the aid of
/top-helpers
, showing a list like this:Everything beyond that is manual, i.e.:
#hall-of-fame
The problem with this is that, because its mostly manual work, we kept forgetting and skipping it for a few months already. Not good 😢
Solution
This PR introduces a semi-automatic routine that does the Top Helper assignment and announcement automatically with minimal and simple investman by a human.
The routine starts on the 1st of each month at 12:00 o'clock (or can be started manually using
/top-helpers assign
) and creates a dialog in a trusted channel (e.g.community-commands
).The dialog proposes a list of Top Helpers (as known from the existing
/top-helpers
command, now renamed to/top-helpers show
).A trusted user can now select from this list everyone who should become Top Helper. The routine then continues automatically clearing the Top Helper role and assigning the role to the selected people.
Additionally, the routine then continues to ask if it should make a generic announcement (in
#hall-of-fame
) as well or if that should be skipped.Impressions
Flexibility
The routine can be cancelled at any point in case needed by pressing the
Cancel
button. It can also be started outside of the normal schedule by using/top-helpers assign
.Adding users to the list of Top Helpers that were not proposed by the bot or otherwise modifying the list is possible by simply clicking
No
on the question for the generic announcement. Like, roll with the automatic assignment of the routine, dont send an automatic announcement and then edit the Role manually as we used to before this PR already. Then, if desired, simply send an announcement with the modified list yourself.The last step with the generic announcement was made optional on purpose since a huge appeal of the "announcements made by a human" always was to include a funny image. We still want this to be possible, so if someone is in the mood to create such an image they can click on
No
and skip the generic announcement, writing one themselves. In practice however, people will often not have the time for this, so the generic announcement is a good way out, definitely better than no announcement.Config
This introduces a new block to the config:
Further, the patterns for the
Top Helper
role have been adjusted everywhere in the config fromTop Helpers .+
toTop Helper.*
. This allows for renaming the role from e.g.Top Helpers August
toTop Helper
, while still being compatible with the current role name.(this is a user suggestion with lots of upvotes, so it will likely be put in action soon anyways)
Discord
So while at it, it is proposed (but not required) to rename the role
Top Helpers August
etc toTop Helper
.Commands
The existing slashcommand
/top-helpers
was changed into two subcommands:/top-helpers show
: shows the list of top helpers, i.e. what/top-helpers
did already/top-helpers assign
: manual trigger for the routine added in this PRCode
Most changes in this PR revolve around the addition of a new class called
TopHelpersAssignmentRoutine
, which deals with the entire dialog.Some of the logic in this routine is shared with the existing slash command
TopHelpersCommand
, so a helper class was added to allow shared logic:TopHelpersService
.The service mostly deals with the actual computation of Top Helpers (using the DB), retrieval of user data (helper IDs to JDA
Member
) and visual presentation (ASCII table). This code is not new and merely moved fromTopHelpersCommand
over toTopHelpersService
, sometimes with slight modifications to also fit the needs ofTopHelpersAssignmentRoutine
.Schedule
The routine needs the ability to execute "once a month at a fixed time". We already had similar code to enable that present in
ModAuditLogRoutine
. This code has been elevated from there toRoutine
, to allow others to use the logic as well.I.e. while the methods
Schedule.atFixedRateFromNextFixedTime(...)
andSchedule.atFixedHour(...)
are new, their code is not new and was part ofModAuditLogRoutine
already.Slash Command (
TopHelpersCommand
)The slash command stays mostly unchanged. Modifications include it now using sub-commands (see its constructor), simple routing code in
onSlashCommand
to determine which sub-command was triggered and in case of the new sub-commandshow
it just forwards inassignTopHelpers
to the routineassignmentRoutine.startDialogFor(...)
.To enable that, it got the routine as dependency at construction.
Routine (
TopHelpersAssignmentRoutine
)The code for this is mostly straightforward, albeit a bit annoying due to JDA not making it easy to create dialogs and constantly interrupting code-flow.
The method order goes from top to bottom to represent the flow of the routine, it should mostly read in a straightforward way without jumping around much.
Error Handling
Error handling is mostly similar but has slight differences. These mostly revolve around the fact whether there was already a message posted by the bot that can be edited or if a new message needs to be posted.
And if there was a message already, whether it includes buttons and similar that need to be cleared.
Remarks
A few things to highlight:
...editOriginal(message + "\n...")
.setActionRow(...)
or alsosetComponents()
componentIdInteractor
manually as it is not a traditional slash or context command. I.e. it can not extend from something likeSlashCommandAdapter
bringing component-ID logic out of the box and needs to instead implementUserInteractor
manually. We did that already in the past withScamBlocker
in the same way.