Feature: Cake Day #1042

christolis · 2024-03-03T00:07:53Z

I know that this is not a certain feature to be added, but I still wanted to experiment and see what I could come up with :)

Description

This pull request introduces the addition of the cake day feature to the server, allowing members to commemorate their annual membership anniversary with a unique "Cake day" role, with which they can celebrate within the community.

This feature uses the database and makes a cake_days table in order to store each member's joined date, as well the guild in which they joined (in case the bot ever gets to handle multiple guilds). The date stored in the database is split into two columns: the month and the day in one (joined_month_day), and the year in an other (joined_year). The reason the join date is split into these two columns is so that an index can be added for the joined_month_day column in order to make reading from the database faster. Therefore, it will be easier for the bot to find all cake days by the month and day, a search that would occur daily by this feature's routine.

If the bot finds that the cake_days table has no records, then it attempts to populate the table from all members of all guilds in a separate thread with batched insert queries in order to optimize the table population task.

Configuration changes

Property	Description	Type	Default
`cakeDayConfig.rolePattern`	A pattern of the cake day role to give to members.	String	`"Cake Day"`

Diagram

TODO

Write JavaDocs for all new code.
Implement cake-day role re-assignment daily. First it should revoke the role from all members, then based on the day's month and day, it should find all matching cake days and for the matches where the year difference is bigger than 0, assign them the role and whatever cosmetic stuff necessary.
Talk with the team about whether changing the member cache policy as well as the chunking filter to all is a valid approach for fetching all members of the server (is it worth storing all member references in memory just for one bulk database insert that will not even happen more than once?)

Closes #1035.

application/src/main/java/org/togetherjava/tjbot/config/Config.java

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

application/config.json.template

application/src/main/java/org/togetherjava/tjbot/config/CakeDayConfig.java

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

ankitsmt211 · 2024-03-10T21:40:27Z

It would be nice if one of the @Together-Java/moderators can review some of the critical parts of this feature when you can.

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayListener.java

Co-authored-by: TheCodeMr <151576372+TheCodeMr@users.noreply.github.com> Co-authored-by: cab <161495905+cabagbe@users.noreply.github.com> Co-authored-by: Devansh Tiwari <65783463+devloves@users.noreply.github.com>

This commit aims to fix a bug where the CakeDayService#addTodayMembersCakeDayRole() method would add the cake day role to all members who have been at least one year into the server, disregarding the month and date in which they joined. The documentation has also been made more clean and concise, while the CakeDayService#addCakeDayRole() which required a UserSnowflake as one of its inputs has been removed and now the other version of this function is used which only requires a Member instance. Passing the Guild would be unnecessary as it could be easily acquired from the Member instance, and additionally it helps make sure that the right Member and Guild are used to call this method. Finally, this commit adds an extra condition in the select-from query found in CakeDayService#findCakeDaysTodayFromDatabase() which makes sure that we get all the cake days for the right guild, instead of getting them all unconditionally.

SquidXTV · 2025-07-01T11:37:16Z

application/src/main/java/org/togetherjava/tjbot/config/CakeDayConfig.java

+    /**
+     * Configuration constructor for the Cake Day feature.
+     */
+    public CakeDayConfig {


This comment really needed? It would maybe make sense if it links to the corresponding classes. Though even that should be done as some @see in the records comment instead.

Possible that we require javadoc on this one (public class). But yeah, we can add some details or a @see.

SquidXTV · 2025-07-01T11:45:05Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

+     * @param database the database for storing cake day information
+     */
+    public CakeDayService(Config config, Database database) {
+        this.config = config.getCakeDayConfig();


I really dont like the naming here. Both CakeDayConfig and Config variables are called config.

id rename the parameter then though. dunno, i think for the pov of the class and where its being used in the methods it makes sense that config is the config for this feature. so perhaps the parameter Config can then be called fullConfig or something instead here.

SquidXTV · 2025-07-01T11:48:50Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

+            return;
+        }
+
+        refreshMembersCakeDayRoles(cakeDayRole.get(), guild);


could use Optional#ifPresent here

SquidXTV · 2025-07-01T12:13:34Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

+            boolean isAnniversaryDay = hasMemberCakeDayToday(member);
+            int yearsSinceJoin = OffsetDateTime.now().getYear() - cakeDayRecord.getJoinedYear();
+
+            if (yearsSinceJoin > 0 && isAnniversaryDay) {


Hm so this is confusing me a bit. hasMemberCakeDayToday checks if today is the cake day using the Member member which includes checking if the anniversary year is > 0. This looks like checking the same thing twice (yearsSinceJoin calculation uses the db record though). Also is the member query needed for this check? Doesn't the database also store the join date (make sure it is up2date though)? You would only need the member instance fo adding the role later on, so you could move that getMemberById later in the chain and thus making it more efficient.

SquidXTV · 2025-07-01T12:18:08Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

+            return;
+        }
+
+        guild.addRoleToMember(snowflake, cakeDayRole.get()).complete();


A member is a UserSnowflake, so it makes no sense that you get the snowflake from the member.

SquidXTV · 2025-07-01T12:49:08Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

+                .where(CAKE_DAYS.JOINED_MONTH_DAY.eq(todayMonthDay))
+                .and(CAKE_DAYS.GUILD_ID.eq(guild.getIdLong()))
+                .fetch())
+            .collect(Collectors.toList());


