Introduce NotificationDeleteWorker #6460
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,93 @@ | ||||||||||||||||||||||||
| package io.homeassistant.companion.android.common.notifications | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import android.content.Context | ||||||||||||||||||||||||
| import androidx.work.CoroutineWorker | ||||||||||||||||||||||||
| import androidx.work.Data | ||||||||||||||||||||||||
| import androidx.work.OneTimeWorkRequestBuilder | ||||||||||||||||||||||||
| import androidx.work.OutOfQuotaPolicy | ||||||||||||||||||||||||
| import androidx.work.WorkManager | ||||||||||||||||||||||||
| import androidx.work.WorkerParameters | ||||||||||||||||||||||||
| import dagger.hilt.EntryPoint | ||||||||||||||||||||||||
| import dagger.hilt.EntryPoints | ||||||||||||||||||||||||
| import dagger.hilt.InstallIn | ||||||||||||||||||||||||
| import dagger.hilt.components.SingletonComponent | ||||||||||||||||||||||||
| import io.homeassistant.companion.android.common.data.servers.ServerManager | ||||||||||||||||||||||||
| import io.homeassistant.companion.android.database.notification.NotificationDao | ||||||||||||||||||||||||
| import kotlinx.coroutines.CancellationException | ||||||||||||||||||||||||
| import timber.log.Timber | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Worker that fires the "mobile_app_notification_cleared" event to the Home Assistant server. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| internal class NotificationDeleteWorker(context: Context, params: WorkerParameters) : | ||||||||||||||||||||||||
| CoroutineWorker(context.applicationContext, params) { | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| companion object { | ||||||||||||||||||||||||
| private const val KEY_DATABASE_ID = "database_id" | ||||||||||||||||||||||||
| private const val KEY_EVENT_DATA_KEYS = "event_data_keys" | ||||||||||||||||||||||||
| private const val KEY_EVENT_DATA_VALUES = "event_data_values" | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * A bug in the AndroidX Hilt compiler that caused a StackOverflow in our codebase | ||||||||||||||||||||||||
| * tracked in https://github.com/google/dagger/issues/4702 forces us to use an entry point. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| @EntryPoint | ||||||||||||||||||||||||
| @InstallIn(SingletonComponent::class) | ||||||||||||||||||||||||
| internal interface NotificationDeleteWorkerEntryPoint { | ||||||||||||||||||||||||
| fun serverManager(): ServerManager | ||||||||||||||||||||||||
| fun notificationDao(): NotificationDao | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Enqueues work to fire the notification delete event to the Home Assistant server. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * @param context The context to use for obtaining [WorkManager]. | ||||||||||||||||||||||||
| * @param databaseId The database ID of the notification that was cleared. | ||||||||||||||||||||||||
| * @param eventDataKeys The keys of the event data to send to the server. | ||||||||||||||||||||||||
| * @param eventDataValues The values of the event data to send to the server, matching [eventDataKeys] by index. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| internal fun enqueue( | ||||||||||||||||||||||||
| context: Context, | ||||||||||||||||||||||||
| databaseId: Long, | ||||||||||||||||||||||||
| eventDataKeys: Array<String?>, | ||||||||||||||||||||||||
| eventDataValues: Array<String?>, | ||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||
| val data = Data.Builder() | ||||||||||||||||||||||||
| .putLong(KEY_DATABASE_ID, databaseId) | ||||||||||||||||||||||||
| .putStringArray(KEY_EVENT_DATA_KEYS, eventDataKeys) | ||||||||||||||||||||||||
| .putStringArray(KEY_EVENT_DATA_VALUES, eventDataValues) | ||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| val request = OneTimeWorkRequestBuilder<NotificationDeleteWorker>() | ||||||||||||||||||||||||
| .setInputData(data) | ||||||||||||||||||||||||
| // We want the event to be sent right away if it is possible | ||||||||||||||||||||||||
| .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) | ||||||||||||||||||||||||
|
Member
Author
There was a problem hiding this comment.
|
||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| WorkManager.getInstance(context).enqueue(request) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| override suspend fun doWork(): Result { | ||||||||||||||||||||||||
| val databaseId = inputData.getLong(KEY_DATABASE_ID, 0) | ||||||||||||||||||||||||
| val keys = inputData.getStringArray(KEY_EVENT_DATA_KEYS) ?: return Result.failure() | ||||||||||||||||||||||||
| val values = inputData.getStringArray(KEY_EVENT_DATA_VALUES) ?: return Result.failure() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| val entryPoints = EntryPoints.get(applicationContext, NotificationDeleteWorkerEntryPoint::class.java) | ||||||||||||||||||||||||
| val serverManager = entryPoints.serverManager() | ||||||||||||||||||||||||
| val notificationDao = entryPoints.notificationDao() | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return try { | ||||||||||||||||||||||||
| val eventData = keys.zip(values).toMap() | ||||||||||||||||||||||||
|
There was a problem hiding this comment. There's no validation that There was a problem hiding this comment. Type safety issue:
Suggested change
|
||||||||||||||||||||||||
| val serverId = notificationDao.get(databaseId.toInt())?.serverId ?: ServerManager.SERVER_ID_ACTIVE | ||||||||||||||||||||||||
| serverManager.integrationRepository(serverId).fireEvent("mobile_app_notification_cleared", eventData) | ||||||||||||||||||||||||
| Timber.d("Notification cleared event successful") | ||||||||||||||||||||||||
| Result.success() | ||||||||||||||||||||||||
| } catch (e: CancellationException) { | ||||||||||||||||||||||||
| throw e | ||||||||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||||||||
| Timber.e(e, "Issue sending notification cleared event to Home Assistant") | ||||||||||||||||||||||||
| Result.failure() | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| package io.homeassistant.companion.android.common.notifications | ||
|
|
||
| import android.content.Context | ||
| import androidx.work.Data | ||
| import androidx.work.ListenableWorker | ||
| import androidx.work.WorkerParameters | ||
| import dagger.hilt.EntryPoints | ||
| import io.homeassistant.companion.android.common.data.integration.IntegrationRepository | ||
| import io.homeassistant.companion.android.common.data.servers.ServerManager | ||
| import io.homeassistant.companion.android.common.notifications.NotificationDeleteWorker.Companion.NotificationDeleteWorkerEntryPoint | ||
| import io.homeassistant.companion.android.database.notification.NotificationDao | ||
| import io.homeassistant.companion.android.database.notification.NotificationItem | ||
| import io.homeassistant.companion.android.testing.unit.ConsoleLogExtension | ||
| import io.mockk.coEvery | ||
| import io.mockk.coVerify | ||
| import io.mockk.every | ||
| import io.mockk.mockk | ||
| import io.mockk.mockkStatic | ||
| import kotlinx.coroutines.test.runTest | ||
| import org.junit.jupiter.api.Assertions.assertEquals | ||
| import org.junit.jupiter.api.BeforeEach | ||
| import org.junit.jupiter.api.Test | ||
| import org.junit.jupiter.api.extension.ExtendWith | ||
|
|
||
| @ExtendWith(ConsoleLogExtension::class) | ||
| class NotificationDeleteWorkerTest { | ||
|
|
||
| private val serverManager: ServerManager = mockk() | ||
| private val notificationDao: NotificationDao = mockk() | ||
| private val integrationRepository: IntegrationRepository = mockk(relaxed = true) | ||
| private val context: Context = mockk() | ||
| private val workerParams: WorkerParameters = mockk(relaxed = true) | ||
|
|
||
| @BeforeEach | ||
| fun setup() { | ||
| every { context.applicationContext } returns context | ||
| coEvery { serverManager.integrationRepository(any()) } returns integrationRepository | ||
|
|
||
| mockkStatic(EntryPoints::class) | ||
| every { | ||
| EntryPoints.get(any(), NotificationDeleteWorkerEntryPoint::class.java) | ||
| } returns mockk { | ||
| every { serverManager() } returns serverManager | ||
| every { notificationDao() } returns notificationDao | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given valid input when doWork then fire event and return success`() = runTest { | ||
| val eventData = mapOf("action" to "cleared", "tag" to "test-tag") | ||
| val databaseId = 42L | ||
| val serverId = 5 | ||
| setupWorkerInput(databaseId = databaseId, eventData = eventData) | ||
| coEvery { notificationDao.get(databaseId.toInt()) } returns notificationItem(serverId = serverId) | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.success(), result) | ||
| coVerify(exactly = 1) { | ||
| serverManager.integrationRepository(serverId) | ||
| integrationRepository.fireEvent("mobile_app_notification_cleared", eventData) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given notification not in database when doWork then use active server and return success`() = runTest { | ||
| val eventData = mapOf("action" to "cleared") | ||
| val databaseId = 99L | ||
| setupWorkerInput(databaseId = databaseId, eventData = eventData) | ||
| coEvery { notificationDao.get(databaseId.toInt()) } returns null | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.success(), result) | ||
| coVerify(exactly = 1) { | ||
| serverManager.integrationRepository(ServerManager.SERVER_ID_ACTIVE) | ||
| integrationRepository.fireEvent("mobile_app_notification_cleared", eventData) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given missing event data when doWork then return failure`() = runTest { | ||
| every { workerParams.inputData } returns Data.Builder() | ||
| .putLong("database_id", 1L) | ||
| .build() | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.failure(), result) | ||
| coVerify(exactly = 0) { integrationRepository.fireEvent(any(), any()) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given server throws when doWork then return failure`() = runTest { | ||
| val eventData = mapOf("action" to "cleared") | ||
| val databaseId = 42L | ||
| setupWorkerInput(databaseId = databaseId, eventData = eventData) | ||
| coEvery { notificationDao.get(databaseId.toInt()) } returns notificationItem(serverId = 1) | ||
| coEvery { integrationRepository.fireEvent(any(), any()) } throws IllegalStateException("Server unavailable") | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.failure(), result) | ||
| } | ||
|
|
||
| private fun setupWorkerInput(databaseId: Long, eventData: Map<String, String>) { | ||
| every { workerParams.inputData } returns Data.Builder() | ||
| .putLong("database_id", databaseId) | ||
| .putStringArray("event_data_keys", eventData.keys.toTypedArray()) | ||
| .putStringArray("event_data_values", eventData.values.toTypedArray()) | ||
| .build() | ||
| } | ||
|
|
||
| private fun notificationItem(serverId: Int): NotificationItem = | ||
| NotificationItem( | ||
| id = 1, | ||
| received = 0L, | ||
| message = "test", | ||
| data = "{}", | ||
| source = "test", | ||
| serverId = serverId, | ||
| ) | ||
| } | ||
|
Comment on lines
+26
to
+127
There was a problem hiding this comment. Missing test coverage for the case where the |
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
databaseIdis null,putExtrawill storenull, butgetLongExtrawill return the default value of 0 regardless of whether null was stored or the key is missing. This means you cannot distinguish between a missing database ID and an actual ID of 0. Consider usinghasExtra()to check if the key exists before retrieving the value, or handle the null case explicitly by storing a sentinel value like -1 for missing database IDs.