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

EPP-146 amend CHANGE SUBSTANCES page #76

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

Conversation

PaolaDMadd-Pro
Copy link
Contributor

@PaolaDMadd-Pro PaolaDMadd-Pro commented Feb 17, 2025

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?

  • 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

Testing?

n/a manual

Screenshots (optional)

Anything Else? (optional)

Check list

  • I have reviewed my own pull request for linting issues (e.g. adding new lines)
  • I have written tests (if relevant)
  • I have created a JIRA number for my branch
  • I have created a JIRA number for my commit
  • I have followed the chris beams method for my commit https://cbea.ms/git-commit/
    here is an example commit
  • Ensure drone builds are green especially tests
  • I will squash the commits before merging

@PaolaDMadd-Pro PaolaDMadd-Pro force-pushed the EPP-146-amend-CHANGE-SUBSTANCES branch from 2b1769b to 9980156 Compare February 17, 2025 20:00
@PaolaDMadd-Pro PaolaDMadd-Pro changed the title EPP-146-amend CHANGE SUBSTANCES page EPP-146 amend CHANGE SUBSTANCES page Feb 17, 2025
Copy link

@william-gu-hof william-gu-hof left a 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

Copy link
Contributor

@vivekkumar-ho vivekkumar-ho left a 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
@PaolaDMadd-Pro PaolaDMadd-Pro force-pushed the EPP-146-amend-CHANGE-SUBSTANCES branch from 71294bf to 70fb661 Compare February 28, 2025 12:08
@PaolaDMadd-Pro
Copy link
Contributor Author

Please rebase or create a new PR to avoid the additional changes not related to this PR.

I rebased again . what do you think about the behaviour?

@PaolaDMadd-Pro PaolaDMadd-Pro force-pushed the EPP-146-amend-CHANGE-SUBSTANCES branch from f2ac836 to 809e157 Compare March 3, 2025 11:01
- 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
@PaolaDMadd-Pro PaolaDMadd-Pro force-pushed the EPP-146-amend-CHANGE-SUBSTANCES branch from 809e157 to dc98b3b Compare March 3, 2025 11:25
if (allFieldsNo) {
return res.redirect(`${req.baseUrl}/no-details-amend`);
} else if (req.form.values[currentField] === 'no' && !allFieldsNo
&& currentField === 'amend-change-substances-options') {

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?

Copy link
Contributor Author

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
@PaolaDMadd-Pro PaolaDMadd-Pro mentioned this pull request Mar 3, 2025
7 tasks
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.

3 participants