refactor: Extract battery level logic and simplify battery reporting

- Create AndroidDeviceInfoProvider class for device info operations - Inject AndroidDeviceInfoProvider into TrmnlDisplayRepository - Remove inline battery reporting from getNextDisplayData/getCurrentDisplayData - Add reportDeviceBatteryStatus() as public API for battery reporting - Make reportBatteryStatus() private (internal implementation) - Update TrmnlImageRefreshWorker to call reportDeviceBatteryStatus after successful refresh - Remove Context dependency from TrmnlDisplayRepository - Update tests to reflect new architecture - Add comprehensive tests for reportDeviceBatteryStatus This refactoring improves separation of concerns and makes the code more maintainable and testable.
2026-04-29 13:35:26 -07:00 · 2026-01-31 14:13:33 -05:00
parent ff54c47a59
commit 60e18686b5
4 changed files with 125 additions and 115 deletions
@@ -1,7 +1,5 @@
 package ink.trmnl.android.data

-import android.content.Context
-import android.os.BatteryManager
 import com.slack.eithernet.ApiResult
 import com.slack.eithernet.exceptionOrNull
 import com.squareup.anvil.annotations.optional.SingleIn
@@ -9,7 +7,6 @@ import ink.trmnl.android.BuildConfig.USE_FAKE_API
 import ink.trmnl.android.data.fake.generateFakeDeviceSetupInfo
 import ink.trmnl.android.data.fake.generateFakeTrmnlDisplayInfo
 import ink.trmnl.android.di.AppScope
-import ink.trmnl.android.di.ApplicationContext
 import ink.trmnl.android.model.SupportedDeviceModel
 import ink.trmnl.android.model.TrmnlDeviceConfig
 import ink.trmnl.android.model.TrmnlDeviceType
@@ -28,11 +25,9 @@ import ink.trmnl.android.network.model.TrmnlUser
 import ink.trmnl.android.network.util.constructApiUrl
 import ink.trmnl.android.network.util.extractHttpResponseMetadata
 import ink.trmnl.android.network.util.extractHttpResponseMetadataFromFailure
+import ink.trmnl.android.util.AndroidDeviceInfoProvider
 import ink.trmnl.android.util.HTTP_500
 import ink.trmnl.android.util.isHttpOk
-import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.launch
 import timber.log.Timber
 import javax.inject.Inject

@@ -49,29 +44,12 @@ import javax.inject.Inject
 class TrmnlDisplayRepository
    @Inject
    constructor(
-        @ApplicationContext private val context: Context,
        private val apiService: TrmnlApiService,
        private val userApiService: TrmnlUserApiService,
        private val imageMetadataStore: ImageMetadataStore,
        private val repositoryConfigProvider: RepositoryConfigProvider,
+        private val androidDeviceInfoProvider: AndroidDeviceInfoProvider,
    ) {
-        /**
-         * Gets the current battery level of the Android device.
-         *
-         * @return Battery percentage (0-100), or null if unable to retrieve
-         */
-        private fun getBatteryLevel(): Int? =
-            try {
-                val batteryManager = context.getSystemService(Context.BATTERY_SERVICE) as? BatteryManager
-                val batteryLevel =
-                    batteryManager?.getIntProperty(BatteryManager.BATTERY_PROPERTY_CAPACITY)
-                Timber.i("Current battery level: $batteryLevel%")
-                batteryLevel
-            } catch (e: Exception) {
-                Timber.e(e, "Failed to get battery level")
-                null
-            }
-
        /**
         * Fetches display data for next plugin from the server using the provided access token.
         * If the app is in debug mode, it uses mock data instead.
@@ -128,19 +106,6 @@ class TrmnlDisplayRepository
                            displayInfo.imageUrl,
                            displayInfo.refreshIntervalSeconds,
                        )
-
-                        // Report battery status for BYOD devices (non-blocking)
-                        if (trmnlDeviceConfig.type == TrmnlDeviceType.BYOD &&
-                            trmnlDeviceConfig.deviceId != null &&
-                            trmnlDeviceConfig.userApiToken != null
-                        ) {
-                            val batteryLevel = getBatteryLevel()
-                            if (batteryLevel != null) {
-                                CoroutineScope(Dispatchers.IO).launch {
-                                    reportBatteryStatus(trmnlDeviceConfig, batteryLevel)
-                                }
-                            }
-                        }
                    }

                    return displayInfo
@@ -201,19 +166,6 @@ class TrmnlDisplayRepository
                            displayInfo.imageUrl,
                            displayInfo.refreshIntervalSeconds,
                        )
-
-                        // Report battery status for BYOD devices (non-blocking)
-                        if (trmnlDeviceConfig.type == TrmnlDeviceType.BYOD &&
-                            trmnlDeviceConfig.deviceId != null &&
-                            trmnlDeviceConfig.userApiToken != null
-                        ) {
-                            val batteryLevel = getBatteryLevel()
-                            if (batteryLevel != null) {
-                                CoroutineScope(Dispatchers.IO).launch {
-                                    reportBatteryStatus(trmnlDeviceConfig, batteryLevel)
-                                }
-                            }
-                        }
                    }

                    return displayInfo
@@ -481,6 +433,45 @@ class TrmnlDisplayRepository
             */
        }

