🐛 Handle server side redirects

[Viicos]

Fixes open-formulieren/open-forms#3362
Linked to open-formulieren/open-forms#3574

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (dd342fd) 71.27% compared to head (54d1198) 73.29%.

Files	Patch %	Lines
src/components/routingActions.js	71.42%	1 Missing and 1 partial ⚠️
src/sdk.js	92.59%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
+ Coverage   71.27%   73.29%   +2.01%     
==========================================
  Files         211      212       +1     
  Lines        4286     4314      +28     
  Branches     1161     1168       +7     
==========================================
+ Hits         3055     3162     +107     
+ Misses       1196     1108      -88     
- Partials       35       44       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Viicos]

Maybe we should add an action for form steps as well: action: form, action_args: <form_step>

Edit: this is already the case 🤦

[sergei-maertens]

Found my unpublished comment 😬

[sergei-maertens]

if redirectPath has query params and there's an uncontrolled query param like foo=bar from the CMS, you will end up with:

https://example.com/base-path/stap/step-1?submissionId=123?foo=bar

which is of course not the correct form. I think getRedirectPathname should return an object with the URL parts:

{
  "path": relativePath,
  "query": queryObj,
}

and then depending on the useHashRouting compose the final URL in this method.

Note that this (probably) works correctly with hash-based routing.

[Viicos]

if redirectPath has query params and there's an uncontrolled query param like foo=bar from the CMS, you will end up with:

But shouldn't we keep unrelated query params? If the CMS needs foo to work correctly and we strip it off, it can break things, can it?

[sergei-maertens]

yes we need to keep it, but now the implementation is naive in that there will only be before-hash query params or after-hash query params while both can be present at the same time - the problem is the double ? in the resulting URL if you don't use hash based routing.

So you need to:

1. Server loads URL with query params, possibly with CMS-required params
2. SDK initializes
3. SDK does pick<query, '_of_action' | '_of_action_args'>
4. SDK processes action & action args into the (relative) path & SDK-specific query params
5. SDK builds a new URL:
   • Join basePath + relative path
   • Merge (original) params with SDK-specific query params from 4.
   • Delete _of_action and `_of_action_args params
6. SDK does pushState with newly constructed URL
   • it retains the "unrelated" query params
   • it drops the private _action routing params
   • it merges in any query params required by the SDK (like submission_uuid and ?productId=... for appointments)

The current implementation fails at 5.2 - the merging is not done properly

[Viicos]

I think the new implementation should cover this

[sergei-maertens]

that's odd, shouldn't it go to ...#/stap/step-1?

[Viicos]

I suspect the api mocks to not cover an api call, so it goes back to the start page as it doesn't know about step-1 but I'm not sure about that

[Viicos]

Edit, figured out while adding tests:

open-forms-sdk/src/sdk.spec.js

Lines 177 to 212 in c87f004

	it.each([
	// With a base path:
	[
	'/base-path/?_of_action=afspraak-annuleren&unrelated_q=1',
	'http://localhost/base-path?unrelated_q=1#/afspraak-annuleren',
	],
	[
	'/base-path/?_of_action=afspraak-maken&unrelated_q=1',
	'http://localhost/base-path?unrelated_q=1#/afspraak-maken',
	],
	[
	'/base-path/?_of_action=cosign&_of_action_params=submission_uuid%3Dabc&unrelated_q=1',
	'http://localhost/base-path?unrelated_q=1#/cosign?submission_uuid=abc',
	],
	[
	'/base-path/?_of_action=resume&_of_action_params=next_step%3Dstep-1&unrelated_q=1',
	'http://localhost/base-path?unrelated_q=1#/stap/step-1',
	],
	// Without a base path:
	[
	'/?_of_action=afspraak-annuleren&unrelated_q=1',
	'http://localhost/?unrelated_q=1#/afspraak-annuleren',
	],
	[
	'/?_of_action=afspraak-maken&unrelated_q=1',
	'http://localhost/?unrelated_q=1#/afspraak-maken/producten', // SDK redirects to producten
	],
	[
	'/?_of_action=cosign&_of_action_params=submission_uuid%3Dabc&unrelated_q=1',
	'http://localhost/?unrelated_q=1#/cosign?submission_uuid=abc',
	],
	[
	'/?_of_action=resume&_of_action_params=next_step%3Dstep-1&unrelated_q=1',
	'http://localhost/?unrelated_q=1#/startpagina', // SDK redirects to start page
	],
	])(

The behavior is different depending on having a basepath or not with hash routing, something might be going wrong

[sergei-maertens]

please investigate further what's causing this 😬

[Viicos]

Tested locally:

When accessing localhost:3000/?_of_action=resume&_of_action_params=%7B%22next_step%22%3A%22test-step%22%7D ($\Leftrightarrow$ localhost:3000/?_of_action=resume&_of_action_params=encodeURIComponent(JSON.stringify({next_step: 'test-step'})), I am redirected to startpagina as expected, because I haven't started the form (it works and I get redirected to http://localhost:3000/#/stap/test-step otherwise).

So I think having a basepath breaks for now. Looking into it

[sergei-maertens]

so the resume flow relies on the submission_id (query) param, as that gets loaded from the backend first before the actual step is rendered. Many of these components are wrapped in a <RequiredSubmission> component/decorator.

Probably best to manually test the behaviour with a real submission, it doesn't make sense to redirect to a form/submission that's not in your session anyway.

[Viicos]

This ended up being a bit more complicated:
We have to differentiate between the react router base path and the "real" browser base path with hash based routing. In hash based routing, router base path will always be "", but we need the extra browserBasePath to be able to correctly set this.clientBaseUrl, as the CMS basepath needs to be taken into account

[Viicos]

Note for later: URLSearchParams is treating %2B the wrong way: %2B is +, and + is being replaced by once decoded. It should stay as +