Closed
Bug 1429189
Opened 6 years ago
Closed 6 years ago
Show shipping address errors on the summary screen
Categories
(Firefox :: WebPayments UI, enhancement, P1)
Firefox
WebPayments UI
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: MattN, Assigned: jaws)
References
()
Details
(Whiteboard: [webpayments])
Attachments
(1 file)
If an error with the shipping address happens and it's isn't a result of a shippingaddresschange event (from the user editing it) then we need to indicate the error on the summary view (rather than preventing the user from leaving the edit screen). Example: 1) Have only a partial/invalid shipping address in autofill storage 2) Open a Payment Request 3) Address from #1 is chosen by default 4) Don't change the shipping address but complete the request I don't believe shippingaddresschange should be fired in the above steps but upon payment time the site will get the shipping address and may reject it.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•6 years ago
|
Priority: P3 → P1
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8953281 [details] Bug 1429189 - Show shipping address errors on the summary screen. https://reviewboard.mozilla.org/r/222562/#review228452 Code analysis found 2 defects in this patch: - 2 defects 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 ::: toolkit/components/payments/test/PaymentTestUtils.jsm:42 (Diff revision 1) > }, > > + updateWith: async ({eventName, details}) => { > + if (details.error && > + (!details.shippingOptions || details.shippingOptions.length)) { > + ok(false, "Need to clear the shipping options to show error text"); Error: 'ok' is not defined. [eslint: no-undef] ::: toolkit/components/payments/test/PaymentTestUtils.jsm:45 (Diff revision 1) > + if (details.error && > + (!details.shippingOptions || details.shippingOptions.length)) { > + ok(false, "Need to clear the shipping options to show error text"); > + } > + if (!details.total) { > + ok(false, "`total: { label, amount: { value, currency } }` is required for updateWith"); Error: 'ok' is not defined. [eslint: no-undef]
Comment hidden (mozreview-request) |
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8953281 [details] Bug 1429189 - Show shipping address errors on the summary screen. https://reviewboard.mozilla.org/r/222562/#review228506 ::: toolkit/components/payments/content/paymentDialogWrapper.js:253 (Diff revision 2) > data, > messageType, > }); > }, > > + onDetailsUpdated() { Nit: Since this isn't a listener and isn't just about the PaymentRequest `details`, I think `updateRequest` may be a clearer name. ::: toolkit/components/payments/content/paymentDialogWrapper.js:253 (Diff revision 2) > + onDetailsUpdated() { > + let requestSerialized = this._serializeRequest(this.request); I am surprised this works without doing ```js this.request = paymentSrv.getPaymentRequestById(requestId); ``` Please double-check this. If the request is live we should probably have a comment about that and you mentioned we could make it a `const` ::: toolkit/components/payments/paymentUIService.js:85 (Diff revision 2) > + if (!dialog) { > + this.log.debug("updatePayment: no dialog found"); > + return; > + } Not sure if we should bother trying to mask this potential error as it may hide bugs. At a minimum this should be a log.warn or probably better as log.error so it logs by default as it will help with bug reports. ::: toolkit/components/payments/paymentUIService.js:100 (Diff revision 2) > + if (win) { > + this.log.debug(`closing: ${win.name}`); > + win.close(); > + return true; > + } > + return false; Nit: ```js if (!win) { return false; } … ``` ::: toolkit/components/payments/res/paymentRequest.xhtml:74 (Diff revision 2) > + <div><label id="error-text"></label></div> > </section> I think this is better at the top of the page or in the footer ::: toolkit/components/payments/res/paymentRequest.xhtml:74 (Diff revision 2) > <address-picker class="shippingRequested" selected-state-key="selectedShippingAddress"></address-picker> > <div class="shippingRequested"><label>&shippingOptionsLabel;</label></div> > <shipping-option-picker class="shippingRequested"></shipping-option-picker> > <div><label>&paymentMethodsLabel;</label></div> > <payment-method-picker selected-state-key="selectedPaymentCard"></payment-method-picker> > + <div><label id="error-text"></label></div> Why use a label if it's not labelling a control? And why not set the text content of the div directly without a wrapper? ::: toolkit/components/payments/test/browser/browser_shippingaddresschange_error.js:55 (Diff revision 2) > + await ContentTask.spawn(browser, { > + eventName: "shippingoptionchange", > + }, PTU.ContentTasks.awaitPaymentRequestEventPromise); > + > + let errors = await spawnPaymentDialogTask(frame, PTU.DialogContentTasks.getErrorDetails); > + is(errors.text, EXPECTED_ERROR_TEXT, "Error text should be present on dialog"); Can you check that it's visible?
Attachment #8953281 -
Flags: review?(MattN+bmo) → review+
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b281723b73dd Show shipping address errors on the summary screen. r=mattn
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b281723b73dd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
Whiteboard: [webpayments]
Updated•6 years ago
|
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
You need to log in
before you can comment on or make changes to this bug.
Description
•