feat(ui-editor): add lookup components #14516
Conversation
📝 WalkthroughWalkthroughThis pull request introduces new functionality for organization and person lookup in the backend and frontend of the application. The changes include adding two new action methods in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/ui-editor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
backend/tests/Designer.Tests/Controllers/PreviewController/OrganisationLookupTests.cs (1)
12-12: Use a more descriptive constant name.Consider renaming the constant to better indicate its test purpose:
- protected const string organisationNumber = "123456789"; + protected const string VALID_TEST_ORGANISATION_NUMBER = "123456789";frontend/packages/shared/src/types/ComponentSpecificConfig.ts (2)
52-55: Add JSDoc comments to document the type.Consider adding JSDoc comments to document the purpose of the type and its properties:
+/** + * Data model bindings for the OrganisationLookup component + */ type DataModelBindingsOrganisationLookup = { + /** Organization number binding (required) */ organisation_lookup_orgnr: string | InternalBindingFormat; + /** Organization name binding (optional) */ organisation_lookup_name?: string | InternalBindingFormat; };
57-60: Add JSDoc comments to document the type.Consider adding JSDoc comments to document the purpose of the type and its properties:
+/** + * Data model bindings for the PersonLookup component + */ type DataModelBindingsPersonLookup = { + /** Social security number binding (required) */ person_lookup_ssn: string | InternalBindingFormat; + /** Person name binding (required) */ person_lookup_name: string | InternalBindingFormat; };frontend/packages/ux-editor/src/data/formItemConfig.ts (2)
393-398: Add default data model bindings and consider using a more appropriate icon.
- Add default data model bindings to match the pattern of other form components:
[ComponentType.OrganisationLookup]: { name: ComponentType.OrganisationLookup, itemType: LayoutItemType.Component, - defaultProperties: {}, + defaultProperties: { + dataModelBindings: { + organisation_lookup_orgnr: '', + organisation_lookup_name: '' + } + }, icon: ShortTextIcon, },
- Consider using a more appropriate icon (e.g.,
GroupIconorClipboardIcon) that better represents an organization lookup.
431-436: Add default data model bindings and consider using a more appropriate icon.
- Add default data model bindings to match the pattern of other form components:
[ComponentType.PersonLookup]: { name: ComponentType.PersonLookup, itemType: LayoutItemType.Component, - defaultProperties: {}, + defaultProperties: { + dataModelBindings: { + person_lookup_ssn: '', + person_lookup_name: '' + } + }, icon: ShortTextIcon, },
- Consider using a more appropriate icon (e.g.,
PersonIconor similar) that better represents a person lookup.backend/src/Designer/Controllers/PreviewController.cs (1)
679-692: Enhance the organization lookup endpoint with validation and error handling.Consider the following improvements:
- Add input validation for the organization number format
- Return more realistic mock data with additional fields
- Add error handling for invalid organization numbers
[HttpGet] [Route("api/v1/lookup/organisation/{organisationNumber}")] public IActionResult OrganisationLookup(string org, string app, string organisationNumber) { + // Validate organization number format (e.g., 9 digits for Norwegian org numbers) + if (!Regex.IsMatch(organisationNumber, @"^\d{9}$")) + { + return BadRequest(new { success = false, message = "Invalid organization number format" }); + } + + // Return more realistic mock data string lookupResponse = $@"{{ ""success"": true, ""organisationDetails"": {{ ""orgNr"": ""{organisationNumber}"", - ""name"": ""Test AS (preview)"" + ""name"": ""Test AS (preview)"", + ""organizationType"": ""AS"", + ""status"": ""Active"", + ""address"": {{ + ""street"": ""Test Street 1"", + ""postalCode"": ""0123"", + ""city"": ""Oslo"" + }} }} }}"; return Ok(lookupResponse); }frontend/packages/ux-editor/src/testing/schemas/json/component/OrganisationLookup.schema.v1.json (1)
1-148: Add organization-specific validation patterns and configuration options.The schema is well-structured but could be enhanced with:
- Add validation pattern for organization number binding:
"dataModelBindings": { "type": "object", "properties": { - "organisation_lookup_orgnr": { "$ref": "#/definitions/IRawDataModelBinding" }, + "organisation_lookup_orgnr": { + "$ref": "#/definitions/IRawDataModelBinding", + "pattern": "^\\d{9}$", + "description": "Must be a 9-digit organization number" + }, "organisation_lookup_name": { "$ref": "#/definitions/IRawDataModelBinding" } }, "required": ["organisation_lookup_orgnr"], "additionalProperties": false }
- Add configuration options for the lookup behavior:
{ "properties": { + "lookupOptions": { + "type": "object", + "properties": { + "debounceMs": { + "type": "number", + "description": "Debounce time in milliseconds for the lookup request", + "default": 300 + }, + "minLength": { + "type": "number", + "description": "Minimum length of input before triggering lookup", + "default": 9 + }, + "validateOnBlur": { + "type": "boolean", + "description": "Whether to validate the organization number on blur", + "default": true + } + }, + "additionalProperties": false + }, // ... existing properties } }frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json (3)
5-10: Consider enhancing ID pattern documentation.While the pattern is well-defined, it would be helpful to include examples of valid and invalid IDs in the description to make it more developer-friendly.
Consider updating the description to include examples:
- "description": "The component ID. Must be unique within all layouts/pages in a layout-set. Cannot end with <dash><number>.", + "description": "The component ID. Must be unique within all layouts/pages in a layout-set. Cannot end with <dash><number>. Valid examples: 'person-lookup', 'person2', 'lookup-123456'. Invalid examples: 'person-123', '-person', '123'.",
28-49: Consider using enum for page break values.Instead of using examples and string references, consider defining an enum type for the break values to ensure type safety.
"breakBefore": { "title": "Page break before", "description": "PDF only: Value or expression indicating whether a page break should be added before the component. Can be either: 'auto' (default), 'always', or 'avoid'.", - "examples": ["auto", "always", "avoid"], + "enum": ["auto", "always", "avoid"], "default": "auto", - "$ref": "expression.schema.v1.json#/definitions/string" + "type": "string" },
91-135: Consider additional accessibility text bindings.While the schema includes
summaryAccessibleTitle, consider adding more accessibility-focused text bindings for screen readers.Consider adding:
ariaLabel: For the main componenterrorMessageAriaLabel: For validation errorshelpTextAriaLabel: For help text that might differ from visual text
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/src/Designer/Controllers/PreviewController.cs(1 hunks)backend/tests/Designer.Tests/Controllers/PreviewController/OrganisationLookupTests.cs(1 hunks)backend/tests/Designer.Tests/Controllers/PreviewController/PersonLookupTests.cs(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(2 hunks)frontend/packages/shared/src/types/ComponentType.ts(1 hunks)frontend/packages/ux-editor/src/data/formItemConfig.ts(4 hunks)frontend/packages/ux-editor/src/testing/componentSchemaMocks.ts(2 hunks)frontend/packages/ux-editor/src/testing/schemas/json/component/OrganisationLookup.schema.v1.json(1 hunks)frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/shared/src/types/ComponentSpecificConfig.ts
[error] 351-351: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 353-353: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (7)
frontend/packages/shared/src/types/ComponentType.ts (1)
32-32: LGTM!The new component types are properly added and follow the existing naming conventions and alphabetical ordering.
Also applies to: 37-37
frontend/packages/ux-editor/src/testing/componentSchemaMocks.ts (1)
31-31: Verify the existence and content of referenced schema files.The new schema files are imported but not included in this PR. Please ensure:
- The schema files exist at the specified paths
- The schemas define appropriate validation rules for the lookup components
Also applies to: 36-36, 78-78, 83-83
✅ Verification successful
Schema files are present and properly structured ✅
Both
OrganisationLookup.schema.v1.jsonandPersonLookup.schema.v1.jsonexist and contain valid JSON Schema definitions with appropriate validation rules and data model bindings for their respective lookup components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the schema files exist and contain valid JSON schema for schema in "OrganisationLookup.schema.v1.json" "PersonLookup.schema.v1.json"; do echo "Checking schema: $schema" fd -t f "$schema" -x cat {} | jq 'has("$schema") and has("type") and has("properties")' doneLength of output: 611
Script:
#!/bin/bash # Display the content of schema files for schema in "OrganisationLookup.schema.v1.json" "PersonLookup.schema.v1.json"; do echo "=== Content of $schema ===" fd -t f "$schema" -x cat {} doneLength of output: 13346
frontend/packages/shared/src/types/ComponentSpecificConfig.ts (1)
343-346: LGTM! Component configurations follow the established pattern.The OrganisationLookup and PersonLookup component configurations are correctly integrated into the ComponentSpecificConfig type, following the same pattern as other form components.
Also applies to: 354-357
frontend/packages/ux-editor/src/data/formItemConfig.ts (1)
593-599: LGTM! Components are correctly registered.The OrganisationLookup and PersonLookup components are appropriately added to the form category.
frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json (3)
1-3: LGTM! Schema metadata is properly defined.The schema correctly specifies both the canonical URI and JSON Schema version.
146-149: LGTM! Required fields are properly defined.The schema correctly enforces the minimum required properties for the component to function.
136-145: Review PII handling in data model bindings.The component handles sensitive personal information (SSN and name). Ensure that:
- The data is properly encrypted in transit and at rest
- The component implements proper access controls
- The data is masked appropriately in the UI
Let's check for security measures in related files:
| [Fact] | ||
| public async Task Post_PersonLookup_Ok() | ||
| { | ||
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/person"; | ||
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Post, dataPathWithData); | ||
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | ||
|
|
||
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | ||
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
|
|
||
| string responseBody = await response.Content.ReadAsStringAsync(); | ||
| Assert.Contains("Test T. Testesen", responseBody); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for error scenarios.
The current test only covers the successful case. Consider adding tests for:
- Invalid request parameters
- Missing referrer header
- Error responses from the lookup service
Example test case:
[Fact]
public async Task Post_PersonLookup_MissingReferrer_ReturnsBadRequest()
{
string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/person";
using HttpRequestMessage httpRequestMessage = new(HttpMethod.Post, dataPathWithData);
// No referrer header set
using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage);
Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
}| [Fact] | ||
| public async Task Get_OrganisationLookup_Ok() | ||
| { | ||
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{organisationNumber}"; | ||
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData); | ||
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | ||
|
|
||
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | ||
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
|
|
||
| string responseBody = await response.Content.ReadAsStringAsync(); | ||
| Assert.Contains(organisationNumber, responseBody); | ||
| Assert.Contains("Test AS", responseBody); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional scenarios.
Similar to PersonLookupTests, consider adding tests for:
- Invalid organisation numbers (e.g., non-numeric, wrong length)
- Missing referrer header
- Error responses
Example test case:
[Theory]
[InlineData("12345")] // Too short
[InlineData("1234567890")] // Too long
[InlineData("abcdefghi")] // Non-numeric
public async Task Get_OrganisationLookup_InvalidNumber_ReturnsBadRequest(string invalidNumber)
{
string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{invalidNumber}";
using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData);
httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet=");
using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage);
Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
}| /// <summary> | ||
| /// Action for mocking the POST method for person lookup for v4 apps | ||
| /// </summary> | ||
| /// <param name="org">The org</param> | ||
| /// <param name="app">The app</param> | ||
| /// <returns></returns> | ||
| [HttpPost] | ||
| [Route("api/v1/lookup/person")] | ||
| public IActionResult PersonLookup(string org, string app) | ||
| { | ||
| string mockSsn = "12345678912"; | ||
| string lookupResponse = $"{{\"success\":true,\"personDetails\":{{\"ssn\":\"{mockSsn}\",\"name\":\"Test T. Testesen (preview)\", \"lastName\":\"Testesen (preview)\"}}}}"; | ||
| return Ok(lookupResponse); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the person lookup endpoint with request validation and error handling.
Consider the following improvements:
- Add request body model and validation
- Return more realistic mock data
- Add error handling
- Document the expected request format
+/// <summary>
+/// Request model for person lookup
+/// </summary>
+public class PersonLookupRequest
+{
+ public string SearchTerm { get; set; }
+ public string SearchType { get; set; } // e.g., "ssn", "name"
+}
[HttpPost]
[Route("api/v1/lookup/person")]
-public IActionResult PersonLookup(string org, string app)
+public IActionResult PersonLookup(string org, string app, [FromBody] PersonLookupRequest request)
{
+ if (request == null || string.IsNullOrEmpty(request.SearchTerm))
+ {
+ return BadRequest(new { success = false, message = "Invalid request" });
+ }
+
string mockSsn = "12345678912";
- string lookupResponse = $"{{\"success\":true,\"personDetails\":{{\"ssn\":\"{mockSsn}\",\"name\":\"Test T. Testesen (preview)\", \"lastName\":\"Testesen (preview)\"}}}}";
+ string lookupResponse = $@"{{
+ ""success"": true,
+ ""personDetails"": {{
+ ""ssn"": ""{mockSsn}"",
+ ""firstName"": ""Test"",
+ ""middleName"": ""T."",
+ ""lastName"": ""Testesen (preview)"",
+ ""address"": {{
+ ""street"": ""Test Street 1"",
+ ""postalCode"": ""0123"",
+ ""city"": ""Oslo""
+ }}
+ }}
+ }}";
return Ok(lookupResponse);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Action for mocking the POST method for person lookup for v4 apps | |
| /// </summary> | |
| /// <param name="org">The org</param> | |
| /// <param name="app">The app</param> | |
| /// <returns></returns> | |
| [HttpPost] | |
| [Route("api/v1/lookup/person")] | |
| public IActionResult PersonLookup(string org, string app) | |
| { | |
| string mockSsn = "12345678912"; | |
| string lookupResponse = $"{{\"success\":true,\"personDetails\":{{\"ssn\":\"{mockSsn}\",\"name\":\"Test T. Testesen (preview)\", \"lastName\":\"Testesen (preview)\"}}}}"; | |
| return Ok(lookupResponse); | |
| } | |
| /// <summary> | |
| /// Request model for person lookup | |
| /// </summary> | |
| public class PersonLookupRequest | |
| { | |
| public string SearchTerm { get; set; } | |
| public string SearchType { get; set; } // e.g., "ssn", "name" | |
| } | |
| /// <summary> | |
| /// Action for mocking the POST method for person lookup for v4 apps | |
| /// </summary> | |
| /// <param name="org">The org</param> | |
| /// <param name="app">The app</param> | |
| /// <returns></returns> | |
| [HttpPost] | |
| [Route("api/v1/lookup/person")] | |
| public IActionResult PersonLookup(string org, string app, [FromBody] PersonLookupRequest request) | |
| { | |
| if (request == null || string.IsNullOrEmpty(request.SearchTerm)) | |
| { | |
| return BadRequest(new { success = false, message = "Invalid request" }); | |
| } | |
| string mockSsn = "12345678912"; | |
| string lookupResponse = $@"{{ | |
| ""success"": true, | |
| ""personDetails"": {{ | |
| ""ssn"": ""{mockSsn}"", | |
| ""firstName"": ""Test"", | |
| ""middleName"": ""T."", | |
| ""lastName"": ""Testesen (preview)"", | |
| ""address"": {{ | |
| ""street"": ""Test Street 1"", | |
| ""postalCode"": ""0123"", | |
| ""city"": ""Oslo"" | |
| }} | |
| }} | |
| }}"; | |
| return Ok(lookupResponse); | |
| } |
frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json
Show resolved
Hide resolved
| "grid": { | ||
| "properties": { | ||
| "xs": { "default": "auto", "$ref": "#/definitions/IGridSize" }, | ||
| "sm": { "default": "auto", "$ref": "#/definitions/IGridSize" }, | ||
| "md": { "default": "auto", "$ref": "#/definitions/IGridSize" }, | ||
| "lg": { "default": "auto", "$ref": "#/definitions/IGridSize" }, | ||
| "xl": { "default": "auto", "$ref": "#/definitions/IGridSize" }, | ||
| "labelGrid": { "$ref": "#/definitions/IGridStyling" }, | ||
| "innerGrid": { "$ref": "#/definitions/IGridStyling" } | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Fix incorrect grid definition references
The schema references #/definitions/IGridSize and #/definitions/IGridStyling, but these definitions don't exist. Instead, use:
- Replace
#/definitions/IGridSizewith#/definitions/gridValuefrom common-defs.schema.v1.json - Replace
#/definitions/IGridStylingwith#/definitions/gridPropsfrom common-defs.schema.v1.json
🔗 Analysis chain
Verify referenced grid definitions.
The schema references #/definitions/IGridSize and #/definitions/IGridStyling, but these definitions are not present in the current schema.
Let's verify the existence of these definitions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if grid definitions exist in any related schema files
# Search for grid-related definitions in schema files
rg -l "IGridSize|IGridStyling" "frontend/packages/ux-editor/src/testing/schemas/json/"
Length of output: 4399
Script:
#!/bin/bash
# Check the content of Grid.schema.v1.json and look for definitions
cat "frontend/packages/ux-editor/src/testing/schemas/json/component/Grid.schema.v1.json"
# Also search for the actual definitions in all schema files
rg '"IGridSize"|"IGridStyling"' "frontend/packages/ux-editor/src/testing/schemas/json/" -A 5
Length of output: 5580
Script:
#!/bin/bash
# Look for potential base/common schema files
fd . "frontend/packages/ux-editor/src/testing/schemas/json/" -e json | grep -iE 'base|common|definitions|shared'
# Also check if there are any .json files directly in the schemas/json directory
ls -la frontend/packages/ux-editor/src/testing/schemas/json/
# Search for files containing "definitions" section
rg '"definitions":\s*{' "frontend/packages/ux-editor/src/testing/schemas/json/" -A 10
Length of output: 677
Script:
#!/bin/bash
# Check the contents of the common definitions file
cat "frontend/packages/ux-editor/src/testing/schemas/json/component/common-defs.schema.v1.json"
# Also try a simpler regex to find definitions
rg '"definitions"' "frontend/packages/ux-editor/src/testing/schemas/json/component/common-defs.schema.v1.json" -A 2
Length of output: 19391
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
backend/tests/Designer.Tests/Controllers/PreviewController/OrganisationLookupTests.cs (1)
12-12: Document the test data.Add XML documentation to explain the significance of the organisation number used in tests.
- protected const string organisationNumber = "123456789"; + /// <summary> + /// Sample organisation number used for testing. + /// Format: 9 digits as per Norwegian organization number standard. + /// </summary> + protected const string organisationNumber = "123456789";frontend/packages/shared/src/types/ComponentSpecificConfig.ts (2)
52-55: Add documentation for the DataModelBindingsOrganisationLookup type.Consider adding JSDoc comments to document the purpose of this type and its fields.
+/** + * Data model bindings for the OrganisationLookup component. + * @property organisation_lookup_orgnr - Required field for storing the organization number + * @property organisation_lookup_name - Optional field for storing the organization name + */ type DataModelBindingsOrganisationLookup = { organisation_lookup_orgnr: string | InternalBindingFormat; organisation_lookup_name?: string | InternalBindingFormat; };
57-60: Add documentation for the DataModelBindingsPersonLookup type.Consider adding JSDoc comments to document the purpose of this type and its fields.
+/** + * Data model bindings for the PersonLookup component. + * @property person_lookup_ssn - Required field for storing the social security number + * @property person_lookup_name - Required field for storing the person's name + */ type DataModelBindingsPersonLookup = { person_lookup_ssn: string | InternalBindingFormat; person_lookup_name: string | InternalBindingFormat; };frontend/packages/ux-editor/src/data/formItemConfig.ts (2)
393-398: Enhance the OrganisationLookup configuration.Consider the following improvements:
- Add default data model bindings to match other form components
- Use a more appropriate icon (e.g.,
GroupIconorFileTextIcon) that better represents an organization lookup[ComponentType.OrganisationLookup]: { name: ComponentType.OrganisationLookup, itemType: LayoutItemType.Component, - defaultProperties: {}, + defaultProperties: { + dataModelBindings: { + organisation_lookup_orgnr: '', + organisation_lookup_name: '', + }, + }, - icon: ShortTextIcon, + icon: GroupIcon, },
431-436: Enhance the PersonLookup configuration.Consider the following improvements:
- Add default data model bindings to match other form components
- Use a more appropriate icon that better represents a person lookup
[ComponentType.PersonLookup]: { name: ComponentType.PersonLookup, itemType: LayoutItemType.Component, - defaultProperties: {}, + defaultProperties: { + dataModelBindings: { + person_lookup_ssn: '', + person_lookup_name: '', + }, + }, - icon: ShortTextIcon, + icon: TextIcon, },backend/src/Designer/Controllers/PreviewController.cs (2)
679-692: Enhance the OrganisationLookup endpoint implementation.Consider the following improvements:
- Add input validation for the organization number format
- Return more realistic mock data with additional fields
- Add error handling for invalid organization numbers
[HttpGet] [Route("api/v1/lookup/organisation/{organisationNumber}")] public IActionResult OrganisationLookup(string org, string app, string organisationNumber) { + if (!IsValidOrganisationNumber(organisationNumber)) + { + return BadRequest(new { success = false, message = "Invalid organisation number format" }); + } + + // Mock different responses based on the organization number + var mockData = new + { + success = true, + organisationDetails = new + { + orgNr = organisationNumber, + name = "Test AS (preview)", + type = "AS", + status = "Active", + address = "Test Street 1", + postalCode = "0123", + city = "Oslo" + } + }; - string lookupResponse = $"{{\"success\":true,\"organisationDetails\":{{\"orgNr\":\"{organisationNumber}\",\"name\":\"Test AS (preview)\"}}}}"; - return Ok(lookupResponse); + return Ok(mockData); } + +private static bool IsValidOrganisationNumber(string orgNr) +{ + return !string.IsNullOrEmpty(orgNr) && orgNr.Length == 9 && orgNr.All(char.IsDigit); +}
694-707: Enhance the PersonLookup endpoint implementation.Consider the following improvements:
- Add request body model and validation
- Return more realistic mock data with additional fields
- Add error handling for invalid SSNs
+public class PersonLookupRequest +{ + public string Ssn { get; set; } +} + [HttpPost] [Route("api/v1/lookup/person")] -public IActionResult PersonLookup(string org, string app) +public IActionResult PersonLookup(string org, string app, [FromBody] PersonLookupRequest request) { - string mockSsn = "12345678912"; - string lookupResponse = $"{{\"success\":true,\"personDetails\":{{\"ssn\":\"{mockSsn}\",\"name\":\"Test T. Testesen (preview)\", \"lastName\":\"Testesen (preview)\"}}}}"; - return Ok(lookupResponse); + if (request?.Ssn == null || !IsValidSsn(request.Ssn)) + { + return BadRequest(new { success = false, message = "Invalid SSN format" }); + } + + var mockData = new + { + success = true, + personDetails = new + { + ssn = request.Ssn, + firstName = "Test", + middleName = "T.", + lastName = "Testesen", + name = "Test T. Testesen (preview)", + address = "Test Street 1", + postalCode = "0123", + city = "Oslo" + } + }; + return Ok(mockData); } + +private static bool IsValidSsn(string ssn) +{ + return !string.IsNullOrEmpty(ssn) && ssn.Length == 11 && ssn.All(char.IsDigit); +}frontend/packages/ux-editor/src/testing/schemas/json/component/OrganisationLookup.schema.v1.json (1)
136-144: Add validation rules for organization number format.Consider adding pattern validation for the organization number field to ensure it follows the correct format.
"dataModelBindings": { "type": "object", "properties": { - "organisation_lookup_orgnr": { "$ref": "#/definitions/IRawDataModelBinding" }, + "organisation_lookup_orgnr": { + "$ref": "#/definitions/IRawDataModelBinding", + "pattern": "^[0-9]{9}$", + "description": "Organization number must be exactly 9 digits" + }, "organisation_lookup_name": { "$ref": "#/definitions/IRawDataModelBinding" } }, "required": ["organisation_lookup_orgnr"], "additionalProperties": false }frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json (2)
8-9: Consider strengthening the ID pattern regex.The current pattern
^[0-9a-zA-Z][0-9a-zA-Z-]*(-?[a-zA-Z]+|[a-zA-Z][0-9]+|-[0-9]{6,})$allows for potentially confusing IDs. Consider:
- Adding a maximum length constraint
- Restricting consecutive hyphens
- Adding word boundaries to prevent confusion
51-54: Security warning needs more emphasis.The readOnly description contains an important security warning about API manipulation. Consider:
- Moving this warning to a separate security-focused property
- Making it more prominent using stronger formatting
- "description": "Boolean value or expression indicating if the component should be read only/disabled. Defaults to false. <br /> <i>Please note that even with read-only fields in components, it may currently be possible to update the field by modifying the request sent to the API or through a direct API call.<i/>", + "description": "Boolean value or expression indicating if the component should be read only/disabled. Defaults to false.", + "security_warning": "⚠️ SECURITY NOTICE: Read-only fields can still be modified through direct API calls or request manipulation. Server-side validation is required for security-critical fields.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/src/Designer/Controllers/PreviewController.cs(1 hunks)backend/tests/Designer.Tests/Controllers/PreviewController/OrganisationLookupTests.cs(1 hunks)backend/tests/Designer.Tests/Controllers/PreviewController/PersonLookupTests.cs(1 hunks)frontend/packages/shared/src/types/ComponentSpecificConfig.ts(2 hunks)frontend/packages/shared/src/types/ComponentType.ts(1 hunks)frontend/packages/ux-editor/src/data/formItemConfig.ts(4 hunks)frontend/packages/ux-editor/src/testing/componentSchemaMocks.ts(2 hunks)frontend/packages/ux-editor/src/testing/schemas/json/component/OrganisationLookup.schema.v1.json(1 hunks)frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
frontend/packages/shared/src/types/ComponentSpecificConfig.ts
[error] 351-351: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 353-353: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (9)
frontend/packages/shared/src/types/ComponentType.ts (1)
32-32: LGTM!The new component types are correctly added and maintain the alphabetical ordering of the enum.
Also applies to: 37-37
frontend/packages/ux-editor/src/testing/componentSchemaMocks.ts (1)
31-31: LGTM! Verify schema files exist.The new schema imports and component mappings are correctly added. Let's verify the referenced schema files exist.
Also applies to: 36-36, 78-78, 83-83
✅ Verification successful
Schema files verified successfully
Both schema files exist at the expected locations in the component schema directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the schema files exist for schema in "OrganisationLookup.schema.v1.json" "PersonLookup.schema.v1.json"; do fd --type f "$schema" || echo "Missing schema file: $schema" doneLength of output: 441
frontend/packages/shared/src/types/ComponentSpecificConfig.ts (1)
343-346: LGTM! The lookup components are well integrated.The component configurations properly extend the base form component props and include the correct data model bindings.
Also applies to: 354-357
frontend/packages/ux-editor/src/data/formItemConfig.ts (1)
531-532: LGTM! Components are properly registered.The lookup components are correctly added to both the
schemaComponentsarray and theformcategory inallComponents.Also applies to: 597-598
frontend/packages/ux-editor/src/testing/schemas/json/component/PersonLookup.schema.v1.json (5)
1-3: LGTM! Schema metadata follows best practices.The schema uses proper versioning, HTTPS URL, and a stable JSON Schema specification version.
91-91: LGTM! Type constraint is properly defined.The use of
constensures type safety for the PersonLookup component.
147-149: LGTM! Required fields are properly defined.The schema correctly enforces the essential fields needed for the component to function.
136-145: Review PII handling for SSN data.The schema handles sensitive personal information (SSN). Ensure:
- Compliance with privacy regulations
- Proper data encryption in transit and at rest
- Logging practices that avoid storing PII
92-135: Verify XSS protection for text resource bindings.All text fields should be properly sanitized before rendering to prevent XSS attacks.
✅ Verification successful
Text resource bindings are secure by design
The expression system uses a strictly validated schema that:
- Enforces type safety for all expressions
- Limits string operations to safe transformations (concat, case conversion)
- Handles text resources as plain text without HTML interpretation
- Provides no mechanisms for raw HTML injection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for XSS protection in text rendering components # Look for sanitization functions or security measures rg -A 5 'sanitize|escape|xss|security' frontend/packages/ux-editor/src # Check if any HTML sanitization libraries are used grep -r "DOMPurify\|sanitize-html\|xss" package.json package-lock.jsonLength of output: 2040
Script:
#!/bin/bash # Find and examine expression schema echo "=== Expression Schema ===" fd expression.schema.v1.json # Look for text resource rendering echo -e "\n=== Text Resource Rendering ===" ast-grep --pattern 'textResourceBindings' # Check expression evaluation echo -e "\n=== Expression Evaluation ===" rg -A 5 'evaluateExpression|renderText|textResource' frontend/packages/ux-editor/src # Look for React components that might handle text rendering echo -e "\n=== React Text Components ===" ast-grep --pattern 'const $_ = ($props) => { $$$ textResource $$$ }'Length of output: 66697
Script:
#!/bin/bash # Check expression schema definition echo "=== Expression Schema Definition ===" cat frontend/packages/ux-editor/src/testing/schemas/json/layout/expression.schema.v1.json # Look for text rendering components and security measures echo -e "\n=== Text Rendering & Security ===" rg -A 5 'dangerouslySetInnerHTML|sanitize|xss|DOMPurify|escapeHtml' frontend/packages/ux-editor/src frontend/libs/studio-components/src # Check for React components that render text echo -e "\n=== Text Rendering Components ===" ast-grep --pattern 'const $_ = ($props) => { $$$ return ( <$_> {$_} </$_> ) }'Length of output: 18910
| [Fact] | ||
| public async Task Post_PersonLookup_Ok() | ||
| { | ||
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/person"; | ||
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Post, dataPathWithData); | ||
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | ||
|
|
||
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | ||
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
|
|
||
| string responseBody = await response.Content.ReadAsStringAsync(); | ||
| Assert.Contains("Test T. Testesen", responseBody); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test only covers the happy path. Consider adding:
- Test cases for invalid requests
- Validation of the complete response structure
- Edge cases (empty/invalid search criteria)
+ [Fact]
+ public async Task Post_PersonLookup_InvalidRequest_ReturnsBadRequest()
+ {
+ string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/person";
+ using HttpRequestMessage httpRequestMessage = new(HttpMethod.Post, dataPathWithData);
+ httpRequestMessage.Content = new StringContent("invalid_request_body");
+
+ using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage);
+ Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+ }Committable suggestion skipped: line range outside the PR's diff.
| [Fact] | ||
| public async Task Get_OrganisationLookup_Ok() | ||
| { | ||
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{organisationNumber}"; | ||
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData); | ||
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | ||
|
|
||
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | ||
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | ||
|
|
||
| string responseBody = await response.Content.ReadAsStringAsync(); | ||
| Assert.Contains(organisationNumber, responseBody); | ||
| Assert.Contains("Test AS", responseBody); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test only covers the happy path. Consider adding:
- Test with invalid organisation number format
- Test with non-existent organisation number
- Validation of complete response structure
+ [Fact]
+ public async Task Get_OrganisationLookup_InvalidFormat_ReturnsBadRequest()
+ {
+ string invalidOrgNumber = "invalid";
+ string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{invalidOrgNumber}";
+ using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData);
+ httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet=");
+
+ using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage);
+ Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Fact] | |
| public async Task Get_OrganisationLookup_Ok() | |
| { | |
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{organisationNumber}"; | |
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData); | |
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | |
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | |
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | |
| string responseBody = await response.Content.ReadAsStringAsync(); | |
| Assert.Contains(organisationNumber, responseBody); | |
| Assert.Contains("Test AS", responseBody); | |
| } | |
| [Fact] | |
| public async Task Get_OrganisationLookup_Ok() | |
| { | |
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{organisationNumber}"; | |
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData); | |
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | |
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | |
| Assert.Equal(HttpStatusCode.OK, response.StatusCode); | |
| string responseBody = await response.Content.ReadAsStringAsync(); | |
| Assert.Contains(organisationNumber, responseBody); | |
| Assert.Contains("Test AS", responseBody); | |
| } | |
| [Fact] | |
| public async Task Get_OrganisationLookup_InvalidFormat_ReturnsBadRequest() | |
| { | |
| string invalidOrgNumber = "invalid"; | |
| string dataPathWithData = $"{Org}/{AppV4}/api/v1/lookup/organisation/{invalidOrgNumber}"; | |
| using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, dataPathWithData); | |
| httpRequestMessage.Headers.Referrer = new Uri($"{MockedReferrerUrl}?org={Org}&app={AppV4}&selectedLayoutSet="); | |
| using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); | |
| Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); | |
| } |
nkylstad
changed the title
Description
Added new lookup-components as described by layout jsonschema:
Added new endpoints in PreviewController to return mock response to the lookup-call from the components.
These endpoints return a basic string, with no typing.
Ideally we should use the Altinn.App.Api models as return types here, but upgrading from v8.0.0-rc1 to v8.5.0 seemed out of scope for this change 🤔
Suggest we create a separate issue to perform the upgrade, and consider introducing Altinn.App.Api as a dependency once that is done.
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Technical Improvements
The release introduces two new lookup components that allow users to retrieve organization and personal information within the application.