Notify the user when they completely missed a ban due to inactivity #465
Conversation
WalkthroughDetects global bans created after a user's last login and surfaces them as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginService
participant LoginLogRepo as LoginLogRepository
participant BanRepo as BanRepository
participant Hydra
rect rgb(245,250,245)
User->>LoginService: attemptLogin(credentials)
LoginService->>LoginLogRepo: findLastLoginTime(user.id?)
LoginService->>BanRepo: findActiveGlobalBan(user)
alt Active global ban found
BanRepo-->>LoginService: active Ban
LoginService-->>User: LoginResult.ActiveGlobalBan
else No active ban
LoginService->>BanRepo: findMissedGlobalBan(user, lastLogin)
alt Missed ban found
BanRepo-->>LoginService: missed Ban (createTime, expiresAt, reason)
LoginService-->>User: LoginResult.MissedBan(reason, startTime, endTime)
note right of Hydra: MissedBan treated as recoverable failure
LoginService->>Hydra: map to failed/recoverable response
else No missed ban
LoginService-->>User: LoginResult.Success
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/resources/i18n/messages.properties (1)
56-57: Let translations own the full time range sentenceRight now the intro text ends in a bare “to”, with the end time appended in code. That makes it hard for other locales to reorder or format the sentence.
Consider letting the property take both start and end times:
-ban.missed.intro=You have been banned from FAForever from {0} to +ban.missed.intro=You have been banned from FAForever from {0} to {1}and pass both values from the UI. This keeps the full sentence inside the translation and improves i18n flexibility.
src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt (1)
170-175: Centralize ban time formatting and keep i18n flexibleThe MissedBan message is partly constructed in code and partly in translations, and relies on
toString()for times:
ban.missed.introgets onlystartTime;endTimeis concatenated in Kotlin.startTime/endTimeare rendered via defaulttoString(), unlike the explicit formatter used in the banned case.To improve consistency and i18n, consider:
- is LoginResult.MissedBan -> { - val intro = "${getTranslation("ban.missed.intro", loginError.startTime)} ${loginError.endTime}" + is LoginResult.MissedBan -> { + val intro = getTranslation( + "ban.missed.intro", + loginError.startTime, + loginError.endTime, + ) val reason = "${getTranslation("ban.reason")} ${loginError.reason}" val explanation = getTranslation("ban.missed") "$intro $reason\n$explanation" }paired with the updated
ban.missed.introproperty (from {0} to {1}as suggested in messages.properties). You can also optionally format the times withDateTimeFormatterfor consistency with the existing banned message.src/main/kotlin/com/faforever/userservice/backend/domain/Ban.kt (1)
13-15: BancreateTimeand nullable lookup look appropriateAdding
createTimeand exposing it viaBanRepository.findGlobalBansByPlayerId(playerId: Int?)fits the MissedBan use case and keeps call sites simple by returning an empty list fornullids. If you like, you could useemptyList()instead oflistOf()for clarity, but the current behavior is fine.Also applies to: 44-45, 57-60
src/test/kotlin/com/faforever/userservice/backend/domain/LoginServiceTest.kt (1)
88-89: Tests cover key MissedBan scenarios and updated Ban constructorThe updated
Baninstantiations withcreateTimeand the two new tests (loginWithPreviouslyBannedUser,loginWithPreviouslyBannedUserAfterAcknowledgement) accurately express the new behavior: detect a recently expired ban when the last login predates it, and allow login after a post-ban login.If you want the tests to be maximally deterministic, you could compute a single
val now = LocalDateTime.now()/OffsetDateTime.now()per test and derive all offsets from that, but the current use of hours/days offsets is acceptable.Also applies to: 102-103, 153-189, 191-216
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt(3 hunks)src/main/kotlin/com/faforever/userservice/backend/domain/Ban.kt(3 hunks)src/main/kotlin/com/faforever/userservice/backend/domain/LoginLog.kt(1 hunks)src/main/kotlin/com/faforever/userservice/backend/hydra/HydraService.kt(1 hunks)src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt(1 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/test/kotlin/com/faforever/userservice/backend/domain/LoginServiceTest.kt(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt (1)
src/main/kotlin/com/faforever/userservice/backend/i18n/I18n.kt (1)
getTranslation(28-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
src/main/kotlin/com/faforever/userservice/backend/hydra/HydraService.kt (1)
74-78: MissedBan mapping fits existing failure handlingMapping
LoginResult.MissedBantoLoginResponse.FailedLoginkeeps it aligned with other recoverable login failures and allows the UI to surface the message without changing the Hydra error flow. Looks good.
| data class MissedBan( | ||
| val reason: String, | ||
| val startTime: LocalDateTime, | ||
| val endTime: OffsetDateTime, | ||
| ) : RecoverableLoginFailure |
There was a problem hiding this comment.
MissedBan logic can silently fail due to ordering and log timing
The core MissedBan behavior has two structural problems:
- Ban selection is not guaranteed to pick the most recent relevant ban
val lastRelevantBan = banRepository.findGlobalBansByPlayerId(user.id)
.firstOrNull { ... } ?: return nullSince the repository list has no explicit ordering, firstOrNull may return an older ban and ignore a more recent one that also matches the criteria. In multi-ban histories this can cause you to miss the ban the user actually “missed”.
- Current login can be treated as the “last login”
You call logLogin before computing MissedBan:
logLogin(usernameOrEmail, user, ip)
val activeGlobalBan = findActiveGlobalBan(user)
...
val missedGlobalBan = findMissedGlobalBan(user)findMissedGlobalBan internally calls loginLogRepository.findLastLoginTime(user.id). With typical JPA behavior, the just-persisted LoginLog for this attempt may already be visible to that query, so lastLogin can equal “now”. That makes lastLogin.isBefore(lastRelevantBan.createTime) false and prevents MissedBan from ever being returned, even when the user truly missed a ban between logins.
To align the implementation with the intended behavior (“since their last login”), I recommend:
- Compute the previous last-login timestamp before logging the current attempt.
- Pass that value into
findMissedGlobalBan. - Within
findMissedGlobalBan, pick the newest relevant ban, not the first from the list.
For example:
@@
- logLogin(usernameOrEmail, user, ip)
-
- val activeGlobalBan = findActiveGlobalBan(user)
+ val lastLogin = loginLogRepository.findLastLoginTime(user.id)
+
+ logLogin(usernameOrEmail, user, ip)
+
+ val activeGlobalBan = findActiveGlobalBan(user)
@@
- val missedGlobalBan = findMissedGlobalBan(user)
+ val missedGlobalBan = findMissedGlobalBan(user, lastLogin)
@@
- private fun findActiveGlobalBan(user: User): Ban? =
- banRepository.findGlobalBansByPlayerId(user.id)
- .firstOrNull { it.isActive }
+ private fun findActiveGlobalBan(user: User): Ban? =
+ banRepository.findGlobalBansByPlayerId(user.id)
+ .firstOrNull { it.isActive }
@@
- private fun findMissedGlobalBan(user: User): Ban? {
- val lastRelevantBan = banRepository.findGlobalBansByPlayerId(user.id)
- .firstOrNull { it.revokeTime == null && it.expiresAt != null &&
- it.expiresAt!!.isAfter(OffsetDateTime.now().minusDays(90))} ?: return null
-
- val lastLogin = loginLogRepository.findLastLoginTime(user.id)
- return if (lastLogin == null || lastLogin.isBefore(lastRelevantBan.createTime))
- lastRelevantBan else null
- }
+ private fun findMissedGlobalBan(user: User, lastLogin: LocalDateTime?): Ban? {
+ val lastRelevantBan = banRepository.findGlobalBansByPlayerId(user.id)
+ .filter { ban ->
+ ban.revokeTime == null &&
+ ban.expiresAt != null &&
+ ban.expiresAt!!.isAfter(OffsetDateTime.now().minusDays(90))
+ }
+ .maxByOrNull { it.createTime }
+ ?: return null
+
+ return if (lastLogin == null || lastLogin.isBefore(lastRelevantBan.createTime)) {
+ lastRelevantBan
+ } else {
+ null
+ }
+ }This keeps logging behavior intact while ensuring:
- You compare against the previous login time.
- You consider the most recent non-revoked, expiring ban in the last 90 days when deciding whether to return
LoginResult.MissedBan.
Also applies to: 100-112, 130-133, 134-142
🤖 Prompt for AI Agents
In src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt
around lines 42-46 (and similarly at ranges 100-112, 130-133, 134-142), the
MissedBan logic can silently fail because you call logLogin before reading the
previous last-login and you pick the first ban from an unordered repository
result; fix by reading and storing the previous lastLogin
(loginLogRepository.findLastLoginTime(user.id)) before calling logLogin, pass
that previousLastLogin into findMissedGlobalBan, and change findMissedGlobalBan
to select the most recent relevant ban (e.g. sort by createTime or use
maxBy/createTime) and then compare that ban.createTime against the
previousLastLogin and ban revocation/expiry rules; apply the same change
wherever missed-ban detection occurs so the comparison uses the prior login
timestamp and the newest applicable ban.
src/main/kotlin/com/faforever/userservice/backend/domain/LoginLog.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt (1)
176-188: FixendTimeformatting to useloginError.endTimeinstead ofloginError.startTimeLine 182 incorrectly formats both
startTimeandendTimefromloginError.startTime, causing the ban end time to display the same as the start time. TheLoginResult.MissedBanclass has bothstartTimeandendTimeproperties, so the fix is:- val endTime = loginError.startTime.format(dateTimeFormatter) + val endTime = loginError.endTime.format(dateTimeFormatter)
♻️ Duplicate comments (1)
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt (1)
135-151: MissedBan still depends on unordered ban list; newest relevant ban is not guaranteed
findMissedGlobalBancurrently picks the first matching ban fromfindGlobalBansByPlayerId(user.id):val lastRelevantBan = banRepository.findGlobalBansByPlayerId(user.id) .firstOrNull { it.revokeTime == null && it.expiresAt != null && it.expiresAt!!.isAfter(OffsetDateTime.now().minusDays(90)) } ?: return nullBecause the repository query has no explicit ordering, the “first” ban is effectively undefined and may not be the most recent relevant ban. In a history with multiple bans matching these criteria, you can end up informing the user about an older one instead of the latest, or missing a newer one entirely.
To align with “last relevant ban” semantics, consider:
- Selecting the newest matching ban by
createTime, e.g.:val lastRelevantBan = banRepository.findGlobalBansByPlayerId(user.id) .filter { ban -> ban.revokeTime == null && ban.expiresAt != null && ban.expiresAt!!.isAfter(OffsetDateTime.now().minusDays(90)) } .maxByOrNull { it.createTime } ?: return nullThe subsequent comparison against
lastLogincan remain as-is.
🧹 Nitpick comments (2)
src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt (1)
63-73: Locale-dependent formatter is nice, but consider null-safety ofUI.getCurrent()Using a localized
DateTimeFormatterandwhite-space: pre-lineis a good fit for multi-line MissedBan / UserBanned messages.However,
dateTimeFormatteris initialized withUI.getCurrent().localeat field init time. If this view were ever constructed off the UI thread (e.g. during certain tests or background usage),UI.getCurrent()could be null and cause an NPE.If that scenario is possible in your stack, consider lazily resolving the formatter when needed, with a safe fallback:
private fun uiDateTimeFormatter(): DateTimeFormatter { val locale = UI.getCurrent()?.locale ?: Locale.getDefault() return DateTimeFormatterBuilder() .append(DateTimeFormatter.ofLocalizedDate(FormatStyle.LONG)) .appendLiteral(" ") .append(DateTimeFormatter.ofLocalizedTime(FormatStyle.SHORT)) .toFormatter(locale) }Then call
uiDateTimeFormatter()where you format timestamps.Also applies to: 75-80, 209-217
src/main/kotlin/com/faforever/userservice/backend/domain/Ban.kt (1)
43-45: BancreateTimeand nullable player ID handling look good (minor type nit)Adding a non-null
createTimeand makingfindGlobalBansByPlayerIdgracefully handlenullIDs fits the MissedBan logic and avoids repeated null checks in callers.There is a small asymmetry where
Ban.playerIdisLongbut the repository method parameter isInt?. It works via implicit widening, but if you ever touch this area again it may be worth aligning these types (e.g.,Long?) for clarity and to avoid subtle mapping surprises.Also applies to: 56-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/themes/faforever/styles.css(0 hunks)src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt(3 hunks)src/main/kotlin/com/faforever/userservice/backend/domain/Ban.kt(2 hunks)src/main/kotlin/com/faforever/userservice/backend/domain/LoginLog.kt(1 hunks)src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt(5 hunks)src/main/resources/i18n/messages.properties(1 hunks)src/test/kotlin/com/faforever/userservice/backend/domain/LoginServiceTest.kt(4 hunks)
💤 Files with no reviewable changes (1)
- frontend/themes/faforever/styles.css
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/i18n/messages.properties
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt (1)
src/main/kotlin/com/faforever/userservice/backend/i18n/I18n.kt (1)
getTranslation(28-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
src/main/kotlin/com/faforever/userservice/backend/domain/LoginLog.kt (1)
41-46:findLastLoginTimesemantics now match its nameThe repository now explicitly orders by
createTime descand handlesnulluser IDs up front, so callers reliably get the most recent login attempt ornull.This aligns well with MissedBan’s “since last login” semantics.
src/test/kotlin/com/faforever/userservice/backend/domain/LoginServiceTest.kt (1)
88-101: Tests correctly cover MissedBan and post-acknowledgement flowsThe updated Ban constructors and the two new scenarios:
loginWithPreviouslyBannedUser(lastLogin before ban.createTime →MissedBan)loginWithPreviouslyBannedUserAfterAcknowledgement(lastLogin after ban.createTime →SuccessfulLogin)accurately reflect the intended timing behavior and guard against regressions in MissedBan detection. Using
now().minus…with hour/day offsets should be robust enough in practice.Also applies to: 114-127, 162-186, 188-240
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt (2)
38-47: MissedBan result shape looks appropriate for UI messagingDefining
LoginResult.MissedBanwithreason,startTime, and non-nullendTimematches the UI’s need to show a time-bounded ban the user never saw. Requiring a non-nullendTimeis consistent with howfindMissedGlobalBanfilters only bans withexpiresAt != null.
86-101: Good: last-login is read before logging the current attemptMoving
val lastLogin = loginLogRepository.findLastLoginTime(user.id)beforelogLogin(...)ensures the MissedBan comparison uses the previous login attempt and not the one being processed now. That fixes the earlier risk where the current attempt could be treated as “last login” and prevent MissedBan from ever triggering.The subsequent MissedBan creation correctly passes
reason,createTime, andexpiresAt!!from the selected Ban.Also applies to: 107-116
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt (1)
110-114: Consider avoiding non-null assertion onexpiresAt.Line 113 uses
missedGlobalBan.expiresAt!!, which is safe given the filter on line 142 checksexpiresAt != null. However, Kotlin's smart cast should eliminate the need for!!if the type system recognizes the null check.If smart cast doesn't work due to mutability concerns, consider using
let:- return LoginResult.MissedBan( - missedGlobalBan.reason, - missedGlobalBan.createTime, - missedGlobalBan.expiresAt!!, - ) + missedGlobalBan.expiresAt?.let { expiresAt -> + return LoginResult.MissedBan( + missedGlobalBan.reason, + missedGlobalBan.createTime, + expiresAt, + ) + }This is defensive but makes the null safety explicit without assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt(3 hunks)src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/com/faforever/userservice/ui/view/oauth2/LoginView.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt (2)
42-46: LGTM! Well-structured data class.The
MissedBandata class appropriately captures the essential information needed to inform users about a ban they missed during inactivity.
98-98: Good fix - addresses timing concern from previous review.Retrieving
lastLoginbefore logging the current attempt ensures the missed-ban detection compares against the user's previous login timestamp rather than the current one.
| val missedGlobalBan = findMissedGlobalBan(user, lastLogin ?: LocalDateTime.now().minusDays(90)) | ||
| if (missedGlobalBan != null) { | ||
| LOG.debug("User '{}' missed a ban {} and needs to be informed about it", usernameOrEmail, missedGlobalBan) | ||
| return LoginResult.MissedBan( | ||
| missedGlobalBan.reason, | ||
| missedGlobalBan.createTime, | ||
| missedGlobalBan.expiresAt!!, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missed-ban selection needs ordering and recency constraints.
The current implementation has three issues:
-
firstOrNulldoesn't guarantee the most recent ban (line 141)banRepository.findGlobalBansByPlayerId()returns an unordered list- When multiple bans match, you may surface an older, less relevant one
-
No explicit check that the ban has expired
- The logic relies on
findActiveGlobalBancatching active bans first - Adding an explicit
expiresAt < nowcheck makes intent clearer and prevents edge cases
- The logic relies on
-
No recency window for expired bans (line 142-143)
- A ban that expired years ago could still be shown
- The 90-day fallback on line 107 only applies when
lastLoginis null
Apply this diff to select the most recent relevant ban and enforce a 90-day recency window:
- private fun findMissedGlobalBan(user: User, lastLogin: LocalDateTime): Ban? {
- return banRepository.findGlobalBansByPlayerId(user.id)
- .firstOrNull {
- it.revokeTime == null && it.expiresAt != null && it.createTime.toLocalDateTime().isAfter(lastLogin)
- }
- }
+ private fun findMissedGlobalBan(user: User, lastLogin: LocalDateTime): Ban? {
+ val now = OffsetDateTime.now()
+ return banRepository.findGlobalBansByPlayerId(user.id)
+ .filter {
+ it.revokeTime == null &&
+ it.expiresAt != null &&
+ it.expiresAt.isBefore(now) &&
+ it.expiresAt.isAfter(now.minusDays(90)) &&
+ it.createTime.toLocalDateTime().isAfter(lastLogin)
+ }
+ .maxByOrNull { it.createTime }
+ }This ensures:
- You pick the newest qualifying ban (via
maxByOrNull) - The ban has actually expired (
expiresAt < now) - The ban expired within the last 90 days (still relevant)
- The ban was created after the user's last login (they missed it)
Based on learnings
Also applies to: 139-144
🤖 Prompt for AI Agents
In src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt
around lines 107-115 (and similarly 139-144), the missed-ban selection must be
tightened: replace the current firstOrNull selection with filtering to only bans
that have an expiresAt timestamp before now, whose expiresAt is within 90 days
of now, and whose createTime is after the user's lastLogin (use lastLogin ?:
now.minusDays(90) as the cutoff), then pick the most recent qualifying ban with
maxByOrNull on createTime; return that ban as the MissedBan. Ensure you use a
consistent now = LocalDateTime.now() and null-safety for expiresAt when applying
the comparisons so only truly expired, recent, and post-login bans are
considered.
src/main/kotlin/com/faforever/userservice/backend/account/LoginService.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/kotlin/com/faforever/userservice/backend/domain/LoginServiceTest.kt (3)
88-100: Ban test fixtures updated correctly; consider a shared builder/helperThe added
OffsetDateTime.now()argument forcreateTimein these Ban instances looks correct and keeps existing semantics for “currently banned” and “perma banned” users. However, the long positional argument list is now repeated across multiple tests and will become brittle ifBangains more fields again.Consider extracting small helper/builders (e.g.,
activeGlobalBan(...),permaGlobalBan(...)) or using named factory functions so tests remain readable and resilient to future Ban shape changes.Also applies to: 114-126
167-186:loginWithUnbannedUsereffectively models a revoked ban; optional naming/fixture tweakThis test correctly exercises the case where a global ban exists in history but is no longer active (revoked one day ago) and verifies that login succeeds without hitting
MissedBan.If you want to make the intent clearer for future readers, consider:
- Renaming the test to something like
loginWithRevokedGlobalBan(or similar), and/or- Introducing a small helper like
revokedBan()to distinguish “revoked” from “expired” bans at the call site.Purely cosmetic, the current test is logically sound.
188-213: Missed-ban behavior is well covered across time-based scenarios; minor suggestions on magic durationsThe four new tests nicely cover the main MissedBan matrix:
loginAfterMissedBanExpired: user’s last login is beforecreateTime, ban expired recently →LoginResult.MissedBan.loginAfterKnownBanExpired: last login occurred while the ban was active → normalSuccessfulLoginafter expiry.loginAfterRecentBanExpiredWithFailedLoginLog: no login logs but a recent expired global ban →MissedBan.loginAfterOldBanExpiredWithFailedLoginLog: no login logs and a very old expired ban (99/100 days) →SuccessfulLogin.This aligns well with the PR’s intent to only surface bans that were plausibly “missed” and still relevant.
Two optional maintainability tweaks you might consider:
Avoid hard-coded “old vs recent” cutoffs in tests
If
LoginServiceuses a specific constant/Duration to decide what counts as an “old” missed ban, it may be worth exposing that (or a derived value) so tests don’t rely on implicit numbers like 99/100 days. That keeps tests in sync if the threshold changes.Document the time relationships
A brief comment in each test summarizing the intended timeline (e.g., “ban created 1 day ago, expired 1 hour ago, last login 2 days ago → user never saw it”) would help future maintainers quickly validate the scenarios without re-deriving the timeline from the offsets.
Functionally these tests look solid; these are just minor readability/maintenance improvements.
Also applies to: 216-240, 243-267, 270-294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/kotlin/com/faforever/userservice/backend/domain/LoginServiceTest.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
1 participant
Short bans can lead to users not even noticing the ban, because it has expired before they log in again. This is bad because it is still important that players get feedback when they broke the rules. It also limits moderators in the minimum ban length they can reasonably apply.
When we notify users that there is a now expired ban, we can close the information gap. This way we also enable moderators to reliably message players that should receive a warning, by issuing a short-duration ban.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
Tests