From 684da8f6391eed20324014b8c2626f3b1389f598 Mon Sep 17 00:00:00 2001 From: Sergejs Luhmirins Date: Wed, 19 Mar 2025 14:08:51 +0200 Subject: [PATCH 1/3] MS-245 Fix log messages in duplicate checks --- .../enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt index 68bab49214..e2ac3d32e8 100644 --- a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt +++ b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt @@ -18,7 +18,7 @@ internal class HasDuplicateEnrolmentsUseCase @Inject constructor() { return when { fingerprintResponse == null && faceResponse == null -> { - Simber.i("No capture response. Must be either fingerprint, face or both", tag = ENROLMENT) + Simber.w("No match response. Must be either fingerprint, face or both", tag = ENROLMENT) true } From 68a44e6533f664552d9400a56a9ffc22b760734a Mon Sep 17 00:00:00 2001 From: Sergejs Luhmirins Date: Wed, 19 Mar 2025 14:19:11 +0200 Subject: [PATCH 2/3] MS-245 Show more appropriate error message when no match results --- .../screen/EnrolLastBiometricFragment.kt | 2 ++ .../screen/EnrolLastBiometricViewModel.kt | 10 +++---- .../enrollast/screen/EnrolLastState.kt | 1 + ... => CheckForDuplicateEnrolmentsUseCase.kt} | 13 +++++---- .../screen/EnrolLastBiometricViewModelTest.kt | 23 +++++++-------- ...ckDuplicateEnrolmentsErrorsUseCaseTest.kt} | 28 +++++++++---------- 6 files changed, 39 insertions(+), 38 deletions(-) rename feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/{HasDuplicateEnrolmentsUseCase.kt => CheckForDuplicateEnrolmentsUseCase.kt} (87%) rename feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/{HasDuplicateEnrolmentsUseCaseTest.kt => CheckDuplicateEnrolmentsErrorsUseCaseTest.kt} (85%) diff --git a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricFragment.kt b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricFragment.kt index 24ddf317b9..fcf0f06270 100644 --- a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricFragment.kt +++ b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricFragment.kt @@ -20,6 +20,7 @@ import com.simprints.feature.enrollast.R import com.simprints.feature.enrollast.screen.EnrolLastState.ErrorType import com.simprints.feature.enrollast.screen.EnrolLastState.ErrorType.DUPLICATE_ENROLMENTS import com.simprints.feature.enrollast.screen.EnrolLastState.ErrorType.GENERAL_ERROR +import com.simprints.feature.enrollast.screen.EnrolLastState.ErrorType.NO_MATCH_RESULTS import com.simprints.infra.config.store.models.GeneralConfiguration.Modality import com.simprints.infra.events.event.domain.models.AlertScreenEvent import com.simprints.infra.logging.LoggingConstants.CrashReportTag.ORCHESTRATION @@ -84,6 +85,7 @@ internal class EnrolLastBiometricFragment : Fragment(R.layout.fragment_enrol_las } private fun getAlertMessage(errorType: ErrorType) = when (errorType) { + NO_MATCH_RESULTS -> IDR.string.enrol_last_biometrics_alert_message DUPLICATE_ENROLMENTS -> IDR.string.enrol_last_biometrics_alert_message_duplicate_records GENERAL_ERROR -> IDR.string.enrol_last_biometrics_alert_message } diff --git a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModel.kt b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModel.kt index c34ca748df..0216e1849d 100644 --- a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModel.kt +++ b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModel.kt @@ -9,10 +9,9 @@ import com.simprints.core.livedata.send import com.simprints.core.tools.time.TimeHelper import com.simprints.feature.enrollast.EnrolLastBiometricParams import com.simprints.feature.enrollast.EnrolLastBiometricStepResult -import com.simprints.feature.enrollast.screen.EnrolLastState.ErrorType.DUPLICATE_ENROLMENTS import com.simprints.feature.enrollast.screen.EnrolLastState.ErrorType.GENERAL_ERROR import com.simprints.feature.enrollast.screen.usecase.BuildSubjectUseCase -import com.simprints.feature.enrollast.screen.usecase.HasDuplicateEnrolmentsUseCase +import com.simprints.feature.enrollast.screen.usecase.CheckForDuplicateEnrolmentsUseCase import com.simprints.infra.config.sync.ConfigManager import com.simprints.infra.enrolment.records.repository.EnrolmentRecordRepository import com.simprints.infra.enrolment.records.repository.domain.models.Subject @@ -32,7 +31,7 @@ internal class EnrolLastBiometricViewModel @Inject constructor( private val configManager: ConfigManager, private val eventRepository: SessionEventRepository, private val enrolmentRecordRepository: EnrolmentRecordRepository, - private val hasDuplicateEnrolments: HasDuplicateEnrolmentsUseCase, + private val checkForDuplicateEnrolments: CheckForDuplicateEnrolmentsUseCase, private val buildSubject: BuildSubjectUseCase, ) : ViewModel() { val finish: LiveData> @@ -63,8 +62,9 @@ internal class EnrolLastBiometricViewModel @Inject constructor( ) return@launch } - if (hasDuplicateEnrolments(projectConfig, params.steps)) { - _finish.send(EnrolLastState.Failed(DUPLICATE_ENROLMENTS, modalities)) + val duplicateEnrolmentError = checkForDuplicateEnrolments(projectConfig, params.steps) + if (duplicateEnrolmentError != null) { + _finish.send(EnrolLastState.Failed(duplicateEnrolmentError, modalities)) return@launch } diff --git a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastState.kt b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastState.kt index e4a59f4498..538baebd41 100644 --- a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastState.kt +++ b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/EnrolLastState.kt @@ -13,6 +13,7 @@ internal sealed class EnrolLastState { ) : EnrolLastState() enum class ErrorType { + NO_MATCH_RESULTS, DUPLICATE_ENROLMENTS, GENERAL_ERROR, } diff --git a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt similarity index 87% rename from feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt rename to feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt index e2ac3d32e8..48daab4387 100644 --- a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCase.kt +++ b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt @@ -1,17 +1,18 @@ package com.simprints.feature.enrollast.screen.usecase import com.simprints.feature.enrollast.EnrolLastBiometricStepResult +import com.simprints.feature.enrollast.screen.EnrolLastState import com.simprints.infra.config.store.models.ProjectConfiguration import com.simprints.infra.logging.LoggingConstants.CrashReportTag.ENROLMENT import com.simprints.infra.logging.Simber import javax.inject.Inject -internal class HasDuplicateEnrolmentsUseCase @Inject constructor() { +internal class CheckForDuplicateEnrolmentsUseCase @Inject constructor() { operator fun invoke( projectConfig: ProjectConfiguration, steps: List, - ): Boolean { - if (!projectConfig.general.duplicateBiometricEnrolmentCheck) return false + ): EnrolLastState.ErrorType? { + if (!projectConfig.general.duplicateBiometricEnrolmentCheck) return null val fingerprintResponse = getFingerprintMatchResult(steps) val faceResponse = getFaceMatchResult(steps) @@ -19,15 +20,15 @@ internal class HasDuplicateEnrolmentsUseCase @Inject constructor() { return when { fingerprintResponse == null && faceResponse == null -> { Simber.w("No match response. Must be either fingerprint, face or both", tag = ENROLMENT) - true + EnrolLastState.ErrorType.NO_MATCH_RESULTS } isAnyResponseWithHighConfidence(projectConfig, fingerprintResponse, faceResponse) -> { Simber.i("There is a subject with confidence score above the high confidence level", tag = ENROLMENT) - true + EnrolLastState.ErrorType.DUPLICATE_ENROLMENTS } - else -> false + else -> null } } diff --git a/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModelTest.kt b/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModelTest.kt index 1ff2a99e34..e3421e3011 100644 --- a/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModelTest.kt +++ b/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/EnrolLastBiometricViewModelTest.kt @@ -1,7 +1,7 @@ package com.simprints.feature.enrollast.screen import androidx.arch.core.executor.testing.InstantTaskExecutorRule -import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.* import com.jraska.livedata.test import com.simprints.core.domain.tokenization.asTokenizableRaw import com.simprints.core.tools.time.TimeHelper @@ -9,7 +9,7 @@ import com.simprints.core.tools.time.Timestamp import com.simprints.feature.enrollast.EnrolLastBiometricParams import com.simprints.feature.enrollast.EnrolLastBiometricStepResult import com.simprints.feature.enrollast.screen.usecase.BuildSubjectUseCase -import com.simprints.feature.enrollast.screen.usecase.HasDuplicateEnrolmentsUseCase +import com.simprints.feature.enrollast.screen.usecase.CheckForDuplicateEnrolmentsUseCase import com.simprints.infra.config.store.models.ProjectConfiguration import com.simprints.infra.config.sync.ConfigManager import com.simprints.infra.enrolment.records.repository.EnrolmentRecordRepository @@ -20,12 +20,8 @@ import com.simprints.infra.events.event.domain.models.EnrolmentEventV4 import com.simprints.infra.events.event.domain.models.PersonCreationEvent import com.simprints.infra.events.session.SessionEventRepository import com.simprints.testtools.common.coroutines.TestCoroutineRule -import io.mockk.MockKAnnotations -import io.mockk.coEvery -import io.mockk.coVerify -import io.mockk.every +import io.mockk.* import io.mockk.impl.annotations.MockK -import io.mockk.mockk import kotlinx.coroutines.test.runTest import org.junit.Before import org.junit.Rule @@ -54,7 +50,7 @@ internal class EnrolLastBiometricViewModelTest { lateinit var enrolmentRecordRepository: EnrolmentRecordRepository @MockK - lateinit var hasDuplicateEnrolments: HasDuplicateEnrolmentsUseCase + lateinit var checkForDuplicateEnrolments: CheckForDuplicateEnrolmentsUseCase @MockK lateinit var buildSubject: BuildSubjectUseCase @@ -83,7 +79,7 @@ internal class EnrolLastBiometricViewModelTest { configManager, eventRepository, enrolmentRecordRepository, - hasDuplicateEnrolments, + checkForDuplicateEnrolments, buildSubject, ) } @@ -159,7 +155,7 @@ internal class EnrolLastBiometricViewModelTest { @Test fun `returns failure when has duplicate enrolments`() = runTest { - every { hasDuplicateEnrolments.invoke(any(), any()) } returns true + every { checkForDuplicateEnrolments.invoke(any(), any()) } returns EnrolLastState.ErrorType.DUPLICATE_ENROLMENTS viewModel.enrolBiometric(createParams(listOf())) @@ -174,7 +170,7 @@ internal class EnrolLastBiometricViewModelTest { @Test fun `returns success when no duplicate enrolments`() = runTest { - every { hasDuplicateEnrolments.invoke(any(), any()) } returns false + every { checkForDuplicateEnrolments.invoke(any(), any()) } returns null coEvery { buildSubject.invoke(any()) } returns subject viewModel.enrolBiometric(createParams(listOf())) @@ -188,7 +184,7 @@ internal class EnrolLastBiometricViewModelTest { @Test fun `saves event and record when no duplicate enrolments`() = runTest { - every { hasDuplicateEnrolments.invoke(any(), any()) } returns false + every { checkForDuplicateEnrolments.invoke(any(), any()) } returns null coEvery { buildSubject.invoke(any()) } returns subject viewModel.enrolBiometric(createParams(listOf())) @@ -199,7 +195,7 @@ internal class EnrolLastBiometricViewModelTest { @Test fun `returns failure record saving fails`() = runTest { - every { hasDuplicateEnrolments.invoke(any(), any()) } returns false + every { checkForDuplicateEnrolments.invoke(any(), any()) } returns null coEvery { buildSubject.invoke(any()) } returns subject coEvery { enrolmentRecordRepository.performActions(any(), any()) } throws Exception() @@ -215,6 +211,7 @@ internal class EnrolLastBiometricViewModelTest { @Test fun `Uses all BiometricReferenceCreationEvent for Enrolment event`() = runTest { + every { checkForDuplicateEnrolments.invoke(any(), any()) } returns null val biometricReferenceCreationEvent1 = mockk { every { id } returns "biometricReferenceCreationEventId1" every { payload } returns mockk { diff --git a/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCaseTest.kt b/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/CheckDuplicateEnrolmentsErrorsUseCaseTest.kt similarity index 85% rename from feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCaseTest.kt rename to feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/CheckDuplicateEnrolmentsErrorsUseCaseTest.kt index d9eff243f9..b27cf0b8b4 100644 --- a/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/HasDuplicateEnrolmentsUseCaseTest.kt +++ b/feature/enrol-last-biometric/src/test/java/com/simprints/feature/enrollast/screen/usecase/CheckDuplicateEnrolmentsErrorsUseCaseTest.kt @@ -1,21 +1,21 @@ package com.simprints.feature.enrollast.screen.usecase -import com.google.common.truth.Truth.assertThat +import com.google.common.truth.Truth.* import com.simprints.feature.enrollast.EnrolLastBiometricStepResult import com.simprints.feature.enrollast.MatchResult +import com.simprints.feature.enrollast.screen.EnrolLastState import com.simprints.infra.config.store.models.DecisionPolicy import com.simprints.infra.config.store.models.ProjectConfiguration -import io.mockk.every -import io.mockk.mockk +import io.mockk.* import org.junit.Before import org.junit.Test -class HasDuplicateEnrolmentsUseCaseTest { - private lateinit var useCase: HasDuplicateEnrolmentsUseCase +class CheckDuplicateEnrolmentsErrorsUseCaseTest { + private lateinit var useCase: CheckForDuplicateEnrolmentsUseCase @Before fun setUp() { - useCase = HasDuplicateEnrolmentsUseCase() + useCase = CheckForDuplicateEnrolmentsUseCase() } @Test @@ -25,7 +25,7 @@ class HasDuplicateEnrolmentsUseCaseTest { steps = emptyList(), ) - assertThat(result).isFalse() + assertThat(result).isNull() } @Test @@ -40,7 +40,7 @@ class HasDuplicateEnrolmentsUseCaseTest { ), ) - assertThat(result).isFalse() + assertThat(result).isNull() } @Test @@ -56,7 +56,7 @@ class HasDuplicateEnrolmentsUseCaseTest { ), ) - assertThat(result).isFalse() + assertThat(result).isNull() } @Test @@ -71,7 +71,7 @@ class HasDuplicateEnrolmentsUseCaseTest { ), ) - assertThat(result).isFalse() + assertThat(result).isNull() } @Test @@ -87,7 +87,7 @@ class HasDuplicateEnrolmentsUseCaseTest { ), ) - assertThat(result).isFalse() + assertThat(result).isNull() } @Test @@ -97,7 +97,7 @@ class HasDuplicateEnrolmentsUseCaseTest { steps = emptyList(), ) - assertThat(result).isTrue() + assertThat(result).isEqualTo(EnrolLastState.ErrorType.NO_MATCH_RESULTS) } @Test @@ -112,7 +112,7 @@ class HasDuplicateEnrolmentsUseCaseTest { ), ) - assertThat(result).isTrue() + assertThat(result).isEqualTo(EnrolLastState.ErrorType.DUPLICATE_ENROLMENTS) } @Test @@ -128,7 +128,7 @@ class HasDuplicateEnrolmentsUseCaseTest { ), ) - assertThat(result).isTrue() + assertThat(result).isEqualTo(EnrolLastState.ErrorType.DUPLICATE_ENROLMENTS) } private fun mockProjectConfig( From a9881958c4691170179c36b69d61fbbc52de4b7e Mon Sep 17 00:00:00 2001 From: Sergejs Luhmirins Date: Thu, 20 Mar 2025 14:17:34 +0200 Subject: [PATCH 3/3] MS-245 Add dedicated exception to log as non-fatal when there are no match results --- .../screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt index 48daab4387..fc14b0b1f4 100644 --- a/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt +++ b/feature/enrol-last-biometric/src/main/java/com/simprints/feature/enrollast/screen/usecase/CheckForDuplicateEnrolmentsUseCase.kt @@ -19,7 +19,11 @@ internal class CheckForDuplicateEnrolmentsUseCase @Inject constructor() { return when { fingerprintResponse == null && faceResponse == null -> { - Simber.w("No match response. Must be either fingerprint, face or both", tag = ENROLMENT) + Simber.e( + "No match response. Must be either fingerprint, face or both", + MissingMatchResultException(), + tag = ENROLMENT + ) EnrolLastState.ErrorType.NO_MATCH_RESULTS } @@ -62,4 +66,6 @@ internal class CheckForDuplicateEnrolmentsUseCase @Inject constructor() { return fingerprintResponse?.results?.any { it.confidenceScore >= fingerprintThreshold } == true || faceResponse?.results?.any { it.confidenceScore >= faceThreshold } == true } + + private class MissingMatchResultException() : IllegalStateException("No match response in duplicate check.") }