diff --git a/app/src/main/java/pl/firmatpp/kierowca/data/ApiErrorMapper.kt b/app/src/main/java/pl/firmatpp/kierowca/data/ApiErrorMapper.kt index 16719cd..6c0a5a7 100644 --- a/app/src/main/java/pl/firmatpp/kierowca/data/ApiErrorMapper.kt +++ b/app/src/main/java/pl/firmatpp/kierowca/data/ApiErrorMapper.kt @@ -50,7 +50,10 @@ object ApiErrorMapper { map(throwable).kind != ApiErrorKind.Network fun mapHttpStatus(statusCode: Int, body: String?): ApiError { - val code = body?.let { """"code"\s*:\s*"([^"]+)"""".toRegex().find(it)?.groupValues?.getOrNull(1) } + val code = body?.let { + """"code"\s*:\s*"([^"]+)"""".toRegex().find(it)?.groupValues?.getOrNull(1) + ?: """"reason"\s*:\s*"([^"]+)"""".toRegex().find(it)?.groupValues?.getOrNull(1) + } val message = body?.let { """"message"\s*:\s*"([^"]+)"""".toRegex().find(it)?.groupValues?.getOrNull(1) } val retryable = body?.let { """"retryable"\s*:\s*(true|false)""".toRegex().find(it)?.groupValues?.getOrNull(1)?.toBooleanStrictOrNull() } @@ -81,15 +84,20 @@ object ApiErrorMapper { } val defaultRetryable = statusCode == 429 || statusCode in 500..599 val resolvedRetryable = retryable ?: defaultRetryable - val resolvedMessage = message?.takeIf { it.isNotBlank() } ?: when (kind) { - ApiErrorKind.Auth -> "Sesja wygasła. Zaloguj się ponownie." - ApiErrorKind.Forbidden -> "Brak uprawnień do tej operacji." - ApiErrorKind.Validation -> "Serwer odrzucił operację. Nie została zapisana." - ApiErrorKind.Conflict -> "Operacja jest w konflikcie z aktualnym stanem danych." - ApiErrorKind.RateLimited -> "Za dużo prób. Aplikacja spróbuje ponownie później." - ApiErrorKind.Server -> "Serwer nie potwierdził operacji. Aplikacja spróbuje ponownie." - ApiErrorKind.Network -> "Nie udało się połączyć z serwerem. Operacja nie została potwierdzona." - ApiErrorKind.Unknown -> "Wystąpił błąd. Operacja nie została potwierdzona." + val resolvedMessage = when { + code == "INVALID_OTP" -> "Kod jest nieprawidłowy lub wygasł. Użyj kodu z najnowszego SMS-a." + kind == ApiErrorKind.RateLimited -> "Za dużo prób. Odczekaj chwilę i spróbuj ponownie." + !message.isNullOrBlank() -> message + else -> when (kind) { + ApiErrorKind.Auth -> "Sesja wygasła. Zaloguj się ponownie." + ApiErrorKind.Forbidden -> "Brak uprawnień do tej operacji." + ApiErrorKind.Validation -> "Serwer odrzucił operację. Nie została zapisana." + ApiErrorKind.Conflict -> "Operacja jest w konflikcie z aktualnym stanem danych." + ApiErrorKind.RateLimited -> "Za dużo prób. Odczekaj chwilę i spróbuj ponownie." + ApiErrorKind.Server -> "Serwer nie potwierdził operacji. Aplikacja spróbuje ponownie." + ApiErrorKind.Network -> "Nie udało się połączyć z serwerem. Operacja nie została potwierdzona." + ApiErrorKind.Unknown -> "Wystąpił błąd. Operacja nie została potwierdzona." + } } return ApiError( @@ -103,7 +111,11 @@ object ApiErrorMapper { } private fun mapHttpException(exception: HttpException): ApiError { - val body = runCatching { exception.response()?.errorBody()?.string() }.getOrNull() + val body = runCatching { + val source = exception.response()?.errorBody()?.source() ?: return@runCatching null + source.request(Long.MAX_VALUE) + source.buffer.clone().readUtf8() + }.getOrNull() return mapHttpStatus(exception.code(), body) } } diff --git a/app/src/main/java/pl/firmatpp/kierowca/ui/DriverApp.kt b/app/src/main/java/pl/firmatpp/kierowca/ui/DriverApp.kt index 6556413..c12a812 100644 --- a/app/src/main/java/pl/firmatpp/kierowca/ui/DriverApp.kt +++ b/app/src/main/java/pl/firmatpp/kierowca/ui/DriverApp.kt @@ -247,8 +247,10 @@ fun DriverApp( } } SmsUserConsentEffect( - enabled = state.screen == DriverScreen.Phone || state.screen == DriverScreen.Otp, - onCode = viewModel::updateOtpCode, + enabled = state.otpRequestPending || + (state.screen == DriverScreen.Otp && !state.loading), + requestId = state.otpConsentRequestId, + onCode = viewModel::updateOtpCodeFromSms, ) LaunchedEffect(initialRouteId, initialRouteTarget) { viewModel.openRouteFromNotification(initialRouteId, initialRouteTarget) @@ -282,7 +284,13 @@ fun DriverApp( onUseOfflineNow = viewModel::useOfflineStartupDataNow, ) DriverScreen.Phone -> PhoneScreen(state, viewModel::requestOtp) - DriverScreen.Otp -> OtpScreen(state, viewModel::updateOtpCode, viewModel::verifyOtp, viewModel::back) + DriverScreen.Otp -> OtpScreen( + state = state, + onCodeChange = viewModel::updateOtpCode, + onSubmit = viewModel::verifyOtp, + onResend = { viewModel.requestOtp(state.phone) }, + onBack = viewModel::back, + ) DriverScreen.Routes -> RoutesScreen( state = state, onRefresh = viewModel::refreshRoutesSilently, @@ -840,8 +848,19 @@ private fun OtpScreen( state: DriverUiState, onCodeChange: (String) -> Unit, onSubmit: (String) -> Unit, + onResend: () -> Unit, onBack: () -> Unit, ) { + var nowEpochMillis by remember(state.otpResendAvailableAtEpochMillis) { mutableStateOf(System.currentTimeMillis()) } + val resendDelaySeconds = otpResendDelaySeconds(state.otpResendAvailableAtEpochMillis, nowEpochMillis) + + LaunchedEffect(state.otpResendAvailableAtEpochMillis) { + while (nowEpochMillis < state.otpResendAvailableAtEpochMillis) { + delay(1_000L) + nowEpochMillis = System.currentTimeMillis() + } + } + AuthShell(title = "Kod SMS", subtitle = "Wpisz kod wyslany na ${state.maskedPhone}.", onBack = onBack) { OutlinedTextField( value = state.otpCode, @@ -853,7 +872,20 @@ private fun OtpScreen( ) ErrorText(state.error) Spacer(Modifier.height(if (state.error.isNullOrBlank()) 18.dp else 8.dp)) - PrimaryButton("Zaloguj") { onSubmit(state.otpCode) } + PrimaryButton( + label = "Zaloguj", + enabled = isRetrievedOtpCode(state.otpCode) && !state.loading, + ) { onSubmit(state.otpCode) } + TextButton( + onClick = onResend, + enabled = resendDelaySeconds == 0 && !state.loading, + modifier = Modifier.align(Alignment.CenterHorizontally), + ) { + Text( + if (resendDelaySeconds > 0) "Wyślij kod ponownie za $resendDelaySeconds s" + else "Wyślij kod ponownie", + ) + } } } @@ -4390,9 +4422,10 @@ private fun PhotoPreviewErrorOverlay() { } @Composable -private fun PrimaryButton(label: String, onClick: () -> Unit) { +private fun PrimaryButton(label: String, enabled: Boolean = true, onClick: () -> Unit) { Button( onClick = onClick, + enabled = enabled, modifier = Modifier.fillMaxWidth().height(56.dp), colors = ButtonDefaults.buttonColors(containerColor = TppTheme.colors.forest), shape = MaterialTheme.shapes.small, @@ -4544,28 +4577,48 @@ private fun bestLastKnownLocation(context: Context): Location? { } @Composable -private fun SmsUserConsentEffect(enabled: Boolean, onCode: (String) -> Unit) { +private fun SmsUserConsentEffect(enabled: Boolean, requestId: Long, onCode: (Long, String) -> Unit) { val context = LocalContext.current + var listenGeneration by remember(requestId) { mutableStateOf(0) } + var launchedRequestId by remember { mutableStateOf(null) } val launcher = rememberLauncherForActivityResult(ActivityResultContracts.StartActivityForResult()) { result -> + val completedRequestId = launchedRequestId + var codeDelivered = false if (result.resultCode == Activity.RESULT_OK) { val message = result.data?.getStringExtra(SmsRetriever.EXTRA_SMS_MESSAGE).orEmpty() - OtpCodeExtractor.extract(message)?.let(onCode) + if (completedRequestId != null) { + OtpCodeExtractor.extract(message)?.let { code -> + codeDelivered = true + onCode(completedRequestId, code) + } + } + } + if (!codeDelivered && enabled && completedRequestId == requestId) { + listenGeneration += 1 } } - DisposableEffect(enabled, context) { - if (!enabled) return@DisposableEffect onDispose { } + LaunchedEffect(enabled, requestId, listenGeneration) { + if (enabled && requestId > 0L) { + SmsRetriever.getClient(context).startSmsUserConsent(null) + } + } - SmsRetriever.getClient(context).startSmsUserConsent(null) + DisposableEffect(enabled, context, requestId) { + if (!enabled) return@DisposableEffect onDispose { } val receiver = object : BroadcastReceiver() { override fun onReceive(receiverContext: Context?, intent: Intent?) { if (intent?.action != SmsRetriever.SMS_RETRIEVED_ACTION) return val status = intent.smsRetrieverStatus() ?: return - if (status.statusCode != CommonStatusCodes.SUCCESS) return - - intent.smsConsentIntent()?.let(launcher::launch) + when (status.statusCode) { + CommonStatusCodes.SUCCESS -> intent.smsConsentIntent()?.let { consentIntent -> + launchedRequestId = requestId + launcher.launch(consentIntent) + } + CommonStatusCodes.TIMEOUT -> listenGeneration += 1 + } } } diff --git a/app/src/main/java/pl/firmatpp/kierowca/ui/DriverViewModel.kt b/app/src/main/java/pl/firmatpp/kierowca/ui/DriverViewModel.kt index 78ea18e..b6fdf82 100644 --- a/app/src/main/java/pl/firmatpp/kierowca/ui/DriverViewModel.kt +++ b/app/src/main/java/pl/firmatpp/kierowca/ui/DriverViewModel.kt @@ -68,6 +68,9 @@ data class DriverUiState( val startupOfflineAvailable: Boolean = false, val phone: String = "", val otpCode: String = "", + val otpConsentRequestId: Long = 0L, + val otpRequestPending: Boolean = false, + val otpResendAvailableAtEpochMillis: Long = 0L, val maskedPhone: String = "", val driver: DriverDto? = null, val routes: List = emptyList(), @@ -203,6 +206,7 @@ class DriverViewModel(application: Application) : AndroidViewModel(application) private val diagnosticSnapshotProvider = DiagnosticSnapshotProvider(application) private val _state = MutableStateFlow(DriverUiState(loading = true)) private val otpAutoSubmitPolicy = OtpAutoSubmitPolicy() + private val otpAttemptCoordinator = OtpAttemptCoordinator() private val liveSyncClient = DriverLiveSyncClient( repository = repository, onConnected = { viewModelScope.launch { checkRemoteSyncState() } }, @@ -312,18 +316,63 @@ class DriverViewModel(application: Application) : AndroidViewModel(application) } } - fun requestOtp(phone: String) = runLoading("request_otp") { - if (!_state.value.isOnline) throw IOException("Logowanie wymaga połączenia z internetem.") - val response = repository.requestOtp(phone) + fun requestOtp(phone: String) { + val requestSnapshot = _state.value + val now = System.currentTimeMillis() + val resendDelay = if (requestSnapshot.phone == phone) { + otpResendDelaySeconds(requestSnapshot.otpResendAvailableAtEpochMillis, now) + } else { + 0 + } + if (resendDelay > 0) { + _state.update { it.copy(error = "Nowy SMS możesz wysłać za $resendDelay s.") } + return + } + + val isResend = requestSnapshot.screen == DriverScreen.Otp && requestSnapshot.phone == phone + val retainedCode = requestSnapshot.otpCode.takeIf { isResend }.orEmpty() + val attemptId = otpAttemptCoordinator.beginAttempt() otpAutoSubmitPolicy.reset() - _state.update { - it.copy( - screen = DriverScreen.Otp, - phone = phone, - otpCode = "", - maskedPhone = response.phoneMasked.orEmpty(), - error = null, - ) + + viewModelScope.launch { + _state.update { + it.copy( + loading = true, + phone = phone, + otpCode = retainedCode, + otpConsentRequestId = attemptId, + otpRequestPending = true, + error = null, + ) + } + kotlinx.coroutines.yield() + + val result = runCatching { + if (!_state.value.isOnline) throw IOException("Logowanie wymaga połączenia z internetem.") + repository.requestOtp(phone) + } + + if (!otpAttemptCoordinator.isCurrent(attemptId)) return@launch + + result.onSuccess { response -> + val bufferedCode = otpAttemptCoordinator.takeBufferedCode(attemptId) + _state.update { + it.copy( + screen = DriverScreen.Otp, + loading = false, + otpCode = bufferedCode ?: retainedCode, + maskedPhone = response.phoneMasked.orEmpty(), + otpRequestPending = false, + otpResendAvailableAtEpochMillis = System.currentTimeMillis() + OTP_RESEND_COOLDOWN_MILLIS, + error = null, + ) + } + bufferedCode?.let { applyOtpCode(it, retrievedFromSms = true) } + }.onFailure { throwable -> + otpAttemptCoordinator.discard(attemptId) + reportHandledException("request_otp", throwable) + _state.update { it.withApiError(throwable).copy(loading = false, otpRequestPending = false) } + } } } @@ -336,11 +385,35 @@ class DriverViewModel(application: Application) : AndroidViewModel(application) } fun updateOtpCode(code: String) { - val sanitized = code.filter(Char::isDigit).take(6) - _state.update { it.copy(otpCode = sanitized) } + applyOtpCode(sanitizeOtpCode(code), retrievedFromSms = false) + } - if (otpAutoSubmitPolicy.shouldSubmit(sanitized, _state.value.loading)) { - verifyOtp(sanitized) + fun updateOtpCodeFromSms(attemptId: Long, code: String) { + val sanitized = sanitizeOtpCode(code) + if (!isRetrievedOtpCode(sanitized)) return + + val snapshot = _state.value + if (attemptId != snapshot.otpConsentRequestId || !otpAttemptCoordinator.isCurrent(attemptId)) return + + if (snapshot.otpRequestPending) { + otpAttemptCoordinator.bufferRetrievedCode(attemptId, sanitized) + return + } + if (snapshot.screen != DriverScreen.Otp) return + + applyOtpCode(sanitized, retrievedFromSms = true) + } + + private fun applyOtpCode(code: String, retrievedFromSms: Boolean) { + _state.update { it.copy(otpCode = code) } + + val shouldSubmit = if (retrievedFromSms) { + otpAutoSubmitPolicy.shouldSubmitRetrieved(code, _state.value.loading) + } else { + otpAutoSubmitPolicy.shouldSubmit(code, _state.value.loading) + } + if (shouldSubmit) { + verifyOtp(code) } } diff --git a/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAttemptCoordinator.kt b/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAttemptCoordinator.kt new file mode 100644 index 0000000..75a0ecd --- /dev/null +++ b/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAttemptCoordinator.kt @@ -0,0 +1,45 @@ +package pl.firmatpp.kierowca.ui + +class OtpAttemptCoordinator { + private var currentAttemptId: Long = 0L + private var bufferedCode: String? = null + + fun beginAttempt(): Long { + currentAttemptId += 1L + bufferedCode = null + return currentAttemptId + } + + fun isCurrent(attemptId: Long): Boolean = attemptId == currentAttemptId + + fun bufferRetrievedCode(attemptId: Long, code: String): Boolean { + if (!isCurrent(attemptId) || !isRetrievedOtpCode(code)) return false + bufferedCode = code + return true + } + + fun takeBufferedCode(attemptId: Long): String? { + if (!isCurrent(attemptId)) return null + return bufferedCode.also { bufferedCode = null } + } + + fun discard(attemptId: Long) { + if (isCurrent(attemptId)) bufferedCode = null + } +} + +internal fun sanitizeOtpCode(code: String): String = + code.filter(Char::isDigit).take(OTP_MAX_LENGTH) + +internal fun isRetrievedOtpCode(code: String): Boolean = + code.length in OTP_MIN_LENGTH..OTP_MAX_LENGTH && code.all(Char::isDigit) + +internal fun otpResendDelaySeconds(availableAtEpochMillis: Long, nowEpochMillis: Long): Int { + val remainingMillis = (availableAtEpochMillis - nowEpochMillis).coerceAtLeast(0L) + return ((remainingMillis + 999L) / 1_000L).toInt() +} + +internal const val OTP_MIN_LENGTH = 4 +internal const val OTP_DEFAULT_LENGTH = 6 +internal const val OTP_MAX_LENGTH = 10 +internal const val OTP_RESEND_COOLDOWN_MILLIS = 30_000L diff --git a/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicy.kt b/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicy.kt index 4aaf09e..7c093ef 100644 --- a/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicy.kt +++ b/app/src/main/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicy.kt @@ -4,14 +4,20 @@ class OtpAutoSubmitPolicy { private var lastSubmittedCode: String? = null fun shouldSubmit(code: String, loading: Boolean): Boolean { - if (code.length < OTP_LENGTH) { + return shouldSubmitCompleteCode(code, loading, code.length == OTP_DEFAULT_LENGTH) + } + + fun shouldSubmitRetrieved(code: String, loading: Boolean): Boolean { + return shouldSubmitCompleteCode(code, loading, isRetrievedOtpCode(code)) + } + + private fun shouldSubmitCompleteCode(code: String, loading: Boolean, complete: Boolean): Boolean { + if (!complete) { lastSubmittedCode = null return false } - if (loading || code.length != OTP_LENGTH || code == lastSubmittedCode) { - return false - } + if (loading || code == lastSubmittedCode) return false lastSubmittedCode = code return true @@ -20,8 +26,4 @@ class OtpAutoSubmitPolicy { fun reset() { lastSubmittedCode = null } - - private companion object { - const val OTP_LENGTH = 6 - } } diff --git a/app/src/test/java/pl/firmatpp/kierowca/data/ApiErrorMapperTest.kt b/app/src/test/java/pl/firmatpp/kierowca/data/ApiErrorMapperTest.kt index a3adb0e..f4de18d 100644 --- a/app/src/test/java/pl/firmatpp/kierowca/data/ApiErrorMapperTest.kt +++ b/app/src/test/java/pl/firmatpp/kierowca/data/ApiErrorMapperTest.kt @@ -3,12 +3,16 @@ package pl.firmatpp.kierowca.data import java.io.IOException import java.net.UnknownHostException import kotlin.coroutines.cancellation.CancellationException +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.ResponseBody.Companion.toResponseBody import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertSame import org.junit.Assert.assertTrue import org.junit.Assert.fail import org.junit.Test +import retrofit2.HttpException +import retrofit2.Response class ApiErrorMapperTest { @Test @@ -84,4 +88,30 @@ class ApiErrorMapperTest { assertEquals("Storage timeout", error.message) assertEquals(body, error.responseBody) } + + @Test + fun mapsInvalidOtpReasonToActionableMessage() { + val error = ApiErrorMapper.mapHttpStatus(422, """{"ok":false,"reason":"INVALID_OTP"}""") + + assertEquals("INVALID_OTP", error.code) + assertEquals("Kod jest nieprawidłowy lub wygasł. Użyj kodu z najnowszego SMS-a.", error.message) + } + + @Test + fun translatesRateLimitResponses() { + val error = ApiErrorMapper.mapHttpStatus(429, """{"message":"Too Many Attempts."}""") + + assertEquals(ApiErrorKind.RateLimited, error.kind) + assertEquals("Za dużo prób. Odczekaj chwilę i spróbuj ponownie.", error.message) + } + + @Test + fun preservesApiProblemWhenSameExceptionIsMappedMoreThanOnce() { + val body = """{"ok":false,"reason":"INVALID_OTP"}""" + .toResponseBody("application/json".toMediaType()) + val exception = HttpException(Response.error(422, body)) + + assertEquals("INVALID_OTP", ApiErrorMapper.map(exception).code) + assertEquals("INVALID_OTP", ApiErrorMapper.map(exception).code) + } } diff --git a/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAttemptCoordinatorTest.kt b/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAttemptCoordinatorTest.kt new file mode 100644 index 0000000..c7d9f63 --- /dev/null +++ b/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAttemptCoordinatorTest.kt @@ -0,0 +1,48 @@ +package pl.firmatpp.kierowca.ui + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +class OtpAttemptCoordinatorTest { + @Test + fun buffersSmsReceivedBeforeOtpRequestCompletes() { + val coordinator = OtpAttemptCoordinator() + val attemptId = coordinator.beginAttempt() + + assertTrue(coordinator.bufferRetrievedCode(attemptId, "123456")) + assertEquals("123456", coordinator.takeBufferedCode(attemptId)) + assertNull(coordinator.takeBufferedCode(attemptId)) + } + + @Test + fun ignoresConsentResultFromPreviousRequest() { + val coordinator = OtpAttemptCoordinator() + val previousAttempt = coordinator.beginAttempt() + val currentAttempt = coordinator.beginAttempt() + + assertFalse(coordinator.bufferRetrievedCode(previousAttempt, "111111")) + assertTrue(coordinator.bufferRetrievedCode(currentAttempt, "222222")) + assertEquals("222222", coordinator.takeBufferedCode(currentAttempt)) + } + + @Test + fun acceptsOnlyOtpLengthsSupportedByApi() { + val coordinator = OtpAttemptCoordinator() + val attemptId = coordinator.beginAttempt() + + assertFalse(coordinator.bufferRetrievedCode(attemptId, "123")) + assertTrue(coordinator.bufferRetrievedCode(attemptId, "1234")) + assertTrue(coordinator.bufferRetrievedCode(attemptId, "1234567890")) + assertFalse(coordinator.bufferRetrievedCode(attemptId, "12345678901")) + } + + @Test + fun roundsResendCooldownUpToFullSeconds() { + assertEquals(30, otpResendDelaySeconds(40_000L, 10_000L)) + assertEquals(1, otpResendDelaySeconds(10_001L, 10_000L)) + assertEquals(0, otpResendDelaySeconds(10_000L, 10_001L)) + } +} diff --git a/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicyTest.kt b/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicyTest.kt index 2e464de..6778108 100644 --- a/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicyTest.kt +++ b/app/src/test/java/pl/firmatpp/kierowca/ui/OtpAutoSubmitPolicyTest.kt @@ -38,4 +38,13 @@ class OtpAutoSubmitPolicyTest { assertFalse(policy.shouldSubmit("123456", loading = true)) assertFalse(policy.shouldSubmit("1234567", loading = false)) } + + @Test + fun submitsCompleteRetrievedCodesAcceptedByApi() { + val policy = OtpAutoSubmitPolicy() + + assertTrue(policy.shouldSubmitRetrieved("1234", loading = false)) + assertTrue(policy.shouldSubmitRetrieved("1234567890", loading = false)) + assertFalse(policy.shouldSubmitRetrieved("123", loading = false)) + } }