Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PRISM-10044 - Add Risk package #264

Conversation

precious-ossai-cko
Copy link
Contributor

@precious-ossai-cko precious-ossai-cko commented Feb 14, 2024

Issue

https://checkout.atlassian.net/browse/PRISM-10044

Proposed changes

Add Risk package to Frames

Test Steps

If there's any functionality change, please list a step by step guide of how to verify the changes, and/or upload a screen recording for any visible changes.

For instance:

  1. Go to the main screen
  2. Tap on primary button
  3. Verify the image is shown on the screen

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • Reviewers assigned
  • I have performed a self-review of my code and manual testing
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if applicable)

Further comments

@fabio-insolia-cko
Copy link
Contributor

@precious-ossai-cko
This build is currently failing our CI.
Some of the compiling issues can be solved in the EventLogger.kt file by changing the following

override fun resetSession() {
        val correlationId = UUID.randomUUID().toString()
        logger.addMetadata(MetadataKey.correlationId, correlationId)
        sentLogs.clear()
    }

Other issues that I found are related to ktlintCheck here

@fabio-insolia-cko
Copy link
Contributor

@jheng-hao-lin-cko @chintan-soni-cko
We had a couple of tests that, after the Risk implementation, failed because of kotlinx.coroutines.test.UncompletedCoroutinesError at TestBuilders.kt:349.
While I was not able to associate this misbehaviour between the tests and the Risk library, I have added an UnconfinedTestDispatcher for those failing unit tests and now they should work, however, I want to hear your opinion about this fix.

@jheng-hao-lin-cko
Copy link
Contributor

@jheng-hao-lin-cko @chintan-soni-cko We had a couple of tests that, after the Risk implementation, failed because of kotlinx.coroutines.test.UncompletedCoroutinesError at TestBuilders.kt:349. While I was not able to associate this misbehaviour between the tests and the Risk library, I have added an UnconfinedTestDispatcher for those failing unit tests and now they should work, however, I want to hear your opinion about this fix.

Have a quick check, looks like those test failed given the fact that Kotlin coroutines version is upgraded (1.4.2 -> 1.7.3) by the event logger 2.0.2, and the original composed tests wasn't applied anymore.

Currently Risk SDK uses event logger 2.0.2, while Frames uses 1.0.1, they are using different version of coroutines.

I would suggest to update the Risk SDK to use event logger 1.0.1, so it's compitable with Frames without any issue :)

@jheng-hao-lin-cko
Copy link
Contributor

Here is some sample code to increase unit tests for RiskSdkUseCase

public class RiskSdkUseCaseTest {
    private val environment: RiskEnvironment = RiskEnvironment.SANDBOX
    private val riskInstanceProvider: RiskInstanceProvider = mockk()
    private val riskInstance: Risk = mockk()
    private val context: Context = mockk()
    private val tokenDetails: TokenDetails = mockk()
    private lateinit var useCase: RiskSdkUseCase

    @BeforeEach
    public fun setup() {
        coEvery { tokenDetails.token } returns TOKEN
        coEvery { riskInstanceProvider.provide(context, PUBLIC_KEY, environment) } returns riskInstance
        useCase = RiskSdkUseCase(
            environment = Environment.SANDBOX,
            context = context,
            publicKey = PUBLIC_KEY,
            riskInstanceProvider = riskInstanceProvider,
        )
    }

    @Test
    public fun `Success result should trigger publishData`() {
        runBlocking {
            useCase.execute(TokenResult.Success(tokenDetails))
            coVerify { riskInstanceProvider.provide(context, PUBLIC_KEY, environment) }
            coVerify { riskInstance.publishData(TOKEN) }
        }
    }

    @Test
    public fun `Failed result should not trigger publishData`() {
        runBlocking {
            launch {
                useCase.execute(TokenResult.Failure(CheckoutError("error", "message")))
                coVerify(exactly = 0) { riskInstance.publishData(any()) }
            }
        }
    }

    private companion object {
        private const val PUBLIC_KEY: String = "pk_123"
        private const val TOKEN = "TOKEN"
    }
}

@chintan-soni-cko
Copy link
Contributor

@jheng-hao-lin-cko @chintan-soni-cko We had a couple of tests that, after the Risk implementation, failed because of kotlinx.coroutines.test.UncompletedCoroutinesError at TestBuilders.kt:349. While I was not able to associate this misbehaviour between the tests and the Risk library, I have added an UnconfinedTestDispatcher for those failing unit tests and now they should work, however, I want to hear your opinion about this fix.

Have a quick check, looks like those test failed given the fact that Kotlin coroutines version is upgraded (1.4.2 -> 1.7.3) by the event logger 2.0.2, and the original composed tests wasn't applied anymore.

Currently Risk SDK uses event logger 2.0.2, while Frames uses 1.0.1, they are using different version of coroutines.

I would suggest to update the Risk SDK to use event logger 1.0.1, so it's compitable with Frames without any issue :)

+1 to @jheng-hao-lin-cko for Logger, we intentionally used an older version and not 2.0.2 because 2.0.2 does have Certificate transparency which we don't require for frames, resulting in benefits by having reduced SDK size.

@fabio-insolia-cko fabio-insolia-cko force-pushed the PRISM-10044-risk-android-is-integrated-within-frames-android branch from 7798cab to 5317328 Compare March 7, 2024 15:38
@jheng-hao-lin-cko
Copy link
Contributor

https://github.com/checkout/frames-android/actions/runs/8204617782/job/22439672388?pr=264

Run ./gradlew :checkout:ktlint locally to verify whether the lint has been fixed or how to fix

opt + cmd + L should auto-format the code for the most cases

@fabio-insolia-cko
Copy link
Contributor

@precious-ossai-cko please fix the Ktlint issues by doing
opt + cmd + L on the file that is giving issues

or doing it manually
here is the list

Task :checkout:ktlint

/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:61:1: Unexpected indentation (16) (should be 12) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:62:1: Unexpected indentation (20) (should be 16) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:63:1: Unexpected indentation (20) (should be 16) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:64:1: Unexpected indentation (20) (should be 16) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:65:1: Unexpected indentation (20) (should be 16) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:66:1: Unexpected indentation (16) (should be 12) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:68:1: Unexpected indentation (16) (should be 12) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:69:1: Unexpected indentation (20) (should be 16) (standard:indent)
/home/runner/work/frames-android/frames-android/checkout/src/main/java/com/checkout/CheckoutApiServiceFactory.kt:70:1: Unexpected indentation (16) (should be 12) (standard:indent)

Copy link

@jheng-hao-lin-cko jheng-hao-lin-cko merged commit 6b87c41 into master Mar 15, 2024
28 checks passed
@jheng-hao-lin-cko jheng-hao-lin-cko deleted the PRISM-10044-risk-android-is-integrated-within-frames-android branch March 15, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants