feat: Remove featureflag multiple data models per task #14171
Conversation
github-actions
bot
added
area/ui-editor
lassopicasso
changed the title
…he type for simpleBinding with InternalBindingFormat
|
This may need to be set to blocked after review until this is solved #14104, to make sure that number 4 works, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14171 +/- ##
==========================================
- Coverage 95.65% 95.63% -0.02%
==========================================
Files 1884 1884
Lines 24473 24470 -3
Branches 2811 2810 -1
==========================================
- Hits 23409 23403 -6
- Misses 804 805 +1
- Partials 260 262 +2 ☔ View full report in Codecov by Sentry. |
…//github.com/Altinn/altinn-studio into remove-featureflag-multipleDataModelsPerTask
Thx! :) |
|
Tested OK in dev! 🙂🚀 but I didn’t realize I had already done the CR, so I can’t add the second approval on this one 🫤 |
|
Hi! just tested doing the 5 five test cases case 1: tested OK!✅ case 2: It worked to switch between different data models in the data model bindings for a component, but when I switched to a data model that is used in another task/layoutset, the preview crashed. When I switched to an unused datamodel, it worked perfectly fine! If that is something we can't/won't fix, maybe we should explain what's happening? case 3: tested OK!✅ I have one thought about the data model bindings select. It might seem a bit odd that it looks like a data model binding field named "choose data model field" is already selected when switching between data models. Wouldn't it be better to have an empty select with a validation message indicating that you need to choose a data model field instead? case 4: Commented on this in case 2 case 5: Tested OK (as far as I understood it 😅) |
|
Thx for testing, @Annikenkbrathen! I will investigate case 2 and your other comments 🙂 |
I am unable to reproduce this as described. Do you remember the app name used in dev? Here is what i did, in case i did something different:
|
@Jondyr I used an app I made called https://dev.altinn.studio/editor/ttd/arbeidsflate/ui-editor?layout=Side1 |
|
It looks to me like the issue here is that the datamodel is incorrectly defined. The preview throws this error: I am unable to edit this datamodel without getting errors so i'm not sure how to properly test it with this specific app. |
oh okay, my bad😰 Thanks for checking it out! Then all is good |
|
@Annikenkbrathen or @Jondyr - if i understand correctly that testing went ok, could one of you approve this PR? I will solve the merge conflicts now. |
📝 WalkthroughWalkthroughThis pull request involves modifications across multiple frontend files, primarily focusing on removing 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 (
|
![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
🔭 Outside diff range comments (1)
frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditDataModelBinding.test.tsx (1)
Add test coverage for data model switching scenarios
The current test suite lacks coverage for data model switching functionality, despite having the infrastructure in place. Consider adding the following test cases to
EditDataModelBinding.test.tsx:
- Test switching between
defaultDataModelandsecondDataModel- Verify bindings remain correct after switching data models
- Test preview functionality with non-default data models
The test file already has the necessary setup with both data models defined and queryClient configured, making it straightforward to add these test cases.
🔗 Analysis chain
Line range hint
1-180: Consider adding test coverage for data model switching scenarios.Given that this PR removes the
multipleDataModelsPerTaskfeature flag and the PR objectives mention testing data model switching functionality, consider adding test cases for:
- Switching between default and alternative data models
- Verifying preview functionality with non-default data models
- Testing layout configuration displays correct bindings after switches
This would help ensure the removal of the feature flag doesn't introduce regressions.
Let's verify if there are any existing tests for these scenarios:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests related to data model switching rg -A 5 "test.*data.*model.*switch|test.*switch.*data.*model" frontend/packages/ux-editor/srcLength of output: 95
Script:
#!/bin/bash # Search for data model related tests more broadly echo "=== Searching for data model related tests ===" rg -l "describe.*Data.*Model|test.*data.*model|it.*data.*model" frontend/packages/ux-editor/src/ echo -e "\n=== Checking EditDataModelBinding test content ===" cat frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditDataModelBinding.test.tsx echo -e "\n=== Searching for dataType tests ===" rg "test.*dataType|it.*dataType" frontend/packages/ux-editor/src/Length of output: 10736
🧹 Nitpick comments (3)
frontend/testing/playwright/tests/ui-editor/ui-editor-data-model-binding-and-gitea.spec.ts (1)
Line range hint
32-45: Clean up unused feature flag logic in setup function.Since the feature flag functionality is being removed, the
setupAndVerifyUiEditorPagefunction should be simplified by removing the unused feature flag parameter and its associated logic.const setupAndVerifyUiEditorPage = async ( page: Page, - testAppName: string, - featureFlag?: string[], + testAppName: string ): Promise<UiEditorPage> => { const uiEditorPage = new UiEditorPage(page, { app: testAppName }); await uiEditorPage.loadUiEditorPage(); await uiEditorPage.verifyUiEditorPage(); - if (featureFlag) { - await page.evaluate((flag) => { - localStorage.setItem('featureFlags', JSON.stringify(flag)); - }, featureFlag); - } await uiEditorPage.clickOnPageAccordion(pageName); await uiEditorPage.verifyUiEditorPage(pageName); return uiEditorPage; };frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/EditBinding.test.tsx (2)
46-50: Improve type safety in component change handler.The type casting of
dataModelBindings[defaultBindingKey]toInternalBindingFormatcould be risky.Consider adding a type guard:
-const fieldBinding = formItem.dataModelBindings[defaultBindingKey] as InternalBindingFormat; +const binding = formItem.dataModelBindings[defaultBindingKey]; +const fieldBinding = typeof binding === 'object' && binding !== null ? binding : { field: binding };
Line range hint
326-355: Add test coverage for window.confirm.The test mocks window.confirm but doesn't verify it was called.
Add an assertion to verify the confirmation dialog:
expect(window.confirm).toHaveBeenCalledWith( expect.stringContaining('delete') );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/packages/shared/src/types/ComponentSpecificConfig.ts(2 hunks)frontend/packages/shared/src/utils/featureToggleUtils.ts(0 hunks)frontend/packages/ux-editor/src/components/Properties/DataModelBindings.test.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/EditBinding.test.tsx(13 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/EditBinding.tsx(1 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.tsx(2 hunks)frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditDataModelBinding.test.tsx(1 hunks)frontend/packages/ux-editor/src/utils/component.ts(1 hunks)frontend/testing/playwright/tests/ui-editor/ui-editor-data-model-binding-and-gitea.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/shared/src/utils/featureToggleUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (10)
frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/SelectDataModelBinding.tsx (1)
4-4: LGTM! Code simplification improves maintainability.The removal of feature flag checks and consistent use of
FormFieldwithStudioNativeSelectmakes the code more maintainable and easier to understand.Also applies to: 43-69
frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/EditBinding.tsx (1)
50-52: LGTM! Simplified value assignment logic.The removal of feature flag checks simplifies the code while maintaining the same functionality.
frontend/packages/ux-editor/src/utils/component.ts (1)
144-146: LGTM! Improved code readability.The changes enhance readability by:
- Destructuring all required properties in a single statement
- Simplifying the componentType assignment logic
frontend/testing/playwright/tests/ui-editor/ui-editor-data-model-binding-and-gitea.spec.ts (1)
156-156: LGTM! Removed feature flag parameters.The removal of feature flag parameters from
setupAndVerifyUiEditorPagecalls aligns with the PR objectives.Also applies to: 168-168
frontend/packages/shared/src/types/ComponentSpecificConfig.ts (1)
10-10: LGTM! Type definitions updated to support internal binding format.The changes properly extend the type system to handle both string and InternalBindingFormat for data model bindings, which is a good preparation for removing the feature flag.
Also applies to: 42-42, 47-47
frontend/packages/ux-editor/src/components/Properties/DataModelBindings.test.tsx (1)
20-20: Verify the impact of renaming defaultModel.The variable has been renamed from 'testModel' to 'testModelField', which seems to better reflect its purpose as a field name. However, this variable is used in multiple test assertions.
Let's verify all usages to ensure the rename doesn't change test semantics:
✅ Verification successful
Rename of defaultModel improves code clarity ✅
The rename from 'testModel' to 'testModelField' better reflects its purpose as a field identifier, while 'testModel' is correctly preserved in the separate
defaultDataModelvariable. All usages are consistent and contained within the test file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all occurrences of defaultModel in test files rg "defaultModel" "frontend/packages/ux-editor/src/components/Properties/DataModelBindings.test.tsx" -A 2 -B 2Length of output: 818
frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditBinding/EditBinding.test.tsx (3)
20-20: LGTM! Added constant for second data model field.Good practice to define the test data as constants at the top of the file.
Line range hint
124-144: LGTM! Improved test descriptions and assertions.The test cases are now more specific and better structured:
- Clear test descriptions that explain what's being tested
- Separate assertions for data model and field selectors
- Proper use of ARIA roles for selector identification
Also applies to: 166-169
Line range hint
238-270: LGTM! Comprehensive test coverage for data model changes.Good test coverage for both field and model changes:
- Tests the correct structure of the binding format
- Verifies the callback parameters
- Checks both field and data type updates
Also applies to: 282-314
frontend/packages/ux-editor/src/components/config/editModal/EditDataModelBinding/EditDataModelBinding.test.tsx (1)
65-67: LGTM! More specific selector improves test robustness.The addition of the name parameter using the i18n key makes the combobox selector more specific and maintainable.
I haven't done any testing here, so I think it's more appropriate for @Annikenkbrathen to approve/object to the changes. |
Description
This PR removes the feature flag:
'multipleDataModelsPerTaskand the code related to this + updates tests.What should be tested before merging this to main
Pre-Test Setup
Test cases
Default data model binding for layout set
a. Create a new layout set i prosesser
b. Bind the layout set to a default data model
c. Go to “Utforming” section and add a component
d. Verify that the default data model is selected in the data model bindings for the component.
Select an Alternative Data Model in Data Model Bindings
a. Confirm that you can select a data model other than the default in the data model bindings for a component.
Switch between data models in data model bindings
a. Test switching between different data models in the data model bindings for a component
b. Ensure the related data model fields are displayed correctly in the dropdown list for each selection.
Preview Functionality with non-default data models
a. Verify that the preview functionality works as expected when a data model other than the default is selected.
b. See releated issue: Preview Fails When Linking Component to Non-Default Data Model #14104
Layout configuration – correct bindings displayed
a. In the layout configuration in “se siste endringer”:
i. Check that the correct bindings are displayed when selecting data model fields for the default data model
ii. Check that the correct bindings are displayed when selecting a non-default data model and one of its data model fields
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
Refactor
Tests
Chores