Terms of service checkbox bug

[raamSoftwire]

Context

The terms of service checkbox doesn't persist answer if there is a validation error with the email address on the sign up page.

Changes proposed in this pull request

Edit the sign up form template to read in the @sign_up_form object for the checkbox value.

Guidance to review

To test this manually:

• Create new account on staging
• Navigate to terms of service agreement page
• Check checkbox, but omit email address
• Submit page
• Get returned to page without checkbox persisted.

Link to Trello card

https://trello.com/c/zWvC0NDW/2035-bug-terms-of-service-checkbox-doesnt-persist-answer-if-there-is-a-validation-error-with-the-email-address

Things to check

• This code does not rely on migrations in the same Pull Request
• If this code includes a migration adding or changing columns, it also backfills existing records for consistency
• API release notes have been updated if necessary
• New environment variables have been added to the Azure config

[raamSoftwire]

This solves the bug, but I'm not convinced it is the correct solution. I've had a look through the docs which suggests that this is the attribute for controlling the checkbox value.

But having a look at other checkboxes in the codebase, we haven't implemented similar changes elsewhere. The second attribute is left as true rather than taking in the property on the object.

[tvararu]

'true' seems to work just as well

[raamSoftwire]

'true' does work on the page but I ran in to some difficulty when writing the tests.

Both true and 'true' add a value='true' attribute to the input element in both the unchecked and checked states, which meant that you couldn't distinguish between the states for the test. I don't really understand how it is working on the page.

@sign_up_form.accept_ts_and_cs only adds value='on' in the checked state, which is means we can test it easily and it is easier to reason about how it is working.

[tijmenb]

Took me a bit to understand this!

For my understanding: what the form builder does is compare @sign_up_form.accept_ts_and_cs with whatever is the second argument to govuk_check_box.

• When we use the boolean true, the boolean is cast to the string "true" as the value=. This means that the for compares true == "true"
• When we use "true" or any other string, it will work because we compare the the correct things
• When we use @sign_up_form.accept_ts_and_cs, this is initially nil, so no value= is provided. When submitting we then submit the string "on", which is compared correctly again

[tvararu]

Discussed offline, LGTM once we add a system spec 👍

[tvararu]

'true' seems to work just as well

[tijmenb]

Capybara has a cool helper for this:

Suggested change

	expect(find_field(t('authentication.sign_up.accept_terms_checkbox')).value).to eq 'on'
	expect(page).to have_checked_field t('authentication.sign_up.accept_terms_checkbox')

[raamSoftwire]

Thanks, I've added that in now

[tijmenb]

Took me a bit to understand this!

For my understanding: what the form builder does is compare @sign_up_form.accept_ts_and_cs with whatever is the second argument to govuk_check_box.

• When we use the boolean true, the boolean is cast to the string "true" as the value=. This means that the for compares true == "true"
• When we use "true" or any other string, it will work because we compare the the correct things
• When we use @sign_up_form.accept_ts_and_cs, this is initially nil, so no value= is provided. When submitting we then submit the string "on", which is compared correctly again

[raamSoftwire]

Thanks @tijmenb for explaining what is going on and for raising the issue in the form builder repo. I've tidied up my commits and tested it all and so I think this is now ready to merge in.

[tijmenb]

👍 Nice, thanks!