Closed Bug 1476345 Opened 3 years ago Closed 3 years ago

Address add/edit page error handling fixes

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [webpayments] [user-testing])

User Story

Problems to fix:
* Save/Add button not disabled initially on an empty form
* Error message is in normal black text
* Don't forget to consider the contact/payer form

Attachments

(4 files)

Bug 1476345 - Fix debugging names and add records with missing required fields.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476345 - Disable the address form save button when the form is invalid.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476345 - Only enable relevant fields in address forms and update tests.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1476345 - Fix console listener to not spew when .message doesn't exist.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Similar to bug 1476204 but for the address page. There are various improvements to make to the address add/edit page related to error handling/validation.
Whiteboard: [webpayments] → [webpayments] [triage]
Duplicate of this bug: 1476284
Whiteboard: [webpayments] [triage] → [webpayments]
Whiteboard: [webpayments] → [webpayments] [user-testing]
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
User Story: (updated)
After talking with Jacqueline:
* Fields shouldn't be marked invalid immediately for an add form, only an edit form.
User Story: (updated)
Priority: P2 → P1
Comment on attachment 8994740 [details]
Bug 1476345 - Fix debugging names and add records with missing required fields.

https://reviewboard.mozilla.org/r/259260/#review266464
Attachment #8994740 - Flags: review?(jaws) → review+
User Story: (updated)
Flags: qe-verify+
QA Contact: hani.yacoub
Duplicate of this bug: 1476297
Depends on: 1482808
Duplicate of this bug: 1478332
Depends on: 1483412
Depends on: 1483425
Depends on: 1483427
I'm moving the following two to their own bugs since there are already a ton of other issues I've hit with this bug.
* Saving an invalid form (basic-card too) reverts values to the original, losing changes. (bug 1483427)
* Fields shouldn't be marked invalid immediately for an add form, only an edit form. (bug 1483425)
User Story: (updated)
Comment on attachment 8994741 [details]
Bug 1476345 - Disable the address form save button when the form is invalid.

https://reviewboard.mozilla.org/r/259262/#review269540

::: browser/components/payments/res/containers/address-form.js:91
(Diff revision 3)
> +      // Add these event listeners after the same event listeners are added in
> +      // the EditAddress constructor so that they run first and update the

Can you update this comment to remove ambiguity of which event listeners you want to run first?
Attachment #8994741 - Flags: review?(jaws) → review+
Comment on attachment 9000133 [details]
Bug 1476345 - Fix console listener to not spew when .message doesn't exist.

https://reviewboard.mozilla.org/r/261686/#review269542
Attachment #9000133 - Flags: review?(jaws) → review+
Comment on attachment 8994742 [details]
Bug 1476345 - Only enable relevant fields in address forms and update tests.

https://reviewboard.mozilla.org/r/259264/#review269544

::: browser/components/payments/test/browser/head.js:435
(Diff revision 5)
> +    if (typeof(address.country) != "undefined") {
> +      // Set the country first so that the appropriate fields are visible.
> +      let countryField = content.document.getElementById("country");
> +      ok(!countryField.disabled, "Country Field shouldn't be disabled");
> +      await content.fillField(countryField, address.country);
> +      is(countryField.value, address.country, `country value is correct after fillField`);

nit, this doesn't need to be a template string.

::: browser/extensions/formautofill/content/autofillEditForms.js:133
(Diff revision 5)
> +        fieldClasses = fieldClasses.concat(mailingFieldsOrder);
> +        // `country` isn't part of the `mailingFieldsOrder` so add it when filling a mailing-address
> +        requestedFieldClasses.splice(requestedFieldClasses.indexOf("mailing-address"), 1,
> +                                     "country");
> +      }
> +      if (requestedFieldClasses.includes("name") && requestedFieldClasses.includes("mailing-address")) {

I don't think this will ever be true.

If requestedFieldClasses included "mailing-address", it would have been removed on line 130.
Attachment #8994742 - Flags: review?(jaws) → review+
Comment on attachment 8994742 [details]
Bug 1476345 - Only enable relevant fields in address forms and update tests.

https://reviewboard.mozilla.org/r/259264/#review269544

> I don't think this will ever be true.
> 
> If requestedFieldClasses included "mailing-address", it would have been removed on line 130.

Good catch… I guess that was leftover from a slightly different approach.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d44b1a94dbd5
Fix console listener to not spew when .message doesn't exist. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4b3942b9af
Fix debugging names and add records with missing required fields. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2ee996553e5
Disable the address form save button when the form is invalid. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/05b0499d4be3
Only enable relevant fields in address forms and update tests. r=jaws
Verification steps:
* Verify the fixes mentioned in the user story field
* Note that comment 12 split some stuff to their own bugs
* Verify that all of the duplicates are also resolved: bug 1476284, bug 1476297, bug 1478332
Depends on: 1484721
I verified the following on the latest Firefox Nightly 63.0a1 (2018-08-27) on on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and Ubuntu 16.04 x64:
* Save/Add button not disabled initially on an empty form
* Don't forget to consider the contact/payer form
* Fields shouldn't be marked invalid immediately for an add form, only an edit form
* All of the duplicates are also resolved: bug 1476284, bug 1476297, bug 1478332

But, I'm still not sure about this point "Error message is in normal black text". Where should this error be displayed?
Flags: needinfo?(MattN+bmo)
We discussed the error text today and I showed Hani the change. It can’t be seen by real users anymore/yet.
Flags: needinfo?(MattN+bmo)
Verified as fixed on Firefox Nightly 63.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and Ubuntu 18.04 x64.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.