Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewriting form data storage #1712

Merged
merged 201 commits into from
Dec 28, 2023
Merged

Conversation

olemartinorg
Copy link
Contributor

@olemartinorg olemartinorg commented Dec 28, 2023

Description

The headliners:

  • Form data is now kept in a react Context, and all components can fetch + update form data using the new FD export, which is a toolbox providing methods for getting/setting form data state.
  • Debouncing is now global, meaning there is no useDelayedSaveState() anymore. Instead of keeping a copy of the form data locally and waiting to push these changes up to the global form data model, we push changes up to the context immediately while the user is typing in a form field, and the global debounce() method on the form data will run (by default after 400ms, but configurable for some components via the saveWhileTyping property). As soon as debouncing happens, rules and expressions will run, components may be hidden/shown, and we'll send a request to save the data to the backend. Automatic saving can still be disabled, and we may want to offer a configuration option later to allow to wait a bit longer after debouncing until we actually send a save request (especially for apps without a need to continually change the form data in ProcessDataWrite).
  • The saveWhileTyping property now only allows numbers. All components will 'save' while typing into the form, and it is only a matter of time until that (locally saved) data is saved to the backend as well. It is assumed that the most common reason to set saveWhileTyping: false was to wait until the user more explictly navigates out of the form field, as a way to prevent expensive actions in ProcessDataWrite from running needlessly/repeatedly because of too early automatic saving. If you've set saveWhileTyping: false before, you might want to consider using the CustomButton component instead, to make the user trigger expensive actions more explicitly. Components with saveWhileTyping: false will have the same effect as not setting the property at all.
  • We store the whole data model as-is in app-frontend-react, not a flattened dot-notation variant of it. This also means that we have full support for empty objects, and we have everything needed to properly support non-string data types in the future. Most components still do store/save their data as strings for now, and we still assume the backend will convert numeric data into numeric data types when parsing the JSON through their strict data model definition.
  • Adding/removing rows from repeating groups now immediately trigger a save operation to the backend (if auto-saving is on), so that ProcessDataWrite can potentially populate new rows with prefill data as soon as the new rows appear (they will appear as empty objects in the data model). Previously, adding a new row would not trigger a save operation until the user started filling out data into the row. Thus, visiting a repeating group with openByDefault may trigger a save operation as soon as the user navigates to the form page.
  • The state regarding which repeating group row is open for editing is now kept locally close to the repeating group itself. This means it changes slightly in how the state appears in the UI. For example, if you open a row for editing, move to another page in the form and then go back again the page containing the repeating group, the row you opened for editing will now have been closed again. This happens because the state is short-lived, and will be reset once React notices the context is no longer rendered in the tree. This also has an effect which reduces the effiency of openByDefault: true, as we'll only open (and add) a new row the first time the user visits the page with the repeating group, but the next time the group row will already exist and will not be opened by default (as is correct, per the earlier behaviour of the openByDefault functionality). If you always want an existing row to be open when the user sees the repeating group, you might want to consider setting edit.openByDefault = "first", edit.openByDefault = "last", or maybe instead setting edit.mode = "showAll".

