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

Multipage Surveys #40

Merged
merged 11 commits into from
Nov 19, 2021
Merged

Multipage Surveys #40

merged 11 commits into from
Nov 19, 2021

Conversation

karansampath
Copy link
Member

Working on ensuring that the numbering of questions works properly. The rest of the implementation should be correct.

sumants-dev and others added 3 commits October 26, 2021 16:20
@karansampath karansampath mentioned this pull request Nov 1, 2021
@markwhiting
Copy link
Member

markwhiting commented Nov 1, 2021

@karansampath looks good overall. I can do a review once you've finalized the numbering?

Also I notice that the failing test (Missing CSRF middleware) is requesting we add back CSRF. @sumants-dev or @karansampath I thought we had left it out because it is now built in to Express, but perhaps we should explicitly put it back?

@sumants-dev sumants-dev linked an issue Nov 1, 2021 that may be closed by this pull request
@sumants-dev
Copy link
Collaborator

sumants-dev commented Nov 1, 2021

I removed it because it added some complications to the flow since we weren't using cookies. I added it back. @karansampath If you have question how it works, DM me, I can go into it. Basically, we have generate a new token for each post request.

@karansampath On unrelated note to csurf protection, please look into this.

  1. the final page submission doesn't seem to work. On the last page of the survey for both one page and multipage. It just returns to the survey. You need to add this commented part back for the last page submission.
// if (admin) {
  //   res.render("thanks", {
  //     code: JSON.stringify(req.body, null, 2),
  //     admin: admin,
  //   });
  // } else res.redirect("/"); // Needs to be updated to deal with multiple page surveys
  1. Add some logic to change the submit on pages to next. You can do this by creating another PUG form for next pages. Or add a conditional in PUG where a passed parameter from server side indicates this is the last page. https://pugjs.org/language/conditionals.html

@markwhiting
Copy link
Member

Thanks @sumants-dev and makes sense re CSRF.

@karansampath
Copy link
Member Author

Ah, thanks for pointing out 1, didn't see that and will get that fixed.

On 2, I thought about adding next but realized that since workers aren't going to be able to access previous pages it may be slightly misleading for them. Won't just a submit make it clear that these answers couldn't be changed later on?

@markwhiting
Copy link
Member

If you're thinking about how to make it more clear that people can't go back, perhaps we do something like "Submit page x of y"?

@karansampath
Copy link
Member Author

@sumants-dev I'm slightly unsure how to fix the submission issue, it seems my conditionals aren't working and I'm unsure why. Could you please take a quick look at it? Thanks!

Have fixed the other issues.

@sumants-dev
Copy link
Collaborator

sumants-dev commented Nov 15, 2021 via email

@sumants-dev sumants-dev enabled auto-merge (squash) November 15, 2021 20:41
@karansampath
Copy link
Member Author

Tested and fixed numbering. The rendering of the survey looks different so let me know if that's okay otherwise I think the PR is ready to merge @markwhiting.

Copy link
Member

@markwhiting markwhiting left a comment

Choose a reason for hiding this comment

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

Overall looks great. I just think we could have a minor tweak to get the question format more consistent but otherwise seems good to go.

Copy link
Member

@markwhiting markwhiting left a comment

Choose a reason for hiding this comment

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

👍

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.

Multi page surveys
3 participants