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

Fix: no owners on template error when loading unsaved draft form #4195

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

bryan-robitaille
Copy link
Contributor

Summary | Résumé

This PR fixes a bug where an error was being thrown when a user accesses the responses page directly when using the "0000" temporary form ID. This use case was probably occurring when a user bookmarked the responses page with the "0000" id in the url.

Screenshot 2024-08-15 at 8 55 29 AM

The logic set to redirect a user at the layout level was wrapped in a try / catch which did not allow the redirect to properly bubble up to the NextJS Router.

The fetchSubmissions function was also checking for a formID of "0000" to short circuit but undefined was being passed in as it is the value stored in the TemplateStore for a draft form.

A small refactor was done on getUnprocessedSubmissionsForUser in lib/users to remove an unused argument that was never referenced in the codebase.

Before After
bug fixed

Test instructions | Instructions pour tester la modification

  1. Create a new draft form
  2. Navigate to the responses page
  3. Bookmark the responses page (url should show 0000 as formID)
  4. close tab to clear session storage
  5. Open bookmark
  6. Should now be on a new draft form with no error in terminal

Unresolved questions / Out of scope | Questions non résolues ou hors sujet

There is still an issue where the "0000" remains in the URL even though the session storage has been updated with a proper form ID.

Pull Request Checklist

Please complete the following items in the checklist before you request a review:

  • Have you completely tested the functionality of change introduced in this PR? Is the PR solving the problem it's meant to solve within the scope of the related issue?
  • The PR does not introduce any new issues such as failed tests, console warnings or new bugs.
  • If this PR adds a package have you ensured its licensed correctly and does not add additional security issues?
  • Is the code clean, readable and maintainable? Is it easy to understand and comprehend.
  • Does your code have adequate comprehensible comments? Do new functions have docstrings?
  • Have you modified the change log and updated any relevant documentation?
  • Is there adequate test coverage? Both unit tests and end-to-end tests where applicable?
  • If your PR is touching any UI is it accessible? Have you tested it with a screen reader? Have you tested it with automated testing tools such as axe?

Copy link
Contributor

craigzour
craigzour previously approved these changes Aug 15, 2024
Copy link
Contributor

@craigzour craigzour left a comment

Choose a reason for hiding this comment

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

Nice work! I tested it locally and I did not find any issue.

@@ -46,10 +46,6 @@ export default async function Layout({
if (initialForm === null) {
redirect(`/${locale}/404`);
}

if (initialForm.isPublished) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bryan-robitaille how did you catch this? I ran multiple tests locally and I did not notice anything wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cypress was throwing an error when it was trying to load the settings pages with a published form. Any form that is published with the old logic would just keep redirecting to the /settings page in an infinite loop.

@bryan-robitaille bryan-robitaille enabled auto-merge (squash) August 15, 2024 16:05
@craigzour craigzour self-requested a review August 15, 2024 16:36
@bryan-robitaille bryan-robitaille merged commit 9806cbe into develop Aug 15, 2024
13 checks passed
@bryan-robitaille bryan-robitaille deleted the fix/no_owners_on_template branch August 15, 2024 16:36
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.

2 participants