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

Add payment email section to payment summary page #890

Merged
merged 13 commits into from
Jul 20, 2020

Conversation

Cruikshanks
Copy link
Member

@Cruikshanks Cruikshanks commented Jul 15, 2020

https://eaflood.atlassian.net/browse/RUBY-1141
https://eaflood.atlassian.net/browse/RUBY-1137

Having removed our first design for solving the problem of capturing a user's preferred Worldpay payment confirmation email address (see PR #889) this implements our second version!

That is to show a hidden section in the payment summary screen which will appear when a user selects 'Pay by credit or debit card'. Along with a header and some content, it will display a field, pre-populated with the contact email address. Whatever is in there when submitted will be saved as the receipt_email.

This is based on example used by the Get a fishing licence service. In its How can we send you your licence details? if the user selects 'Email or text message' a hidden section with those fields will appear.

Get a fishing licence example

Screenshot 2020-07-15 at 16 45 18

This change focuses on adding the section and validating it correctly. For example, we only care about the field being populated correctly if the user is paying by card. If they are not then we shouldn't be forcing them to correct any invalid changes they have made to the field.

A subsequent change will focus on the logic of hiding and showing the section at the appropriate times.

Screenshot of the change Screenshot 2020-07-16 at 00 06 38

https://eaflood.atlassian.net/browse/RUBY-1141
https://eaflood.atlassian.net/browse/RUBY-1137

Having removed our first design for solving the problem of capturing a user's preferred Worldpay payment confirmation email address (see [PR #889](#889) this implements our second version!

This change adds a hidden section in the payment summary screen which will appear when a user selects 'Pay by credit or debit card'. Along with a header and some content it will display a field, pre-populated with the contact email address. Whatever is in there when submitted will be saved as the `receipt_email`.

This PR will focus on adding the section and validating it correctly. For example, we only care about the field being populated correctly if the user is paying by card. If they are not then we shouldn't be forcing them to correct any invalid changes they have made to the field.

A subsequent PR will focus on the logic of hiding and showing the section at the appropriate times.
@Cruikshanks Cruikshanks added the enhancement New feature or request label Jul 15, 2020
@Cruikshanks Cruikshanks self-assigned this Jul 15, 2020
We build on the previous change adding the help text content expected to sit above the input field. We also add the label and input field, plus a matching attribute on the payment_summary_form. By setting the value of `:receipt_email` to `transient_registration.email_to_send_receipt` we get our pre-populated field which should handle the scenario of a user returning to this page having set a different email address.

Finally we have moved the section out of the 'multiple-choice' div. We lose the indentation. But the problem was our input was picking up the 'multiple-choice' class so was actually be set as invisible, as a small text box, and placed under the radio button!

The design for this was inspired by the 'Get a fishing licence service', specifically its 'How can we send you your licence details?'. If you select 'Email or text message' it displays a hidden section which contains an email field. Our section is displaying in the same way as it, with the lack of any indentation. So for now we're judging this as acceptable and moving on.
Following the pattern that seems to be more recently used we delegate receipt_email to the transient_registration rather than creating our own attribute.

This means we can hook in the property getting saved and validated. We have also added `paying_by_card?` to handle the scenario of only validating the email if payment by card is selected.
Using the new initialize callbacks built into `BaseForm` we call a method that will set the receipt email to whatever the transient_registration's `email_to_send_receipt()` logic tells us it should be. We expect 99.999% of the time it will be the current contact_email because receipt_email will not have been set. Though in 3 years time if the service is still running it will be when they come to renew 😁
Try to cover the fact recipt_email is now a field, but that we only care about it if the payment type is card.
We have an email field on the form now so makes sense to include this shared test.
Missed these when updating the tests.
@Cruikshanks
Copy link
Member Author

For quick reference, Sonarcloud is complaining about

  • the TODO comment
  • the fact the new section in the view contains a fieldset without a legend

The new section was very much based on our contact_email form and view, and along with most other pages use of a legend does not appear to be the norm.

If what Sonarcloud is saying is true we need to review this pattern. But I think for now it's better to be consistent and to review this issue at a later date.

@Cruikshanks Cruikshanks marked this pull request as ready for review July 16, 2020 06:27
Copy link
Member

@irisfaraway irisfaraway left a comment

Choose a reason for hiding this comment

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

Some typo nitpicks and a suggested change. Let me know what you think about this!

@irisfaraway
Copy link
Member

@Cruikshanks I think I have some working Javascript! Did it as a separate PR here, designed to be merged into your branch: #892

It will need a minor tweak to the host apps to work as well so will get those PRs going on.

Essentially

- we want to save the email only if its is valid and the user is paying by card
- we want to ignore completely (do not save or validate) the email if the user is paying by bank transfer

In the previous solution we always saved the email, but only validated it if the payment type was card.

This change should deliver what we want. To implement and get working though it has meant

- all attributes now need to be on the form and not delegated to
- we need to update the form attributes using the params during submission else the call to `valid?()` in the baseform fails (it sees everything as blank so always returns `false`)
@Cruikshanks Cruikshanks force-pushed the add-payment-conf-section-to-payment-summary branch from bbc8f90 to 51183a4 Compare July 17, 2020 10:57
Copy link
Member

@irisfaraway irisfaraway left a comment

Choose a reason for hiding this comment

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

Love it! 👏

@Cruikshanks Cruikshanks merged commit 448e1de into main Jul 20, 2020
@Cruikshanks Cruikshanks deleted the add-payment-conf-section-to-payment-summary branch July 20, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants