Adding the /modmail command

[DevSerendipity]

Commands which redirects command called from Guild or Dm to a dedicated channel in Config#getModAuditLogChannelPattern()

Samples of message being public (can be seen which user called the command) /modmail message:test from guild without message private option message-private:true Sample of message being private (can not be seen which user called the command) /modmail message:test from guild with message private option message-private:true

[DevSerendipity]

I have within the code added some code between comments and I was wondering would it be overengineering if I turned those to methods for readability and cleanness

[marko-radosavljevic]

Thanks :heart:

In #311, we decided to send modmail messages to a different channel, and not #mod_audit_log. Something like 'modmail' is fine by me. You will have to make small adjustments to Config to make that happen. ^^

Also, like always, don't forget to make our pipeline checks pass!

[marko-radosavljevic]

Since I haven't followed conversation on discord closely, I have a few questions about UI/UX:

• Why are we using file attachments? Because message might be too long? If it's too long for embed, wouldn't it be too long for the command as well?
• Would it be possible to ping the user that sent the modmail, instead of just saying 'user#123'. If user is pinged, we can just right-click and message them, or copy id, without searching for them. (User wouldn't actually be pinged, because they have no permissions to view the channel)
• Does file extension .md mean we can send markdown modmails?

Also, maybe I'm nitpicking, but since we are discussing UI (i.e. how pretty it is)… "Modmail command invoked... Message from user... who used /modmail command". Has a lot of redundant information. Just one of these is enough. For example, 'Modmail from user#123'.

What do you think? :relaxed:

[DevSerendipity]

• Why are we using file attachments? Because message might be too long? If it's too long for embed, wouldn't it be too long for the command as well?

I have been told that it has be be a file or embed otherwise people could inject pings and other things

• Would it be possible to ping the user that sent the modmail, instead of just saying 'user#123'. If user is pinged, we can just right-click and message them, or copy id, without searching for them. (User wouldn't actually be pinged, because they have no permissions to view the channel)

When I tried to retrieve the user event.getUser().getAsMention() all I get is his id <@975000935698935858> but perhaps that could work

• Does file extension .md mean we can send markdown modmails?

Not really

Also, maybe I'm nitpicking, but since we are discussing UI (i.e. how pretty it is)… "Modmail command invoked... Message from user... who used /modmail command". Has a lot of redundant information. Just one of these is enough. For example, 'Modmail from user#123'.

What do you think? ☺️

Sounds much nicer

[mikerasch]

The < @ ID> is the format to @ the user.

[DevSerendipity]

I am not sure how to properly name this properly (since in the if statement already talks about what this code block do) or if it even needs to be turned into a method

// replies to the user who called the command if the command is on cooldown
        MessageChannel messageChannel = event.getChannel();
        if (isChannelOnCooldown(messageChannel)) {
            event
                .reply("Please wait a bit, this command can only be used once per %d %s.".formatted(
                        COOLDOWN_DURATION_VALUE,
                        COOLDOWN_DURATION_UNIT.toString().toLowerCase(Locale.US)))
                .setEphemeral(true)
                .queue();
            return;
        }
        channelIdToLastCommandInvocation.put(messageChannel.getIdLong(), Instant.now());

and this piece of code from ChangeHelpCategoryCommand

        channelIdToLastCommandInvocation = Caffeine.newBuilder()
            .maximumSize(1_000)
            .expireAfterAccess(COOLDOWN_DURATION_VALUE, TimeUnit.of(COOLDOWN_DURATION_UNIT))
            .build();

[marko-radosavljevic]

I have been told that it has be be a file or embed otherwise people could inject pings and other things

Yeah, that make sense. Would it be nicer UI to have message in the embed, and just have title with "Modmail from @user"?

Not really

Knew it, but had to ask. :( One day, one day...

Thanks ^^

[marko-radosavljevic]

I am not sure how to properly name this properly (since in the if statement already talks about what this code block do) or if it even needs to be turned into a method

It doesn't need a new method, but it's your choice. Like you said, from the if, it pretty clearly reads "If channel is on cooldown, notify the user and early exit"

and this piece of code from ChangeHelpCategoryCommand

Good name, I know exactly what it represents. And most importantly, it's consistent with our naming scheme for similar cache maps.

---

Speaking of naming and methods, I would rather have a nicely named method here:

        // this message is creating the embed

        String user = event.getUser().getAsMention();
        boolean optionalMessage =
                Objects.requireNonNull(event.getOption(OPTION_MESSAGE_PRIVATE)).getAsBoolean();
        if (optionalMessage) {
            user = "Anonymous";
        }
        MessageAction message = auditLogChannel.orElseThrow().sendMessageEmbeds(messageEmbed(user));

        message = buildAttachment(attachments, message);
        message.queue();
        // ends here

Because I need a few seconds of reading to understand what it does.

Your comment is a good giveaway that you might want a method, because it outlines an isolated piece of code, that needs additional explanation to be easily understood. :relaxed:

[DevSerendipity]

and this piece of code from ChangeHelpCategoryCommand

Good name, I know exactly what it represents. And most importantly, it's consistent with our naming scheme for similar cache maps.

I think you misunderstood me, I was mentioning from which class I got it from, not the name I named it. But I will think of a name for it, when I do should I also change it in ChangeHelpCategoryCommand?

[DevSerendipity]

This PR also seem to handle what #219 has mentioned so It should be linked?

[marko-radosavljevic]

This PR also seem to handle what https://github.com/Together-Java/TJ-Bot/issues/219 has mentioned so It should be linked?

I think we will leave that issue for now, since use-case for report feature would be a bit different.

With report, we need additional information like the member being reported. Then message that is being replied to, so we can easily jump to the interaction, etc. Otherwise, we would have to do a lot of work to find the reported member, and comb through their history to find the offending behaviour.

[DevSerendipity]

This PR also seem to handle what #219 has mentioned so It should be linked?

I think we will leave that issue for now, since use-case for report feature would be a bit different.

With report, we need additional information like the member being reported. Then message that is being replied to, so we can easily jump to the interaction, etc. Otherwise, we would have to do a lot of work to find the reported member, and comb through their history to find the offending behaviour.

Okay thanks for the explanation

[DevSerendipity]

Is the code good to go? Do I need to fix anything beside the merge?

[marko-radosavljevic]

Is the code good to go? Do I need to fix anything beside the merge?

Before we can merge, you have to:

• Resolve unresolved conversations. The best way to do that is to let the reviewer know that you have done what they requested by commenting "done" and optionally pinging them, so they can resolve the review comments. If not, we can do it for you! :relaxed:
• Resolve the merge conflict in Features.java
• Get 2 passing reviews. I will give you my review after conflicts are resolved, and all automated checks in our pipeline pass. :heart:

[Taz03]

can we get the user's message in the embed instead of a separate file, ig that'll look better

[marko-radosavljevic]

Can you post some fresh demo images, want to see how's UI/UX looking ^^

[DevSerendipity]

Can you post some fresh demo images, want to see how's UI/UX looking ^^

It still looks the same as above

[Taz03]

edit the original message instead of sending one

[marko-radosavljevic]

Can you post some fresh demo images, want to see how's UI/UX looking ^^

It still looks the same as above

Ah, I thought we agreed on UI/UX improvements

Would it be possible to ping the user that sent the modmail, instead of just saying 'user#123'. If user is pinged, we can just right-click and message them, or copy id, without searching for them. (User wouldn't actually be pinged, because they have no permissions to view the channel)

Also, maybe I'm nitpicking, but since we are discussing UI (i.e. how pretty it is)… "Modmail command invoked... Message from user... who used /modmail command". Has a lot of redundant information. Just one of these is enough. For example, 'Modmail from user#123'.

Why are we using file attachments? Because message might be too long? If it's too long for embed, wouldn't it be too long for the command as well

I have been told that it has be be a file or embed otherwise people could inject pings and other things

Yeah, that makes sense. Would it be nicer UI to have message in the embed, and just have title with "Modmail from @user"?

[Taz03]

Can we get a better ui?

[marko-radosavljevic]

I think UI/UX improved since the PR is last time updated, can you update it @DevSerendipity, so we are all on the same page? :relaxed:

Would be nice to try to do a final push on this, it's been a while. Feel free to resolve old conversations that you have already fixed.

[DevSerendipity]

I think UI/UX improved since the PR is last time updated, can you update it @DevSerendipity, so we are all on the same page? ☺️

Would be nice to try to do a final push on this, it's been a while. Feel free to resolve old conversations that you have already fixed.

[Taz03]

what happens when u click the button?

[DevSerendipity]

what happens when u click the button?

You see the users profile

[DevSerendipity]

• [x] Think of a method for DM message duplication
• [x] Fix a bunch of text errors

[Taz03]

is the ui still the same? the embed?

[Taz03]

also y not use modals, now that we have modals. it'll be easier to write long modmails in a modal than a slash command option

[DevSerendipity]

also y not use modals, now that we have modals. it'll be easier to write long modmails in a modal than a slash command option

shhh next pr

[Taz03]

@DevSerendipity what about the ui, iirc marko suggested a better ui

[DevSerendipity]

@DevSerendipity what about the ui, iirc marko suggested a better ui

That was resolved, the UI is good enough