+        /**
+         * Reports the device's battery status to the TRMNL API for BYOD devices.
+         *
+         * This is a convenience method that checks if the device is a BYOD device with the necessary
+         * configuration (deviceId and userApiToken), retrieves the current battery level,
+         * and reports it to the server.
+         *
+         * This method should be called after successful image refresh operations.
+         *
+         * @param config Device configuration containing device type, device ID, and user API token
+         */
+        suspend fun reportDeviceBatteryStatus(config: TrmnlDeviceConfig) {
+            // Only report battery for BYOD devices with required configuration
+            if (config.type != TrmnlDeviceType.BYOD) {
+                Timber.d("Battery reporting skipped: not a BYOD device (type: ${config.type})")
+                return
+            }
+
+            if (config.deviceId == null) {
+                Timber.w("Battery reporting skipped: device ID is null")
+                return
+            }
+
+            if (config.userApiToken == null) {
+                Timber.w("Battery reporting skipped: user API token is null")
+                return
+            }
+
+            // Get current battery level
+            val batteryLevel = androidDeviceInfoProvider.getBatteryLevel()
+            if (batteryLevel == null) {
+                Timber.w("Battery reporting skipped: unable to get battery level")
+                return
+            }
+
+            // Report battery status
+            reportBatteryStatus(config, batteryLevel)
+        }
+
        /**
         * Reports the device's battery status to the TRMNL API.
         *
@@ -493,7 +484,7 @@ class TrmnlDisplayRepository
         * @param batteryPercent The current battery percentage (0-100)
         * @return A Result containing Unit on success or an exception on failure
         */
