1481481 - Share validation between the form pages and the summary page and surface the errors on both views

Status: REOPENED

(Firefox :: WebPayments UI, enhancement, P5)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Currently the summary view will show errors if the selected option is missing required fields, and the form pages will show errors if the record being added/edited is missing fields or has other validation issues such as invalid CC number (Luhn algorithm), invalid email address, valid postal code, and valid region.

We will need to create a shared module of some sorts that can do this validation in one place, and then can be referenced by the two pages such that we don't have to duplicate this validation logic.

Comment 1

[Matthew N. [:MattN]]

One idea I had would be to have a separate <address-form> for each address picker so behind the scenes we populate the fields with the selected address and then we can use the same form validation logic for the summary screen by checking the validity of the corresponding form.

Comment 2

[Matthew N. [:MattN]]

I also noticed that field-specific merchant errors also need to be surfaced on the summary page.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Broadly, the plan is to create separate forms for the shipping address, billing address and payer address forms. This will help as each has different rules and navigating these is the source of some of the complexity we are dealing with today in this code. We can then re-use these forms when validating an address picker by delegating to the relevant form's checkValidity() method.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Create separate forms for shipping, payer and billing address

* New AddressForm instances for each address type
* Move selected-state-key out of state and into a form attribute
* Remove passing form title via state - form can use state (for editing or not, and for shippingType) to pick correct data-attribute

TODO:
* Fix up remaining tests in browser/
* Moving validation logic to the forms should be in a patch layered on this one

Comment 7

[Sam Foster [:sfoster] (he/him)]

I've updated the first patch so that it breaks out the different forms, moves the selectedStateKey out of the page state and into the form attribute. There should be no other functional changes. We could land something like this ahead of the other validation changes to reduce conflicts with other work. 

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=838ad2014e5521e032c1e566c42f5c9319035a4e

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Create separate forms for shipping, payer and billing address. r?MattN

* New AddressForm instances for each address type
* Move selected-state-key out of state for the address forms, and into an attribute
* Remove passing form title via state - form can use state (for editing or not, and for shippingType) to pick correct da
* Remove form title logic from address picker.
* Amend onboarding logic to direct to billing address form when necessary
* Fix-up tests

Comment 9

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Create separate forms for shipping, payer and billing address. r?MattN

* New AddressForm instances for each address type
* Move selected-state-key out of state for the address forms, and into an attribute
* Remove passing form title via state, just use data-title-edit and data-title-add on the element
* Remove shippingOption form title logic from address picker, just set the right attribute values from PaymentDialog
* Move setting the extraRequiredFields data attribute the payer form needs out to the payment-dialog's render.
* Amend onboarding logic to direct to billing address form when necessary
* Fix-up tests

Comment 10

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] from comment #9)
> Created attachment 9026634 [details]
> Bug 1481481 - Create separate forms for shipping, payer and billing address.
> r?MattN

Sorry about the noise. Attachment 9024156 [details] (D11545) has now been updated with all the requested changes.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c026a6f2c6cd
Create separate forms for shipping, payer and billing address r=MattN

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c026a6f2c6cd

Comment 13

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Use selectedStateKey rather than page state's .guid property in the page forms. r?MattN

* Remove .guid property from the page state use the value referenced by selectedStateKey as the current value in AddressForm and BasicCardForm
* Fix up mochitests, browser tests

Comment 14

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Use forms to validate the picker selected option. r?MattN

Depends on D13326

Comment 15

[Sam Foster [:sfoster] (he/him)]

attachment 9028473 [details] is part II and just removes the {pageid}.guid property in favor of using the selected-state-key to references the record the given picker and form should be loading. That is pretty stable probably ready for review. 

attachment 9028474 [details] is WIP still. All the tests/mochitests are now passing, and manual testing looks good. But I still need to fix up tests/browser. 

I'm not sure why moz-phab created 2 revisions for these patches :/

Comment 16

[Sam Foster [:sfoster] (he/him)]

MattN: I've updated the first patch (D13327  Bug 1481481 - Use form to validate the picker selected option) to make sure all the selectedStateKey -related stuff is in there, and all the tests are passing at that point. 

The 2nd patch is WIP still as discussed.

Comment 17

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] from comment #16)
 > The 2nd patch is WIP still as discussed.

Rebased and updated. More detail in the phab revision comments.

Comment 18

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Wait for updateState event before resolving paymentRequest.updateAutofillRecord. r?MattN

• Dispatch the same messages when add/updating temporary collections
• Wait for both the response message and updateState message when saving records from card and address forms

Depends on D13326

Comment 19

[Sam Foster [:sfoster] (he/him)]

I've added another patch to the series which defers resolving paymentRequest.updateAutofillRecord until after the updateState event has been received.

The selectedStateKey / .guid patch is unchanged bar restoring the test skippage.

The form validation patch is now ready for review also. I'll update the commit message in phabricator, but basically the only thing that I'm aware of that is outstanding is a test failure: test_address_edit in browser_change_shipping.js is failing locally; the expected selected option index is wrong after updating the record in the formautofill store. I'm not sure this patch caused that but I'll look some more before trying to land anything.

This is a large patch, apologies for that. Let me know if you want me to walk through it, break it up some more or whatever will help.

Comment 20

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1481481 - Get test suite into a passing state when re-enabled. r?MattN

Comment 21

[Sam Foster [:sfoster] (he/him)]

Last time I checked, it was becoming increasingly difficult to get the (disabled) test suite to run. I don't anticipate updating these patches. If that changes I'll re-open.

Comment 22

[Matthew N. [:MattN]]

We haven't closed the other Web Payments bugs so I think this should stay open for when work on this component resumes.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Comment 26

[Matthew N. [:MattN]]

This code is getting removed in bug 1721229 but clearing leave-open in the meantime.