-
Notifications
You must be signed in to change notification settings - Fork 3
chore: update-routes #365
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
chore: update-routes #365
Conversation
|
View your CI Pipeline Execution ↗ for commit 73f522d
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
- Coverage 57.58% 55.47% -2.11%
==========================================
Files 32 32
Lines 2044 2044
Branches 321 341 +20
==========================================
- Hits 1177 1134 -43
- Misses 867 910 +43 🚀 New features to boost your workflow:
|
|
Deployed 9cd81fe to https://ForgeRock.github.io/ping-javascript-sdk/pr-365/9cd81feda4202aa1717199bc80b1406815490966 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/oidc-client - 21.1 KB (+3.1 KB, +17.4%) 📊 Minor Changes📈 @forgerock/sdk-types - 5.9 KB (+0.0 KB) ➖ No Changes➖ @forgerock/protect - 152.3 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
50b93b6 to
c731902
Compare
|
c731902 to
57e2e6b
Compare
cerebrl
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.
Just left a few comments.
| const acr_value = urlParams?.acr_values ?? ''; | ||
|
|
||
| const response = yield* handleAuthorize(urlParams); | ||
| if (!acr_value) { |
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.
Why are we requiring the use of acr_values?
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.
because i have no way of directing the code to a given flow, unless I provide a default.
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.
We could have a fallback, but I see what you're saying. We can address this later.
| const acr_value = urlParams?.acr_values ?? ''; | ||
| console.log('acr_value', acr_value); | ||
|
|
||
| if (!acr_value) { | ||
| return yield* Effect.fail(new HttpApiError.NotFound()); | ||
| } |
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.
Are ACR Values used within this route? I thought ACR was an /authorize.
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.
I need to know what flow to pull from, unless we keep the state in another cookie, i have no way of mapping the current request to the responseMap.
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.
I think a cookie is a good idea, but let's not worry about it right now.
| */ | ||
| if (!stepIndexCookie) { | ||
| console.log('no step index'); | ||
| return yield* Effect.fail(new HttpApiError.NotFound()); |
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 this error be more detailed, rather than just 404?
| const ProtectSDKRequestFormData = Schema.Struct({ | ||
| value: Schema.Struct({ | ||
| protectsdk: Schema.String, | ||
| }), | ||
| }); |
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.
We can remove this now as this has been removed from DaVinci was never released.
d582832 to
4817b73
Compare
cerebrl
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.
I'd still like to remove the old protectsdk field from this as it is no longer relevant and removed from DaVinci, but this looks good otherwise.
4817b73 to
73f522d
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4260
https://pingidentity.atlassian.net/browse/SDKS-4263
Description
Update routes, clean up issues. Needs further testing and addition of responses.