Auto-free inactive help channels

[interacsion]

Overview

Implements and closes #392.

Automatically frees all help channels that are inactive for more than 2 hours:

Details

Solved by introducing a AutoFreeRoutine which executes each 5 minutes, processing the channels.

Functionality that has to be used by both, FreeCommand and the AutoFreeRoutine, has been moved over to FreeChannelMonitor, which both now take as constructor parameter.

Config changes

The config slightly changed, introducing two new args to the FreeCommandConfig, looks like this:

    "freeCommand": [
        {
            "inactiveChannelDuration": "PT2H",
            "messageRetrieveLimit": 10,
            "statusChannel": <put_a_channel_id_here>,
            "monitoredChannels": [
                    <put_a_channel_id_here>
                  ]
        }
   ],

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

0.0% Coverage
0.0% Duplication

[borgrel]

u do realise that all of the logic for this has already been written?? (its currently set to an hr but thats easy to change)

all u need to do is create a routine, give it a reference to to free channel and call checkBusyStatusAllChannels(jda)

so its litterally like 5 lines of code

new Routine().setAction(10 minutes, freecommand ::checkBusyStatusAllChannels)

thats all thats needed .... obviously not that exact code but u have to add a one liner to a routine class thats it.

[Zabuzard]

Cool. Nice to see how well you prepared for this feature already. Unfortunate, that we overlooked it 😆

[borgrel]

how do you suggest we send a message then? or maybe we don't?

@MaiTheLord the purpose of programming is NOT to do each and every tiny thing that needs doing to perform a task as part of that task each and every time u want the task done .... this leads to inconsistency, bugs, unmaintainable code, no chain or responsibility and just about every bad practice that exists for programming!

the purpose is to group all actions required for a task in a single place so that u never have to do each tiny thing manually and so that there is only ever one place in the code where that action is controlled and so that the action always happens the same way.

since the channel should NEVER be free'd without users getting a message telling them that it is free .... u dont have to do it urself because its already written as part of the code to free a channel

[interacsion]

@MaiTheLord the purpose of programming is NOT to do each and every tiny thing that needs doing to perform a task as part of that task each and every time u want the task done

I never said anything like this.
I believe checkBusyStatusAllChannels(jda) doesn't send a message, does it?

[borgrel]

it calls other methods and when it does (eventually) change the status of a channel, the method that is called to do so ALSO sends a channel message to notify users

thats the point ... the checkStatus method IS NOT RESPONSIBLE for changing the status of a channel, so it SHOULD NOT be responsible to sending a message that states the channel is free

tasks get grouped and called in a logical structured manner so that things like sending a message when the status changes does not get forgotten

by expecting checkStatus to send messages is the same as saying "my apartment building is so cool its got a doorman to open the doors for visitors. If u want to visit just call 5 mins before so i go down and unlock the door so he can let u in" ..... whats the point of the doorman if ur still doing all of the work urself?

[interacsion]

checkBusyStatusAllChannels() calls updateStatusFor(), which then calls setChannelFree that calls this.

    public synchronized void setFree() {
        status = ChannelStatusType.FREE;
    }

where is the part that sends the message?

[borgrel]

checking .......

    @Override
    public void onSlashCommand(@NotNull final SlashCommandEvent event) {
        ......
        channelMonitor.setChannelFree(id);
        displayStatus(channelMonitor.getStatusChannelFor(requiresGuild(event)));
        event.reply(UserStrings.MARK_AS_FREE.message()).queue();
    }

oh bleh :(
its because a slash command is an interaction so the reply message is confined to replying to an interaction, which is a fundamental (bad) restriction of the api.

So yeah, more than just 5 lines of code, but not a lot more.

That still doesn't change the point that the routine should not be sending messages to tell that the the channel is free. That is outside the scope of its reponsibility (which is to keep track of time and get the free system to update itself at a certain interval)

Rewriting all of the code that is basically already found in free command for the routine was not ideal.

.
.
TL:DR
i would suggest seeing if the bot can trigger slash commands itself (like a regular user) and change the updateStatusFor to trigger new /free SlashCommandEvents for the channels that need updating (i dont know if its possible)

if not then u just move the action outside of the slashcommand

    public void freeChannel(TextChannel channel, @Nullable Interaction event) {
        channelMonitor.setChannelFree(channel.getIdLong());
        final String message = UserStrings.MARK_AS_FREE.message();

        if (event != null) {
            displayStatus(channelMonitor.getStatusChannelFor(requiresGuild(event)));
            event.reply(message).queue();
        } else {
            channel.sendMessage(message).queue();
        }
   }

The point is keep the message sending all in one place instead of having some in free command, some in routine, and some who knows where else.

You will never debug it if the same thing happens in more than one place, only one of them is making a problem and you dont know which one.

[interacsion]

i would suggest seeing if the bot can trigger slash commands itself (like a regular user) and change the updateStatusFor to trigger new /free SlashCommandEvents for the channels that need updating (i dont know if its possible)

Why can't we just add a .peek() to updateStatusFor()?

.filter(this::isChannelInactive)
.peek(channel -> {
    if (shouldSendMessage) {
        channel.sendMessage("This channel is now...").queue();
    }
})
.map(TextChannel::getIdLong)
.forEach(this::setChannelFree);

[borgrel]

dont use peek, its strictly !!! for debugging

it can be short circuited and all sorts of other nasty side effects.

u have identified the common method though, well done
change the setChannelFree methods' signature to accept an interaction
if the interaction is null send a message
if the interaction exists, reply with the same message

[marko-radosavljevic]

@MaiTheLord Hey, are you still working on this PR? ^^

[interacsion]

I was busy lately (and still am), so not really.

[marko-radosavljevic]

I see, thanks for the update. ❤️

You wouldn't mind if someone takes over?

[interacsion]

I wouldn't mind.
Thanks for asking though.

[Zabuzard]

Taken over, rewritten based on the previous general approach by @MaiTheLord .

[interacsion]

Looks good. can't approve as I originally opened the PR

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication