Skip to content

MultiFieldRepeat page controller#609

Open
micolgiannelli2 wants to merge 41 commits into
v2from
micol/213-repeating_page
Open

MultiFieldRepeat page controller#609
micolgiannelli2 wants to merge 41 commits into
v2from
micol/213-repeating_page

Conversation

@micolgiannelli2

@micolgiannelli2 micolgiannelli2 commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Description

Created a new component to remove hard to maintain tech debt and extend existing functionality of repeat field component

https://ukhsa.atlassian.net/browse/SBS-213

Context

TODO..

Changes

  • Change A
  • Change B

Challenges encountered and work arounds

  • The old version in our forms was using SummaryViewModel so that the summary transormations would be implemented I am not sure what the other possible solutions to this would be

Type of change

What is the type of change you are making?

  • Chore or documentation (non-breaking change that does not add functionality)
  • ADR (Architectural Decision Record, non-breaking change that documents or proposes a decision)
  • Refactor (non-breaking change that improves code quality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR title

PR titles should be prefixed with the type of change you are making, based on the README.md#versioning.
This is so that when performing a squash merge, the PR title is automatically used as the commit message.

Have you updated the PR title to match the type of change you are making?

  • Yes
  • No, I need help or guidance

Testing

Automated tests

Have you added automated tests?

  • Yes, unit or integration tests
  • Yes, end-to-end (cypress) tests
  • No, tests are not required for this change
  • No, I need help or guidance
  • No (explain why tests are not required or can't be added at this time)

Manual tests

Have you manually tested your changes?

  • Yes
  • No, manual tests are not required or sufficiently covered by automated tests

Have you attached an example form JSON or snippet for the reviewer in this PR?

  • Yes
  • No, any existing form can be used
  • No, it is not required or not applicable

Steps to test

  1. Step 1
  2. Step 2

Documentation

Have you updated the documentation?

  • Yes, I have updated ./docs for this change since additional explanation or steps to use/configure the feature is required
  • Yes, I have added or updated an ADR for this change since it is large, complex, or has significant architectural implications
  • Yes, I have added inline comments for hard-to-understand areas
  • No, I am not sure if documentation is required
  • No, documentation is not required for this change

Discussion

Warning

Large or complex changes may require discussion with the maintainers before they can be merged. If it has not yet been discussed, it may delay the review process

Have you discussed this change with the maintainers?

  • Yes, I have discussed this change with the maintainers on slack, email or via GitHub issues
  • Yes, this change is an ADR to help kick-off discussion
  • No, this change is small and does not require discussion
  • No, I am not sure if one is required

@micolgiannelli2 micolgiannelli2 marked this pull request as ready for review June 11, 2026 15:57
@micolgiannelli2 micolgiannelli2 requested a review from a team as a code owner June 11, 2026 15:57
@@ -0,0 +1,18 @@
Feature: Repeating multi field - add multiple sections

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

File is called addAndEditSection.featuure (spelling error)
Test does not edit

There's a fair bit of overlap in the tests, would it be easier to have a single one for add, edit and delete?

And the summary card "Item 2" contains a row "Your name" with value "Bob"
And the summary card "Item 2" contains a row "Language" with value "Italian"
And the summary card "Item 2" has a "Change" link to "?view=1"
And the summary card "Item 2" has a "Remove" link to "?remove=1" No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test doesn't actually verify deletion, should it or is it enough with verifying there's a link to remove?

And the summary card "Item 2" has a "Change" link to "?view=1"
And the summary card "Item 2" has a "Remove" link to "?remove=1"
And I edit the summary card "Item 2"
// TODO: view syntax for editing a field that has already been pre-filled

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Todo needs sorting
Again a fair bit of overlap in test coverage
This test doesn't seem to test anything more than "addAndDeleteSection.feature‎" ?

);

// State schema (Pass 2) — shape only
// TODO: fix landline number is no-longer read

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Todo

Comment thread runner/src/server/plugins/engine/pageControllers/helpers.ts Outdated
name: String(index), // macro delete link → ?remove={{ data.name }}
title: this.cardTitle(index), // "Item 1"
index,
card: `?view=${index}`, // macro change link → href="{{ data.card }}" /// TODO: unsure Where this si actually used

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Todo

// This somewhat duplicates the logic in Summary View Model --- consider whether to re-use the Summary View Model instead of having this logic in two places
// Decision to not do the above was postponed for now as it would require some refactoring of the Summary View Model
// The other suitable option would be to create a View Model that extends Summary View Model and contains this additional logic for handling multiple entries, and then use this new View Model in both the Summary Page Controller and the Repeated Multi Field Summary Page Controller
getViewModel(formData: any) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we avoid any

title: this.cardTitle(index), // "Item 1"
index,
card: `?view=${index}`, // macro change link → href="{{ data.card }}" /// TODO: unsure Where this si actually used
items: (this.inputComponents ?? []).map((comp: any) => ({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we avoid any

}

private cardTitle(index: number): string {
// TODO: the name of this should probably have another default as opposed to "Item {index}", and this default should be used in the summary page template as well, so that it's consistent with the case where there is no custom text at all

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Todo

}

// I don't understand why we need this extra function this should be the same as everyere else
private formatValue(comp: any, value: unknown): string {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Left in comment
also can we avoid any

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.

2 participants