-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor PingOneProtectInitializeCallback #153
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #153 +/- ##
=============================================
- Coverage 47.66% 47.60% -0.07%
+ Complexity 1198 1195 -3
=============================================
Files 293 293
Lines 8553 8548 -5
Branches 1166 1166
=============================================
- Hits 4077 4069 -8
- Misses 3958 3960 +2
- Partials 518 519 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nd add agent configuration options
…tion properties and assertions
ed45f1a to
d1ed404
Compare
…es from test cases
spetrov
left a comment
There was a problem hiding this 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 - the implementation follows the requirement in the ticket.
However, the ticket seems to be a bit misleading, and some of the new requirements appear to be not applicable to the native SDKs, so let's clarify the questions I put in my comments before merging the PR...
| private set | ||
|
|
||
| var lazyMetadata: Boolean = false | ||
| var agentIdentification: Boolean = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, it is not clear to me if we should include these new 3 "agent" fields - according the a comment in the ticket:
Agent configuration is only applicable to web SDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the fields then? If they're not being used in our SDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vibhorgoswami, it looks like there are many moving parts on the server side. From what I see the callback changes are not finalized... we better hold on with these changes until we get clarity on the requirements...
protect/src/main/kotlin/com/pingidentity/protect/journey/PingOneProtectInitializeCallback.kt
Outdated
Show resolved
Hide resolved
...ect/src/test/kotlin/com/pingidentity/protect/journey/PingOneProtectInitializeCallbackTest.kt
Outdated
Show resolved
Hide resolved
…es and update related tests
JIRA Ticket
SDKS-4548
Description
Fix the properties on the callback