-
Notifications
You must be signed in to change notification settings - Fork 237
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
Make it more difficult to accidentally clear the session data #588
Conversation
app/views/clears-data.html
Outdated
<div class="govuk-grid-column-two-thirds"> | ||
{{ govukBackLink({ | ||
"text": "Back", | ||
"href": "javascript: window.history.go(-1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking comment:
I believe we can populate this href using the referrer in the server, this is more effort and could be done as a followup.
app/views/clears-data.html
Outdated
|
||
{{ govukButton({ | ||
text: "Clear data", | ||
href: "/prototype-admin/clear-data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking:
I'd expect this kind of interaction to be a <form>
that POSTs to an endpoint, rather than a link to a new page.
@charge-valtech could you add an entry to CHANGELOG under the "Unreleased" heading ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke to Joe
The files that are in app/view
are for users to prototype with, so since these files are not important for people to see, we could consider moving them.
Could we put the clear data page in lib/prototype-admin
instead?
Good point @NickColley - I wasn't sure where to put it but that makes much more sense. |
@charge-valtech would you be able to rebase to fix that CHANGELOG conflict? |
@charge-valtech I hope you don't mind, I've rebased this against master and cleaned up an errant merge from master (that 'Update branch' button in the GitHub interface seems to do more harm than good most of the time) I'm happy this can be merged now – @NickColley would you mind giving it a second pass? |
@charge-valtech thanks for your patience with us! This is a great addition. |
Nice one, thanks @36degrees, @igloosi and @NickColley 👍 |
This builds on the work done in #588. Previously the ‘clear data’ feature used two GET requests – one to display the confirmation screen, and a second to actually clear the session data. This is not ideal because GET requests are meant to be nullipotent (to have no side effects), and links from the docs part of the kit were still pointing to the old URL, which bypassed the confirmation screen. This adds a form to the confirmation screen which POSTs to itself (/prototype-admin/clear-data), and changes the old data-clearing route to match.
Referenced by this issue: #587
A user (low digital skills) was testing our prototype on their iPad and half way through, they managed to rest their palm on the "Clear data" link. Causing quite a bit of disruption to the test.
This change introduces another page which the user comes to, allowing them to go back, or continue to clear the session data.