Closed Bug 1476204 Opened Last year Closed Last year

Credit card add/edit page error handling fixes

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments])

User Story

Problems to fix:
* Can save with a blank credit card
* No empty year option (inconsistent with month)
* Save/Add button not disabled initially on an empty form
* Error message is in normal black text
* The field validation should trigger a red border on the cc-number field upon change if invalid

Attachments

(5 files)

There are various improvements to make to the basic card add/edit page. See the user story.
Duplicate of this bug: 1476258
User Story: (updated)
Duplicate of this bug: 1476260
User Story: (updated)
Comment on attachment 8993821 [details]
Bug 1476204 - Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid.

https://reviewboard.mozilla.org/r/258498/#review265542


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/extensions/formautofill/content/autofillEditForms.js:25
(Diff revision 1)
>      for (let field of this._elements.form.elements) {
>        let value = record[field.id];
> -      field.value = typeof(value) == "undefined" ? "" : value;
> +      field.setAttribute("value", typeof(value) == "undefined" ? "" : value);
>      }
> +    // Reset the dirty value flag and validity state.
> +    //this._elements.form.reset();

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Comment on attachment 8993820 [details]
Bug 1476204 - Make the basic-card-option field order and alignment match the <option>.

https://reviewboard.mozilla.org/r/258496/#review265558

::: browser/components/payments/res/components/basic-card-option.css:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  basic-card-option {
> -  grid-row-gap: 5px;
> -  grid-column-gap: 10px;
> +  grid-column-gap: 1ch;
> +  grid-template-areas: "type cc-number cc-exp cc-name";

I'm not sure I understand the intention here. I read the commit message to mean the order of the values in the <option> should match the order of the fields - in the add/edit form. But those are number, name, expiry. Whereas this orders the values as number, expiry name. 

I'll call this a nit as its easy to address either way but color me confused :)
Attachment #8993820 - Flags: review?(sfoster) → review+
Comment on attachment 8993820 [details]
Bug 1476204 - Make the basic-card-option field order and alignment match the <option>.

https://reviewboard.mozilla.org/r/258496/#review265558

> I'm not sure I understand the intention here. I read the commit message to mean the order of the values in the <option> should match the order of the fields - in the add/edit form. But those are number, name, expiry. Whereas this orders the values as number, expiry name. 
> 
> I'll call this a nit as its easy to address either way but color me confused :)

The commit message is talking about <basic-card-option> (closed state) vs. <option> (open state).
Comment on attachment 8993821 [details]
Bug 1476204 - Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid.

https://reviewboard.mozilla.org/r/258498/#review265676
Attachment #8993821 - Flags: review?(jaws) → review+
Comment on attachment 8994103 [details]
Bug 1476204 - Replace duplicate #error-text with .page-error.

https://reviewboard.mozilla.org/r/258726/#review265680

::: browser/components/payments/res/paymentRequest.xhtml:131
(Diff revision 1)
>            <address-picker class="payer-related"
>                            label="&payerLabel;"
>                            data-add-link-label="&payer.addLink.label;"
>                            data-edit-link-label="&payer.editLink.label;"
>                            selected-state-key="selectedPayerAddress"></address-picker>
> -          <div id="error-text"></div>
> +          <div class="page-error" aria-live="polite"></div>

I didn't realize that we had two #error-text before, but now we would still have two .page-error. Our code just gets the first reference and sets/gets the textContent of that first reference.

We should either remove this second instance or update the code to set the textContent on each instance.
Attachment #8994103 - Flags: review-
Comment on attachment 8994103 [details]
Bug 1476204 - Replace duplicate #error-text with .page-error.

https://reviewboard.mozilla.org/r/258726/#review265878

::: browser/components/payments/res/paymentRequest.xhtml:131
(Diff revision 1)
>            <address-picker class="payer-related"
>                            label="&payerLabel;"
>                            data-add-link-label="&payer.addLink.label;"
>                            data-edit-link-label="&payer.editLink.label;"
>                            selected-state-key="selectedPayerAddress"></address-picker>
> -          <div id="error-text"></div>
> +          <div class="page-error" aria-live="polite"></div>

This one looks like its supposed to be the inline (field-specific) error? The spec shows the page error at the top of the page. If its for inline error messages it should probably get a different class. 

But if that's true then shouldn't we have containers for error messages associated with the other fields? 

So I'm not sure what's going on here.
Attachment #8994103 - Flags: review?(sfoster)
User Story: (updated)
Comment on attachment 8994103 [details]
Bug 1476204 - Replace duplicate #error-text with .page-error.

https://reviewboard.mozilla.org/r/258726/#review265680

> I didn't realize that we had two #error-text before, but now we would still have two .page-error. Our code just gets the first reference and sets/gets the textContent of that first reference.
> 
> We should either remove this second instance or update the code to set the textContent on each instance.

Oops, good catch, somehow I thought the one was on a different page. I split moving this error to a separate commit.
Comment on attachment 8994103 [details]
Bug 1476204 - Replace duplicate #error-text with .page-error.

https://reviewboard.mozilla.org/r/258726/#review265878

> This one looks like its supposed to be the inline (field-specific) error? The spec shows the page error at the top of the page. If its for inline error messages it should probably get a different class. 
> 
> But if that's true then shouldn't we have containers for error messages associated with the other fields? 
> 
> So I'm not sure what's going on here.

This wasn't a fields-specific one but I can see why you may think that. I've moved it to the header in a new commit.
I moved "Saving an invalid form reverts values to the original, losing changes." to bug 1476345 since that is complicated enough and needs more testing.
Comment on attachment 8994103 [details]
Bug 1476204 - Replace duplicate #error-text with .page-error.

https://reviewboard.mozilla.org/r/258726/#review266096
Attachment #8994103 - Flags: review?(jaws) → review+
After talking with Jacqueline on Friday, we agreed to do:
* Fields shouldn't be marked invalid immediately for an add form, only an edit form.

I will also do this in bug 1476345.
Comment on attachment 8994394 [details]
Bug 1476204 - Handle autofill record update state changes in the unpriv. PR forms.

https://reviewboard.mozilla.org/r/258962/#review266448

::: browser/components/payments/content/paymentDialogWrapper.js:582
(Diff revision 2)
> -      }
> +        }
> -    }
> +      }
> +
> -    let isTemporary = record.isTemporary;
> +      let isTemporary = record.isTemporary;
> -    let collection = isTemporary ? this.temporaryStore[collectionName] :
> +      let collection = isTemporary ? this.temporaryStore[collectionName] :
> -                                   formAutofillStorage[collectionName];
> +          formAutofillStorage[collectionName];

