Run openByDefault: true on initRepeatingGroups

[bjosttveit]

Description

This small change fixes an edge-case experienced by SSB. In short, there are two repeating groups working on the same array, they each set parts of the data in the array. The first group has openByDefault: true which causes a new element to be added when entering the first group. The second group has openByDefault: first to open the first row created by the first group. The problem seems to be that, the "first" logic for the second group happens on page load and will not fire because there are no elements in the group at that point. The logic where it opens a new row when empty happens when the react component loads; this will not run either since at that point the first group has already added elements so it is no longer empty. Running the "first" logic again can be problematic since we dont want to just open the first element multiple times. This solution instead runs the normal openByDefault logic (where it adds a new element) also in the initRepeatingGroups (on page load). The consequences of this is that in the groupContainer, the openByDefault logic will not have to run the first time (since it checks the length). The other consequence is that it will set the index (length) and editIndex to 0, making it open. It seems that if multiple rows are added to the first group it will update the length of the second repeating group as well when saving, and importantly, it preserves the editIndex; so it behaves as expected.

I guess I lied when I said "in short" 😅

Fell free to point out any potential problems I might have missed. If it turns out to be too complicated to fix I think we don't have to, as using several groups on the same data is not something we officially support.

Related Issue(s)

• closes Repeating groups: "openByDefault" åpner ikke gruppen som deles på flere sider #1111
• https://altinn.slack.com/archives/C02EVE4RU82/p1681814744234169

Verification/QA

• Manual functionality testing
  • I have tested these changes manually
  • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
  • No testing done/necessary
• Automated tests
  • Unit test(s) have been added/updated
  • Cypress E2E test(s) have been added/updated
  • No automatic tests are needed here (no functional changes/additions)
  • I want someone to help me make some tests
• UU/WCAG (follow these guidelines until we have our own)
  • I have tested with a screen reader/keyboard navigation/automated wcag validator
  • No testing done/necessary (no DOM/visual changes)
  • I want someone to help me perform accessibility testing
• User documentation @ altinn-studio-docs
  • Has been added/updated
  • No functionality has been changed/added, so no documentation is needed
  • I will do that later/have created an issue
• Changes/additions to component properties
  • Changes are reflected in both src/layout/layout.d.ts and layout.schema.v1.json, and these are all backwards-compatible
  • No changes made
• Support in Altinn Studio
  • Issue(s) created for support in Studio
  • This change/feature does not require any changes to Altinn Studio
• Sprint board
  • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
  • I don't have permissions to do that, please help me out
• Labels
  • I have added a kind/* label to this PR for proper release notes grouping
  • I don't have permissions to add labels, please help me out

[olemartinorg]

Hmm! I think this is something we absolutely should support, as it is kind-of the point behind the hiddenRow functionality. The code change looks simple enough, but we should make sure this works with the cypress tests, and preferably add a test for this as well. I know it's a bit of a chore to add a test case here, and it might feel like this is super obscure functionality, but as you've come to find out now - it's also very easy to break this functionality.

[bjosttveit]

It seems to break every test where it tries to go through the group step fast (this injects formData?). The group test itself works fine though. It also works to go through frontend-test normally.

It is kind of a nightmare to debug the repeating group functionality in the sagas, this is by far the most complicated saga-logic we have right?

[olemartinorg]

It seems to break every test where it tries to go through the group step fast (this injects formData?). The group test itself works fine though. It also works to go through frontend-test normally.

Tests are passing now - great! 🥳

It is kind of a nightmare to debug the repeating group functionality in the sagas, this is by far the most complicated saga-logic we have right?

Test the use-case, not the saga. That is, it might be time for us to add a second page in frontend-test with another repeating group (which possibly also shows the rows that would be hidden in the first group). When something like that is set up, it could be fairly easy to write a cypress test that navigates the pages and looks at the groups with openByDefault intercepted to be true.

[bjosttveit]

Test the use-case, not the saga. That is, it might be time for us to add a second page in frontend-test with another repeating group (which possibly also shows the rows that would be hidden in the first group). When something like that is set up, it could be fairly easy to write a cypress test that navigates the pages and looks at the groups with openByDefault intercepted to be true.

Yes this should be relatively straightforward to test if a new page is added. Luckily the tests passed, so I did not have to try to figure out any subtle error in the sagas for now 😅

[bjosttveit]

Update: having now added a test case for this by injecting a new layoutPage (and intercepting page order). It is now clear that this very simple fix does not work in the general case. It worked in the SSB app because they are using ProcessdataWrite, which calls initRepeatingGroups again when it changes the data. This caused the group length of the second group to be updated properly when the data changes elsewhere, but this does not happen when not using dataprocessing.

Then the question is how to deal with that. InitRepeatingGroups is relatively heavy? And maybe we dont want to call that over and over again? Looking for some feedback on this one. @olemartinorg

Will leave this as a draft until we figure out the way forward.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[olemartinorg]

If we take a step back to look at what is happening here, is it correct to say that:

• Adding a new row to a repeating group just updates the editIndex and index properties of the repeatingGroups redux state.
• Adding a new row does not, in fact, add a new row to the group data model. That's because the way we store the data model makes it impossible to store an empty object (which is, in essence, how a new empty group should be stored).
• So adding a new row to the first group only affects the first group, and will not add a new row to the second group, because the repeatingGroups redux state is indexed by the group component ID, not the data model path?

In time, I want us to migrate over to a new way of storing the data model in app-frontend-react, which also allows us to store empty groups/objects (which the backend supports, but we throw away). That also means that we should store the index state in the data model, and only keep the editIndex state outside of the data model. Fixing that should make it easy to fix #555 as well.

In other words, I think this might be a bigger/systemic issue that we should solve at the appropriate level. Instead of quick-fixes and patches, we should make sure our state is stored and manipulated in the way we try to make it look like it does.

[bjosttveit]

Yes I agree, a quick fix in this case would be to constantly run initRepeatingGroups, but that would probably hurt performance and would just make the code even worse. So I also think this should be handled as a part of refactoring this logic as you suggest. So we should probably just close this PR?

[olemartinorg]

@bjosttveit Sadly, yep - I think that's best for now. The solution, especially the solution to #555 has been considered to be a breaking change, so maybe we need to introduce a v4 with the proper change at some point.