Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions .changeset/long-singers-do.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@forgerock/davinci-client': minor
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we mark this as major if it is a breaking change?

---

Add support for extension in PhoneNumberCollector

BREAKING CHANGE: `ObjectValueCollectorWithObjectValue` type was removed
1 change: 1 addition & 0 deletions e2e/davinci-app/components/object-value.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export default function objectValueComponent(
updater({
phoneNumber: selectedValue,
countryCode: collector.output.value?.countryCode || '',
extension: collector.output.value?.extension || '',
} as any);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should remove the as any and replace with strict type

});

Expand Down
6 changes: 4 additions & 2 deletions e2e/davinci-suites/src/form-fields.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ test('Should render form fields', async ({ page }) => {

await page.getByRole('button', { name: 'Submit' }).click();
const request = await requestPromise;

const parsedData = JSON.parse(request.postData());
const postData = request.postData();
const parsedData = postData ? JSON.parse(postData) : {};
const data = parsedData.parameters.data;

expect(data.actionKey).toBe('submit');
expect(data.formData).toStrictEqual({
'text-input-key': 'The input',
Expand All @@ -55,6 +56,7 @@ test('Should render form fields', async ({ page }) => {
'phone-field': {
phoneNumber: '1234567890',
countryCode: 'GB',
extension: '4321',
},
});
});
Expand Down
76 changes: 76 additions & 0 deletions packages/davinci-client/src/lib/collector.types.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ import type {
InferSingleValueCollectorType,
InferMultiValueCollectorType,
InferActionCollectorType,
PhoneNumberCollector,
ObjectOptionsCollectorWithObjectValue,
InferValueObjectCollectorType,
PhoneNumberInputValue,
PhoneNumberOutputValue,
PhoneNumberOptions,
} from './collector.types.js';

describe('Collector Types', () => {
Expand Down Expand Up @@ -354,5 +360,75 @@ describe('Collector Types', () => {

expectTypeOf(tCollector).toMatchTypeOf<FlowCollector>();
});

it('should correctly infer PhoneNumberCollector Type', () => {
const tCollector: InferValueObjectCollectorType<'PhoneNumberCollector'> = {
category: 'ObjectValueCollector',
error: null,
type: 'PhoneNumberCollector',
id: '',
name: '',
input: {
key: '',
value: { countryCode: '', phoneNumber: '', extension: '' },
type: '',
validation: null,
},
output: {
key: '',
label: '',
type: '',
options: { showExtension: false },
value: { countryCode: '', phoneNumber: '', extension: '' },
},
};

expectTypeOf(tCollector).toEqualTypeOf<PhoneNumberCollector>();
});
});

describe('ObjectValueCollector Types', () => {
it('should validate PhoneNumberCollector structure', () => {
expectTypeOf<PhoneNumberCollector>().toEqualTypeOf<
ObjectOptionsCollectorWithObjectValue<
'PhoneNumberCollector',
PhoneNumberInputValue,
PhoneNumberOutputValue,
PhoneNumberOptions
>
>();
expectTypeOf<PhoneNumberCollector>()
.toHaveProperty('category')
.toEqualTypeOf<'ObjectValueCollector'>();
expectTypeOf<PhoneNumberCollector>()
.toHaveProperty('type')
.toEqualTypeOf<'PhoneNumberCollector'>();
expectTypeOf<PhoneNumberCollector['input']['value']>().toEqualTypeOf<PhoneNumberInputValue>();
expectTypeOf<PhoneNumberCollector['output']['options']>().toEqualTypeOf<PhoneNumberOptions>();
});

it('should validate PhoneNumberCollector base type constraints', () => {
const collector: PhoneNumberCollector = {
category: 'ObjectValueCollector',
type: 'PhoneNumberCollector',
error: null,
id: 'test',
name: 'Test',
input: {
key: 'phone',
value: { countryCode: '+1', phoneNumber: '5555555555', extension: '' },
type: 'string',
validation: null,
},
output: {
key: 'phone',
label: 'Phone Number',
type: 'phone',
options: { showExtension: true },
value: { countryCode: '+1', phoneNumber: '5555555555' },
},
};
expectTypeOf(collector).toEqualTypeOf<PhoneNumberCollector>();
});
});
});
47 changes: 16 additions & 31 deletions packages/davinci-client/src/lib/collector.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,11 +297,17 @@ export interface DeviceValue {
export interface PhoneNumberInputValue {
countryCode: string;
phoneNumber: string;
extension: string;
}

export interface PhoneNumberOutputValue {
countryCode?: string;
phoneNumber?: string;
extension?: string;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines 303 to +307
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This type gets me nervous. It's not very strict, and it's becoming a mutable mess. Since it's an output value, is it just that "One of these" must be there?

Or some can be there some may not? Maybe we need to type it and use Partial instead?

We may want to think through what this type should represent before patching it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is where the pre-filled data goes so it's possible that some or none of them are there. What is the benefit of Partial over this sort of type?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It allows us to have a strict type

export interface PhoneNumberOutputValue {
  countryCode?: string;
  phoneNumber?: string;
  extension?: string;
}

And use it by requiring part of the type elsewhere instead of making the type flimsy everywhere to conform.

There's a number of ways we can do this but I think we would want to get real information on this type from the server team.

Roughly (untested):

type PhoneNumberOutputValue = {
  countryCode: string;
  phoneNumber: string;
  extension: string;
} | {
  countryCode?: string;
  phoneNumber: string;
} |
{
  phoneNumber: string;
  extension: string;
}

The issue with this is i'm not positive of the benefit and maintenance costs to this. It's technically going to do a better job of what the type is but for what benefit? Not sure theres any.

With Partial, if we have some reason to need the full type, we can enforce it at the type level, and Partial can be used (or looser structural typing requirements like { phoneNumber: string } ) can be used where that is needed.


export interface PhoneNumberOptions {
showExtension: boolean;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

export interface ObjectOptionsCollectorWithStringValue<
Expand Down Expand Up @@ -331,6 +337,7 @@ export interface ObjectOptionsCollectorWithObjectValue<
T extends ObjectValueCollectorTypes,
V = Record<string, string>,
D = Record<string, string>,
O = Record<string, string>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This parameter is widening the type to Record<string, string> so we're losing strictness guarantee's on all collector types.

DeviceAuthenticationCollector inherits ValidationPhoneNumber as a valid rule, which is type-loosening through consolidation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Right. The more I think about it maybe I should have kept the ObjectValueCollectorWithObjectValue type for phone number.... I was trying to avoid having two very similar types but their options are very different shapes. If we separate the types we may have better typing and avoid a breaking change.

> {
category: 'ObjectValueCollector';
error: string | null;
Expand All @@ -341,38 +348,14 @@ export interface ObjectOptionsCollectorWithObjectValue<
key: string;
value: V;
type: string;
validation: ValidationRequired[] | null;
};
output: {
key: string;
label: string;
type: string;
options: DeviceOptionWithDefault[];
value?: D | null;
};
}

export interface ObjectValueCollectorWithObjectValue<
T extends ObjectValueCollectorTypes,
IV = Record<string, string>,
OV = Record<string, string>,
> {
category: 'ObjectValueCollector';
error: string | null;
type: T;
id: string;
name: string;
input: {
key: string;
value: IV;
type: string;
Comment on lines -355 to -368
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We could mark with

/* @deprecated Use ObjectOptionsCollectorWithObjectValue */

That way consumers have time to migrate.

validation: (ValidationRequired | ValidationPhoneNumber)[] | null;
};
output: {
key: string;
label: string;
type: string;
value?: OV | null;
options: O;
value?: D | null;
};
}

Expand All @@ -396,21 +379,23 @@ export type ObjectValueCollectors =

export type ObjectValueCollector<T extends ObjectValueCollectorTypes> =
| ObjectOptionsCollectorWithObjectValue<T>
| ObjectOptionsCollectorWithStringValue<T>
| ObjectValueCollectorWithObjectValue<T>;
| ObjectOptionsCollectorWithStringValue<T>;

export type DeviceRegistrationCollector = ObjectOptionsCollectorWithStringValue<
'DeviceRegistrationCollector',
string
>;
export type DeviceAuthenticationCollector = ObjectOptionsCollectorWithObjectValue<
'DeviceAuthenticationCollector',
DeviceValue
DeviceValue,
Record<string, string>,
DeviceOptionWithDefault[]
>;
export type PhoneNumberCollector = ObjectValueCollectorWithObjectValue<
export type PhoneNumberCollector = ObjectOptionsCollectorWithObjectValue<
'PhoneNumberCollector',
PhoneNumberInputValue,
PhoneNumberOutputValue
PhoneNumberOutputValue,
PhoneNumberOptions
>;

/** *********************************************************************
Expand Down
53 changes: 53 additions & 0 deletions packages/davinci-client/src/lib/collector.utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@ describe('returnPhoneNumberCollector', () => {
type: 'PHONE_NUMBER',
required: true,
validatePhoneNumber: true,
showExtension: false,
};

const result = returnObjectValueCollector(mockField, 1, {});
Expand All @@ -581,6 +582,7 @@ describe('returnPhoneNumberCollector', () => {
value: {
countryCode: '',
phoneNumber: '',
extension: '',
},
type: mockField.type,
validation: [
Expand All @@ -600,9 +602,11 @@ describe('returnPhoneNumberCollector', () => {
key: mockField.key,
label: mockField.label,
type: mockField.type,
options: { showExtension: false },
value: {
countryCode: '',
phoneNumber: '',
extension: '',
},
},
});
Expand All @@ -616,6 +620,7 @@ describe('returnPhoneNumberCollector', () => {
type: 'PHONE_NUMBER',
required: false,
validatePhoneNumber: false,
showExtension: false,
};
const result = returnObjectValueCollector(mockField, 1, {});
expect(result).toEqual({
Expand All @@ -629,6 +634,7 @@ describe('returnPhoneNumberCollector', () => {
value: {
countryCode: mockField.defaultCountryCode,
phoneNumber: '',
extension: '',
},
type: mockField.type,
validation: null,
Expand All @@ -637,9 +643,11 @@ describe('returnPhoneNumberCollector', () => {
key: mockField.key,
label: mockField.label,
type: mockField.type,
options: { showExtension: false },
value: {
countryCode: mockField.defaultCountryCode,
phoneNumber: '',
extension: '',
},
},
});
Expand All @@ -653,6 +661,7 @@ describe('returnPhoneNumberCollector', () => {
type: 'PHONE_NUMBER',
required: false,
validatePhoneNumber: false,
showExtension: false,
};
const prefillMock: PhoneNumberOutputValue = {
countryCode: 'CA',
Expand All @@ -669,6 +678,7 @@ describe('returnPhoneNumberCollector', () => {
value: {
countryCode: prefillMock.countryCode,
phoneNumber: '',
extension: '',
},
type: mockField.type,
validation: null,
Expand All @@ -677,9 +687,11 @@ describe('returnPhoneNumberCollector', () => {
key: mockField.key,
label: mockField.label,
type: mockField.type,
options: { showExtension: false },
value: {
countryCode: prefillMock.countryCode,
phoneNumber: '',
extension: '',
},
},
});
Expand All @@ -695,6 +707,7 @@ describe('returnPhoneNumberCollector', () => {
type: 'PHONE_NUMBER',
required: false,
validatePhoneNumber: false,
showExtension: false,
};
const prefillMock: PhoneNumberOutputValue = {
phoneNumber: '1234567890',
Expand All @@ -711,6 +724,7 @@ describe('returnPhoneNumberCollector', () => {
value: {
countryCode: '',
phoneNumber: prefillMock.phoneNumber,
extension: '',
},
type: mockField.type,
validation: null,
Expand All @@ -719,9 +733,11 @@ describe('returnPhoneNumberCollector', () => {
key: mockField.key,
label: mockField.label,
type: mockField.type,
options: { showExtension: false },
value: {
countryCode: '',
phoneNumber: prefillMock.phoneNumber,
extension: '',
},
},
});
Expand All @@ -735,6 +751,7 @@ describe('returnPhoneNumberCollector', () => {
type: 'PHONE_NUMBER',
required: false,
validatePhoneNumber: false,
showExtension: false,
};
const prefillMock: PhoneNumberOutputValue = {
countryCode: 'CA',
Expand All @@ -752,6 +769,7 @@ describe('returnPhoneNumberCollector', () => {
value: {
countryCode: prefillMock.countryCode,
phoneNumber: prefillMock.phoneNumber,
extension: '',
},
type: mockField.type,
validation: null,
Expand All @@ -760,13 +778,48 @@ describe('returnPhoneNumberCollector', () => {
key: mockField.key,
label: mockField.label,
type: mockField.type,
options: { showExtension: false },
value: {
countryCode: prefillMock.countryCode,
phoneNumber: prefillMock.phoneNumber,
extension: '',
},
},
});
});

it('showExtension is reflected in output options', () => {
const mockField: PhoneNumberField = {
key: 'phone-number-key',
defaultCountryCode: null,
label: 'Phone Number',
type: 'PHONE_NUMBER',
required: false,
validatePhoneNumber: false,
showExtension: true,
};
const result = returnObjectValueCollector(mockField, 1, {});
expect(result.output.options).toEqual({ showExtension: true });
});

it('prefilled extension is set on collector', () => {
const mockField: PhoneNumberField = {
key: 'phone-number-key',
defaultCountryCode: null,
label: 'Phone Number',
type: 'PHONE_NUMBER',
required: false,
validatePhoneNumber: false,
showExtension: true,
};
const prefillMock: PhoneNumberOutputValue = {
phoneNumber: '1234567890',
extension: '123',
};
const result = returnObjectValueCollector(mockField, 1, prefillMock);
expect(result.input.value.extension).toBe('123');
expect(result.output.value?.extension).toBe('123');
});
});

describe('No Value Collectors', () => {
Expand Down
Loading
Loading