Four-space indent seems odd. Shouldn't this either be using two-space indent or have this align with the `this.temporaryStore` above it?

::: browser/components/payments/content/paymentDialogWrapper.js:609
(Diff revision 2)
> -      }
> -
> -      this.sendMessageToContent("updateState", successStateChange);
>      } catch (ex) {
> -      this.sendMessageToContent("updateState", errorStateChange);
> +      responseMessage.error = true;
> +      this.sendMessageToContent("updateAutofillRecord:Response", responseMessage);

This line is duplicated above at the end of the `try` block.

Can you move it out of the `catch` block? Or put it in a `finally` block?
Attachment #8994394 - Flags: review?(jaws) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/ed92000eca20
Make the basic-card-option field order and alignment match the <option>. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/f0f464e3c27c
Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid. r=jaws
https://hg.mozilla.org/integration/autoland/rev/001f13f2dd21
Replace duplicate #error-text with .page-error. r=jaws
https://hg.mozilla.org/integration/autoland/rev/a88879ea32e8
Implement the payment summary error bar. r=jaws
https://hg.mozilla.org/integration/autoland/rev/10f3d1014592
Handle autofill record update state changes in the unpriv. PR forms. r=jaws
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/438769e48bce
Backed out 5 changesets for browser chrome failures on browser_editCreditCardDialog. CLOSED TREE
Backed out for browser chrome failures on browser_editCreditCardDialog.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=10f3d10145925ae3048b859a7f0b6023f969fe89&tochange=438769e48bcee3d2ddfa57f0167342f50a4f4e18&selectedJob=190149325

Failures log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=190149325&repo=autoland&lineNumber=3820
https://treeherder.mozilla.org/logviewer.html#?job_id=190141520&repo=autoland&lineNumber=13504
https://treeherder.mozilla.org/logviewer.html#?job_id=190149289&repo=autoland&lineNumber=2748


01:08:45    ERROR -  1139 INFO TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js | Unable to find a rejection expected by expectUncaughtRejection. - 1 == 0 - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoMoreExpectedRejections :: line 273
01:08:45     INFO -  Stack trace:
01:08:45     INFO -  resource://testing-common/PromiseTestUtils.jsm:assertNoMoreExpectedRejections:273
01:08:45     INFO -  chrome://mochikit/content/browser-test.js:nextTest:746
01:08:45     INFO -  chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1397
01:08:45     INFO -  chrome://mochikit/content/browser-test.js:run:1334
01:08:45     INFO -  GECKO(5588) | MEMORY STAT | vsize 717MB | vsizeMaxContiguous 789MB | residentFast 248MB | heapAllocated 101MB
Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/d717ad76e616
Make the basic-card-option field order and alignment match the <option>. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/c63340378a52
Check Luhn algorithm in the basic-card-form and in storage and disable save button when invalid. r=jaws
https://hg.mozilla.org/integration/autoland/rev/9ad00680f9aa
Replace duplicate #error-text with .page-error. r=jaws
https://hg.mozilla.org/integration/autoland/rev/d57443c3e4ba
Implement the payment summary error bar. r=jaws
https://hg.mozilla.org/integration/autoland/rev/45cf26c3c53c
Handle autofill record update state changes in the unpriv. PR forms. r=jaws
QA Contact: hani.yacoub
Depends on: 1480023
Verified - Fixed on the latest Nightly 63.0a1 (2018-08-16) on Windows 7/10 x64, Ubuntu 16.04 and Mac OS 10.13.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.