/modmail slash command #329

talentedasian · 2021-12-15T15:57:26Z

Pull-request

I have read the contributing guidelines.
I have read the code guidelines.
I have a relating issue.

Changes

Existing code
New feature

Closes Issue: #311

Description

This is a draft of my PR on the /modmail command. The commit body contains
most of my work but I also wanted to address one more thing. I'm not entirely
sure if I should use the findMemberWithRoles on Guild class by JDA because:

Well first I don't really understand the warning that it says "You MUST NOT use blocking operations such as Task.get()! The response handling happens on the event thread by default."
Second, I could use just the getMemberWithRoles although it only takes the mods that are in thecache.
If i decide to hardcode the mods to be chosen by the user who invoked the command then I could remove the newly added config guildId. This was at the bottom of my list; I don't even know all the mods in the server.
I could use buttons, but I prefer selection menus. Buttons hurt my eyes!!

* Add a "guildId" config in Config.json for later use in finding the moderators of the guild. * Initially, the bot response should've been ephemeral but since I was using a selection menu, the way I was disabling it does not work if I set the bot response to ephemeral.

Tais993 · 2021-12-15T21:52:09Z

There's a big issue in this code, it retrieves every single member of the server each time you call findMemberWithRoles
So this should only be used once preferably at all time, maybe even never depending on how you solve it.

So instead, retrieve once and create an selection menu in advance.

This means you find all the members with the role, create a selection menu based on the members found.
And you always re-use this selection menu.

To improve UX for the moderators, add a command only moderators can use to reload the list

and use it for subsequent invocation of the command.

CLAassistant · 2021-12-16T04:11:38Z

All committers have signed the CLA.

highly unlikely, send an ephemeral message and log the unusual behavior

talentedasian · 2021-12-16T05:15:17Z

Right now, the private message does get sent and the buttons are getting disabled as well but the interaction somehow fails.

…. This also fixes the failed interaction by discord

the /modmail command. * Introduce a utility class for use by other classes when interacting with the ModMailCommand. * Some javadoc.

talentedasian · 2021-12-16T15:30:43Z

Most of the work now is done, I'll now put this up as ready for review.

talentedasian · 2021-12-16T15:42:44Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModmailCommand.java

