feat(davinci-client): support form agreements with AgreementCollector#579
feat(davinci-client): support form agreements with AgreementCollector#579
Conversation
📝 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 docstrings
🧪 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 2690d5b
☁️ 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: 2690d5b 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.18%
===========================================
Files 53 154 +101
Lines 2021 26685 +24664
Branches 377 1134 +757
===========================================
+ Hits 1433 4199 +2766
- Misses 588 22486 +21898
🚀 New features to boost your workflow:
|
|
Deployed 78fda76 to https://ForgeRock.github.io/ping-javascript-sdk/pr-579/78fda761aef551163b4aa89f163a0471ca4cc283 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/oidc-client - 25.2 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
2 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