Doesn't database.read already return a list?

SquidXTV · 2025-07-01T12:51:05Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java

+                .fetch())
+            .collect(Collectors.toList())
+            .stream()
+            .findFirst();


You sure the .collect(...).stream().findFirst() is really needed? Is there no other method to use here?

if all u need is the first hit (or any hit, bc there is no sorting), best is to modify the query to only return that result on DB level and not everything. im pretty sure jooq has a method for that and i recall us using it somewhere already

SquidXTV · 2025-07-01T12:53:39Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayRoutine.java

+     *
+     * @param cakeDayService an instance of the cake day service
+     */
+    public CakeDayRoutine(CakeDayService cakeDayService) {


This comment seems pretty useless.

SquidXTV · 2025-07-01T12:54:33Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayListener.java

+     *
+     * @param cakeDayService the {@link CakeDayService} to be used by this listener
+     */
+    public CakeDayListener(CakeDayService cakeDayService) {


This comment seems pretty useless.

SquidXTV · 2025-07-01T12:55:48Z

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayListener.java

+            return;
+        }
+
+        cakeDayService.insertMemberCakeDayToDatabase(member, guildId);


You could use Optional#ifPresent here.

christolis self-assigned this Mar 3, 2024

christolis force-pushed the feature/cake-day branch from 60b2118 to c7bff99 Compare March 3, 2024 13:24

christolis marked this pull request as ready for review March 5, 2024 12:44

christolis requested review from a team as code owners March 5, 2024 12:44

SquidXTV requested changes Mar 5, 2024

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/config/Config.java Outdated Show resolved Hide resolved

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayService.java Outdated Show resolved Hide resolved

application/config.json.template Outdated Show resolved Hide resolved

christolis marked this pull request as draft March 5, 2024 21:05

christolis marked this pull request as ready for review March 6, 2024 21:33

SquidXTV requested changes Mar 6, 2024

View reviewed changes

christolis requested a review from SquidXTV March 6, 2024 23:42

SquidXTV previously approved these changes Mar 6, 2024

View reviewed changes

christolis dismissed SquidXTV’s stale review via d8c7206 March 13, 2024 15:46

tj-wazei reviewed Mar 17, 2024

View reviewed changes

application/src/main/java/org/togetherjava/tjbot/features/cakeday/CakeDayListener.java Show resolved Hide resolved

christolis force-pushed the feature/cake-day branch from d8c7206 to 20886ad Compare March 20, 2024 10:09

christolis force-pushed the feature/cake-day branch from 20886ad to eab0bdf Compare April 4, 2024 06:51

christolis force-pushed the feature/cake-day branch from eab0bdf to e98eb35 Compare May 17, 2024 19:26

ankitsmt211 added the config-changes if your PR contains any changes related to config file label May 18, 2024

christolis force-pushed the feature/cake-day branch from e98eb35 to 079d68f Compare October 6, 2024 11:33

christolis and others added 11 commits October 6, 2024 15:21

feat(cake-day): implement batch insert cake days routine

206ddf2

feat(cake-day): implement actual routine logic

767ee85

docs: add JavaDocs

401f1e6

refactor: move cake day feature to its own directory

8192fdf

fix: improve role refresh handling and make it work

6aee3ba

refactor: remove unused method

dd479c7

docs: add JavaDocs

c1f7448

fix: include anniversary logic for cake day

4259568

Co-authored-by: TheCodeMr <151576372+TheCodeMr@users.noreply.github.com> Co-authored-by: cab <161495905+cabagbe@users.noreply.github.com> Co-authored-by: Devansh Tiwari <65783463+devloves@users.noreply.github.com>

fix: add Objects#requireNonNull() on cakeDayConfig

ee4d908

feat(config): add proper default value for key

c418ba6

feat(cake-day): improve removeRoleFromMembers() method

20f224a

christolis added 4 commits October 6, 2024 15:21

feat: add CakeDayConfig record constructor

dc3382a

feat: use Optional properly

2f06eb4

refactor: better use of Optional

47b17b8

christolis force-pushed the feature/cake-day branch from 079d68f to 47b17b8 Compare October 6, 2024 12:21

Zabuzard added the priority: normal label Jul 1, 2025

SquidXTV requested changes Jul 1, 2025

View reviewed changes

Uh oh!

Feature: Cake Day #1042

Are you sure you want to change the base?

Feature: Cake Day #1042

Conversation

christolis commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Configuration changes

Diagram

TODO

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ankitsmt211 commented Mar 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

christolis commented Mar 3, 2024 •

edited

Loading