feat(davinci-client): support form agreements with AgreementCollector#579
feat(davinci-client): support form agreements with AgreementCollector#579
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds support for form agreements through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 6c13d93
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/davinci-client/src/lib/collector.types.ts (1)
485-555:⚠️ Potential issue | 🟡 MinorAlign the exported
NoValueCollector<T>generic withAgreementCollectorspecifics.The generic
NoValueCollector<T>on line 555 is exported in the public API but still aliasesNoValueCollectorBase<T>, which meansNoValueCollector<'AgreementCollector'>resolves to an incomplete type missing the requiredtitleEnabled,title,agreement, andenabledfields. While the internal codebase correctly usesInferNoValueCollectorType<T>which handles this, external consumers of the public type could inadvertently use the incorrect generic path.Proposed type alignment
-export type NoValueCollector<T extends NoValueCollectorTypes> = NoValueCollectorBase<T>; +export type NoValueCollector<T extends NoValueCollectorTypes> = + T extends 'QrCodeCollector' + ? QrCodeCollectorBase + : T extends 'AgreementCollector' + ? AgreementCollector + : NoValueCollectorBase<T>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.types.ts` around lines 485 - 555, The exported generic NoValueCollector<T> currently aliases NoValueCollectorBase<T>, which makes NoValueCollector<'AgreementCollector'> miss AgreementCollector-specific fields; change the exported type definition of NoValueCollector<T> to use the existing conditional helper InferNoValueCollectorType<T> (i.e., export type NoValueCollector<T extends NoValueCollectorTypes> = InferNoValueCollectorType<T>) so consumers get the correct narrowed types for 'AgreementCollector' and others while keeping NoValueCollectorBase, QrCodeCollectorBase and AgreementCollector definitions unchanged.packages/davinci-client/src/lib/collector.utils.ts (1)
732-758:⚠️ Potential issue | 🟡 MinorInconsistent null-safety on agreement output fields.
field.agreement?.id ?? '',useDynamicAgreement ?? false, andenabled ?? falseall defensively handle missing values, buttitleEnabledandtitleare assigned raw. Given the existing "missing content" error path is tolerant of malformed fields (see thereturnAgreementCollectorerror-case test which passes{ type: 'AGREEMENT', key: '...' }), these two properties can end up asundefinedat runtime while theAgreementCollectorinterface declares them asboolean/string(non-optional).Either tighten the defaults here or mark them optional on the type — whichever better matches the DaVinci contract.
🔧 Suggested defensive defaults
if (collectorType === 'AgreementCollector' && field.type === 'AGREEMENT') { output = { - titleEnabled: field.titleEnabled, - title: field.title, + titleEnabled: field.titleEnabled ?? false, + title: field.title ?? '', agreement: { id: field.agreement?.id ?? '', useDynamicAgreement: field.agreement?.useDynamicAgreement ?? false, }, enabled: field.enabled ?? false, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 732 - 758, The AgreementCollector output can yield undefined for titleEnabled and title while other fields use safe defaults; update the output construction in the AgreementCollector branch (the object built when collectorType === 'AgreementCollector' and field.type === 'AGREEMENT') to apply defensive defaults like titleEnabled: field.titleEnabled ?? false and title: field.title ?? '' (keep agreement.id, agreement.useDynamicAgreement, and enabled defaults as-is) so the produced object matches the AgreementCollector shape expected by InferNoValueCollectorType<CollectorType>.
🧹 Nitpick comments (3)
packages/davinci-client/src/lib/collector.utils.test.ts (1)
888-929: Consider adding a test for nullish-fallback behavior.The happy path and the missing-
contenterror case are covered, butreturnNoValueCollector's AGREEMENT branch has explicit nullish coalescing forfield.agreement?.id ?? '',agreement?.useDynamicAgreement ?? false, andfield.enabled ?? false. None of those fallbacks are exercised here. A test with a field missingagreement/enabledwould lock in the documented behavior and catch regressions if the fallbacks are ever changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.test.ts` around lines 888 - 929, Add a test exercising the nullish-fallbacks in returnAgreementCollector (the AGREEMENT branch of returnNoValueCollector): create a mock AgreementField that omits the agreement and enabled properties (e.g., { type: 'AGREEMENT', key: 'agreement-field', content: 'label' }) call returnAgreementCollector(mockField, 0) and assert the resulting output.agreement.id === '' , output.agreement.useDynamicAgreement === false, and output.enabled === false (also assert id/name follow the '-0' suffix as in other tests) to lock in the documented fallback behavior.packages/davinci-client/src/lib/collector.utils.ts (1)
720-744: Consider extracting per-type output builders as this union grows.
returnNoValueCollectornow handles three distinct field shapes (ReadOnly, QrCode viareturnQrCodeCollector, Agreement here) with alet output = {}+ conditional block pattern. As more NoValueCollector variants are added (e.g., the deferredSINGLE_CHECKBOX), this will get harder to keep coherent withInferNoValueCollectorType. A small per-type builder (mirroring howreturnQrCodeCollectorlayers on top of the base) would scale better than stacking moreif (collectorType === ...)branches inside the base factory.Non-blocking — safe to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/collector.utils.ts` around lines 720 - 744, returnNoValueCollector currently builds different variant outputs inline (using let output = {} and conditional branches for Agreement and others) which will not scale; extract per-type builder functions (e.g., buildAgreementCollector, buildReadOnlyCollector) that accept the Field and idx and return the specific output shape, then have returnNoValueCollector delegate to these builders (or use a map keyed by CollectorType) and call returnQrCodeCollector where appropriate to keep layering consistent; update signatures and any type guards to use those new builders and remove the growing collectorType conditional blocks inside returnNoValueCollector.packages/davinci-client/src/lib/node.reducer.test.ts (1)
455-497: LGTM — AGREEMENT reducer coverage matches the collector contract.Expected collector shape aligns with
AgreementCollector(category, type, id/name with-0suffix, and full output mapping).Nit: unlike the other
node/nexttests in this suite, the payload omitsformData. Not a functional problem since the reducer path for AGREEMENT doesn't consume it, but addingformData: {}would keep the test payloads uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/node.reducer.test.ts` around lines 455 - 497, The test "should handle AGREEMENT field type" omits formData in the action payload which breaks uniformity with other node/next tests; update the payload passed to nodeCollectorReducer in this test to include formData: {} (i.e., add formData: {} inside action.payload) so the test payloads are consistent while leaving nodeCollectorReducer and AgreementCollector expectations unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/davinci-client/src/lib/collector.types.ts`:
- Around line 485-555: The exported generic NoValueCollector<T> currently
aliases NoValueCollectorBase<T>, which makes
NoValueCollector<'AgreementCollector'> miss AgreementCollector-specific fields;
change the exported type definition of NoValueCollector<T> to use the existing
conditional helper InferNoValueCollectorType<T> (i.e., export type
NoValueCollector<T extends NoValueCollectorTypes> =
InferNoValueCollectorType<T>) so consumers get the correct narrowed types for
'AgreementCollector' and others while keeping NoValueCollectorBase,
QrCodeCollectorBase and AgreementCollector definitions unchanged.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 732-758: The AgreementCollector output can yield undefined for
titleEnabled and title while other fields use safe defaults; update the output
construction in the AgreementCollector branch (the object built when
collectorType === 'AgreementCollector' and field.type === 'AGREEMENT') to apply
defensive defaults like titleEnabled: field.titleEnabled ?? false and title:
field.title ?? '' (keep agreement.id, agreement.useDynamicAgreement, and enabled
defaults as-is) so the produced object matches the AgreementCollector shape
expected by InferNoValueCollectorType<CollectorType>.
---
Nitpick comments:
In `@packages/davinci-client/src/lib/collector.utils.test.ts`:
- Around line 888-929: Add a test exercising the nullish-fallbacks in
returnAgreementCollector (the AGREEMENT branch of returnNoValueCollector):
create a mock AgreementField that omits the agreement and enabled properties
(e.g., { type: 'AGREEMENT', key: 'agreement-field', content: 'label' }) call
returnAgreementCollector(mockField, 0) and assert the resulting
output.agreement.id === '' , output.agreement.useDynamicAgreement === false, and
output.enabled === false (also assert id/name follow the '-0' suffix as in other
tests) to lock in the documented fallback behavior.
In `@packages/davinci-client/src/lib/collector.utils.ts`:
- Around line 720-744: returnNoValueCollector currently builds different variant
outputs inline (using let output = {} and conditional branches for Agreement and
others) which will not scale; extract per-type builder functions (e.g.,
buildAgreementCollector, buildReadOnlyCollector) that accept the Field and idx
and return the specific output shape, then have returnNoValueCollector delegate
to these builders (or use a map keyed by CollectorType) and call
returnQrCodeCollector where appropriate to keep layering consistent; update
signatures and any type guards to use those new builders and remove the growing
collectorType conditional blocks inside returnNoValueCollector.
In `@packages/davinci-client/src/lib/node.reducer.test.ts`:
- Around line 455-497: The test "should handle AGREEMENT field type" omits
formData in the action payload which breaks uniformity with other node/next
tests; update the payload passed to nodeCollectorReducer in this test to include
formData: {} (i.e., add formData: {} inside action.payload) so the test payloads
are consistent while leaving nodeCollectorReducer and AgreementCollector
expectations unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd168374-d95a-460f-adfd-0ec59e5d8d27
📒 Files selected for processing (12)
.changeset/silent-ideas-joke.mde2e/davinci-app/components/agreement.tse2e/davinci-app/main.tspackages/davinci-client/src/lib/collector.types.test-d.tspackages/davinci-client/src/lib/collector.types.tspackages/davinci-client/src/lib/collector.utils.test.tspackages/davinci-client/src/lib/collector.utils.tspackages/davinci-client/src/lib/davinci.types.tspackages/davinci-client/src/lib/node.reducer.test.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/node.types.test-d.tspackages/davinci-client/src/lib/node.types.ts
🦋 Changeset detectedLatest commit: 6c13d93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
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 |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (15.73%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
===========================================
- Coverage 70.90% 15.73% -55.17%
===========================================
Files 53 154 +101
Lines 2021 26683 +24662
Branches 377 1133 +756
===========================================
+ Hits 1433 4199 +2766
- Misses 588 22484 +21896
🚀 New features to boost your workflow:
|
|
Deployed 173b0eb to https://ForgeRock.github.io/ping-javascript-sdk/pr-579/173b0ebb62f79484c55d44b07a63f5130bca7062 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-9.9 KB, -100.0%) 📊 Minor Changes📉 @forgerock/device-client - 9.9 KB (-0.0 KB) ➖ No Changes➖ @forgerock/protect - 150.1 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Looks good! Just left a minor comment!
Able to see the agreement collector on sign in page with this client id: http://localhost:5829/?clientId=60de77d5-dd2c-41ef-8c40-f8bb2381a359
| */ | ||
| export function returnNoValueCollector< | ||
| Field extends ReadOnlyField | QrCodeField, | ||
| Field extends ReadOnlyField | QrCodeField | AgreementField, |
There was a problem hiding this comment.
Should we update this extends to be on the ReadOnlyFields type
export type ReadOnlyFields = ReadOnlyField | QrCodeField | AgreementField;
ancheetah
force-pushed
the
SDKS-4691-agreements
branch
from
2690d5b to
6c13d93
Compare
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4691
Description
Adds a new NoValueCollector called AgreementCollector that stores AGREEMENT field options in the
output. Includes unit tests. Agreement component has been added to e2e app but e2e tests will be handled in another ticket once we can create more flows in DaVinci. SINGLE_CHECKBOX will be addressed in another ticket.Summary by CodeRabbit
Release Notes
New Features
Tests