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

🐛 Handle hash based frontend routing #3574

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Nov 2, 2023

Fixes #3362
Linked to open-formulieren/open-forms-sdk#586

This adds a new query parameter redirect_path, that the frontend sdk should special case on window.onload to redirect to the correct path based on the configured routing method.

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer a query param _action=foo rather than redirect_path so that we can decouple the actions from the actual client-side URLs.

We only need to synchronize the list of literal action names then and our backend and frontend become more agnostic of each other.

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

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

Comparison is base (141529b) 95.97% compared to head (a6d6282) 95.96%.

❗ Current head a6d6282 differs from pull request most recent head 2dbe608. Consider uploading reports for the commit 2dbe608 to get more accurate results

Files Patch % Lines
src/openforms/frontend.py 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3574      +/-   ##
==========================================
- Coverage   95.97%   95.96%   -0.01%     
==========================================
  Files         680      681       +1     
  Lines       21893    21892       -1     
  Branches     2531     2533       +2     
==========================================
- Hits        21011    21009       -2     
  Misses        611      611              
- Partials      271      272       +1     

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

@Viicos
Copy link
Contributor Author

Viicos commented Nov 8, 2023

One thought that I had: The number of action_args arguments passed in get_frontend_redirect_url could be validated depending on the action used, but this is only beneficial for the developer. overloads could be useful but a bit overkill and cumbersome.

I took a look at the possible redirects from the backend and it seems to be all covered. There're still some old URL formats in tests but I think they mimic what is emitted by the SDK (e.g. in authentication with the next URL).

@Viicos Viicos force-pushed the issue/3362-hash-redirects branch 2 times, most recently from db24f88 to 6eb625e Compare November 8, 2023 13:57
@Viicos Viicos force-pushed the issue/3362-hash-redirects branch from c6ead31 to 64a6914 Compare November 13, 2023 13:19
.add(
{
"_action": "afspraak-annuleren",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so right now with hash based routing, this would result in a URL like example.com/myform?time=...&submission_uuid=...#/afspraak-annuleren while it should be:

example.com/myform#/afspraak-annuleren?time=...&submission_uuid=... - i.e. the time & submission_uuid really should be _action_params as they're directly related to the action.

src/openforms/appointments/tests/test_views.py Outdated Show resolved Hide resolved
src/openforms/appointments/tests/test_views.py Outdated Show resolved Hide resolved
src/openforms/appointments/tests/test_views.py Outdated Show resolved Hide resolved
@Viicos
Copy link
Contributor Author

Viicos commented Nov 15, 2023

There are still some failing tests, mainly because:

if action_params:
# furl takes care of quoting the query parameters if necessary, so we don't use urllib.parse.urlencode
_query["_of_action_params"] = urllib.parse.urlencode(action_params)
return f.add(_query).url

This performs a first round of URL encoding, e.g. query={"_of_action_params": "time=00%3A00"} (with action_params={"time": "00:00"})

Then furl does a second round of URL encoding, resulting in ?_of_action_params=time%3D00%253A00:

  • = is correctly encoded as %3D
  • % was incorrectly encoded as %25

Another option would be to have query=urllib.parse.urlencode(json.dumps(action_params))

Edit: in fact this might be fine


def test_frontend_redirect_hash(self):
submission = SubmissionFactory.create(
form_url="https://example.com/basepath#after_hash",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to have it (and keep it!), but let's also do the data migration where we clean/sanitize the URLs for existing submissions.

Copy link
Contributor Author

@Viicos Viicos Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the migration, not sure if it covers all cases? Let me know if you think some other cases should be taken into account into this migration

docs/developers/sdk/embedding.rst Show resolved Hide resolved
src/openforms/appointments/tests/test_views.py Outdated Show resolved Hide resolved
src/openforms/appointments/tests/test_views.py Outdated Show resolved Hide resolved
src/openforms/submissions/migrations/0079_cleanup_urls.py Outdated Show resolved Hide resolved
src/openforms/submissions/migrations/0079_cleanup_urls.py Outdated Show resolved Hide resolved
src/openforms/submissions/migrations/0079_cleanup_urls.py Outdated Show resolved Hide resolved
src/openforms/tests/test_frontend_utils.py Outdated Show resolved Hide resolved
@Viicos Viicos force-pushed the issue/3362-hash-redirects branch from f2ecfc6 to 031e3f1 Compare November 17, 2023 09:36
@sergei-maertens sergei-maertens merged commit 7b8f512 into master Nov 17, 2023
22 checks passed
@sergei-maertens sergei-maertens deleted the issue/3362-hash-redirects branch November 17, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server side redirects to the frontend break when the SDK uses hash based routing
2 participants