chore: amend dynamic field values #10866

Nil20 · 2025-10-24T08:23:10Z

Description

Country config PR: opencrvs/opencrvs-countryconfig#1138
Farajaland PR: opencrvs/opencrvs-farajaland#1811

Clearly describe what has been changed. Include relevant context or background.
Explain how the issue was fixed (if applicable) and the root cause.

Link this pull request to the GitHub issue (and optionally name the branch ocrvs-<issue #>)

Checklist

I have linked the correct Github issue under "Development"
I have tested the changes locally, and written appropriate tests
I have tested beyond the happy path (e.g. edge cases, failure paths)
I have updated the changelog with this change (if applicable)
I have updated the GitHub issue status accordingly

greptile-apps

Greptile Overview

Important Files Changed

File Analysis

Filename	Score	Overview
packages/commons/src/events/CompositeFieldValue.ts	0/5	Refactored address field schemas to use discriminated unions and added nested data structure for QueryParamReader; contains critical syntax error on line 136
packages/commons/src/events/FieldValue.ts	4/5	Refactored DataFieldValue from record to nested object structure, removed StreetLevelDetailsValue from union
packages/commons/src/events/FieldConfig.ts	5/5	Changed DefaultAddressFieldValue to extend DomesticAddressFieldValue instead of AddressFieldValue

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

packages/commons/src/events/CompositeFieldValue.ts

makelicious

Looks very good, thank you 🙏 . Some type related comments. Let's review it once we are finished

packages/client/src/v2-events/features/events/registered-fields/Address.tsx

packages/commons/src/events/test.utils.ts

makelicious · 2025-10-27T10:08:20Z

packages/commons/src/events/utils.ts

  return omitHiddenFields(
    annotationFields,
-    { ...declaration, ...annotation },
+    { ...declaration, ...annotation } as EventState,


Check if we really need to cast this? If we do, ActionUpdate is the truthful one. At least stripping nulls from annotation (or the combination) should make it EventState

{ ...declaration, ...annotation }, declaration and annotation together can be treated as ActionUpdate since annotation is ActionUpdate and it's values are nullish values(FieldUpdateValue) which can not be EventState(FieldValue)

makelicious · 2025-10-27T10:08:58Z

packages/events/src/router/event/event.actions.correction.test.ts

+        streetLevelDetails: {
+          province: 'a45b982a-5c7b-4bd9-8fd8-a42d0994054c',
+          district: '5ef450bc-712d-48ad-93f3-8da0fa453baa',
+          urbanOrRural: 'RURAL' as const,


I think urbanOrRural is not used anymore. In the best case, this should throw an error.

const StreetLevelDetailsValue = z .record(z.string(), z.string()) .optional()

streetLevelDetails accepts any objects with a string as a key and a value, right? On that basis, i dont think it should throw errors

Wouldn't we validate them against whatever countryconfig specifies?

// src/form/street-address-configuration.ts export const defaultStreetAddressConfiguration = [ { id: 'town', required: false, parent: field('country'), label: { id: 'field.address.town.label', defaultMessage: 'Town', description: 'This is the label for the field' }, conditionals: [ { type: ConditionalType.SHOW, conditional: and( isDomesticAddress(), not(field('district').isUndefined()) ) } ], type: FieldType.TEXT }, { id: 'residentialArea', required: false, parent: field('country'), label: { id: 'field.address.residentialArea.label', defaultMessage: 'Residential Area', description: 'This is the label for the field' }, conditionals: [ { type: ConditionalType.SHOW, conditional: and( isDomesticAddress(), not(field('district').isUndefined()) ) } ], type: FieldType.TEXT }, { id: 'street', required: false, parent: field('country'), label: { id: 'field.address.street.label', defaultMessage: 'Street', description: 'This is the label for the field' }, conditionals: [ { type: ConditionalType.SHOW, conditional: and( isDomesticAddress(), not(field('district').isUndefined()) ) } ], type: FieldType.TEXT }, { id: 'number', required: false, parent: field('country'), label: { id: 'field.address.number.label', defaultMessage: 'Number', description: 'This is the label for the field' }, conditionals: [ { type: ConditionalType.SHOW, conditional: and( isDomesticAddress(), not(field('district').isUndefined()) ) } ], type: FieldType.TEXT }, { id: 'zipCode', required: false, parent: field('country'), label: { id: 'field.address.postcodeOrZip.label', defaultMessage: 'Postcode / Zip', description: 'This is the label for the field' }, conditionals: [ { type: ConditionalType.SHOW, conditional: and( isDomesticAddress(), not(field('district').isUndefined()) ) } ], type: FieldType.TEXT }, { id: 'state', conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], parent: field('country'), required: { message: { id: 'field.address.state.label.required', description: 'State required message', defaultMessage: 'State is required' } }, label: { id: 'field.address.state.label', defaultMessage: 'State', description: 'This is the label for the field' }, type: FieldType.TEXT }, { id: 'district2', parent: field('country'), conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], required: { message: { id: 'field.address.district2.label.required', description: 'District2 required message', defaultMessage: 'District is required' } }, label: { id: 'field.address.district2.label', defaultMessage: 'District', description: 'This is the label for the field' }, type: FieldType.TEXT }, { id: 'cityOrTown', parent: field('country'), conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], required: false, label: { id: 'field.address.cityOrTown.label', defaultMessage: 'City / Town', description: 'This is the label for the field' }, type: FieldType.TEXT }, { id: 'addressLine1', parent: field('country'), conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], required: false, label: { id: 'field.address.addressLine1.label', defaultMessage: 'Address Line 1', description: 'This is the label for the field' }, type: FieldType.TEXT }, { id: 'addressLine2', parent: field('country'), conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], required: false, label: { id: 'field.address.addressLine2.label', defaultMessage: 'Address Line 2', description: 'This is the label for the field' }, type: FieldType.TEXT }, { id: 'addressLine3', parent: field('country'), conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], required: false, label: { id: 'field.address.addressLine3.label', defaultMessage: 'Address Line 3', description: 'This is the label for the field' }, type: FieldType.TEXT }, { id: 'postcodeOrZip', parent: field('country'), conditionals: [ { type: ConditionalType.SHOW, conditional: isInternationalAddress() } ], required: false, label: { id: 'field.address.postcodeOrZip.label', defaultMessage: 'Postcode / Zip', description: 'This is the label for the field' }, type: FieldType.TEXT } ]

@rikukissa @pankaj-pant

greptile-apps

Greptile Overview

Important Files Changed

File Analysis

Filename	Score	Overview
packages/commons/src/events/FieldValue.ts	5/5	Restructured FieldValue and FieldUpdateValue types to use explicit type annotations with z.ZodType, removed StreetLevelDetails as top-level types, and changed DataFieldValue to wrap data in an object structure
packages/commons/src/events/CompositeFieldValue.ts	5/5	Refactored AddressFieldValue to use discriminated unions for DOMESTIC vs INTERNATIONAL, and changed QueryParamReaderFieldValue to wrap params in an object structure
packages/client/src/v2-events/features/events/registered-fields/QueryParamReader.tsx	5/5	Updated to wrap params in `params` object property to match new QueryParamReaderFieldValue structure
packages/client/src/v2-events/features/events/registered-fields/Data.tsx	5/5	Updated to wrap data values in `data` object property and access via `value.data[id]` to match new DataFieldValue structure
packages/client/src/v2-events/features/events/registered-fields/Address.tsx	5/5	Added type guards checking `addressType === AddressType.DOMESTIC` before accessing `administrativeArea` to handle discriminated union properly
packages/commons/src/events/utils.ts	5/5	Added generic type parameters and updated return types to properly handle EventState vs ActionUpdate, added satisfies clause for type safety
packages/events/src/service/indexing/utils.ts	5/5	Updated to access `administrativeArea` only after checking `addressType === DOMESTIC` to properly handle discriminated union

_{23 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

makelicious

Well done on the improvements 🏅 . Could you check the changes again: https://www.chromatic.com/review?appId=67862a3f4457d14c317e5cca&number=10866&type=linked&activeElementId=comparison-test-key-68b70a692b622cd342f08dd5-1200px&view=changes

If I read this right, we lose data on the address review. Is it due bad mock data? If so, could it be updated

makelicious · 2025-10-30T13:15:32Z

packages/commons/src/events/CompositeFieldValue.ts

+  data: z.record(z.string(), z.string())
+})

 const ReadDataValue = z.object({


is this still needed?

It should be. With this, FieldUpdateValue would not compile any values with this shape z.record(z.string(), z.string())

Nil20 · 2025-11-11T12:25:55Z

packages/commons/src/conditionals/conditionals.test.ts

-          'child.details': {
-            nested: 'value'
-          }
+          'child.details': 'value'


These tests were overwritten because typescript were complaining of these unknown keys e.g. - dob, nested being passed as keys of FieldValue. When a key was added to QueryParamReaderFieldValue, rather than being z.record(z.string(), z.string()), these were thrown.

Zangetsu101 · 2025-11-11T09:44:54Z

packages/client/src/v2-events/features/events/components/Review.stories.tsx

+    administrativeArea: '27160bbd-32d1-4625-812f-860226bfb92a',
+    streetLevelDetails: {
+      town: 'Example Town',
+      residentialArea: 'Example Residential Area',
+      street: 'Example Street',
+      number: '55',
+      zipCode: '123456',
+      state: 'Example State',
+      district2: 'Example District 2'
+    }


Are we changing the data intentionally?

Yes! It's tightened by type here with EventState

packages/client/src/v2-events/features/events/registered-fields/Address.tsx

...ages/client/src/v2-events/features/events/registered-fields/IdReader.interaction.stories.tsx

Zangetsu101 · 2025-11-11T10:36:21Z

packages/client/src/v2-events/utils.test.ts

We need to revisit this as the the current mock values probably aren't aligned with the original intention of the test

Zangetsu101 · 2025-11-11T10:42:38Z

packages/commons/src/conditionals/conditionals.test.ts

-          'child.details': {
-            dob: new Date('2125-01-01').toISOString().split('T')[0]
-          }
+          'child.details': new Date('2125-01-01').toISOString().split('T')[0]


The test is validating the support for $now variable even in nested levels, so it should be passing with the previous value. Although as .object has been deprecated, what we could do it use .get('dob') instead

#10866 (comment)

used .get('data')

Zangetsu101 · 2025-11-11T10:43:33Z

packages/commons/src/conditionals/conditionals.test.ts

Let's keep the nested tests intact

yeah! with known keys from FieldValues

Zangetsu101 · 2025-11-11T10:45:56Z

packages/commons/src/conditionals/validate-address.test.ts

      // @NOTE: This happens to map to a valid location in events test environment. Updating it will break tests.
      // @TODO:  Find a way to give out context aware mock values in the future.
-      administrativeArea: '27160bbd-32d1-4625-812f-860226bfb92a' as UUID
+      administrativeArea: '27160bbd-32d1-4625-812f-860226bfb92a' as UUID,


We can remove the todo comments now?

Zangetsu101 · 2025-11-11T10:52:34Z

packages/commons/src/events/DynamicFieldValue.test.ts

+  it('allows optional middlename', () => {
+    const schema = getDynamicNameValue(NameField)
+    expect(() =>
+      schema.parse({
+        firstname: 'Jane',
+        surname: 'Doe'
+      })
+    ).not.toThrow()


I don't see a middlename being passed tho

Zangetsu101 · 2025-11-11T10:58:29Z

packages/commons/src/events/DynamicFieldValue.ts

+  return z.object({
+    country: z.string(),
+    addressType: z.enum([AddressType.DOMESTIC, AddressType.INTERNATIONAL]),
+    administrativeArea: z
+      .string()
+      .uuid()
+      .optional() /* Leaf level admin structure */,
+    streetLevelDetails: StreetLevelDetailsUpdateValue.refine((arg) => {
+      const submittedKeys = Object.keys(arg ?? {})
+      const configIds =
+        (streetAddressConfig && streetAddressConfig.map((a) => a.id)) ?? []
+      const invalidKeys = submittedKeys.filter(
+        (key) => !configIds.includes(key)
+      )
+      if (invalidKeys.length) {
+        // eslint-disable-next-line no-console
+        console.log(
+          'Invalid streetLevelDetails: unknown keys',
+          JSON.stringify(invalidKeys, null, 2)
+        )
+        return false
+      }
+
+      return true
+    })
+  })
+}


Let's check if we can use .shape to reuse the one defined in CompositeValue.ts

can a country be nullish for FieldUpdateValue?

most likely not

ocrvs-bot · 2025-11-12T09:44:57Z

Your environment is deployed to https://ocrvs-10634.e2e-k8s.opencrvs.dev

chore: amend dynamic field values

eb29053

Nil20 added the Skip e2e Helps run lint/unit test only for faster feedback. Remove before merge. label Oct 24, 2025

Merge branch 'develop' into ocrvs-10634

f6ec049

This comment has been minimized.

Sign in to view

Nil20 marked this pull request as draft October 24, 2025 08:23

fix: amend domestic address value

459fc4e

greptile-apps bot reviewed Oct 24, 2025

View reviewed changes

packages/commons/src/events/CompositeFieldValue.ts Show resolved Hide resolved

Nil20 added 2 commits October 24, 2025 15:18

fix: remove ts errors from commons

5a0b436

fix: update syntax for queryParamReader schema

415aaf1

Nil20 force-pushed the ocrvs-10634 branch from 1e54fa0 to 415aaf1 Compare October 24, 2025 09:36

Nil20 and others added 6 commits October 24, 2025 19:12

fix: update client components to adjust new field config

4a2ff86

fix: remove ts errors from events

f4d46ec

Merge branch 'develop' into ocrvs-10634

6058432

fix: remove ts errors from client

617079f

chore(test): amend conditional test

1c35d3a

fix: amend address type

0a0d9aa

makelicious reviewed Oct 27, 2025

View reviewed changes

Nil20 added 4 commits October 28, 2025 13:15

Merge branch 'develop' into ocrvs-10634

7e80d05

fix: implement reviewed changes

9ac7db2

fix: amend events tests

d50ac3f

chore: give field value exact type

a3717ca

Nil20 removed the Skip e2e Helps run lint/unit test only for faster feedback. Remove before merge. label Oct 29, 2025

Nil20 marked this pull request as ready for review October 29, 2025 10:52

Nil20 requested a review from makelicious October 29, 2025 10:52

greptile-apps bot reviewed Oct 29, 2025

View reviewed changes

Nil20 added 3 commits October 30, 2025 14:28

fix: change shape of query param reader value

2c0931f

fix: amend storybook test

98831c7

fix: amend output test

4825d97

makelicious reviewed Oct 30, 2025

View reviewed changes

Nil20 linked an issue Oct 31, 2025 that may be closed by this pull request

Address field allows malformed input #10634

Open

Nil20 and others added 6 commits November 10, 2025 17:17

chore: add test for field value union function

0cd511f

Merge branch 'develop' into ocrvs-10634

5db5cf4

fix: update client tests to have correct address val

1940966

fix: update event test

ede5414

fix: update field count in test

1d60d82

fix story

8e312d1

Nil20 requested a review from makelicious November 10, 2025 12:55

Nil20 and others added 5 commits November 10, 2025 19:28

fix field length in test

85bf450

fix: amend address field update value schema

8ce1229

Merge branch 'develop' into ocrvs-10634

6a31b5b

fix: use street level details update value schema

53f5e37

Merge branch 'develop' into ocrvs-10634

a29215b

Nil20 commented Nov 11, 2025

View reviewed changes

fix: update conditionals test

bd15eb0

Zangetsu101 reviewed Nov 11, 2025

View reviewed changes

Nil20 added 9 commits November 11, 2025 22:38

refactor: extract admin area reader to helper

a76f4a9

chore: update changelog

6aac702

fix: remove comment

12ebe77

fix: amend dynamic value test

59c4989

fix: reuse address schema for refine

c5cef93

fix: amend commons test payload

0024ebb

fix: add missing administrative area in tests

602181e

reorder todo comment

7c5af95

chore: add notify test

c121eec

Nil20 requested a review from Zangetsu101 November 12, 2025 09:44

fix: update valid admin leaf level location validator

def1c8a

Zangetsu101 approved these changes Nov 12, 2025

View reviewed changes

Nil20 merged commit 611cc0c into develop Nov 13, 2025
123 of 124 checks passed

Nil20 deleted the ocrvs-10634 branch November 13, 2025 07:40

chore: amend dynamic field values #10866

chore: amend dynamic field values #10866

Uh oh!

Conversation

Nil20 commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

This comment has been minimized.

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Overview

Important Files Changed

Uh oh!

Uh oh!

makelicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Greptile Overview

Important Files Changed

Uh oh!

makelicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocrvs-bot commented Nov 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Nil20 commented Oct 24, 2025 •

edited

Loading