+    }
+
+    /**
+     * Creates a list of options containing the moderators for use in the modmail slash command.


Forgot to change the javadoc for this. I'll just do it tomorrow.

Zabuzard · 2021-12-16T15:57:55Z

application/src/main/java/org/togetherjava/tjbot/commands/moderation/ReloadModMailCommand.java

+        Guild guild = Objects.requireNonNull(jda.getGuildById(config.getGuildId()),
+                "A Guild is required to use this command. Perhaps the bot isn't on the guild yet");


you should be able to do event.getGuild(), no?

I could but I didn't due to some pedantic reason. If the command was called on Guild A, but the guildId was Guild B, then it would have inconsistent results.

Thinking about that, I think I could perhaps do event.getGuild() on the ModMailCommand as well.

I do think you should SlashCommandEvent#getGuild ^^

If you run the command in Guild B, right now it runs it like it's in Guild A, which is really weird imo

The bot is only in 1 guild, and it'll never join another one

Tais993 · 2021-12-16T16:05:24Z

application/src/main/java/org/togetherjava/tjbot/commands/moderation/ReloadModMailCommand.java

+        Guild guild = Objects.requireNonNull(jda.getGuildById(config.getGuildId()),
+                "A Guild is required to use this command. Perhaps the bot isn't on the guild yet");


I do think you should SlashCommandEvent#getGuild ^^

If you run the command in Guild B, right now it runs it like it's in Guild A, which is really weird imo

The bot is only in 1 guild, and it'll never join another one

Tais993 · 2021-12-16T16:07:38Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModmailCommand.java

+    static final List<SelectOption> mods = new ArrayList<>();
+    static final Map<String, User> modsMap = new HashMap<>();


It shouldn't be done this way, keep the variables private, use the onReady event to load the options/mods.

So keep it within this class, make the variables private and don't make them static.

Tais993 · 2021-12-16T16:09:29Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModmailCommand.java

+        if (mods.size() == 1) {
+            loadMenuOptions();
+        }


Remove this in favor of the onReady event, see above

Tais993 · 2021-12-16T16:13:09Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModmailCommand.java

+    private void loadMenuOptions() {
+        Guild guild = Objects.requireNonNull(jda.getGuildById(config.getGuildId()),
+                "A Guild is required to use this command. Perhaps the bot isn't on the guild yet");
+
+        ModMailUtil.loadMenuOptions(guild);
+    }


See review above, remove this in favor of the onReady event

Tais993 · 2021-12-16T16:18:41Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModmailCommand.java

+        // guild.
+        String userId = args.get(0);
+        if (event.isFromGuild()
+                && !userId.equals(Objects.requireNonNull(event.getMember()).getId())) {


Suggested change

&& !userId.equals(Objects.requireNonNull(event.getMember()).getId())) {

&& !userId.equals(event.getMember().getId())) {

You're effectively doing

if (member == null) { member.toString(); }

Here, which is, useless

Tais993 · 2021-12-16T16:25:52Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModMailUtil.java

+        Role modRole = getModRole(guild)
+            .orElseThrow(() -> new IllegalStateException("No moderator role found"));
+
+        guild.findMembersWithRoles(modRole).get().stream().forEach(mod -> {


Why the get and the stream?

Just use onSuccess?

Tais993 · 2021-12-16T16:27:48Z

application/src/main/java/org/togetherjava/tjbot/commands/modmail/ModmailCommand.java

+    private static final Config config = Config.getInstance();
+
+    static final List<SelectOption> mods = new ArrayList<>();
+    static final Map<String, User> modsMap = new HashMap<>();


Another note, NEVER store Discord entities.

They can be invalidated (garbage collected) which will result in really weird errors.

Instead, store their ID (or username in this case)

Replace all loadOptions with the onReady event but still delegating to the ModmailUtils to query discord. * Move ReloadModmailCommand from a separate class to the ModmailCommand class.

illuminator3 · 2021-12-17T22:21:22Z

application/src/main/java/org/togetherjava/tjbot/commands/Commands.java

@@ -70,6 +71,10 @@
        commands.add(new MuteCommand(actionsStore));
        commands.add(new UnmuteCommand(actionsStore));

+        ModmailCommand modmailCommand = new ModmailCommand();
+        commands.add(modmailCommand);
+        commands.add(modmailCommand.new ReloadModmailCommand());


This will not compile.

Really? I'm sure I've tested and ran it on my machine numerous times.

I also ran gradle build so I'm really sure it works. Do you mind checking it again?

It doesn't compile no, I'm not sure what you're doing there modmailCommand.new ReloadModmailCommand()

The ReloadModmailCommand is an inner class since it's the only solution I could think of to reload the mods in the /modmail command. Thus, I am instantiating the ReloadModmailCommand from the Modmail object.

Ah that explains

I am genuinely confused why it doesn't compile. Do you mind helping me out on this one?

Well, it compiles like you said, it's just done in a weird way.
To my knowledge, you can remove modmailCommand. from modmailCommand.new ReloadModmailCommand()
So please do that, if you can't, refer to the class instead of an instance?

You can't. That's the required syntax for instantiating a non-static inner class. It's really weird.

github-actions · 2022-01-18T07:09:30Z

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Tais993 · 2022-01-20T11:01:10Z

@talentedasian do you've the time to work on this PR (this week, next week)? Otherwise I'll take this PR over so it can be merged

talentedasian · 2022-01-20T11:04:12Z

I do but you can take over this PR if you want.

Tais993 · 2022-01-20T11:07:09Z

If you've the time, you can handle it, it takes some time setting up and you deserve most of the credits :p

Zabuzard · 2022-01-20T11:08:17Z

If you've the time, you can handle it, it takes some time setting up and you deserve most of the credits :p

If you take over, you can also copy their branch and hence basically keep their commits. Or alternatively manually add them as author to a commit of urs (for credits).

Tais993 · 2022-01-20T11:26:01Z

I'll work in the branch in that case, your branch will be copied so you'll be credited ^^

Tais993 · 2022-01-20T12:11:16Z

This PR will be closed in favor of a new PR (will be linked)

In advance, thanks for your help talentedasian! Your branch will be the base of the new PR

Zabuzard added new command Add a new command or group of commands to the bot priority: normal labels Dec 15, 2021

Zabuzard assigned talentedasian Dec 15, 2021

Zabuzard added this to the Improvement phase 1 milestone Dec 15, 2021

Instead of getting the mods with an expensive query, query it just once

52744e5

and use it for subsequent invocation of the command.

Send DM to selected mod. If the selected mod wasn't found, which is

806bee3

highly unlikely, send an ephemeral message and log the unusual behavior

talentedasian added 3 commits December 16, 2021 13:38

Reply an ephemeral message to user after sending the message to a mod…

b563a4c

…. This also fixes the failed interaction by discord

Miscellaneous refactors

7ec6b33

* Add /reloadmod command for moderators to reload the list of choices in

1bfbf42

the /modmail command. * Introduce a utility class for use by other classes when interacting with the ModMailCommand. * Some javadoc.

talentedasian marked this pull request as ready for review December 16, 2021 15:30

talentedasian requested review from a team as code owners December 16, 2021 15:30

talentedasian commented Dec 16, 2021

View reviewed changes

Zabuzard requested changes Dec 16, 2021

View reviewed changes

Tais993 requested changes Dec 16, 2021

View reviewed changes

* Completely redo the way how select options are being generated.

e154d7f

Replace all loadOptions with the onReady event but still delegating to the ModmailUtils to query discord. * Move ReloadModmailCommand from a separate class to the ModmailCommand class.

talentedasian requested a review from Tais993 December 17, 2021 07:11

illuminator3 requested changes Dec 17, 2021

View reviewed changes

github-actions bot added the stale label Jan 18, 2022

Tais993 closed this Jan 20, 2022

Tais993 mentioned this pull request Jan 20, 2022

Feature/modmail #350

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

/modmail slash command #329

/modmail slash command #329

talentedasian commented Dec 15, 2021

Tais993 commented Dec 15, 2021

CLAassistant commented Dec 16, 2021 •

edited

Loading

talentedasian commented Dec 16, 2021 •

edited

Loading

talentedasian commented Dec 16, 2021

talentedasian Dec 16, 2021

Zabuzard Dec 16, 2021

talentedasian Dec 16, 2021

talentedasian Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

Tais993 Dec 16, 2021

illuminator3 Dec 17, 2021

talentedasian Dec 18, 2021

talentedasian Dec 18, 2021

Tais993 Dec 18, 2021

talentedasian Dec 18, 2021

Tais993 Dec 18, 2021

talentedasian Dec 18, 2021

Tais993 Dec 18, 2021 •

edited

Loading

illuminator3 Dec 18, 2021

github-actions bot commented Jan 18, 2022

Tais993 commented Jan 20, 2022

talentedasian commented Jan 20, 2022

Tais993 commented Jan 20, 2022

Zabuzard commented Jan 20, 2022

Tais993 commented Jan 20, 2022

Tais993 commented Jan 20, 2022

		Guild guild = Objects.requireNonNull(jda.getGuildById(config.getGuildId()),
		"A Guild is required to use this command. Perhaps the bot isn't on the guild yet");

		static final List<SelectOption> mods = new ArrayList<>();
		static final Map<String, User> modsMap = new HashMap<>();

	&& !userId.equals(Objects.requireNonNull(event.getMember()).getId())) {
	&& !userId.equals(event.getMember().getId())) {

/modmail slash command #329

/modmail slash command #329

Conversation

talentedasian commented Dec 15, 2021

Pull-request

Changes

Description

Tais993 commented Dec 15, 2021

CLAassistant commented Dec 16, 2021 • edited Loading

talentedasian commented Dec 16, 2021 • edited Loading

talentedasian commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tais993 Dec 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2022

Tais993 commented Jan 20, 2022

talentedasian commented Jan 20, 2022

Tais993 commented Jan 20, 2022

Zabuzard commented Jan 20, 2022

Tais993 commented Jan 20, 2022

Tais993 commented Jan 20, 2022

CLAassistant commented Dec 16, 2021 •

edited

Loading

talentedasian commented Dec 16, 2021 •

edited

Loading

Tais993 Dec 18, 2021 •

edited

Loading