Less major notes and changes:

  • Removed the formData redux slice, and all the sagas that goes with it
  • Added the ability to add TypeScript comments to generated code (used to add a notice about the saveWhileTyping common property).
  • Skipped a bunch of tests regarding validation, as validation currently does not work at all (as the handleDataChange method from GenericComponent no longer exists, and all the sagas that triggered validation does not run anymore).
  • The repeatingGroups redux state is no more. It has been replaced by two providers, RepeatingGroupProvider (which keeps the state for which row is open for editing, along with functions and tools for adding new rows, deleting rows, and checking which rows are visible according to hiddenRow, etc) and RepeatingGroupEditRowProvider (which keeps state for the RepeatingGroupsEditContainer, such as which multiPage each edit container is on). This now has the added benefit that multiPage works along with edit.mode = "showAll", which before most likely made every open row show the same multiPage at the same time (now the navigation happens locally to the edit container, so each edit container has their own state and can navigate independently).
  • When clicking on an error message in ErrorReport, the code to navigate to the correct component/node ran inside ErrorReport.tsx. This has now been moved to NavigateToNodeProvider, which allows any component implementation to register as a handler for how to navigate to a node. This also means that the original code has been spread out all over the place, and repeating groups will now help NavigateToNodeProvider by manipulating their own state so that the repeating group edit state can be changed and allow opening the correct row for editing where the target node is visible.
  • Clicking the 'edit' button in a Summary now triggers the same 'navigate to node' concept as error messages, and will open repeating group rows (and navigate to the correct multiPage) in order to find the component you're wanting to edit.
  • PresentationComponent now renders DevTools. This ensures that our devtools will always be on the 'inside' of any providers.
  • Expanding on our custom context provider tooling, the concept of strict and lax contexts have been altered yet again. I added a way to useLaxCtx() on a context that is normally required: true, which means that you can use that context even when it may not be provided. This is useful, for example for DevTools, because we're not always inside a and even though most providers in there are required, DevTools cannot trust the providers to exist. I added a symbol called ContextNotProvided that might be returned when calling useLaxCtx(), so that we don't have two create split-personality components that need to know beforehand which providers are present to know which hooks to call. This should make it easier to write clutter-free components in the critical path, also without breaking the rules of hooks.
  • I added createZustandContext(), which makes it easy to provide a zustand store as a context. This was very much needed in the form data context, as zustand enabled me to change leaf values in the form data very rapidly (i.e. while typing a single character into an input component) while also making sure to only re-render the components that used that deep leaf value. Those using the debounced copy of the data model will also not re-render until the debouncing runs.
  • All of our current QueryContexts will by default show the <Loader /> component when the query re-fetches data (i.e., when it isLoading). That also happens when the queryKey changes, but for some queries we might want to skip showing the <Loader /> and only show it when the data is initially loaded. That is the case for text resources, as changing the language in the language selector should not show the <Loader /> for a brief moment. Flashing the <Loader /> while changing the language would also reset states in repeating groups, causing edit rows to be closed when the language changed. I added useQueryWithStaleData to fix that problem.
  • The nodes and attachments providers were mutually dependent on each other, as was nodes and AllOptions. Meaning, nodes needed the attachments state and AllOptions to generate the nodes, and both the AllOptions and attachments providers needed nodes to work. I split these two contexts (attachments and AllOptions) into two (2x2=4) contexts so that AttachmentsStoreProvider and AllOptionsStoreProvider is provided before NodesProvider, but the stores are populated when nodes are generated the first time.
  • Instead of letting Form.tsx take care of redirecting to the first form page when none is present, I moved that to the FormFirstPage component. This was in an effort to make the code easier to read, and prevent a blank form page from showing up when navigating to a form (before the first form page was found and loaded).
  • Using our new hooks a lot of places, instead of getting data from redux. I also rewrote a lot of tests to use injected query responses to 'mock' data instead of injecting data into redux. This should help quite a lot in removing redux, which I later removed in about half a days work after merging these changes into the validation-hooks branch.
  • Apparently it is possible to set global page settings in the layout-sets.json file. I now added that global page setting as a default in LayoutSettingsContext to make the auto-save-behaviour tests work again. This looks like it also fixes Layout settings for receipt pages and instance selection #1686, as it is now possible to override these settings globally in layout-sets.json, not just per-page.
  • The legacy rules runner has been fully rewritten to use the new form data objects. Legacy rules will run on debouncing (and after saving form data to the server and getting a response back). Unlike before, rule results may change the data model regardless of there being a component bound to the changed location (solving Kalkuleringer lagrer ikke data til datamodellfelter som ikke er i skjema #707).
  • I commented out some code in ProcessNavigationContext.tsx that should probably be altered a bit to work with the newer validation rewrite. (Note to self, and @bjosttveit)
  • I fixed some state updates outside of a useEffect() in useGetOptions.ts. React nagged me about these.
  • PDFWrapper now replaces the way PDF preview/printing was added to ProcessWrapper/Form. This way the PDF functionality can properly read search params in the URL again.
  • I also fixed a problem in page navigation where search params were lost whenever we redirected the user to the correct form page (and/or the correct current task) in the URL. The search params were needed to make the PDF generator work, and so I also changed the pdf.ts test slightly to make that test use the same URL as the real PDF generator would.
  • In the same vein, I also got rid of the fixed 'form page'-ish URLs for non-data process steps. So for example, instead of a URL like '.../Task_3/confirmation' when on a confirm step, the URL is now just .../Task_3 and the fact that Task_3 is a confirm step should be read out from the app configuration instead of magic URL constants. When doing this I noticed that there was a route in place for stateless apps to render a receipt, but I removed it since it's not possible for a stateless app to 'send in' any data, and such it's also not possible to show the receipt (it would have to turn stateful in order to do that, which is possible, but at that point the route would no longer kick in).
  • I added useAsRef() as a useful tool when creating unchanging callbacks that still should read the most current state. This hook has a striking similiarity to the useEffectEvent() in the validation-hooks branch, which I wish I would have read beforehand. I also used this ref trick to remove the cache key from useWaitForState().
  • The AddressComponent has now been heavily cleaned up, and will be even cleaner in validation-hooks.
  • I moved some files in src/layout/Group, but my changes in them caused git to think I deleted GroupContainer* and added RepeatingGroupContainer*. This may become a pain when merging, sorry about that.
  • Much reduced prop-drilling in repeating group components
  • Extracted CSS from GenericComponent to its own file
  • Moved the RenderLabel, RenderDescription and RenderLegend out to be real components instead of components that are re-generated whenever GenericComponent re-renders. All in all, with reducing the complexity and amount of data passing through GenericComponent it will usually not re-render all of the time.
  • Exposing queryClient in the window object, so that Cypress can inject data into the query cache in cy.changeLayout(). This should also be used for DevTools in some way.
  • PageNavigationProvider was moved from the global scope down into FormProvider. Only forms (data-tasks) have pages, so it's nice to have the provider reset for every task navigation.

Related Issue(s)

Probably also fixes:

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
  • 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

Ole Martin Handeland added 30 commits June 2, 2023 11:45
…an easily switch back and forth between implementations
…we read from the data model to use the new hook instead of the (soon to be old) redux state. I think I'm on the right path here, but there are still errors in my code right now that I don't have great solutions for. Passing the hook-state into every other saga function seems futile (I tried that), so I'm committing now to try some variations.
… or casting. Renaming `global.d.ts` to `modules.d.ts`, because using imports in this file breaks TypeScript everywhere when importing module css files.
…bally on the window object. This lets me rewrite parts of the form data storage system without doing it all at once, and without passing the formData (and future variables) around into every other saga action that needs to read it.
…t should only be accessed directly from the relevant sagas
…eed for it when it is switched on. Making a dummy.
…is left to implement. Committing them now to avoid having these huge commits in the future.
…ly everything re-renders and causes infinite loops. Need to rethink the structure here, because this became a hook-mess pretty quickly.
…changes. Aaaand we're back to where we started with Redux, I suppose. Hoping this works better to avoid the mess, and figuring out why I get an infinite loop of changes (although it isn't working quite yet).
…at making useStateDeepEqual to fix this was hacky, because the root problem is that some state seems to change and make everything fail for some reason, but haven't gotten to the bottom of it quite yet. Happy that it works better now! Using immer for the reducer as well, as this makes things much easier to work with.
…ave(), trying to get state changes down to a minimum
# Conflicts:
#	src/features/formData/submit/submitFormDataSagas.ts
…o 'freeze' debounced data right away after fetching the data
…so JSON formatting the output so that null actually shows up as null
…ype problems with introducing other types than strings
# Conflicts:
#	src/features/confirm/components/ConfirmButton.tsx
#	src/features/dataLists/fetchDataListsSaga.ts
#	src/features/expressions/index.ts
#	src/features/formData/submit/submitFormDataSagas.ts
#	src/features/formRules/checkRulesSagas.ts
#	src/features/layout/update/updateFormLayoutSagas.ts
#	src/features/options/fetch/fetchOptionsSagas.test.ts
#	src/features/options/fetch/fetchOptionsSagas.ts
#	src/features/party/selectPartySagas.ts
#	src/features/toggles.ts
#	src/global.d.ts
#	src/layout/Address/AddressComponent.tsx
#	src/layout/Datepicker/index.tsx
#	src/layout/GenericComponent.tsx
#	src/setupTests.ts
#	src/utils/validation/runClientSideValidation.ts
#	test/e2e/support/app-frontend.ts
#	test/e2e/support/index.ts
#	test/e2e/support/navigation.ts
#	test/tsconfig.json
#	yarn.lock
src/features/formData/FormDataWrite.tsx Fixed Show fixed Hide fixed
src/layout/Group/RepeatingGroupContainer.tsx Dismissed Show dismissed Hide dismissed
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

C Reliability Rating on New Code (required ≥ A)
61.43% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@olemartinorg olemartinorg marked this pull request as ready for review December 28, 2023 14:35
@olemartinorg olemartinorg added kind/breaking-change Issue/pull request containing a breaking change kind/other Pull requests containing chores/repo structure/other changes labels Dec 28, 2023
@olemartinorg olemartinorg merged commit 33bbeb8 into main Dec 28, 2023
7 of 8 checks passed
@olemartinorg olemartinorg deleted the experiment/new-form-data-storage2 branch December 28, 2023 14:36
@olemartinorg olemartinorg restored the experiment/new-form-data-storage2 branch December 28, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/breaking-change Issue/pull request containing a breaking change kind/other Pull requests containing chores/repo structure/other changes
Projects
None yet
1 participant