-
-
Notifications
You must be signed in to change notification settings - Fork 105
Implement Quotes Board #1029
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
base: develop
Are you sure you want to change the base?
Implement Quotes Board #1029
Conversation
application/src/main/java/org/togetherjava/tjbot/features/Features.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
4a68771 to
bc3b7d6
Compare
35ba891 to
47cdcb2
Compare
Taz03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a message about config changes on main pr message, moderators said its hard to keep track of it otherwise
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
47cdcb2 to
6d86c8f
Compare
ed3dbd5 to
59dc245
Compare
59dc245 to
35abd02
Compare
|
maybe it makes more sense to implement this with the new forward feature in discord? |
35abd02 to
7b19538
Compare
@SquidXTV This is now a feature! With the latest changes, we are now forwarding messages instead of embedding messages. Good suggestion :) |
bbde421 to
7d839d6
Compare
Good job <3 |
Thank you! I have updated the PR description. Thanks for pointing it out! |
application/src/main/java/org/togetherjava/tjbot/config/CoolMessagesBoardConfig.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/CoolMessagesBoardManager.java
Outdated
Show resolved
Hide resolved
9f303cd to
8407d3f
Compare
|
gonna help with this |
For this feature, the version of JDA had to be bumped to 5.1.2
8407d3f to
fb4fb5d
Compare
* minimumReactions-5, star symbol instead of encoding
* requests changes by zabuzard except for moving getBoardChannel down and markMessageAsProcessed
* Following JavaDocs guidelines of making the first letter capital
* code refactoring
* refactor: use correct method for reactionsCount
It turns out that for each event fired, every *single* damn time,
messageReaction.hasCount() would always return false. No matter what.
Terrible documentation from JDA's side. As a result, because of the
ternary operator:
messageReaction.hasCount() ? messageReaction.getCount() + 1 : 1
the result of `reactionsCount` would always end up holding the value of
one.
In the following changes, we use `messageReaction.retrieveUsers()` to
get a list of the people reacted, get a `Stream<User>` from that and get
its count. Much more reliable this way and it also happens to be more
readable.
Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
Co-authored-by: Chris Sdogkos <work@chris-sdogkos.com>
Co-authored-by: Surya Tejess <74978874+suryatejess@users.noreply.github.com>
Since 1ade409 (refactor: code review addressed by Zabuzard, 2025-06-28) primarily contains a generaly vague JavaDoc describing what the `QuoteBoardForwarder.java` class is doing, a more descriptive one replaces it. Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
application/src/main/java/org/togetherjava/tjbot/config/Config.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/config/Config.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/config/Config.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/config/QuoteBoardConfig.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/QuoteBoardForwarder.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/QuoteBoardForwarder.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/QuoteBoardForwarder.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/togetherjava/tjbot/features/basic/QuoteBoardForwarder.java
Outdated
Show resolved
Hide resolved
c3c1579 to
a6085db
Compare
* rename coolMessagesConfig to quoteMessagesConfig * removed backticks for QuoteBoardForwarder and added a qualifier statement for QuoteBoardForwarder * param check for reactionEmoji * straight quotes instead of smart quotes * rename isCoolEmoji to isTriggerEmoji * early return when reactionsCount < config.minimumReactions() * early return for isCoolEmoji
Due to some hastiness in resolving the recent merge conflicts, some parts with "coolMessagesConfig" were not renamed to "quoteMessagesConfig". Take care of that. Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
|
Hello! I'm taking over this PR from @christolis. Planning to continue work starting tonight or tomorrow. Updates coming soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your efforts @christolis, I appreciate the fact that the feature works fine after ive done a small fix (latest commit), however, some parts in the code need to be reviewed carefully for the good of the application, i guess
| "quoteMessagesConfig": { | ||
| "minimumReactions": 2, | ||
| "boardChannelPattern": "quotes", | ||
| "reactionEmoji": "⭐" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'd give this property a more readable name like
triggerReaction, or simplyreaction. - I'm not certain about putting emojis in code like this, instead, why not using the discord code of it
:star:? I think it's cleaner
application/config.json.template
Outdated
| "pollIntervalInMinutes": 10 | ||
| }, | ||
| "quoteMessagesConfig": { | ||
| "minimumReactions": 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think to give this property a more readable name like
triggerMinimumReactionsCount. - I think to increase the value, to better indicate the importance of the message, as
2would be easy, if someone has at least one close friend on the server. A more appropriate value like 5 would be fairer
application/config.json.template
Outdated
| }, | ||
| "quoteMessagesConfig": { | ||
| "minimumReactions": 2, | ||
| "boardChannelPattern": "quotes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd give this property a more readable name like hostingChannel, targetChannel or simply channel
application/config.json.template
Outdated
| "videoLinkPattern": "http(s)?://www\\.youtube.com.*", | ||
| "pollIntervalInMinutes": 10 | ||
| }, | ||
| "quoteMessagesConfig": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think to give this parent property a more readable name such as quoteBoardConfig like the Java class name for consistency.
| private final RSSFeedsConfig rssFeedsConfig; | ||
| private final String selectRolesChannelPattern; | ||
| private final String memberCountCategoryPattern; | ||
| private final QuoteBoardConfig quoteMessagesConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I mentioned in config.json.template, it would be better to rename this to quoteBoardConfig for consistency
| public void onMessageReactionAdd(MessageReactionAddEvent event) { | ||
| final MessageReaction messageReaction = event.getReaction(); | ||
| boolean isTriggerEmoji = messageReaction.getEmoji().equals(triggerReaction); | ||
| long guildId = event.getGuild().getIdLong(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is a bit faraway from where it's being called, i'd sugges to move it below the if statements
| * @return the board text channel | ||
| */ | ||
| private Optional<TextChannel> findQuoteBoardChannel(JDA jda, long guildId) { | ||
| return jda.getGuildById(guildId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return type of jda.getGuildById(guildId), is nullable. You have to add a control on that before going to .getTextChannelCache()
| * @param guildId the guild ID | ||
| * @return the board text channel | ||
| */ | ||
| private Optional<TextChannel> findQuoteBoardChannel(JDA jda, long guildId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Id rename this method to
findQuoteBoardChannelByName()and add another paramchannelName; - I'd mark this method as
static; - I'd move it above instance methods in this class.
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(QuoteBoardForwarder.class); | ||
| private final Emoji triggerReaction; | ||
| private final Predicate<String> isQuoteBoardChannelName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As said below, i don't see a reason for this field isQuoteBoardChannelName
| .getTextChannelCache() | ||
| .stream() | ||
| .filter(channel -> isQuoteBoardChannelName.test(channel.getName())) | ||
| .findAny(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the filter result must provide only one result, in case of multiple ones, an error should be thrown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A warning maybe but yeah.
Comments: 1. improved settings 2. using meaningful names 3. validations 4. some debug logs
Closes #1027.
Screenshots
Configuration changes
coolMessagesConfig.minimumReactionfor the target message to
be considered to the board channel.
coolMessagesConfig.boardChannelPatternthat should be quoted get posted to.
"quotes"coolMessagesConfig.reactionEmoji"U+2B50"