-        suspend fun reportBatteryStatus(
+        private suspend fun reportBatteryStatus(
            config: TrmnlDeviceConfig,
            batteryPercent: Int,
        ): Result<Unit> {
@@ -0,0 +1,39 @@
+package ink.trmnl.android.util
+
+import android.content.Context
+import android.os.BatteryManager
+import com.squareup.anvil.annotations.optional.SingleIn
+import ink.trmnl.android.di.AppScope
+import ink.trmnl.android.di.ApplicationContext
+import timber.log.Timber
+import javax.inject.Inject
+
+/**
+ * Provider class for accessing Android device information.
+ *
+ * This class provides utility methods to retrieve device-specific information
+ * such as battery level, which can be used for reporting to the TRMNL API.
+ */
+@SingleIn(AppScope::class)
+class AndroidDeviceInfoProvider
+    @Inject
+    constructor(
+        @ApplicationContext private val context: Context,
+    ) {
+        /**
+         * Gets the current battery level of the Android device.
+         *
+         * @return Battery percentage (0-100), or null if unable to retrieve
+         */
+        fun getBatteryLevel(): Int? =
+            try {
+                val batteryManager = context.getSystemService(Context.BATTERY_SERVICE) as? BatteryManager
+                val batteryLevel =
+                    batteryManager?.getIntProperty(BatteryManager.BATTERY_PROPERTY_CAPACITY)
+                Timber.i("Current battery level: $batteryLevel%")
+                batteryLevel
+            } catch (e: Exception) {
+                Timber.e(e, "Failed to get battery level")
+                null
+            }
+    }
@@ -170,6 +170,9 @@ class TrmnlImageRefreshWorker(
            httpResponseMetadata = trmnlDisplayInfo.httpResponseMetadata,
        )

+        // Report battery status for BYOD devices after successful image refresh
+        displayRepository.reportDeviceBatteryStatus(deviceConfig)
+
        // NOTE: Image metadata caching is handled automatically by `TrmnlDisplayRepository`
        // when the API call succeeds, so we don't need to save it again here.
        // See https://github.com/usetrmnl/trmnl-android/issues/195
@@ -1,6 +1,5 @@
 package ink.trmnl.android.data

-import android.content.Context
 import com.google.common.truth.Truth.assertThat
 import com.slack.eithernet.ApiResult
 import ink.trmnl.android.model.TrmnlDeviceConfig
@@ -10,6 +9,7 @@ import ink.trmnl.android.network.TrmnlUserApiService
 import ink.trmnl.android.network.model.TrmnlCurrentImageResponse
 import ink.trmnl.android.network.model.TrmnlDisplayResponse
 import ink.trmnl.android.network.util.constructApiUrl
+import ink.trmnl.android.util.AndroidDeviceInfoProvider
 import io.mockk.coEvery
 import io.mockk.coVerify
 import io.mockk.every
@@ -29,12 +29,12 @@ import org.junit.Test
@OptIn(com.slack.eithernet.InternalEitherNetApi::class)
 class TrmnlDisplayRepositoryTest {
    private lateinit var repository: TrmnlDisplayRepository
-    private lateinit var context: Context
    private lateinit var apiService: TrmnlApiService
    private lateinit var userApiService: TrmnlUserApiService
    private lateinit var imageMetadataStore: ImageMetadataStore
    private lateinit var repositoryConfigProvider: RepositoryConfigProvider
    private lateinit var deviceConfigDataStore: TrmnlDeviceConfigDataStore
+    private lateinit var androidDeviceInfoProvider: AndroidDeviceInfoProvider

    private val testDeviceConfig =
        TrmnlDeviceConfig(
@@ -62,22 +62,22 @@ class TrmnlDisplayRepositoryTest {

    @Before
    fun setup() {
-        context = mockk(relaxed = true)
        apiService = mockk()
        userApiService = mockk()
        repositoryConfigProvider = mockk()
        deviceConfigDataStore = mockk()
        imageMetadataStore = mockk(relaxed = true)
+        androidDeviceInfoProvider = mockk(relaxed = true)

        every { repositoryConfigProvider.shouldUseFakeData } returns false

        repository =
            TrmnlDisplayRepository(
-                context = context,
                apiService = apiService,
                userApiService = userApiService,
                imageMetadataStore = imageMetadataStore,
                repositoryConfigProvider = repositoryConfigProvider,
+                androidDeviceInfoProvider = androidDeviceInfoProvider,
            )
    }

@@ -587,7 +587,7 @@ class TrmnlDisplayRepositoryTest {
        }

    @Test
-    fun `reportBatteryStatus should succeed with valid BYOD config`() =
+    fun `reportDeviceBatteryStatus should report battery for valid BYOD config`() =
        runTest {
            // Arrange
            val byodConfigWithDeviceId =
@@ -596,6 +596,8 @@ class TrmnlDisplayRepositoryTest {
                    userApiToken = "user_test_token",
                )

+            every { androidDeviceInfoProvider.getBatteryLevel() } returns 75
+
            val expectedApiUrl = "https://server.example.com/api/devices/123"

            coEvery {
@@ -607,22 +609,38 @@ class TrmnlDisplayRepositoryTest {
            } returns ApiResult.success(mockk(relaxed = true))

            // Act
-            val result = repository.reportBatteryStatus(byodConfigWithDeviceId, 85)
-
-            // Assert
-            assertThat(result.isSuccess).isTrue()
+            repository.reportDeviceBatteryStatus(byodConfigWithDeviceId)

+            // Assert - Verify battery status was reported
            coVerify {
                userApiService.updateDevice(
                    fullApiUrl = expectedApiUrl,
                    accessToken = "Bearer user_test_token",
-                    updateRequest = match { it.percentCharged == 85.0 },
+                    updateRequest = match { it.percentCharged == 75.0 },
                )
            }
        }

    @Test
-    fun `reportBatteryStatus should fail when deviceId is null`() =
+    fun `reportDeviceBatteryStatus should skip for non-BYOD device`() =
+        runTest {
+            // Arrange - TRMNL device (not BYOD)
+            val trmnlConfig =
+                testDeviceConfig.copy(
+                    deviceId = 123,
+                    userApiToken = "user_test_token",
+                )
+
+            // Act
+            repository.reportDeviceBatteryStatus(trmnlConfig)
+
+            // Assert - Verify API was NOT called
+            coVerify(exactly = 0) { userApiService.updateDevice(any(), any(), any()) }
+            coVerify(exactly = 0) { androidDeviceInfoProvider.getBatteryLevel() }
+        }
+
+    @Test
+    fun `reportDeviceBatteryStatus should skip when deviceId is null`() =
        runTest {
            // Arrange
            val configWithoutDeviceId =
@@ -632,19 +650,15 @@ class TrmnlDisplayRepositoryTest {
                )

            // Act
-            val result = repository.reportBatteryStatus(configWithoutDeviceId, 85)
+            repository.reportDeviceBatteryStatus(configWithoutDeviceId)

-            // Assert
-            assertThat(result.isFailure).isTrue()
-            assertThat(result.exceptionOrNull()).isInstanceOf(IllegalStateException::class.java)
-            assertThat(result.exceptionOrNull()?.message).contains("Device ID is required")
-
-            // Verify API was NOT called
+            // Assert - Verify API was NOT called
            coVerify(exactly = 0) { userApiService.updateDevice(any(), any(), any()) }
+            coVerify(exactly = 0) { androidDeviceInfoProvider.getBatteryLevel() }
        }

    @Test
-    fun `reportBatteryStatus should fail when userApiToken is null`() =
+    fun `reportDeviceBatteryStatus should skip when userApiToken is null`() =
        runTest {
            // Arrange
            val configWithoutUserToken =
@@ -654,19 +668,15 @@ class TrmnlDisplayRepositoryTest {
                )

            // Act
-            val result = repository.reportBatteryStatus(configWithoutUserToken, 85)
+            repository.reportDeviceBatteryStatus(configWithoutUserToken)

-            // Assert
-            assertThat(result.isFailure).isTrue()
-            assertThat(result.exceptionOrNull()).isInstanceOf(IllegalStateException::class.java)
-            assertThat(result.exceptionOrNull()?.message).contains("User API token is required")
-
-            // Verify API was NOT called
+            // Assert - Verify API was NOT called
            coVerify(exactly = 0) { userApiService.updateDevice(any(), any(), any()) }
+            coVerify(exactly = 0) { androidDeviceInfoProvider.getBatteryLevel() }
        }

    @Test
-    fun `reportBatteryStatus should handle API failure`() =
+    fun `reportDeviceBatteryStatus should skip when battery level unavailable`() =
        runTest {
            // Arrange
            val byodConfigWithDeviceId =
@@ -675,46 +685,13 @@ class TrmnlDisplayRepositoryTest {
                    userApiToken = "user_test_token",
                )

-            val expectedApiUrl = "https://server.example.com/api/devices/123"
-            val apiException = java.io.IOException("Network error")
-            val httpFailure: ApiResult.Failure<Unit> =
-                ApiResult.networkFailure(apiException)
-
-            coEvery {
-                userApiService.updateDevice(
-                    fullApiUrl = expectedApiUrl,
-                    accessToken = "Bearer user_test_token",
-                    updateRequest = any(),
-                )
-            } returns httpFailure
+            every { androidDeviceInfoProvider.getBatteryLevel() } returns null

            // Act
-            val result = repository.reportBatteryStatus(byodConfigWithDeviceId, 85)
+            repository.reportDeviceBatteryStatus(byodConfigWithDeviceId)

-            // Assert
-            assertThat(result.isFailure).isTrue()
-            assertThat(result.exceptionOrNull()).isEqualTo(apiException)
-        }
-
-    @Test
-    fun `reportBatteryStatus should skip API call in fake data mode`() =
-        runTest {
-            // Arrange
-            every { repositoryConfigProvider.shouldUseFakeData } returns true
-
-            val byodConfigWithDeviceId =
-                byodDeviceConfig.copy(
-                    deviceId = 123,
-                    userApiToken = "user_test_token",
-                )
-
-            // Act
-            val result = repository.reportBatteryStatus(byodConfigWithDeviceId, 85)
-
-            // Assert
-            assertThat(result.isSuccess).isTrue()
-
-            // Verify API was NOT called
+            // Assert - Verify battery level was requested but API was NOT called
+            coVerify(exactly = 1) { androidDeviceInfoProvider.getBatteryLevel() }
            coVerify(exactly = 0) { userApiService.updateDevice(any(), any(), any()) }
        }
 }