fix: Account for invalid text resource ids in code list editor #14797
Conversation
# Conflicts: # frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts # frontend/app-development/features/appContentLibrary/test-data/textResources.ts
📝 WalkthroughWalkthroughThis pull request introduces a new utility function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. frontend/libs/studio-components/src/components/StudioTextResourcePicker/utils.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-storybook". (The package "eslint-plugin-storybook" was not found when loaded as a Node module from the directory "/frontend/libs/studio-components".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-storybook" was referenced from the config file in "frontend/libs/studio-components/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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
solution/studio/designer
TomasEng
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14797 +/- ##
=======================================
Coverage 95.77% 95.77%
=======================================
Files 1922 1923 +1
Lines 25077 25082 +5
Branches 2867 2867
=======================================
+ Hits 24018 24023 +5
Misses 799 799
Partials 260 260 ☔ View full report in Codecov by Sentry. |
TomasEng
added
skip-releasenotes
.../libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/libs/studio-components/src/components/StudioTextResourcePicker/utils.test.ts (1)
8-9: Consider making the test more maintainableUsing a hardcoded index (129) makes the test brittle if the mock data changes. Consider using a more descriptive approach to find a text resource for testing, such as finding by a specific ID or using the first item in the array.
- const arbitraryTextResourceIndex = 129; - const textResource = textResources[arbitraryTextResourceIndex]; + // Use a specific text resource that we know exists in the mock data + const textResource = textResources.find(resource => resource.id !== undefined) || textResources[0];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsx(2 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/utils.test.ts(1 hunks)frontend/libs/studio-components/src/components/StudioTextResourcePicker/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
🔇 Additional comments (5)
frontend/libs/studio-components/src/components/StudioTextResourcePicker/StudioTextResourcePicker.tsx (2)
41-44: Great implementation, addressing previous feedback!Good job extracting the logic to a separate utility function and using
useMemoto optimize the computation ofselectedValues. This implementation efficiently handles the case where text resource IDs don't exist in the available resources.
50-50: Correct implementation of component value propThe updated implementation ensures that the combobox only selects values that exist in the text resources, addressing the issue mentioned in the PR objectives where invalid IDs would result in empty selections.
frontend/libs/studio-components/src/components/StudioTextResourcePicker/utils.test.ts (1)
4-22: Comprehensive test suite covering all edge casesThe test cases effectively validate the behavior of
retrieveSelectedValues:
- When a text resource exists
- When a text resource doesn't exist
- When the value is null or undefined
Good use of parameterized testing with
it.eachfor null and undefined values.frontend/libs/studio-components/src/components/StudioTextResourcePicker/utils.ts (2)
3-5: Clean implementation of retrieveSelectedValuesThe function clearly fulfills its purpose by returning an array with the ID if the text resource exists, or an empty array if it doesn't. The implementation is concise and follows the single responsibility principle.
7-9: Efficient implementation of doesTextResourceExistGood use of the
some()method for an efficient search without unnecessary iteration through the entire array. The function handles the optionalidparameter correctly.
There was a problem hiding this comment.
Nice work!
After testing a bit I found this bug, it's not directly related to this PR, but made possible by it.
Skjermopptak.2025-02-26.154345.mp4
The errorcode i get is 400, here is the response from the backend:
{
"errors": {
"[0].Label": [
"The field is required."
]
},
"type": "https://tools.ietf.org/html/rfc9110#section-15.5.1",
"title": "One or more validation errors occurred.",
"status": 400,
"traceId": "00-c8a037410aff4b63b66bed2f43042ea0-5306a91dec31cfbf-01"
}
|
Thanks for testing, @Konrad-Simso. This is the error that I mentioned in the description. I'm working on another pull request that will fix it: #14826 |
Description
This pull request makes the text resource selector account for text resource ID's that does not exist. In this case, no values will be selected in the search combobox. The invalid ID will remain as the current value until the user selects something.
There may be better ways to deal with this case, but we have a separate issue for that. This is relevant not only in code lists, but everywhere where we have text references. Therefore I have chosen the safest and most straightforward solution for now.
Spiller.inn.2025-02-25.103250.mp4
Note
When testing this in the app content library, there will still appear an error message when unsetting the text resource on the label field. That's another issue, which is due to the
labelproperty being required.Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Tests