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

fix(per): Ensure add another responses are rendered correctly #949

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

teneightfive
Copy link
Contributor

Proposed changes

What changed

This passes a responded property to the framework response component
for add another fields.

Why did it change

The framework response component was updated to know about both
responded and prefilled and now requires that the value be passed
as true in order to render the value.

Issue tracking

Before After
image image

Checklists

Testing

Automated testing

  • Added unit tests to cover changes
  • Added end-to-end tests to cover any journey changes

Manual testing

Has been tested in the following browsers:

  • Chrome
  • Firefox
  • Edge
  • Internet Explorer

Environment variables

  • No environment variables were added or changed

Other considerations

Commit messages with a fix or feat type are automatically used to generate the changelog and
GitHub release notes during the release task. Please make sure they will read well on their own in a
summary of changes and that the commit body gives a more detailed description of those changes if necessary.

@teneightfive teneightfive self-assigned this Oct 30, 2020
@teneightfive teneightfive temporarily deployed to booksecuremove-pr-949 October 30, 2020 14:55 Inactive
@@ -611,7 +611,7 @@ describe('Presenters', function () {
{
value: test.value,
valueType: test.valueType,
responded: false,
responded: true,
Copy link
Contributor

@merlinc merlinc Oct 30, 2020

Choose a reason for hiding this comment

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

Would it be worthwhile to have a test for the false value too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this instance it should never actually be false. That was the issue. When this presenter is called, for the add another fields it should always set responded to true so that the value gets displayed.

Copy link

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

tested on heroku app and looks good, thanks for the quick fix

This passes a responded property to the framework response component
for add another fields.

The framework response component was updated to know about both
responded and prefilled and now requires that the value be passed
as true in order to render the value.
@teneightfive teneightfive temporarily deployed to booksecuremove-pr-949 October 30, 2020 15:15 Inactive
@codeclimate
Copy link

codeclimate bot commented Oct 30, 2020

Code Climate has analyzed commit 8d5794e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 97.8% (0.0% change).

View more on Code Climate.

@teneightfive teneightfive merged commit 7f34378 into master Oct 30, 2020
@teneightfive teneightfive deleted the fix/add-another-responses branch October 30, 2020 15:24
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