Skip to content
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

Add Starboard support #881

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Add Starboard support #881

wants to merge 10 commits into from

Conversation

java-coding-prodigy
Copy link
Contributor

This feature adds a Starboard.
Certain emojis which when added as a reaction to a message in a channel which everyone has permissions to, will be sent as an embed in the starboard channel.
Both the emojis and the channel are customizable through config.json

@@ -95,4 +95,8 @@
"rateLimitWindowSeconds": 10,
"rateLimitRequestsInWindow": 3
}
"starboard": {
"emojiNames" : ["star"],
"channelName": "starboard"
Copy link
Member

Choose a reason for hiding this comment

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

channelNamePattern please (and in the code Pattern.compile(...) then, matching the channel)

Copy link
Contributor Author

@java-coding-prodigy java-coding-prodigy Sep 13, 2023

Choose a reason for hiding this comment

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

In that case, I would only have one pattern and not an array right? The design you are referring to I presume is the one where if the user wants multiple emojis they would need to seperate it by a pipe?

Copy link
Member

Choose a reason for hiding this comment

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

the emoij names can stay non-pattern like that. but the channel name should be a pattern, so that we have some flexibility in naming it.

Comment on lines 350 to 351
* Gets the config for the Starboard Contains the List of emoji names recognized by the
* starboard as well as the name of the channel with the starboard.
Copy link
Member

Choose a reason for hiding this comment

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

these details are outdated extremely quick, cause people editing the config wont notice that its duplicated here. get rid of that. instead, describe what a starboard is.

import java.util.Objects;

@JsonRootName("starboard")
public final class StarboardConfig {
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

private final String channelName;

@JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
public StarboardConfig(
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

Comment on lines 36 to 52
* Gets the name of the channel with the starboard Deactivate by using a non-existent channel
* name
*
* @return the name of the channel with the starboard
*/
public String getChannelName() {
return channelName;
}
Copy link
Member

Choose a reason for hiding this comment

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

(pattern, see earlier)
missing dot in the first and second sentence.

Comment on lines 50 to 52
if (database.read(context -> context.selectFrom(STARBOARD_MESSAGES)
.fetch("message_id")
.contains(event.getMessageIdLong()))) {
Copy link
Member

Choose a reason for hiding this comment

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

too complex for a if, put it into a helper method

.queue();
}

private boolean ignoreMessage(String emojiName, Guild guild, GuildChannel channel) {
Copy link
Member

Choose a reason for hiding this comment

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

bad method name. its missing a shouldXXX, otherwise it sounds as if this method, when called, will ignore the message

User author = message.getAuthor();
return new EmbedBuilder().setAuthor(author.getName(), null, author.getAvatarUrl())
.setDescription(message.getContentDisplay())
.build(); // TODO make footer with link and reacted emojis
Copy link
Member

Choose a reason for hiding this comment

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

TODO (dont forget)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sonar and CodeQL won't let me forget it. :)

Comment on lines 1 to 4
CREATE TABLE starboard_messages
(message_id BIGINT NOT NULL PRIMARY KEY
)
Copy link
Member

Choose a reason for hiding this comment

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

formatting looks a bit weird with the (

Comment on lines 50 to 55
if (database.read(context -> context.selectFrom(STARBOARD_MESSAGES)
.fetch("message_id")
.contains(event.getMessageIdLong()))) {
database.write(context -> context.newRecord(STARBOARD_MESSAGES)
.setMessageId(event.getMessageIdLong()));
}
Copy link
Member

Choose a reason for hiding this comment

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

whats the point of the database entry? its not used anywhere right now. id guess u want to check whether it exists to not duplicate-post? but that check seems to be missing right now.

also, u will have to additionally put an in-memory cache as layer above the DB to prevent checking the DB a million times in 5 seconds when someone posts an announcement. u can look in some of the other classes for that, search for Caffeine(cache).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The database is being used for the check, that is what the if is for. However the current approach seems to be insanely slow as I fetch the entire list of entries.

I'll look into it.

Copy link
Member

Choose a reason for hiding this comment

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

i think your missing my point on the question. what is the point of the database entry? it does not control any logic right now.
you have:

if (database doesnt contain it) {
  put it into database
}

but thats it. what point does it serve, sitting in the db? theres nothing in ur code that would control anything based on it.

u likely intended to stop transfering the message if its contained in the db, i.e.

if (database does not contain it) {
  copy message to starboard
}

but u dont have that. you always copy the message to the starboard, regardless of database

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh oops. Ur right mb 😅

@java-coding-prodigy
Copy link
Contributor Author

Uh oh.
I did something wrong apparently.
Well what happened was I just reset my state softly to the first commit. Then cherry-picked #902 ‘s commit and applied all the changes of the stash.
But now even the changes of those commits which have been merged into develop from other branches are showing up in the diff which makes no sense to me.

@java-coding-prodigy
Copy link
Contributor Author

Issue seems to be resolved after fixing merge conflicts. Weird but I’ll take it.

Comment on lines 85 to 110
private static MessageEmbed formEmbed(Message message) {
User author = message.getAuthor();
return new EmbedBuilder().setAuthor(author.getName(), null, author.getAvatarUrl())
.setDescription(message.getContentDisplay())
.appendDescription(" [Link](%s)".formatted(message.getJumpUrl()))
.build();
}
Copy link
Member

Choose a reason for hiding this comment

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

can u also check for attachments and add that in the embed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set the image of the embed as the first image file in the attachments, if any.

Copy link

sonarcloud bot commented Nov 5, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

sonarcloud bot commented Apr 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 New Major Issues (required ≤ 0)
2 New Critical Issues (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link

sonarcloud bot commented May 7, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
2 New Critical Issues (required ≤ 0)
2 New Major Issues (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: normal
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants