Skip to content

Add tophelpers command 🏆 #117

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions application/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ dependencies {
implementation 'com.fasterxml.jackson.dataformat:jackson-dataformat-csv:2.13.0'
implementation 'com.fasterxml.jackson.core:jackson-databind:2.13.0'

testImplementation 'org.mockito:mockito-core:3.12.4'
testRuntimeOnly 'org.mockito:mockito-core:3.12.4'
implementation 'com.github.freva:ascii-table:1.2.0'

testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1'
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.8.1'
testImplementation 'org.mockito:mockito-core:4.0.0'
}

application {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.togetherjava.tjbot.commands.system.CommandSystem;
import org.togetherjava.tjbot.config.Config;
import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.listener.TopHelpersMetadataListener;

import javax.security.auth.login.LoginException;
import java.io.IOException;
Expand Down Expand Up @@ -76,7 +77,8 @@ public static void runBot(String token, Path databasePath) {
Database database = new Database("jdbc:sqlite:" + databasePath.toAbsolutePath());

JDA jda = JDABuilder.createDefault(token)
.addEventListeners(new CommandSystem(database))
.addEventListeners(new CommandSystem(database),
new TopHelpersMetadataListener(database))
.build();
jda.awaitReady();
logger.info("Bot is ready");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,6 @@ public enum Commands {
// available.
return List.of(new PingCommand(), new DatabaseCommand(database), new TeXCommand(),
new TagCommand(tagSystem), new TagManageCommand(tagSystem),
new TagsCommand(tagSystem));
new TagsCommand(tagSystem), new TopHelpers(database));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
package org.togetherjava.tjbot.commands;

import com.github.freva.asciitable.HorizontalAlign;
import net.dv8tion.jda.api.entities.Member;
import net.dv8tion.jda.api.events.interaction.SlashCommandEvent;
import net.dv8tion.jda.api.interactions.commands.OptionMapping;
import net.dv8tion.jda.api.interactions.commands.OptionType;
import net.dv8tion.jda.api.interactions.commands.build.OptionData;
import org.jetbrains.annotations.Nullable;
import org.jooq.Records;
import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.util.PresentationUtils;

import javax.annotation.ParametersAreNonnullByDefault;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not used javax.annotation yet. I would suggest to not use them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why? Why would we prefer a vendor specific annotation and I see no point putting @NotNull/@nullable annotations across each and every method that we write.
If we prefer non-standard third party annotations, its still fine but does it have an equivalent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annotation we are using is already in our dependencies. If you want to use this other annotation, please make sure that the dependencies cover it.

That said, for the null-checks it would probably be best to stick to one and not mix the millions of @NotNull annotations with each other.

I dont have a strong opinion here. Maybe get other opinions (@I-Al-Istannen )

Copy link
Member

@Tais993 Tais993 Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to side with Zabuzard.

Usage of @NotNull and @Nullable makes it clear what the arguments are.
When you look at the docs of a method, it actually shows the arguments can't be null or are nullable. (Because it displays the method signature)
If you use a class-wide annotation like this it's not shown in the docs either.

If we prefer non-standard third party annotations, its still fine but does it have an equivalent?

To my knowledge, javax annotations have been removed since Java 11, so this argument is obsolete.

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import static org.jooq.impl.DSL.*;
import static org.togetherjava.tjbot.db.generated.tables.MessageMetadata.MESSAGE_METADATA;

/**
* Command to retrieve top helpers for last 30 days.
*/
@ParametersAreNonnullByDefault
public final class TopHelpers extends SlashCommandAdapter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Javadoc


public static final String PLAINTEXT_MESSAGE_TEMPLATE = "```\n%s\n```";
private static final String COUNT_OPTION = "count";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

count what? Maybe rename to "day-count" or "days"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... it is actually user count. Yeah, a rename is definitely needed 😆 users or user-count maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesnt get more apparent than that
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zabuzard is also referring to the variable's name if I understand it correctly

private static final String NO_ENTRIES = "No entries";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only used once, I would inline it.


private static final long MIN_COUNT = 10L;
private static final long MAX_COUNT = 30L;

private record TopHelperRow(Integer serialId, Long userId, Long messageCount) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • May I ask, why the wrapper classes? Can it be null?
  • @NotNull

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have a named tuple was a review comment from Istannen since it was a Record3 earlier. Either ways I am fine with both

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields are populated at runtime from the database hence I dont think @NotNull adds anything here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use the primitives instead

}

private final Database database;

/**
* Initializes TopHelpers with a database.
*
* @param database the database to store the key-value pairs in
*/
public TopHelpers(Database database) {
super("tophelpers", "Find top helpers for last 30 days", SlashCommandVisibility.GUILD);
this.database = database;
getData().addOptions(new OptionData(OptionType.INTEGER, COUNT_OPTION,
"Count of top helpers to be retrieved (default is 10 and capped at 30)", false));
Comment on lines +48 to +49
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing an option to tell it how far to look back in days. I would like to be able to request a day filter from 1 to 90, default 30 (1 month).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even cooler would be if you could request a start and end date range (with some limits of course). But we can also move this to a possible second iteration of the command.

My use case is: If someone forgot to do the top helpers or is a couple of days late, they have to look further back and subtract the results from the other overlapping range, like if you are 10 days late, u need look back 10 days minus look back 40 days, which is super cumbersome to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doable but currently any record older than 30 days from current day are purged. If we agree on an upper bound on how far back we can go the same can be accommodated

}

@SuppressWarnings("squid:S1192")
@Override
public void onSlashCommand(SlashCommandEvent event) {
long guildId = event.getGuild().getIdLong();
long count = getBoundedCountAsLong(event.getOption(COUNT_OPTION));
database.read(dsl -> {
List<TopHelperRow> records = dsl.with("TOPHELPERS")
.as(select(MESSAGE_METADATA.USER_ID, count().as("COUNT")).from(MESSAGE_METADATA)
.where(MESSAGE_METADATA.GUILD_ID.eq(guildId))
.groupBy(MESSAGE_METADATA.USER_ID)
.orderBy(count().desc())
.limit(count))
.select(rowNumber().over(orderBy(field(name("COUNT")).desc())).as("#"),
field(name("USER_ID"), Long.class), field(name("COUNT"), Long.class))
.from(table(name("TOPHELPERS")))
.fetch(Records.mapping(TopHelperRow::new));
generateResponse(event, records);
});
}

private static long getBoundedCountAsLong(@Nullable OptionMapping count) {
return count == null ? MIN_COUNT : Math.min(count.getAsLong(), MAX_COUNT);
}

private static String prettyFormatOutput(List<List<String>> dataFrame) {
return String.format(PLAINTEXT_MESSAGE_TEMPLATE,
dataFrame.isEmpty() ? NO_ENTRIES
: PresentationUtils.dataFrameToAsciiTable(dataFrame,
new String[] {"#", "Name", "Message Count (in the last 30 days)"},
new HorizontalAlign[] {HorizontalAlign.RIGHT, HorizontalAlign.LEFT,
HorizontalAlign.RIGHT}));
}

private static void generateResponse(SlashCommandEvent event,
List<TopHelperRow> records) {
List<Long> userIds = records.stream().map(TopHelperRow::userId).toList();
event.getGuild().retrieveMembersByIds(userIds).onSuccess(members -> {
Map<Long, String> activeUserIdToEffectiveNames = members.stream()
.collect(Collectors.toMap(Member::getIdLong, Member::getEffectiveName));
List<List<String>> topHelpersDataframe = records.stream()
.map(topHelperRow -> List.of(topHelperRow.serialId.toString(),
activeUserIdToEffectiveNames.getOrDefault(topHelperRow.userId,
// Any user who is no more a part of the guild is marked as
// [UNKNOWN]
"[UNKNOWN]"),
topHelperRow.messageCount.toString()))
.toList();
event.reply(prettyFormatOutput(topHelpersDataframe)).queue();
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,19 @@ public final class Config {
private final String databasePath;
private final String projectWebsite;
private final String discordGuildInvite;
private final String helpChannelRegex;

@JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
private Config(@JsonProperty("token") String token,
@JsonProperty("databasePath") String databasePath,
@JsonProperty("projectWebsite") String projectWebsite,
@JsonProperty("discordGuildInvite") String discordGuildInvite) {
@JsonProperty("databasePath") String databasePath,
@JsonProperty("projectWebsite") String projectWebsite,
@JsonProperty("discordGuildInvite") String discordGuildInvite,
@JsonProperty("helpChannelRegex") String helpChannelRegex) {
this.token = token;
this.databasePath = databasePath;
this.projectWebsite = projectWebsite;
this.discordGuildInvite = discordGuildInvite;
this.helpChannelRegex = helpChannelRegex;
}

/**
Expand Down Expand Up @@ -94,4 +97,11 @@ public String getProjectWebsite() {
public String getDiscordGuildInvite() {
return discordGuildInvite;
}

/**
* Gets regex pattern to identify help channels.
*
* @return string representation of regex pattern to identify help channels
*/
public String getHelpChannelRegex() { return helpChannelRegex; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package org.togetherjava.tjbot.listener;

import net.dv8tion.jda.api.events.message.guild.GuildMessageReceivedEvent;
import net.dv8tion.jda.api.hooks.ListenerAdapter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.togetherjava.tjbot.db.Database;
import org.togetherjava.tjbot.util.JdaUtils;

import java.time.Instant;
import java.time.temporal.ChronoUnit;

import static org.togetherjava.tjbot.db.generated.tables.MessageMetadata.MESSAGE_METADATA;

/**
* Listener responsible for persistence of text message metadata.
*/
public final class TopHelpersMetadataListener extends ListenerAdapter {
private static final Logger logger = LoggerFactory.getLogger(TopHelpersMetadataListener.class);

private static final int MESSAGE_METADATA_ARCHIVAL_DAYS = 30;

private final Database database;

/**
* Creates a new message metadata listener, using the given database.
*
* @param database the database to store message metadata.
*/
public TopHelpersMetadataListener(Database database) {
this.database = database;
}

/**
* Stores the relevant message metadata for on message received event.
*
* @param event incoming message received event.
*/
@Override
public void onGuildMessageReceived(GuildMessageReceivedEvent event) {
var channel = event.getChannel();
if (!event.getAuthor().isBot() && !event.isWebhookMessage()
&& JdaUtils.isHelpChannel(channel)) {
var messageId = event.getMessage().getIdLong();
var guildId = event.getGuild().getIdLong();
var channelId = channel.getIdLong();
var userId = event.getAuthor().getIdLong();
var createTimestamp = event.getMessage().getTimeCreated().toEpochSecond();
database.write(dsl -> {
dsl.newRecord(MESSAGE_METADATA)
.setMessageId(messageId)
.setGuildId(guildId)
.setChannelId(channelId)
.setUserId(userId)
.setCreateTimestamp(createTimestamp)
.insert();
int noOfRowsDeleted = dsl.deleteFrom(MESSAGE_METADATA)
.where(MESSAGE_METADATA.CREATE_TIMESTAMP.le(Instant.now()
.minus(MESSAGE_METADATA_ARCHIVAL_DAYS, ChronoUnit.DAYS)
.getEpochSecond()))
.execute();
if (noOfRowsDeleted > 0) {
logger.debug(
"{} old records have been deleted based on archival criteria of {} days.",
noOfRowsDeleted, MESSAGE_METADATA_ARCHIVAL_DAYS);
}
});
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/**
* This package contains non-user interactive event listeners.
*/
package org.togetherjava.tjbot.listener;
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package org.togetherjava.tjbot.util;

import net.dv8tion.jda.api.entities.TextChannel;

import java.util.regex.Pattern;

/**
* Miscellaneous utilities for JDA entities.
*/
public final class JDAUtils {
private static final Pattern HELP_CHANNEL_PATTERN = Pattern.compile(".*\\Qhelp\\E.*");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voting to move this into the Config instead, so that adding a new channel that doesnt follow the pattern does not require a code change but just a simple config change + bot restart.
And to give other servers (for example the develop bot) the flexibility to bind it to a different non-help channel.


private JDAUtils() {}

/**
* Checks if a provided channel is a help channel.
*
* @param textChannel provided channel.
* @return true if the provided channel is a help channel, false otherwise.
*/
public static boolean isAHelpChannel(TextChannel textChannel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The A in isAHelpChannel trips me a bit, maybe remove it, just isHelpChannel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add @NotNull.

return HELP_CHANNEL_PATTERN.matcher(textChannel.getName()).matches();
}
Comment on lines +11 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I think it might be better to move those helpers back into where you actually use them, into TopHelperCommand or similar.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package org.togetherjava.tjbot.util;

import net.dv8tion.jda.api.entities.TextChannel;

import java.util.regex.Pattern;

/**
* Miscellaneous utilities for JDA entities.
*/
public final class JdaUtils {
private static final Pattern HELP_CHANNEL_PATTERN = Pattern.compile(".*\\Qhelp\\E.*");

private JdaUtils() {}

/**
* Checks if a provided channel is a help channel.
*
* @param textChannel provided channel.
* @return true if the provided channel is a help channel, false otherwise.
*/
public static boolean isHelpChannel(TextChannel textChannel) {
return HELP_CHANNEL_PATTERN.matcher(textChannel.getName()).matches();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package org.togetherjava.tjbot.util;

import com.github.freva.asciitable.AsciiTable;
import com.github.freva.asciitable.Column;
import com.github.freva.asciitable.ColumnData;
import com.github.freva.asciitable.HorizontalAlign;
import org.jetbrains.annotations.NotNull;

import java.util.List;
import java.util.Objects;
import java.util.stream.IntStream;

/**
* Utility class for representation of data in various formats
*/
public final class PresentationUtils {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class name is kinda confusing me a bit and the Javadoc did not help a lot. Maybe you can find a better name for it. @I-Al-Istannen would probably also say you should just move the method out of utility if only you are using it 🤷

private PresentationUtils() {}
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum pattern or throw exception (see other comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree this is not an enumeration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He said "or"

So, why did you ignore his second statement?


/**
* Flattens a dataFrame to String representation of a table.
*
* eg.
* <pre>
* {@code
* var dataframe = List.of(List.of("Apple", "Fruit"), List.of("Potato", "Vegetable"));
* var columnHeaders = new String[] {"Item", "Category"};
* var horizontalAlignment = new HorizontalAlign[] {HorizontalAlign.LEFT, HorizontalAlign.LEFT};
* dataFrameToAsciiTable(dataframe, columnHeaders, horizontalAlignment);
* }
* </pre>
* will return:
* <pre>
* {@code
* Item | Category
* --------+-----------
* Apple | Fruit
* Potato | Vegetable
* }
* </pre>
*
* @param dataFrame dataframe represented as List<List<String>> where List<String> represents a
* single row
* @param headers column headers for the table
* @param horizontalAligns column alignment for the table
* @return String representation of the dataFrame in tabular form
*/
public static String dataFrameToAsciiTable(@NotNull List<List<String>> dataFrame,
@NotNull String[] headers, @NotNull HorizontalAlign[] horizontalAligns) {
Objects.requireNonNull(dataFrame, "DataFrame cannot be null");
Objects.requireNonNull(headers, "Headers cannot be null");
Objects.requireNonNull(horizontalAligns, "HorizontalAligns cannot be null");
Character[] characters = AsciiTable.BASIC_ASCII_NO_DATA_SEPARATORS_NO_OUTSIDE_BORDER;
List<ColumnData<List<String>>> columnData = IntStream.range(0, headers.length)
.mapToObj(i -> new Column().header(headers[i])
.dataAlign(horizontalAligns[i])
.<List<String>>with(row -> row.get(i)))
.toList();
return AsciiTable.getTable(characters, dataFrame, columnData);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/**
* This package contains miscellaneous utilities.
*/
package org.togetherjava.tjbot.util;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
CREATE TABLE message_metadata(
message_id BIGINT NOT NULL PRIMARY KEY,
guild_id BIGINT NOT NULL,
channel_id BIGINT NOT NULL,
user_id BIGINT NOT NULL,
create_timestamp BIGINT NOT NULL
)