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

BNGP-5126 Prevent resubmission of form via back button #771

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

jd-barnard
Copy link
Contributor

@jd-barnard jd-barnard commented Jun 6, 2024

The main changes are in

packages/webapp/src/plugins/redirect-view.js -> a plugin to redirect to the error view (post-redirect-get pattern)

packages/webapp/src/utils/redirect-view-handler.js -> a helper to add a boolean used by the above plugin (indicating that we want to return h.view following such a redirect).

Other changes are mostly where we implement the usage of the plugin when returning a view to display an error message following a form POST.

@jd-barnard jd-barnard self-assigned this Jun 6, 2024
@jd-barnard jd-barnard changed the title Feat/bngp 5126 post redirect get BNGP-5126 Prevent resubmission of form via back button Jun 10, 2024
name: 'redirect-view',
register: (server, _options) => {
server.decorate('toolkit', 'redirectView', function (route, data) {
if (this.request.isInjected && route !== '/test') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isInjected is where we inject via a test - the exception here is the /test route which is used by the redirect-view.spec.js test to assert the redirection happens. i.e. we don't want a redirect for the existing error view tests on existing routes.

@@ -0,0 +1,8 @@
const addRedirectViewUsed = (handler) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is preferred to having to add the boolean to all routes that return an error following a form submission - i.e. it would be easy to miss.

@jd-barnard jd-barnard marked this pull request as ready for review June 10, 2024 13:52
Copy link
Collaborator

@StuAA78 StuAA78 left a comment

Choose a reason for hiding this comment

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

Linting has picked up a missing comma in the array of redis keys in constants.js that may cause issues? Also a few cases where we're importing a file but don't seem to use it.

Other than that all looks good to me 😃

@@ -186,6 +187,7 @@ const redisKeys = {
SAVE_APPLICATION_SESSION_ON_SIGNOUT_OR_JOURNEY_CHANGE,
PRE_AUTHENTICATION_ROUTE,
SAVE_APPLICATION_SESSION_ON_SIGNOUT,
VIEW_DATA
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing a comma.

Suggested change
VIEW_DATA
VIEW_DATA,

@@ -1,4 +1,6 @@
import constants from '../../utils/constants.js'
import { getValidReferrerUrl } from '../../utils/helpers.js'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't see getValidReferrerUrl being used in this file?

@@ -1,5 +1,6 @@
import constants from '../../utils/constants.js'
import { validateFirstLastNameOfLandownerOrLeaseholder } from '../../utils/helpers.js'
import { getValidReferrerUrl, validateFirstLastNameOfLandownerOrLeaseholder } from '../../utils/helpers.js'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't see getValidReferrerUrl being used in this file?

@@ -1,5 +1,6 @@
import constants from '../../utils/constants.js'
import { validateAddress } from '../../utils/helpers.js'
import { redirectAddress, validateAddress } from '../../utils/helpers.js'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't see redirectAddress being used in this file?

# Conflicts:
#	packages/webapp/src/routes/credits-purchase/nationality.js
#	packages/webapp/src/routes/credits-purchase/upload-metric-file.js
#	packages/webapp/src/routes/developer/check-metric-file.js
#	packages/webapp/src/routes/developer/upload-consent-to-allocate-gains.js
#	packages/webapp/src/routes/developer/upload-metric-file.js
#	packages/webapp/src/routes/developer/upload-written-authorisation.js
Copy link

StuAA78
StuAA78 previously approved these changes Jul 30, 2024
Copy link
Collaborator

@StuAA78 StuAA78 left a comment

Choose a reason for hiding this comment

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

👍

Copy link

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.

4 participants