Jenmaddox
commented
1 year ago Why do I keep getting these emails?
@.***
On Oct 20, 2022, at 11:15 AM, Jay Newstrom @.***> wrote:
@JayNewstrom commented on this pull request.
In totp/src/main/java/com/okta/totp/ui/theme/Theme.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000814421:
- primary = Purple500,
- primaryVariant = Purple700,
- secondary = Teal200
- /* Other default colors to override
- background = Color.White,
- surface = Color.White,
- onPrimary = Color.White,
- onSecondary = Color.Black,
- onBackground = Color.Black,
- onSurface = Color.Black,
- */ +)
@.*** +fun SamplesandroidTheme(
Can you rename this to SamplesAndroidTheme
In totp/src/main/java/com/okta/totp/ui/theme/Theme.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000814759:
+import androidx.compose.material.darkColors +import androidx.compose.material.lightColors +import androidx.compose.runtime.Composable + +private val DarkColorPalette = darkColors(
- primary = Purple200,
- primaryVariant = Purple700,
- secondary = Teal200 +)
+private val LightColorPalette = lightColors(
- primary = Purple500,
- primaryVariant = Purple700,
- secondary = Teal200
- /* Other default colors to override
We should remove commented out code (there are a few other places like this as well).
In totp/src/main/java/com/okta/totp/ui/theme/Color.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000816539:
@@ -0,0 +1,8 @@ +package com.okta.totp.ui.theme
Need to add copyright headers.
In totp/src/main/java/com/okta/totp/barcode_scan/BarcodeScanScreen.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000818923:
+import androidx.core.content.ContextCompat +import com.google.accompanist.permissions.ExperimentalPermissionsApi +import com.google.accompanist.permissions.PermissionStatus +import com.google.accompanist.permissions.rememberPermissionState +import com.google.accompanist.permissions.shouldShowRationale +import com.google.mlkit.vision.barcode.BarcodeScanner +import com.google.mlkit.vision.barcode.BarcodeScanning + @.*** +fun BarcodeScanScreen(barcodeScanViewModel: BarcodeScanViewModel, onNavigateToOtpScreen: () -> Unit) {
- Scaffold(
- topBar = {
- Row(modifier = Modifier.background(color = Color.LightGray)) {
- Icon(
- imageVector = Icons.Default.ArrowBack,
- contentDescription = "Go Back",
We should add these to strings.xml (there's a lot of these, but I'll just tag this one).
In totp/src/main/java/com/okta/totp/barcode_scan/BarcodeScanScreen.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000821732:
+import com.google.accompanist.permissions.rememberPermissionState +import com.google.accompanist.permissions.shouldShowRationale +import com.google.mlkit.vision.barcode.BarcodeScanner +import com.google.mlkit.vision.barcode.BarcodeScanning + @.*** +fun BarcodeScanScreen(barcodeScanViewModel: BarcodeScanViewModel, onNavigateToOtpScreen: () -> Unit) {
- Scaffold(
- topBar = {
- Row(modifier = Modifier.background(color = Color.LightGray)) {
- Icon(
- imageVector = Icons.Default.ArrowBack,
- contentDescription = "Go Back",
- modifier = Modifier
- .clickable {
- onNavigateToOtpScreen()
The syntax for this can be simplified to remove the extra function reference: .clickable(onClick = onNavigateToOtpScreen)
In totp/src/main/java/com/okta/totp/barcode_scan/BarcodeScanScreen.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000829115:
- "The camera is important for this app. Please grant the permission."
- } else {
- "Camera permission required for this feature to be available. " +
- "Please grant the permission"
- }
- Text(textToShow)
- Button(onClick = { cameraPermissionState.launchPermissionRequest() }) {
- Text("Request permission")
- }
- }
- }
- } +}
@.*** +fun CameraPreview(
This code looks like it's quite prone to errors and/or leaking. Please run some manual tests here relating to app switching, rotation, and screen on/off.
In totp/src/main/java/com/okta/totp/parsing/OtpUriParser.kthttps://github.com/okta/samples-android/pull/91#discussion_r1000847460:
- name = label[1].trim('/')
- }
- else -> return OtpUriParsingResults.Error(
- "Totp URI has invalid label"
- )
- }
- val paramNames = uri.queryParameterNames
- if (!paramNames.contains(SECRET)) {
- return OtpUriParsingResults.Error(
- "OTP URI is missing required parameter: secret"
- )
- }
- if (paramNames.contains(ISSUER)) {
- val issuerParam = uri.getQueryParameter(ISSUER)!!
This isn't safe. The query param could have no value, even if it exists as a key. foo=&bar=with_a_value. It might be better to do uri.getQueryParameter(ISSUER)?.let { to prevent an error like this.
— Reply to this email directly, view it on GitHubhttps://github.com/okta/samples-android/pull/91#pullrequestreview-1149524244, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARQAW2QJU3ZKS653SZEPJGDWEFV2DANCNFSM6AAAAAARJISQTI. You are receiving this because you are subscribed to this thread.Message ID: @.***>
JayNewstrom
rajdeepnanua-okta
Still have to add unit tests
But, I decided to put up a draft PR to show the general approach for the sample. This app was also tested by scanning the same QR code in both Google authenticator, and totp app. Both generated the same results for the default period, digits, and algorithm parameters. This app also allows customizing period, digits, and algorithm. I couldn't find a good test with those parameters as non-default values.