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

feat: social-login #128

Merged
merged 10 commits into from
Mar 6, 2025
Merged

feat: social-login #128

merged 10 commits into from
Mar 6, 2025

Conversation

ryanbas21
Copy link
Collaborator

@ryanbas21 ryanbas21 commented Feb 24, 2025

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-3718

Description

adds social login via davinci/pingone. supports Google and Facebook, while Apple should work, there are errors being reported to davinci team.

Copy link

changeset-bot bot commented Feb 24, 2025

🦋 Changeset detected

Latest commit: f9066bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@forgerock/davinci-client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

nx-cloud bot commented Feb 24, 2025

View your CI Pipeline Execution ↗ for commit f9066bf.

Command Status Duration Result
nx affected -t build typecheck lint test e2e-ci ✅ Succeeded 1m 9s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-03-06 09:31:05 UTC

@ryanbas21 ryanbas21 force-pushed the sdks-3718 branch 3 times, most recently from 867baf3 to e3fbe10 Compare February 25, 2025 00:51
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 17.35537% with 100 lines in your changes missing coverage. Please review.

Project coverage is 50.00%. Comparing base (ad2c943) to head (f9066bf).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
packages/davinci-client/src/lib/davinci.api.ts 0.00% 52 Missing ⚠️
packages/davinci-client/src/lib/davinci.utils.ts 3.22% 30 Missing ⚠️
packages/davinci-client/src/lib/client.store.ts 0.00% 17 Missing ⚠️
packages/davinci-client/src/lib/node.reducer.ts 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (17.35%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   47.32%   50.00%   +2.67%     
==========================================
  Files          30       21       -9     
  Lines        1327     1232      -95     
  Branches      169      163       -6     
==========================================
- Hits          628      616      -12     
+ Misses        699      616      -83     
Files with missing lines Coverage Δ
packages/davinci-client/src/lib/collector.types.ts 100.00% <ø> (ø)
packages/davinci-client/src/lib/collector.utils.ts 82.89% <100.00%> (-0.50%) ⬇️
packages/davinci-client/src/lib/davinci.types.ts 100.00% <ø> (ø)
packages/davinci-client/src/lib/node.slice.ts 79.27% <ø> (ø)
packages/davinci-client/src/lib/node.types.ts 100.00% <ø> (ø)
packages/davinci-client/src/types.ts 0.00% <ø> (ø)
packages/davinci-client/src/lib/node.reducer.ts 76.13% <0.00%> (ø)
packages/davinci-client/src/lib/client.store.ts 0.00% <0.00%> (ø)
packages/davinci-client/src/lib/davinci.utils.ts 71.32% <3.22%> (-18.85%) ⬇️
packages/davinci-client/src/lib/davinci.api.ts 0.00% <0.00%> (ø)

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -17,14 +17,23 @@ const config: PlaywrightTestConfig = {
trace: process.env.CI ? 'on-first-retry' : 'retain-on-failure',
},
webServer: [
process.env.CI == 'false'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added this for the watch-deps so that changes are reloaded when running playwrights ui mode

@ryanbas21 ryanbas21 marked this pull request as ready for review February 25, 2025 22:00
Copy link
Contributor

github-actions bot commented Feb 25, 2025

Deployed fc9bea7 to https://ForgeRock.github.io/ping-javascript-sdk/pr-128/fc9bea7ead1c0da4e5ae6f137a4cf810b9ff845c branch gh-pages in ForgeRock/ping-javascript-sdk

Comment on lines 49 to 60
if (collectorType === 'SocialLoginCollector' && 'links' in field) {
/**
* Social Login Collector will not have a `key`
* So we should _always_ have this error
* when building this collector,
* so lets reset it if we do.
*
*
* Until Davinci team adds a key that is.
*/
if (error.includes('Key is not found in the field object. ')) {
error = '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just pull this out of the returnActionCollector? Maybe this should use its own function. Technically, it's quite different as all the other actions do not result in redirects.

* @param collector @SocialLoginCollector
* @returns unknown
*/
socialLoginHandler: (collector: SocialLoginCollector) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method should return a function that can be called when the button is clicked. It should match the same patter as the other "collector" update methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make sure our naming is consistent with native. I think they use externalIdP or something like that. Can you check?

@ryanbas21 ryanbas21 force-pushed the sdks-3718 branch 3 times, most recently from bf70538 to aaf4fe9 Compare March 3, 2025 19:21
we added a new type, which made links not guaranteed on
the fields object
saving work at this point google social login works.
fix tests from previous commit
fix the tests
small pr to update type misses and rename the top level api handler
refactor social login
Copy link
Collaborator

@cerebrl cerebrl left a comment

Choose a reason for hiding this comment

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

I think this looks good, but we might want to make sure the collector itself is also named consistently with mobile. We can do this in a later story if desired.

@cerebrl cerebrl merged commit c2ba0e7 into main Mar 6, 2025
3 checks passed
@cerebrl cerebrl deleted the sdks-3718 branch March 6, 2025 09:36
@ryanbas21 ryanbas21 mentioned this pull request Mar 6, 2025
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.

3 participants