feat: add individual name elements to datamodelbinding #3924
Conversation
📝 WalkthroughWalkthroughRefactors PersonLookup to use separate first, middle, and last name bindings (and optional middle name), updates tests and display construction to assemble full name with SSN fallback, and adds new test for full-name resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
HauklandJ
added
backport-ignore
|
/publish |
PR release:
|
|
/publish |
PR release:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/layout/PersonLookup/config.ts (1)
39-65: Minor: Consider reordering bindings for consistency.The bindings are ordered as last_name, first_name, middle_name. While not incorrect, it might be more intuitive to list them in natural order: first_name, middle_name, last_name.
This is purely a stylistic suggestion and doesn't affect functionality.
Suggested reordering
+ new CG.prop( + 'person_lookup_first_name', + new CG.dataModelBinding() + .optional() + .setTitle('Data model binding for the first name of a person') + .setDescription( + 'Describes the location in the data model where the component should store the first name of the person to look up.', + ), + ), + new CG.prop( + 'person_lookup_middle_name', + new CG.dataModelBinding() + .optional() + .setTitle('Data model binding for the middle name of a person') + .setDescription( + 'Describes the location in the data model where the component should store the middle name of the person to look up.', + ), + ), new CG.prop( 'person_lookup_last_name', new CG.dataModelBinding() .optional() .setTitle('Data model binding for the last name of a person') .setDescription( 'Describes the location in the data model where the component should store the last name of the person to look up.', ), ), - new CG.prop( - 'person_lookup_first_name', - new CG.dataModelBinding() - .optional() - .setTitle('Data model binding for the first name of a person') - .setDescription( - 'Describes the location in the data model where the component should store the first name of the person to look up.', - ), - ), - new CG.prop( - 'person_lookup_middle_name', - new CG.dataModelBinding() - .optional() - .setTitle('Data model binding for the middle name of a person') - .setDescription( - 'Describes the location in the data model where the component should store the middle name of the person to look up.', - ), - ),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/features/expressions/shared-tests/functions/displayValue/type-PersonLookup.jsonsrc/features/expressions/shared-tests/functions/displayValue/type-PersonLookupFullName.jsonsrc/layout/PersonLookup/PersonLookupComponent.tsxsrc/layout/PersonLookup/config.tssrc/layout/PersonLookup/index.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/PersonLookup/index.tsxsrc/layout/PersonLookup/config.tssrc/layout/PersonLookup/PersonLookupComponent.tsx
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/PersonLookup/index.tsxsrc/layout/PersonLookup/config.tssrc/layout/PersonLookup/PersonLookupComponent.tsx
src/layout/*/{config,Component,index,config.generated}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Layout components must follow standardized structure with
config.ts,Component.tsx,index.tsx, andconfig.generated.tsfiles
Files:
src/layout/PersonLookup/index.tsxsrc/layout/PersonLookup/config.ts
🧬 Code graph analysis (3)
src/layout/PersonLookup/index.tsx (1)
src/utils/layout/useNodeItem.ts (1)
useNodeFormDataWhenType(97-103)
src/layout/PersonLookup/config.ts (1)
src/codegen/CG.ts (1)
CG(25-57)
src/layout/PersonLookup/PersonLookupComponent.tsx (1)
src/features/validation/selectors/bindingValidationsForNode.ts (1)
useBindingValidationsFor(17-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install
🔇 Additional comments (5)
src/layout/PersonLookup/PersonLookupComponent.tsx (2)
142-144: LGTM! Individual name fields populated correctly.The new name bindings are populated appropriately from the API response, maintaining consistency with the existing
person_lookup_nameandperson_lookup_ssnpattern.
155-157: LGTM! Clear action handles all name fields.The clear logic correctly resets all individual name fields alongside the existing fields, maintaining data consistency.
src/features/expressions/shared-tests/functions/displayValue/type-PersonLookup.json (1)
11-11: LGTM! Test data correctly reflects the new individual name bindings.The test has been properly updated to:
- Use the three new individual name bindings instead of a single name binding
- Provide matching data model values (FirstName, MiddleName, LastName)
- Expect the correctly formatted display value with the full name "Sulten Karl Løve"
This validates the new feature's display behavior when using individual name components.
Also applies to: 22-24, 34-36
src/features/expressions/shared-tests/functions/displayValue/type-PersonLookupFullName.json (1)
1-35: LGTM! Excellent backward compatibility test.This new test file validates that the legacy
person_lookup_namebinding continues to work as expected, ensuring backward compatibility. The test correctly expects the same display output ("44829800753, Sulten Karl Løve") whether using the combined name field or the individual name parts.This is important for ensuring existing implementations won't break when upgrading.
src/layout/PersonLookup/config.ts (1)
33-33: The optionalperson_lookup_namebinding is already properly handled.The code correctly handles the optional binding: the validation logic in
useEmptyFieldValidationAllBindingsonly triggers when the component hasrequired=true, and the implementation inindex.tsx(lines 29-40) has explicit fallback logic that uses individual name fields (person_lookup_first_name,person_lookup_last_name,person_lookup_middle_name) whenperson_lookup_nameis absent. All component consumers properly handle the optional binding with truthy checks and optional chaining, so no validation breaks.
| useDisplayData(baseComponentId: string): string { | ||
| const formData = useNodeFormDataWhenType(baseComponentId, 'PersonLookup'); | ||
| return Object.values(formData ?? {}).join(', '); | ||
| const formData = useNodeFormDataWhenType(baseComponentId, 'PersonLookup') as Record<string, string | undefined>; | ||
| const data = formData ?? {}; | ||
|
|
||
| const parts: string[] = []; | ||
|
|
||
| if (data.person_lookup_ssn) { | ||
| parts.push(String(data.person_lookup_ssn)); | ||
| } | ||
|
|
||
| // Build full name from individual parts or use the Name binding | ||
| if (data.person_lookup_name) { | ||
| parts.push(String(data.person_lookup_name)); | ||
| } else { | ||
| const nameParts: string[] = []; | ||
| if (data.person_lookup_first_name) { | ||
| nameParts.push(String(data.person_lookup_first_name)); | ||
| } | ||
| if (data.person_lookup_middle_name) { | ||
| nameParts.push(String(data.person_lookup_middle_name)); | ||
| } | ||
| if (data.person_lookup_last_name) { | ||
| nameParts.push(String(data.person_lookup_last_name)); | ||
| } | ||
|
|
||
| if (nameParts.length > 0) { | ||
| parts.push(nameParts.join(' ')); | ||
| } | ||
| } | ||
|
|
||
| return parts.join(', '); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the PersonLookup directory structure
find src/layout/PersonLookup -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: Altinn/app-frontend-react
Length of output: 315
🏁 Script executed:
# Search for useNodeFormDataWhenType definition
rg "useNodeFormDataWhenType" --type ts --type tsx -A 5 -B 2 | head -50Repository: Altinn/app-frontend-react
Length of output: 96
🏁 Script executed:
# Check the full PersonLookup index file
wc -l src/layout/PersonLookup/index.tsxRepository: Altinn/app-frontend-react
Length of output: 105
🏁 Script executed:
# Look for type definitions related to PersonLookup form data
rg "PersonLookup.*FormData|IComponentFormData" --type ts --type tsx -B 2 -A 5Repository: Altinn/app-frontend-react
Length of output: 96
🏁 Script executed:
# Search for imports in the PersonLookup index file to understand dependencies
head -30 src/layout/PersonLookup/index.tsxRepository: Altinn/app-frontend-react
Length of output: 1650
🏁 Script executed:
# Read the entire PersonLookup/index.tsx file
cat -n src/layout/PersonLookup/index.tsxRepository: Altinn/app-frontend-react
Length of output: 3860
🏁 Script executed:
# Find useNodeFormDataWhenType definition
rg "function useNodeFormDataWhenType|const useNodeFormDataWhenType" --type ts -A 10Repository: Altinn/app-frontend-react
Length of output: 922
🏁 Script executed:
# Search for PersonLookupDef to understand the class structure
rg "export.*PersonLookupDef" --type ts -A 5Repository: Altinn/app-frontend-react
Length of output: 559
🏁 Script executed:
# Look for PersonLookup type definitions or interfaces
rg "interface.*PersonLookup|type.*PersonLookup" --type ts -B 2 -A 5Repository: Altinn/app-frontend-react
Length of output: 3372
🏁 Script executed:
# Find IComponentFormData definition
rg "interface IComponentFormData|type IComponentFormData" --type ts -A 10Repository: Altinn/app-frontend-react
Length of output: 802
🏁 Script executed:
# Check the config.def.generated file for PersonLookup
head -100 src/layout/PersonLookup/config.def.generated.tsRepository: Altinn/app-frontend-react
Length of output: 175
🏁 Script executed:
# Search for PersonLookup form data bindings definition
rg "person_lookup_ssn|person_lookup_name" --type ts -B 5 -A 2 | head -50Repository: Altinn/app-frontend-react
Length of output: 4396
🏁 Script executed:
# Find IDataModelBindings definition for PersonLookup
rg "IDataModelBindings.*PersonLookup" --type ts -B 3 -A 5Repository: Altinn/app-frontend-react
Length of output: 1438
🏁 Script executed:
# Find BindingToValues type definition
rg "type BindingToValues|interface BindingToValues" --type ts -A 10Repository: Altinn/app-frontend-react
Length of output: 908
🏁 Script executed:
# Check config.ts for PersonLookup bindings structure
cat src/layout/PersonLookup/config.ts | head -150Repository: Altinn/app-frontend-react
Length of output: 2866
Remove unnecessary type casting and redundant String() conversions.
The return type of useNodeFormDataWhenType(baseComponentId, 'PersonLookup') is already IComponentFormData<'PersonLookup'>, which resolves to a typed object with specific properties (all string | undefined). The cast to Record<string, string | undefined> bypasses TypeScript's type checking and loses property information.
Replace the cast with the proper type, and remove the String() conversions throughout the function since values are already typed as string | undefined:
const formData = useNodeFormDataWhenType(baseComponentId, 'PersonLookup') ?? {};
const data: IComponentFormData<'PersonLookup'> = formData;
if (data.person_lookup_ssn) {
parts.push(data.person_lookup_ssn); // No String() needed
}🤖 Prompt for AI Agents
In src/layout/PersonLookup/index.tsx around lines 19 to 50, the code casts
useNodeFormDataWhenType(...) to Record<string, string | undefined> and wraps
values in String(), which loses typing and is redundant; replace the cast by
assigning formData = useNodeFormDataWhenType(baseComponentId, 'PersonLookup') ??
{} and declare data with the proper type IComponentFormData<'PersonLookup'>,
then remove all unnecessary String() conversions (use the string | undefined
values directly when pushing into parts); ensure any needed type import is
present.
| // Filter out middle name validations since it's optional | ||
| const bindingValidations = { | ||
| ...useBindingValidationsFor<'PersonLookup'>(baseComponentId), | ||
| person_lookup_middle_name: undefined, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider handling optional fields in the validation system instead of manual override.
The manual suppression of person_lookup_middle_name validations by setting it to undefined is an unconventional approach that bypasses the validation framework. This creates a maintenance burden and could mask legitimate validation issues if they arise in the future.
Consider one of these alternatives:
- Mark the field as optional in the validation configuration itself
- Filter optional fields in
useBindingValidationsForor create a variant hook - Add an
optionalproperty to the binding definition that the validation system respects
Alternative approach example
If the validation system can't be easily modified, at least centralize this logic:
const rawValidations = useBindingValidationsFor<'PersonLookup'>(baseComponentId);
const bindingValidations = useMemo(() => {
if (!rawValidations) return undefined;
// Filter out validations for optional bindings
return {
...rawValidations,
person_lookup_middle_name: undefined,
};
}, [rawValidations]);Better yet, extract this into a reusable hook that accepts optional field names as a parameter.
🤖 Prompt for AI Agents
In src/layout/PersonLookup/PersonLookupComponent.tsx around lines 93 to 97, the
code manually suppresses person_lookup_middle_name validations by assigning
undefined which bypasses the validation framework; replace this ad-hoc override
by making the field optional in the validation config or centralize the
behavior: update useBindingValidationsFor (or create a variant hook) to accept a
list of optional binding names (or respect an optional property on binding
definitions) and filter out optional-field validations there, then replace the
inline undefined assignment with a call to that hook or pass the optional list
so middle name is treated as optional consistently across components.
|
1 participant
Description
Config in app:
{ "id": "personLookup", "type": "PersonLookup", "dataModelBindings": { "person_lookup_ssn": "StifterPerson.Foedselsnummer", "person_lookup_name": "StifterPerson.Etternavn", // ← REQUIRED "person_lookup_last_name": "StifterPerson.Etternavn", // ← ADDED "person_lookup_first_name": "StifterPerson.Fornavn", // ← ADDED "person_lookup_middle_name": "StifterPerson.Mellomnavn" // ← ADDED }, "textResourceBindings": { "title": "Stifter", "description": "Her legger du inn etternavn og fødselsnummer", "help": "For å kunne søke i registeret må du ha fødselsnummeret til personen" }, "required": true },Screenshot from getting the data element in POSTMAN:
Backward compatible.
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.