Skip to content

fix(davinci-client): add validation props to collectors #413

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 23, 2025
Merged

fix(davinci-client): add validation props to collectors #413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fine-windows-search.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@forgerock/davinci-client': patch
---

Exposes `required` and `validatePhoneNumber` properties on collectors
17 changes: 12 additions & 5 deletions packages/davinci-client/src/lib/client.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import type {
ObjectValueCollectors,
PhoneNumberInputValue,
AutoCollectors,
MultiValueCollectors,
} from './collector.types.js';
import type {
InitFlow,
Expand Down Expand Up @@ -290,11 +291,13 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({

/**
* @method validate - Method for validating the value against validation rules
* @param {SingleValueCollector} collector - the collector to validate
* @param {SingleValueCollectors | ObjectValueCollectors | MultiValueCollectors} collector - the collector to validate
* @returns {function} - a function to call for validating collector value
* @throws {Error} - if the collector is not a SingleValueCollector
* @throws {Error} - if the collector cannot be validated
*/
validate: (collector: SingleValueCollectors): Validator => {
validate: (
collector: SingleValueCollectors | ObjectValueCollectors | MultiValueCollectors,
): Validator => {
if (!collector.id) {
return handleUpdateValidateError(
'Argument for `collector` has no ID',
Expand All @@ -317,9 +320,13 @@ export async function davinci<ActionType extends ActionTypes = ActionTypes>({
return handleUpdateValidateError('Collector not found', 'state_error', log.error);
}

if (collectorToUpdate.category !== 'ValidatedSingleValueCollector') {
if (
collectorToUpdate.category !== 'ValidatedSingleValueCollector' &&
collectorToUpdate.category !== 'ObjectValueCollector' &&
collectorToUpdate.category !== 'MultiValueCollector'
) {
return handleUpdateValidateError(
'Collector is not a SingleValueCollector and cannot be validated',
'Collector is not a SingleValueCollector, ObjectValueCollector, or MultiValueCollector and cannot be validated',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if we're reaching a tipping point with these error messages. We may want to consider rewording these as its possible we have to add more collectors that will fit this error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking this too. Maybe something like Collector does not fall into a category that can be validated

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 hope we don't have to invent new collector categories :D

'state_error',
log.error,
);
Expand Down
12 changes: 11 additions & 1 deletion packages/davinci-client/src/lib/collector.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ interface ValidationRegex {
rule: string;
}

interface ValidationPhoneNumber {
type: 'validatePhoneNumber';
message: string;
rule: boolean;
}

export interface SingleValueCollectorWithValue<T extends SingleValueCollectorTypes> {
category: 'SingleValueCollector';
error: string | null;
Expand Down Expand Up @@ -196,6 +202,7 @@ export interface MultiValueCollectorWithValue<T extends MultiValueCollectorTypes
key: string;
value: string[];
type: string;
validation?: ValidationRequired[];
};
output: {
key: string;
Expand All @@ -216,12 +223,12 @@ export interface MultiValueCollectorNoValue<T extends MultiValueCollectorTypes>
key: string;
value: string[];
type: string;
validation?: ValidationRequired[];
};
output: {
key: string;
label: string;
type: string;
value: string[];
options: SelectorOption[];
};
}
Expand Down Expand Up @@ -308,6 +315,7 @@ export interface ObjectOptionsCollectorWithStringValue<
key: string;
value: V;
type: string;
validation?: ValidationRequired[];
};
output: {
key: string;
Expand All @@ -331,6 +339,7 @@ export interface ObjectOptionsCollectorWithObjectValue<
key: string;
value: V;
type: string;
validation?: ValidationRequired[];
};
output: {
key: string;
Expand All @@ -355,6 +364,7 @@ export interface ObjectValueCollectorWithObjectValue<
key: string;
value: IV;
type: string;
validation?: (ValidationRequired | ValidationPhoneNumber)[];
};
output: {
key: string;
Expand Down
76 changes: 65 additions & 11 deletions packages/davinci-client/src/lib/collector.utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ import type {
RedirectField,
StandardField,
} from './davinci.types.js';
import { PhoneNumberOutputValue, ValidatedTextCollector } from './collector.types.js';
import {
MultiSelectCollector,
PhoneNumberCollector,
PhoneNumberOutputValue,
ValidatedTextCollector,
} from './collector.types.js';

describe('Action Collectors', () => {
describe('returnFlowCollector', () => {
Expand Down Expand Up @@ -410,6 +415,7 @@ describe('Multi-Value Collectors', () => {
const result = returnMultiSelectCollector(comboField, 1, []);
expect(result.type).toBe('MultiSelectCollector');
expect(result.output).toHaveProperty('value', []);
expect(result.input).toHaveProperty('validation');
});
});
});
Expand Down Expand Up @@ -438,7 +444,7 @@ describe('Object value collectors', () => {
description: 'device2-value',
},
],
required: true,
required: false,
};

const transformedDevices = mockField.options.map((device) => ({
Expand Down Expand Up @@ -524,6 +530,13 @@ describe('Object value collectors', () => {
key: mockField.key,
value: '',
type: mockField.type,
validation: [
{
message: 'Value cannot be empty',
rule: true,
type: 'required',
},
],
},
output: {
key: mockField.key,
Expand Down Expand Up @@ -561,6 +574,18 @@ describe('returnPhoneNumberCollector', () => {
phoneNumber: '',
},
type: mockField.type,
validation: [
{
message: 'Value cannot be empty',
rule: true,
type: 'required',
},
{
message: 'Phone number should be validated',
rule: true,
type: 'validatePhoneNumber',
},
],
},
output: {
key: mockField.key,
Expand All @@ -580,8 +605,8 @@ describe('returnPhoneNumberCollector', () => {
defaultCountryCode: 'US',
label: 'Phone Number',
type: 'PHONE_NUMBER',
required: true,
validatePhoneNumber: true,
required: false,
validatePhoneNumber: false,
};
const result = returnObjectValueCollector(mockField, 1, {});
expect(result).toEqual({
Expand Down Expand Up @@ -616,8 +641,8 @@ describe('returnPhoneNumberCollector', () => {
defaultCountryCode: 'US',
label: 'Phone Number',
type: 'PHONE_NUMBER',
required: true,
validatePhoneNumber: true,
required: false,
validatePhoneNumber: false,
};
const prefillMock: PhoneNumberOutputValue = {
countryCode: 'CA',
Expand Down Expand Up @@ -657,8 +682,8 @@ describe('returnPhoneNumberCollector', () => {
defaultCountryCode: null,
label: 'Phone Number',
type: 'PHONE_NUMBER',
required: true,
validatePhoneNumber: true,
required: false,
validatePhoneNumber: false,
};
const prefillMock: PhoneNumberOutputValue = {
phoneNumber: '1234567890',
Expand Down Expand Up @@ -696,8 +721,8 @@ describe('returnPhoneNumberCollector', () => {
defaultCountryCode: 'US',
label: 'Phone Number',
type: 'PHONE_NUMBER',
required: true,
validatePhoneNumber: true,
required: false,
validatePhoneNumber: false,
};
const prefillMock: PhoneNumberOutputValue = {
countryCode: 'CA',
Expand Down Expand Up @@ -778,17 +803,40 @@ describe('Return collector validator', () => {
const validatedTextCollector = {
input: {
validation: [
{ type: 'required', message: 'This field is required' },
{ type: 'required', message: 'This field is required', rule: true },
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

🧩 Analysis chain

Test data change needs verification

The validation rule definition has changed from { type: 'required', message: 'This field is required', rule: true } to include additional test collectors. Please verify that these changes align with the new validation structure.

Also applies to: 812-825

🏁 Script executed:

#!/bin/bash
# Verify the validation rule structure in collector types
echo "=== Check ValidationRequired interface ==="
rg -n -A5 -B2 'ValidationRequired|ValidationPhoneNumber' packages/davinci-client/src/lib/collector.types.ts

echo "=== Check returnValidator implementation ==="
rg -n -A10 -B5 'returnValidator.*collector.*{' packages/davinci-client/src/lib/collector.utils.ts

Length of output: 1598

🏁 Script executed:

#!/bin/bash
set -euo pipefail

types="packages/davinci-client/src/lib/collector.types.ts"
utils="packages/davinci-client/src/lib/collector.utils.ts"
test="packages/davinci-client/src/lib/collector.utils.test.ts"

echo "=== Checking types file ==="
if [ -f "$types" ]; then
  echo "FOUND: $types"
  rg -n -C3 -e 'interface ValidationRequired' -e 'interface ValidationRegex' -e 'interface ValidationPhoneNumber' "$types" || true
  echo "--- validation property occurrences ---"
  rg -n -C3 'validation' "$types" || true
  ln=$(rg -n 'interface ValidationRequired' "$types" | sed -n '1p' | cut -d: -f1 || true)
  if [ -n "$ln" ]; then
    echo "--- ValidationRequired definition ---"
    sed -n "${ln},$((ln+50))p" "$types"
  fi
else
  echo "MISSING: $types"
fi

echo
echo "=== Checking utils file for returnValidator ==="
if [ -f "$utils" ]; then
  echo "FOUND: $utils"
  rg -n -C5 'returnValidator' "$utils" || true
  lnu=$(rg -n 'returnValidator' "$utils" | sed -n '1p' | cut -d: -f1 || true)
  if [ -n "$lnu" ]; then
    start=$((lnu>30?lnu-30:1))
    echo "--- returnValidator surrounding snippet ---"
    sed -n "${start},$((lnu+200))p" "$utils"
  fi
else
  echo "MISSING: $utils"
fi

echo
echo "=== Showing test file snippet around lines 780-840 ==="
if [ -f "$test" ]; then
  sed -n '780,840p' "$test"
else
  echo "MISSING: $test"
fi

Length of output: 8235

returnValidator missing handling for 'validatePhoneNumber'

collector.types.ts defines ValidationPhoneNumber and the test adds { type: 'validatePhoneNumber', ... }, but returnValidator (packages/davinci-client/src/lib/collector.utils.ts) only implements 'required' and 'regex' — implement phone-number validation in returnValidator or remove/adjust the test data.
Locations: packages/davinci-client/src/lib/collector.types.ts (ValidationPhoneNumber), packages/davinci-client/src/lib/collector.utils.ts (returnValidator ~line 619), packages/davinci-client/src/lib/collector.utils.test.ts (~lines 800–820).

{ type: 'regex', message: 'Invalid format', rule: '^[a-zA-Z0-9]+$' },
],
},
} as ValidatedTextCollector;

const objectValueCollector = {
input: {
validation: [
{ type: 'required', message: 'This field is required', rule: true },
{ type: 'validatePhoneNumber', message: 'Phone number should be validated', rule: true },
],
},
} as PhoneNumberCollector;

const multiValueCollector = {
input: {
validation: [{ type: 'required', message: 'This field is required', rule: true }],
},
} as MultiSelectCollector;

const validator = returnValidator(validatedTextCollector);
const objValidator = returnValidator(objectValueCollector);
const multiValueValidator = returnValidator(multiValueCollector);

it('should return an error message for required validation when value is empty', () => {
const result = validator('');
expect(result).toContain('This field is required');

const objResult = objValidator({});
expect(objResult).toContain('This field is required');

const multiValueResult = multiValueValidator({});
expect(multiValueResult).toContain('This field is required');
});

it('should return an error message for regex validation when value does not match the pattern', () => {
Expand All @@ -799,6 +847,12 @@ describe('Return collector validator', () => {
it('should return no error messages when value passes all validations', () => {
const result = validator('validValue123');
expect(result).toEqual([]);

const objResult = objValidator({ countryCode: 'US', phoneNumber: '1234567890' });
expect(objResult).toEqual([]);

const multiValueResult = multiValueValidator(['a', 'b', 'c']);
expect(multiValueResult).toEqual([]);
});

it('should handle invalid regex patterns gracefully', () => {
Expand Down
Loading
Loading