Merge pull request 'chore(sonar): wire JaCoCo coverage and clear actionable smells' (#26) from chore/sonar-coverage-and-cleanup into main

Reviewed-on: #26
Reviewed-by: Colby Whitlock <whitlocktech@gmail.com>

.gitea/workflows/sonarqube.yml

@@ -18,11 +18,12 @@
 # SonarQube Quality Gate, so a failing gate does not fail this job — check the
 # dashboard when you want to.
 #
-# Scope: this analyses the Kotlin source directly (the Sonar scanner reads
-# sonar-project.properties). It does NOT run a Gradle build, so no Android SDK /
-# JDK install is needed — the Kotlin analyzer is source-based. See the "Optional
-# enrichment" note in sonar-project.properties for wiring in Android Lint /
-# coverage reports later.
+# Scope: the Sonar scanner reads sonar-project.properties and analyses the Kotlin
+# source directly. Before the scan we run the JVM unit tests + JaCoCo so SonarQube
+# receives real coverage (sonar.coverage.jacoco.xmlReportPaths) — otherwise it
+# reports 0% and the coverage gate fails despite the test suite existing. That
+# Gradle step needs JDK 17 + the Android SDK (same toolchain as pr-checks.yml);
+# the runner container is bare, so base tools are apt-installed first.
 
 name: SonarQube
 
@@ -40,6 +41,16 @@ jobs:
   analysis:
     runs-on: ubuntu-latest
     steps:
+      # The bare runner container lacks git/curl/unzip (checkout + sdkmanager need
+      # them) and we install JDK 17 from the Ubuntu archive rather than
+      # actions/setup-java (this runner can't reach api.adoptium.net). Mirrors
+      # pr-checks.yml — see its header note.
+      - name: Install base tools + JDK 17
+        run: |
+          apt-get update
+          apt-get install -y git curl unzip openjdk-17-jdk-headless
+          echo "JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64" >> "$GITHUB_ENV"
+
       - name: Check out (full history for accurate new-code + blame)
         uses: actions/checkout@v4
         with:
@@ -47,6 +58,31 @@ jobs:
           # compute "new code". A shallow clone degrades both.
           fetch-depth: 0
 
+      - name: Set up Android SDK
+        uses: android-actions/setup-android@v3
+
+      - name: Install Android SDK packages
+        run: |
+          set +o pipefail
+          yes | sdkmanager "platform-tools" "platforms;android-35" "build-tools;35.0.0"
+
+      - name: Cache Gradle
+        uses: actions/cache@v4
+        with:
+          path: |
+            ~/.gradle/caches
+            ~/.gradle/wrapper
+          key: gradle-${{ runner.os }}-${{ hashFiles('**/*.gradle.kts', 'gradle/libs.versions.toml', 'gradle/wrapper/gradle-wrapper.properties') }}
+          restore-keys: |
+            gradle-${{ runner.os }}-
+
+      # Produce the JaCoCo XML the scan reports as coverage. Scoped to the debug
+      # variant (matches enableUnitTestCoverage) to keep peak memory down.
+      - name: Unit tests + JaCoCo coverage
+        run: |
+          chmod +x ./gradlew
+          ./gradlew --no-daemon testDebugUnitTest jacocoTestReport
+
       - name: Run SonarQube scan
         uses: sonarsource/sonarqube-scan-action@v4
         env:

app/build.gradle.kts

@@ -10,6 +10,11 @@ plugins {
     alias(libs.plugins.kotlin.serialization)
     alias(libs.plugins.ksp)
     alias(libs.plugins.hilt)
+    jacoco
+}
+
+jacoco {
+    toolVersion = "0.8.12"
 }
 
 // Release signing material (PLAN.md §12) is never committed. It is read from, in
@@ -78,6 +83,11 @@ android {
     }
 
     buildTypes {
+        debug {
+            // Produce a JaCoCo .exec from JVM unit tests so SonarQube receives real
+            // coverage (§12.1). Debug-only: the scan analyses the debug variant.
+            enableUnitTestCoverage = true
+        }
         release {
             // R8 full-mode minify + resource shrink (§7: no offline cache, so a lean
             // release APK). Keep rules live in proguard-rules.pro.
@@ -170,3 +180,35 @@ dependencies {
     androidTestImplementation(platform(libs.androidx.compose.bom))
     androidTestImplementation(libs.androidx.compose.ui.test.junit4)
 }
+
+// JaCoCo XML coverage from the JVM unit tests, consumed by SonarQube (§12.1). Generated,
+// DI (Hilt), and Compose-scaffold classes are excluded so they don't dilute the number;
+// pure-@Composable UI is excluded on the Sonar side (sonar.coverage.exclusions) because
+// JVM unit tests can't execute composable bodies without Robolectric.
+tasks.register<JacocoReport>("jacocoTestReport") {
+    dependsOn("testDebugUnitTest")
+    group = "verification"
+    description = "Generates JaCoCo XML/HTML coverage for the debug unit tests."
+
+    reports {
+        xml.required.set(true)
+        html.required.set(true)
+    }
+
+    val coverageExcludes = listOf(
+        "**/R.class", "**/R$*.class", "**/BuildConfig.*", "**/Manifest*.*",
+        "**/*_Hilt*.*", "**/Hilt_*.*", "**/*_Factory*.*", "**/*_MembersInjector*.*",
+        "**/*_Impl*.*", "**/di/**", "**/*Module.*", "**/*Module$*.*",
+        "**/*ComposableSingletons*.*", "**/ComposableSingletons$*.*",
+    )
+    val buildDirFile = layout.buildDirectory.get().asFile
+    classDirectories.setFrom(
+        fileTree("$buildDirFile/tmp/kotlin-classes/debug") { exclude(coverageExcludes) },
+    )
+    sourceDirectories.setFrom(files("src/main/java", "src/main/kotlin"))
+    executionData.setFrom(
+        fileTree(buildDirFile) {
+            include("outputs/unit_test_code_coverage/debugUnitTest/testDebugUnitTest.exec")
+        },
+    )
+}

app/src/main/java/com/runicgateway/app/data/api/dto/NotificationsDto.kt

@@ -60,8 +60,15 @@ data class NotificationStreamsDto(
  * `GET/PUT /auth/me/notifications/subscriptions` — the user's opted-in stream ids.
  * PUT replaces the full set; unknown ids are dropped server-side and the stored set
  * echoed back.
+ *
+ * [streams] intentionally has NO default: this DTO doubles as the PUT body, and the
+ * backend validator requires the `streams` field (`body('streams').isArray()`).
+ * kotlinx omits a property equal to its default (encodeDefaults=false), so a default
+ * of `emptyList()` would drop the field when the user clears their LAST subscription,
+ * sending `{}` → 400 "Validation failed" (the "can't turn off the last one" bug). With
+ * no default the empty list always serializes as `{"streams":[]}`. Do not re-add a default.
  */
 @Serializable
 data class NotificationSubscriptionsDto(
-    val streams: List<String> = emptyList(),
+    val streams: List<String>,
 )

app/src/main/java/com/runicgateway/app/ui/admin/AdminContentScreen.kt

@@ -30,7 +30,6 @@ import androidx.compose.runtime.Composable
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableIntStateOf
 import androidx.compose.runtime.mutableStateOf
-import androidx.compose.runtime.remember
 import androidx.compose.runtime.saveable.rememberSaveable
 import androidx.compose.runtime.setValue
 import androidx.compose.ui.Alignment

app/src/main/java/com/runicgateway/app/ui/admin/AdminSupportScreen.kt

@@ -84,7 +84,6 @@ fun AdminSupportScreen(
 
     replyTo?.let { page ->
         RespondDialog(
-            page = page,
             onDismiss = { replyTo = null },
             onSend = { message, close ->
                 viewModel.respond(page.pageId, message, close)
@@ -122,7 +121,6 @@ private fun SupportPageCard(
 
 @Composable
 private fun RespondDialog(
-    page: SupportPageDto,
     onDismiss: () -> Unit,
     onSend: (message: String, close: Boolean) -> Unit,
 ) {

app/src/main/java/com/runicgateway/app/ui/auth/LoginViewModel.kt

@@ -153,18 +153,11 @@ class LoginViewModel @Inject constructor(
     fun submit() {
         val s = _state.value
         if (s.submitting) return
-        if (s.username.isBlank() || s.password.isBlank()) {
-            _state.update { it.copy(error = LoginError.INVALID_CREDENTIALS) }
+        val validationError = validateForSubmit(s)
+        if (validationError != null) {
+            _state.update { it.copy(error = validationError) }
             return
         }
-        // If 2FA is being requested, the chosen second factor must accompany the resubmit.
-        if (s.totpRequired) {
-            val factor = if (s.useRecoveryCode) s.recoveryCode else s.code
-            if (factor.isBlank()) {
-                _state.update { it.copy(error = LoginError.BAD_CODE) }
-                return
-            }
-        }
 
         _state.update { it.copy(submitting = true, error = null) }
         viewModelScope.launch {
@@ -178,36 +171,50 @@ class LoginViewModel @Inject constructor(
                 recoveryCode = recoveryCode,
                 trustDevice = s.trustDevice,
             )
-            when (result) {
-                is LoginResult.Success ->
-                    // The trusted-device cap (result.trustLimitReached) is an edge case:
-                    // login succeeded but the device wasn't remembered. It's surfaced +
-                    // managed on the Trusted Devices screen rather than blocking sign-in.
-                    _state.update { it.copy(submitting = false, signedIn = true) }
-
-                LoginResult.TotpRequired ->
-                    // Reveal the 2FA fields; a wrong code/recovery code re-lands here as BAD_CODE.
-                    _state.update {
-                        val hadFactor = if (it.useRecoveryCode) it.recoveryCode.isNotBlank() else it.code.isNotBlank()
-                        it.copy(
-                            submitting = false,
-                            totpRequired = true,
-                            error = if (hadFactor) LoginError.BAD_CODE else null,
-                        )
-                    }
-
-                LoginResult.InvalidCredentials ->
-                    _state.update { it.copy(submitting = false, error = LoginError.INVALID_CREDENTIALS) }
-
-                LoginResult.RateLimited ->
-                    _state.update { it.copy(submitting = false, error = LoginError.RATE_LIMITED) }
-
-                LoginResult.ServerError ->
-                    _state.update { it.copy(submitting = false, error = LoginError.SERVER) }
-
-                LoginResult.NetworkError ->
-                    _state.update { it.copy(submitting = false, error = LoginError.NETWORK) }
-            }
+            applyLoginResult(result)
         }
     }
+
+    /** Pre-flight form checks for [submit]; returns the error to surface, or null if ready to send. */
+    private fun validateForSubmit(s: UiState): LoginError? {
+        if (s.username.isBlank() || s.password.isBlank()) return LoginError.INVALID_CREDENTIALS
+        // If 2FA is being requested, the chosen second factor must accompany the resubmit.
+        if (s.totpRequired) {
+            val factor = if (s.useRecoveryCode) s.recoveryCode else s.code
+            if (factor.isBlank()) return LoginError.BAD_CODE
+        }
+        return null
+    }
+
+    /** Folds a [LoginResult] back into the UI state (clears [UiState.submitting] on every path). */
+    private fun applyLoginResult(result: LoginResult) = when (result) {
+        is LoginResult.Success ->
+            // The trusted-device cap (result.trustLimitReached) is an edge case:
+            // login succeeded but the device wasn't remembered. It's surfaced +
+            // managed on the Trusted Devices screen rather than blocking sign-in.
+            _state.update { it.copy(submitting = false, signedIn = true) }
+
+        LoginResult.TotpRequired ->
+            // Reveal the 2FA fields; a wrong code/recovery code re-lands here as BAD_CODE.
+            _state.update {
+                val hadFactor = if (it.useRecoveryCode) it.recoveryCode.isNotBlank() else it.code.isNotBlank()
+                it.copy(
+                    submitting = false,
+                    totpRequired = true,
+                    error = if (hadFactor) LoginError.BAD_CODE else null,
+                )
+            }
+
+        LoginResult.InvalidCredentials ->
+            _state.update { it.copy(submitting = false, error = LoginError.INVALID_CREDENTIALS) }
+
+        LoginResult.RateLimited ->
+            _state.update { it.copy(submitting = false, error = LoginError.RATE_LIMITED) }
+
+        LoginResult.ServerError ->
+            _state.update { it.copy(submitting = false, error = LoginError.SERVER) }
+
+        LoginResult.NetworkError ->
+            _state.update { it.copy(submitting = false, error = LoginError.NETWORK) }
+    }
 }

app/src/main/java/com/runicgateway/app/ui/navigation/Menu.kt

@@ -21,7 +21,12 @@ enum class MenuAccess {
     /** Visible to any signed-in account (§5, "My Account"). */
     SIGNED_IN,
 
-    /** Visible only to a player — the linked game-data groups (§6.3). */
+    /**
+     * The linked game-data groups (§6.3). Visible to any player **or** staff:
+     * staff are a superset of players (all player abilities plus their staff
+     * tools), and the backend's player self-service surface is role-agnostic, so
+     * a signed-in admin/editor/moderator sees + uses their own characters too.
+     */
     PLAYER,
 
     /** Visible to any staff role (admin/editor/moderator) — the M10 staff surface (§1). */
@@ -70,7 +75,7 @@ fun visibleEntries(entries: List<MenuEntry>, session: Session): List<MenuEntry>
         when (entry.access) {
             MenuAccess.PUBLIC -> true
             MenuAccess.SIGNED_IN -> session is Session.SignedIn
-            MenuAccess.PLAYER -> session is Session.SignedIn && session.user.isPlayer
+            MenuAccess.PLAYER -> session is Session.SignedIn && (session.user.isPlayer || session.user.isStaff)
             MenuAccess.STAFF -> session is Session.SignedIn && session.user.isStaff
             MenuAccess.MODERATOR -> session is Session.SignedIn && session.user.isModerator
         }

app/src/test/java/com/runicgateway/app/data/api/dto/NotificationsDtoTest.kt

@@ -3,6 +3,7 @@
  */
 package com.runicgateway.app.data.api.dto
 
+import kotlinx.serialization.encodeToString
 import kotlinx.serialization.json.Json
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
@@ -58,6 +59,15 @@ class NotificationsDtoTest {
         assertEquals(listOf("news.post", "champ.start"), dto.streams)
     }
 
+    @Test fun emptySubscriptionsStillSerializeStreamsField() {
+        // Regression: clearing the LAST subscription sends an empty set. The backend
+        // validator requires `streams`, so it must be present as `[]`, not omitted.
+        // Uses the production Json config (no encodeDefaults) to prove the field is
+        // always emitted because the DTO field has no default.
+        val body = json.encodeToString(NotificationSubscriptionsDto(emptyList()))
+        assertEquals("""{"streams":[]}""", body)
+    }
+
     @Test fun settingsPushBlockDecodes() {
         val dto = json.decodeFromString<SettingsDto>(
             """{"site_title":"Shard","brand":{"name":"Shard"},"push":{"ntfyUrl":"https://ntfy.shard.tld"}}""",

app/src/test/java/com/runicgateway/app/ui/navigation/MenuAccessTest.kt

@@ -37,12 +37,16 @@ class MenuAccessTest {
         assertTrue(visible.contains(Routes.HOME))
     }
 
-    @Test fun staffSeeAccountButNoPlayerOnlyGroups() {
-        val visible = routes(signedIn(Role.EDITOR))
-        assertTrue(visible.contains(Routes.ACCOUNT))
-        // No PLAYER-access entry (the M4 game-data groups) leaks to staff.
-        val playerOnly = APP_MENU.filter { it.access == MenuAccess.PLAYER }.map { it.route }
-        assertTrue(playerOnly.none { visible.contains(it) })
+    @Test fun staffSeeThePlayerGameDataGroups() {
+        // Staff are a superset of players: every staff role sees the PLAYER-access
+        // game-data groups too (their own linked characters, via the role-agnostic
+        // /player self-service surface), on top of their staff entries.
+        val playerGroups = APP_MENU.filter { it.access == MenuAccess.PLAYER }.map { it.route }
+        for (role in listOf(Role.ADMIN, Role.EDITOR, Role.MODERATOR)) {
+            val visible = routes(signedIn(role))
+            assertTrue("$role should see Account", visible.contains(Routes.ACCOUNT))
+            assertTrue("$role should see the player game-data groups", playerGroups.all { visible.contains(it) })
+        }
     }
 
     @Test fun publicEntryCountIsStableAcrossSessions() {
@@ -58,10 +62,13 @@ class MenuAccessTest {
     }
 
     @Test fun playerAccessGatedFunction() {
-        // A synthetic PLAYER-gated entry is visible to a player, hidden from staff/anon.
+        // A PLAYER-gated entry is visible to a player AND to every staff role
+        // (staff superset), hidden only from an unrecognized role and anon.
         val entries = listOf(MenuEntry("game", 0, MenuAccess.PLAYER))
-        assertTrue(visibleEntries(entries, signedIn(Role.PLAYER)).isNotEmpty())
-        assertTrue(visibleEntries(entries, signedIn(Role.ADMIN)).isEmpty())
+        for (role in listOf(Role.PLAYER, Role.ADMIN, Role.EDITOR, Role.MODERATOR)) {
+            assertTrue("$role should see a PLAYER entry", visibleEntries(entries, signedIn(role)).isNotEmpty())
+        }
+        assertTrue(visibleEntries(entries, signedIn(Role.UNKNOWN)).isEmpty())
         assertTrue(visibleEntries(entries, Session.SignedOut).isEmpty())
     }
 

sonar-project.properties

@@ -20,13 +20,26 @@ sonar.exclusions=**/build/**,**/.gradle/**,**/generated/**
 
 sonar.sourceEncoding=UTF-8
 
-# ── Optional enrichment (enable once the reports are produced in CI) ──
-# For richer Kotlin/Android results, run the reporters in sonarqube.yml and point
-# SonarQube at their output:
-#   • Android Lint:  ./gradlew lintDebug   → app/build/reports/lint-results-debug.xml
+# ── Coverage (JaCoCo) ──
+# sonarqube.yml runs `./gradlew testDebugUnitTest jacocoTestReport` before the scan;
+# that task (app/build.gradle.kts) writes this XML. Without it, Sonar reports 0%
+# coverage even though the JVM unit suite (app/src/test) exists.
+sonar.coverage.jacoco.xmlReportPaths=app/build/reports/jacoco/jacocoTestReport/jacocoTestReport.xml
+
+# Exclude from *coverage* (not from analysis): pure-@Composable UI can't be exercised
+# by JVM unit tests without Robolectric, so counting those lines would unfairly sink
+# new-code coverage. Logic (ViewModels, repositories, core, DTOs) stays measured.
+sonar.coverage.exclusions=\
+  app/src/main/java/**/ui/**/*Screen.kt,\
+  app/src/main/java/**/ui/**/*Screen*.kt,\
+  app/src/main/java/**/ui/theme/**,\
+  app/src/main/java/**/RunicApp.kt,\
+  app/src/main/java/**/MainActivity.kt,\
+  app/src/main/java/**/*Application.kt
+
+# ── Optional enrichment (enable once produced in CI) ──
+#   • Android Lint:  ./gradlew lintDebug → app/build/reports/lint-results-debug.xml
 #     sonar.androidLint.reportPaths=app/build/reports/lint-results-debug.xml
-#   • JaCoCo coverage (needs a coverage-enabled test run):
-#     sonar.coverage.jacoco.xmlReportPaths=app/build/reports/jacoco/.../*.xml
 # The alternative to the CLI scanner used here is the SonarQube Gradle plugin
 # (org.sonarqube), which auto-discovers these reports; the CLI + properties file
 # is used instead to keep this repo's setup identical to website/ and link/.