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

PingProtect Implementation #391

Merged
merged 3 commits into from
Feb 21, 2024
Merged

PingProtect Implementation #391

merged 3 commits into from
Feb 21, 2024

Conversation

jeyanthanperiyasamy
Copy link
Contributor

JIRA Ticket

Please link jira ticket here

Description

Briefly describe the change and any information that would help speedup the review and testing.

@jeyanthanperiyasamy jeyanthanperiyasamy force-pushed the SDKS-2900 branch 5 times, most recently from c690693 to c475b58 Compare February 2, 2024 19:00
@jeyanthanperiyasamy jeyanthanperiyasamy force-pushed the SDKS-2900 branch 2 times, most recently from 5176bb5 to a1395d3 Compare February 5, 2024 04:50
@@ -93,7 +94,6 @@ dependencies {

def coroutine_version = '1.7.2'
implementation "org.jetbrains.kotlinx:kotlinx-coroutines-android:$coroutine_version"
implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutine_version"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we dont need this anymore ..all the coroutines are part of org.jetbrains.kotlinx:kotlinx-coroutines-android:

customCSSFile = projectDir.toString() + "/dokka/fr-backstage-styles.css"
customLogoFile = projectDir.toString() + "/dokka/logo-icon.svg"
customTemplatesFolder = file(projectDir.toString() + "/dokka/templates")
}
repositories {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved all the repositories to settings.xml . @spetrov we may need to test the maven release to make sure this change works

@@ -25,6 +25,5 @@ android.useAndroidX=true
GROUP=org.forgerock
VERSION=4.2.0
VERSION_CODE=18
android.defaults.buildfeatures.buildconfig=true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is deprecated , so moved this buildconfig to build.gradle

witrisna
witrisna previously approved these changes Feb 5, 2024
Copy link
Contributor

@witrisna witrisna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, minor comment for the sample App.

FRAuth.start(context, options)
current = options

CoroutineScope(Dispatchers.Default).launch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May remove this from the sample, what about create another navigation menu to initialize the Protect SDK?

Copy link
Contributor

@spetrov spetrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 👍🏻

private val TAG = PingOneProtectInitCallback::class.java.simpleName

/**
* Callback to initialize the ping one protect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use consistent naming across the docs - i.e. PingOne Protect instead od "ping one protect"

Copy link
Contributor

@rodrigoareis rodrigoareis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good. Minor comments to be addressed.

@@ -12,124 +12,135 @@ import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.setValue
import androidx.lifecycle.ViewModel
import kotlinx.coroutines.CoroutineScope
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update copyright

@jeyanthanperiyasamy
Copy link
Contributor Author

comments addressed .

@jeyanthanperiyasamy jeyanthanperiyasamy force-pushed the SDKS-2900 branch 2 times, most recently from 39a48b2 to 0c18f8e Compare February 7, 2024 17:18
witrisna
witrisna previously approved these changes Feb 7, 2024
rodrigoareis
rodrigoareis previously approved these changes Feb 7, 2024
Copy link
Contributor

@rodrigoareis rodrigoareis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good to me

/**
* Callback to initialize the ping one protect
*/
open class PingOneProtectInitCallback : AbstractCallback {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this class be called PingOneProtectInitializeCallback?

@spetrov spetrov merged commit 07f649e into develop Feb 21, 2024
5 of 6 checks passed
@spetrov spetrov deleted the SDKS-2900 branch February 21, 2024 20:41
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