-
Notifications
You must be signed in to change notification settings - Fork 0
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
EPP-146 amend CHANGE SUBSTANCES page #76
base: master
Are you sure you want to change the base?
Conversation
2b1769b
to
9980156
Compare
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.
Had a look through the files, Vivek's comment are good suggestions
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.
Please rebase or create a new PR to avoid the additional changes not related to this PR.
- add some logic to redirect to the correct path if answer is no it will check agains the previous pages answers - add change-substances.html - add fields in index.js, fields.json and pages.json and fiels/index.js - add summary-data-section.js - add some path to next page
71294bf
to
70fb661
Compare
I rebased again . what do you think about the behaviour? |
f2ac836
to
809e157
Compare
- improved check-answer-behaviour.js - reorder summary data sections as per figma - tide up fields.json - correct missing field label in page.json - add new behaviour to change-home-address and amend-details-pages - add unit test
809e157
to
dc98b3b
Compare
if (allFieldsNo) { | ||
return res.redirect(`${req.baseUrl}/no-details-amend`); | ||
} else if (req.form.values[currentField] === 'no' && !allFieldsNo | ||
&& currentField === 'amend-change-substances-options') { |
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.
The above logic doesn't seem to cover AC6 on the ticket?
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.
Since AC6 contains more requirement, can you be more specific? which condition is not covered?
- update index.js add fork only for negative path since positive is by default - update tests check-answer-redirect.spec.js as well as check-answer-redirect.js
What?
as per jira ticket EPP-146 Create a CHANGE SUBSTANCES page
Why?
to allow the user to be directed to change substance flow or to skip this step
How?
Testing?
n/a manual
Screenshots (optional)
Anything Else? (optional)
Check list
here is an example commit