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

ASSIGNED
Assigned to

Status

()

enhancement
P1
normal
ASSIGNED
11 months ago
12 days ago

People

(Reporter: jaws, Assigned: sfoster)

Tracking

(Blocks 1 bug, {leave-open})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webpayments])

User Story

Errors to handle
* field-specific merchant errors
* missing required fields
* custom Fx field validation (Luhn, postal/zip code, etc.)

Attachments

(1 attachment, 6 obsolete attachments)

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.
Flags: qe-verify+
Priority: -- → P2
QA Contact: hani.yacoub
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
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.
I also noticed that field-specific merchant errors also need to be surfaced on the summary page.
User Story: (updated)
Priority: P2 → P3
Priority: P3 → P2
Whiteboard: [webpayments-reserve] → [webpayments]
Blocks: 1491815
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.
Assignee: nobody → sfoster
Priority: P2 → P1
Status: NEW → ASSIGNED
* 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
Attachment #9024156 - Attachment description: Bug 1481481 - WIP: Create separate forms for shipping, payer and billing address → Bug 1481481 - Create separate forms for shipping, payer and billing address
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
* 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
* 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
Attachment #9026507 - Attachment is obsolete: true
Attachment #9026634 - Attachment is obsolete: true
(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.
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c026a6f2c6cd
Create separate forms for shipping, payer and billing address 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
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 :/
Attachment #9028473 - Attachment description: Bug 1481481 - Use selectedStateKey rather than page state's .guid property in the page forms. → Bug 1481481 - Use selectedStateKey rather than page state's .guid property in the page forms. r?MattN
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.
(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.
  • 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

Attachment #9028474 - Attachment description: Bug 1481481 - Use form to validate the picker selected option → Bug 1481481 - Use forms to validate the picker selected option. r?MattN

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.

Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
Attachment #9028473 - Attachment is obsolete: true
Attachment #9028474 - Attachment is obsolete: true
Attachment #9037725 - Attachment is obsolete: true
Attachment #9045778 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.