Adding /remind command

[Zabuzard]

Overview

Originally from #354 , taken over. Implements and closes #325 .

This adds the /remind command which can be used to automatically send reminders to oneself at a future date.

Reminders are sent in the channel the command has been used in (or via DM if something goes wrong).

Details

Technically, reminders are simple saved in a database table, where an automatic routine will pick them up at time, send them and delete them from the table.

Database table

The table is quite straightforward. It has some metadata, data to be able to locate the channel to post to and the actual content and time to fire the reminder.

CREATE TABLE pending_reminders
(
    id         INTEGER   NOT NULL PRIMARY KEY AUTOINCREMENT,
    created_at TIMESTAMP NOT NULL,
    guild_id   BIGINT    NOT NULL,
    channel_id BIGINT    NOT NULL,
    author_id  BIGINT    NOT NULL,
    remind_at  TIMESTAMP NOT NULL,
    content    TEXT      NOT NULL
)

RemindCommand UX

The user can enter a time period as amount and unit, which determine when the reminder will be send.

• amount - an integer between 1 and 1_000
• unit - dropdown of "minutes", "hours", "days", "weeks", "months", "years"

After using the command, it verifies that the time is within reasonable limits (max 3 years) and if the user has not reached the limit of pending reminders per user (100). If not, an error message is send.

Those restrictions are mostly to prevent the database and hence our disk space from overflowing when users are spamming around.

RemindRoutine

The routine is quite trivial. It implements Routine, with a schedule of 30 seconds. In each run, it gets hands on all pending reminders that are due, sends them and deletes them from the table afterwards.

To send them, it locates the corresponding guild and channel, retrieves the original author and builds a slim embed.

Error cases

If the guild and channel could be found, but the author couldnt be retrieved anymore, it sends the message with Unknown User.

If the guild or channel cant be found, the bot attempts to send it via DM to the author instead.

If sending the DM failed as well (user deleted, user disabled DMs, ...), the bot sends a WARN log message:

WARN RemindRoutine - Failed to send a reminder (id '19'), skipping it. This can be due to a network issue, but also happen if the bot disconnected from the target guild and the user has disabled DMs or has been deleted.

Future plans

The command would greatly benefit from JDA/Discord adding a date/time selector. It could then be split into /remind in and /remind at. A // TODO keeps track of this.

Unfortunately, it is not really possible to add /remind at (eventhough the use case is very reasonable) as of today without having hands on the users timezone.

Checklist

• Store reminders
• Send & Delete reminders
• Mention the original user
• Limit period to 3 years
• Limit reminders per user
• Send user DM if unknown guild/channel

[Tais993]

Silently ignoring the reminder shouldn't happen in my honest opinion.

If the channel/guild can't be found we should (try to) DM the user instead.

[Zabuzard]

If the channel/guild can't be found we should (try to) DM the user instead.

Good idea. Will do that.

[Zabuzard]

Send user DM if reminder cant be sent due to unknown guild/channel.

[Zabuzard]

If the channel/guild can't be found we should (try to) DM the user instead.

Implemented.

[Tais993]

Ignore this review, check the one below. Keeping for sake of history

I'm not happy with how you ended up handling it.

This ReminderRoute class had me confused for too long, these onErrorMap's into null are unreadable and it resulted in a mess of ternary and logic which should be separated.

Instead, don't map into null resulting in unreadable ternary mess, handle the error separately.

Right now you just throw a IllegalStateException, after adding a lot of logic which makes it severely more complicated, when this can also just be done by handling the error when queueing

and also, if you remove the null mappers and remove the illegal state exception the behaviour stays the same.
The failure consumer runs, but now it's readable with less logic.

I'm sorry, but this isn't gonna cut it for me.

[Tais993]

After convo and some clearing up in the Discord, new suggestions have come. The old ones have been resolved/deleted by me.

[marko-radosavljevic]

RemindRoutine is much more readable than the last time i glanced over it. Routing logic is still somewhat convoluted tho, possibly extracting it to a new class is something that can be done, since it's not really routine's responsibility to figure out correct channels/users.

Will approve since it's not really worth 10h of combined development effort to go back and forth on naming and such, when this is not an integral feature to our core software. ☺️

[marko-radosavljevic]

Oh yeah, how feasible would it be to add a few test cases for this?

[Zabuzard]

Oh yeah, how feasible would it be to add a few test cases for this?

Not for now. We need clock mocking for this - for the database checks to pickup Instant.now() being after the set reminder points. Can be done and I also wanted to do this at a later point in time (other stuff like temp ban or mod audit log routine need this as well).

Fortunately, Java is well equipped for clock mocking. Its just that it requires a change on all time usages in the whole code base. Like, instead of Instant.now(), you would write Instant.now(clock). And then you need to provide a clock at an accessible place that you can mock (similar to Config for example).

The /remind command can act as a good entry point for doing that though. Just like the /tag family was a good opportunity to introduce database and button action mocking.

Edit: To not forget I added #396

[Tais993]

Last one I promise :p

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

[marko-radosavljevic]

I'm happy with the naming now.