Feat/new db format#80
Conversation
…add success message
There was a problem hiding this comment.
Pull request overview
This PR introduces a new database schema for whitelist/social connections, adds a legacy-to-new migration helper, and extends Discord-side tooling (commands/modals/permissions) to support creating and editing whitelist entries under the new model.
Changes:
- Split the legacy
social_connectionsstorage intosocial_connections_new(connections) +freebuild_whitelists(whitelist state), plus anAuditableLongIdTablebase for audit columns. - Add a console command and migration helper to convert legacy rows into the new tables.
- Add a new
/wl-createcommand + permission, and improve whitelist edit modal flows/messages; minor ticket UX text changes.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/messages.properties | Adds new whitelist-related translations and tweaks delete modal title text. |
| src/main/kotlin/dev/slne/surf/discord/ticket/listener/TicketLeaveListener.kt | Adjusts the “member left thread” behavior to send an embed-based message. |
| src/main/kotlin/dev/slne/surf/discord/ticket/database/whitelist/SocialsTable.kt | Replaces legacy table with SocialConnectionsTable + FreebuildWhitelistTable. |
| src/main/kotlin/dev/slne/surf/discord/ticket/database/whitelist/SocialsMigration.kt | Adds an idempotent helper to migrate legacy social_connections rows into the new tables. |
| src/main/kotlin/dev/slne/surf/discord/ticket/database/whitelist/SocialService.kt | Splits update operations (blocked/minecraft/discord) and adds Discord role transfer logic. |
| src/main/kotlin/dev/slne/surf/discord/ticket/database/whitelist/SocialRepository.kt | Rewrites whitelist persistence/query logic to use the new tables. |
| src/main/kotlin/dev/slne/surf/discord/ticket/database/util/AuditableLongIdTable.kt | Introduces a reusable created_at/updated_at base table. |
| src/main/kotlin/dev/slne/surf/discord/ticket/command/whitelist/CreateWhitelistCommand.kt | Adds a new slash command to create whitelist entries and assign the whitelisted role. |
| src/main/kotlin/dev/slne/surf/discord/ticket/TicketType.kt | Adds a new ticket close reason (“Externer Fehler”). |
| src/main/kotlin/dev/slne/surf/discord/permission/permission-util.kt | Grants the new whitelist-create permission to relevant roles; reorganizes the Twitch Mod block. |
| src/main/kotlin/dev/slne/surf/discord/permission/DiscordPermission.kt | Adds WHITELIST_CREATE. |
| src/main/kotlin/dev/slne/surf/discord/interaction/modal/impl/ticket/whitelist/SurvivalWhitelistEditModal.kt | Enhances edit modal to update blocked/minecraft/discord independently with better validation/messages. |
| src/main/kotlin/dev/slne/surf/discord/config/DatabaseConfig.kt | Registers the new tables for schema creation. |
| src/main/kotlin/dev/slne/surf/discord/command/console/impl/SocialsMigrationCommand.kt | Adds a console command to run the legacy migration. |
| gradle.properties | Bumps snapshot version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| jda.guilds.forEach { guild -> | ||
| val role = guild.getRoleById(botConfig.whitelistedRoleId) | ||
| ?: return@runCatching false | ||
|
|
||
| guild.removeRoleFromMember(oldDiscordUser, role).queue() | ||
| guild.addRoleToMember(newDiscordUser, role).queue() | ||
| } | ||
|
|
||
| socialRepository.editDiscordId(oldDiscordId, discordId) | ||
| true |
There was a problem hiding this comment.
updateDiscordId queues role removals/additions and immediately updates the DB/returns true. Any REST errors from queue() won't be caught by runCatching, and a single guild missing the role triggers return@runCatching false (aborting the whole update, possibly after partial changes in earlier guilds). Consider skipping guilds without the role, awaiting the role updates (or handling failures via callbacks), and only updating the DB once the role operations have succeeded.
| val whitelist = socialService.whitelist(userId, minecraftName) | ||
|
|
||
| val discordUser = event.jda.retrieveUserById(userId).await() | ||
|
|
||
| jda.guilds.forEach { | ||
| val role = it.getRoleById(botConfig.whitelistedRoleId) ?: return@forEach | ||
| it.addRoleToMember(discordUser, role).queue() | ||
| } | ||
|
|
||
| event.reply( | ||
| translatable( | ||
| "whitelist.command.create.success", | ||
| "<@${whitelist?.discordId}>", | ||
| whitelist?.getMinecraftName() ?: whitelist?.minecraftUuid.toString() | ||
| ) | ||
| ) |
There was a problem hiding this comment.
socialService.whitelist(...) can return null when the Minecraft name can't be resolved. The command continues anyway, retrieves the Discord user, assigns roles, and replies with placeholders derived from nullable whitelist (e.g. <@null> / null). Add an early null-check and reply with an error message before performing role updates.
| // Ensure connection exists (insert if missing) | ||
| val existingConnectionId = SocialConnectionsTable.selectAll() | ||
| .where(SocialConnectionsTable.minecraftUuid eq minecraftUuid) | ||
| .map { it[SocialConnectionsTable.id].value } | ||
| .firstOrNull() | ||
|
|
||
| val connectionId = if (existingConnectionId == null) { | ||
| SocialConnectionsTable.insert { | ||
| it[this.discordUserId] = discordId | ||
| it[this.minecraftUuid] = minecraftUuid | ||
| it[this.createdAt] = now | ||
| it[this.updatedAt] = now | ||
| } | ||
|
|
||
| // read back id by unique minecraft uuid | ||
| SocialConnectionsTable.selectAll() | ||
| .where(SocialConnectionsTable.minecraftUuid eq minecraftUuid) | ||
| .map { it[SocialConnectionsTable.id].value } | ||
| .first() | ||
| } else existingConnectionId |
There was a problem hiding this comment.
In whitelist(...), when a connection already exists for the minecraftUuid, the code reuses that connection but never updates SocialConnectionsTable.discordUserId to the passed discordId. This can leave the DB pointing at the old Discord user while the returned SocialEntry uses the new one. Also, the insert path does a separate insert then re-selects the row; consider using insertReturning (and/or checking for an existing connection by discordUserId) to avoid race conditions and unique-constraint failures.
| if (existingConnectionId != null) { | ||
| syncWhitelistEntry(existingConnectionId, blocked, createdAt, updatedAt) | ||
| printProgress(current, total) | ||
| continue | ||
| } |
There was a problem hiding this comment.
When an existing SocialConnectionsTable row is found for the minecraftUuid, the migration only syncs the whitelist entry. Any differences in discord_user_id / twitch_id between legacy and new tables will remain unsynchronized. If the migration is intended to fully mirror legacy data, consider updating those fields as well for existing connections (or documenting why they should be left unchanged).
| textInput { | ||
| id = "whitelist:modal:edit-survival:old-discord-id" | ||
| label = "Interne Verwaltungs-ID (nicht ändern!)" | ||
| value = data[2] | ||
| required = true | ||
| } |
There was a problem hiding this comment.
old-discord-id is used as the record selector for updates, but it's a normal editable text input. Despite the label warning, users can accidentally change it and end up editing a different whitelist entry than intended. Consider not taking this value from user input (e.g., keep the original ID in server-side state / modal custom-id payload), or at least validating it against the originally opened entry before applying changes.
| socialRepository.editMinecraftName(discordId, it) | ||
| } != null |
There was a problem hiding this comment.
updateMinecraftName will currently return true for any resolvable Minecraft name, even if the DB update affected 0 rows. editMinecraftName(...) returns an Int update count (never null), so the != null check is ineffective. Consider returning socialRepository.editMinecraftName(...) > 0 (and/or handling the UUID lookup failure explicitly).
| socialRepository.editMinecraftName(discordId, it) | |
| } != null | |
| socialRepository.editMinecraftName(discordId, it) > 0 | |
| } ?: false |
| val blocked = FreebuildWhitelistTable.selectAll() | ||
| .where(FreebuildWhitelistTable.socialConnectionId eq connectionId) | ||
| .map { it[FreebuildWhitelistTable.blocked] } | ||
| .firstOrNull() ?: false | ||
|
|
||
| SocialEntry( | ||
| discordId = connectionRow[SocialConnectionsTable.discordUserId]!!, | ||
| minecraftUuid = connectionRow[SocialConnectionsTable.minecraftUuid], | ||
| blocked = blocked, | ||
| createdAt = connectionRow[SocialConnectionsTable.createdAt], | ||
| updatedAt = connectionRow[SocialConnectionsTable.updatedAt] | ||
| ) |
There was a problem hiding this comment.
getWhitelist(discordId) treats a missing row in FreebuildWhitelistTable as blocked = false (?: false) and still returns a SocialEntry. That means "no whitelist entry" is interpreted as "whitelisted and not blocked", which disagrees with isWhitelisted(...) (it returns false when the whitelist row is missing). Consider returning null when there is no corresponding FreebuildWhitelistTable row (or enforcing that it always exists).
| val blocked = FreebuildWhitelistTable.selectAll() | ||
| .where(FreebuildWhitelistTable.socialConnectionId eq connectionId) | ||
| .map { it[FreebuildWhitelistTable.blocked] } | ||
| .firstOrNull() ?: false | ||
|
|
||
| SocialEntry( | ||
| discordId = connectionRow[SocialConnectionsTable.discordUserId] | ||
| ?: return@suspendTransaction null, | ||
| minecraftUuid = connectionRow[SocialConnectionsTable.minecraftUuid], | ||
| blocked = blocked, | ||
| createdAt = connectionRow[SocialConnectionsTable.createdAt], | ||
| updatedAt = connectionRow[SocialConnectionsTable.updatedAt] | ||
| ) |
There was a problem hiding this comment.
Same issue as above for getWhitelist(minecraftUuid): if there is no FreebuildWhitelistTable row, blocked defaults to false and a SocialEntry is returned, effectively treating the user as whitelisted. Consider returning null when the whitelist row is absent (or creating it).
| suspend fun migrateLegacy() = suspendTransaction { | ||
| val rows = LegacySocialsTable.selectAll().toList() | ||
| val total = rows.size | ||
|
|
There was a problem hiding this comment.
The migration loads the entire legacy table into memory via selectAll().toList(). For larger datasets this can cause high memory usage and long transactions. Consider streaming/iterating rows (or batching with LIMIT/OFFSET / primary-key ranges) and committing periodically, while still reporting progress.
No description provided.