Closed Bug 1427959 Opened 7 years ago Closed 6 years ago

Show field-specific shippingaddresschange errors on the add/edit screen

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: MattN, Assigned: jaws)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [webpayments])

Attachments

(3 files)

Bug 1427959 - Show field-specific shippingaddresschange errors on the add/edit screen.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1427959 - Clean up some tests WebPayments UI tests.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1427959 - Add some basic styling for the error text on the address-form.
59 bytes, text/x-review-board-request
MattN
: review+
Details
If the merchant provides field-specific shipping address errors, show them to users. See https://github.com/w3c/payment-request/issues/647 Bug 1427958 will handle the generic error string.
See Also: → 1429189
Depends on: 1435157
Priority: P3 → P2
Whiteboard: [webpayments]
Product: Toolkit → Firefox
I think we can implement this for now assuming a structure like https://github.com/w3c/payment-request/issues/647#issuecomment-385852164 is included on nsIPaymentReqest. It shouldn't be hard to change the mapping from the DOM to specific fields if this changes and the larger part of this bug will be rendering the errors.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8975569 [details] Bug 1427959 - Show field-specific shippingaddresschange errors on the add/edit screen. https://reviewboard.mozilla.org/r/243816/#review250080 ::: browser/components/payments/res/containers/address-form.js:120 (Diff revision 3) > + let span = container.querySelector(".error-text"); > + if (!span) { > + span = document.createElement("span"); > + span.className = "error-text"; > + container.appendChild(span); > + } This bug should handle basic styling of the errors too… we can't leave all styling until later… maybe in a next commit. ::: browser/components/payments/test/browser/browser_shippingaddresschange_error.js:152 (Diff revision 3) > + // Note, todo() in ContentTask isn't properly getting reported. > + todo(false, > + "shippingAddressErrors should exist on the paymentDetails object: " + ex.message); > + state = await PTU.DialogContentUtils.waitForState(content, (state) => true, "Get state"); > + } > + > + // Hack due to shippingAddressErrors not supported by platform yet. Please include the existing DOM bug number in the comment and TODO message.
Comment on attachment 8975569 [details] Bug 1427959 - Show field-specific shippingaddresschange errors on the add/edit screen. https://reviewboard.mozilla.org/r/243816/#review250862 Clearing review since I think you can rebase on top of the DOM patch now to remove some of the temporary stuff in the tests.
Attachment #8975569 - Flags: review?(MattN+bmo)
Comment on attachment 8980068 [details] Bug 1427959 - Add some basic styling for the error text on the address-form. https://reviewboard.mozilla.org/r/246230/#review252364
Attachment #8980068 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8975569 [details] Bug 1427959 - Show field-specific shippingaddresschange errors on the add/edit screen. https://reviewboard.mozilla.org/r/243816/#review252366
Attachment #8975569 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8980067 [details] Bug 1427959 - Clean up some tests WebPayments UI tests. https://reviewboard.mozilla.org/r/246228/#review252368 Preliminary comments… ::: browser/components/payments/test/PaymentTestUtils.jsm (Diff revision 1) > - isElementVisible: selector => { > - let element = content.document.querySelector(selector); > - return element.getBoundingClientRect().height > 0; > - }, > - > - getElementTextContent: selector => { > - let doc = content.document; > - let element = doc.querySelector(selector); > - return element.textContent; > - }, Personally I think they were useful and would like more helpers to reduce boilerplate. ::: browser/components/payments/test/PaymentTestUtils.jsm:313 (Diff revision 1) > + UpdateWith: { > + twoShippingOptions: { > + error: "", > + shippingOptions: [ > + { > + id: "1", > + label: "Most Unperfect Shipping", For the future I think it would be useful to reduce the amount of unrelated changes we make in our tests e.g. total changes when we only want to set the error. We could maybe break them down into separate pieces and compose them together with object literals or `Object.assign` and then maybe have functions that take arguments to fill in specific values. The helpers could also inherit values from the existing `PaymentRequest`. Just something to think of for the future. ::: browser/components/payments/test/browser/browser_change_shipping.js:29 (Diff revision 1) > await spawnPaymentDialogTask(frame, PTU.DialogContentTasks.getShippingOptions); > is(shippingOptions.selectedOptionCurrency, "USD", "Shipping options should be in USD"); > is(shippingOptions.optionCount, 2, "there should be two shipping options"); > is(shippingOptions.selectedOptionID, "2", "default selected should be '2'"); > > - await spawnPaymentDialogTask(frame, > + spawnPaymentDialogTask(frame, PTU.DialogContentTasks.selectShippingOptionById, "1"); What's the reasoning for removing `await`? Not using the await with a content task could cause test failures in certain cases where the content process ends up sending a message to the parent and then the praent assumes that was already received. Using `await` in that case wouldn't fail. Basically if `await` is sometimes needed and sometimes not I'd rather err on the side of always using it (other than the dialog closing case).
Comment on attachment 8980067 [details] Bug 1427959 - Clean up some tests WebPayments UI tests. https://reviewboard.mozilla.org/r/246228/#review252368 > Personally I think they were useful and would like more helpers to reduce boilerplate. Each place they were used were just outside of spawnPaymentDialogTask functions, and thus we were exiting the paymentDialogTask, then executing a standalone paymentDialogTask, and often entering right back in to a paymentDialogTask. After merging the usages of this in to the adjacent paymentDialogTasks there were no other uses of these two functions so I removed them. If there were places where we used them and there were no adjacent paymentDialogTasks I would have left them but from looking at the usages they were only used in unproductive ways. > For the future I think it would be useful to reduce the amount of unrelated changes we make in our tests e.g. total changes when we only want to set the error. We could maybe break them down into separate pieces and compose them together with object literals or `Object.assign` and then maybe have functions that take arguments to fill in specific values. The helpers could also inherit values from the existing `PaymentRequest`. Just something to think of for the future. I originally tried to set up this object by reusing the other PTU.Details definitions but it is not possible to reference PaymentTestUtils from within the definition of it. We could add these properties to PaymentTestUtils after definition but I thought that could be confusing. > What's the reasoning for removing `await`? Not using the await with a content task could cause test failures in certain cases where the content process ends up sending a message to the parent and then the praent assumes that was already received. Using `await` in that case wouldn't fail. Basically if `await` is sometimes needed and sometimes not I'd rather err on the side of always using it (other than the dialog closing case). Some places we weren't using await and other places we were. In the instances where we didn't expect a return value I was able to remove the `await` and the tests passed fine on my machine. Though I see now that some of the tests failed on tryserver for non-Windows platforms and this is likely the cause. I will revert this change and add the `await` to the places where it was missing.
Comment on attachment 8980067 [details] Bug 1427959 - Clean up some tests WebPayments UI tests. https://reviewboard.mozilla.org/r/246228/#review252444 ::: browser/components/payments/test/browser/head.js:221 (Diff revision 2) > + content.isHidden = (element) => elementHeight(element) == 0; > + content.isVisible = (element) => elementHeight(element) > 0; So this is similar to the removed DialogContentTasks.isElementVisible function but since all places that wanted to called the old function were adjacent to paymentDialogTasks already it made more sense to me to have a helper like this available within the paymentDialogTask. These two functions keep the code concise and don't force us to have getBoundingClientRect() calls littered through the code. If we decide we want to change how we determine if an element is hidden or not we only have one/two places to update.
Comment on attachment 8980067 [details] Bug 1427959 - Clean up some tests WebPayments UI tests. https://reviewboard.mozilla.org/r/246228/#review252444 > So this is similar to the removed DialogContentTasks.isElementVisible function but since all places that wanted to called the old function were adjacent to paymentDialogTasks already it made more sense to me to have a helper like this available within the paymentDialogTask. > > These two functions keep the code concise and don't force us to have getBoundingClientRect() calls littered through the code. If we decide we want to change how we determine if an element is hidden or not we only have one/two places to update. Great, I like this better!
Comment on attachment 8980067 [details] Bug 1427959 - Clean up some tests WebPayments UI tests. https://reviewboard.mozilla.org/r/246228/#review252702 Thanks for the cleanups ::: browser/components/payments/test/browser/browser_card_edit.js:132 (Diff revision 4) > }, "Check card was not added again"); > > let cardGUIDs = Object.keys(state.savedBasicCards); > is(cardGUIDs.length, 1, "Check there is one card"); > let savedCard = state.savedBasicCards[cardGUIDs[0]]; > - card["cc-number"] = "************1111"; // Card should be masked > + card["cc-number"] = "************9999"; // Card should be masked Just an FYI that I used a valid card number (4111…) which passes the Luhn algorithm to avoid issues in the future when we add validation so we will want to change the test cards to be valid eventually. rs=me if you want to fix that in this bug too.
Attachment #8980067 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #25) > Just an FYI that I used a valid card number (4111…) which passes the Luhn > algorithm to avoid issues in the future when we add validation so we will > want to change the test cards to be valid eventually. rs=me if you want to > fix that in this bug too. Okay, fixed now.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 288b006505967d7a5ed21144c2574d4f43e6ae57 -d 755e1ebfde37: rebasing 465044:288b00650596 "Bug 1427959 - Clean up some tests WebPayments UI tests. r=MattN" merging browser/components/payments/test/browser/browser_payments_onboarding_wizard.js warning: conflicts while merging browser/components/payments/test/browser/browser_payments_onboarding_wizard.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5e0368784d17 Clean up some tests WebPayments UI tests. r=MattN https://hg.mozilla.org/integration/autoland/rev/6003ece89387 Show field-specific shippingaddresschange errors on the add/edit screen. r=MattN https://hg.mozilla.org/integration/autoland/rev/afaba93c711b Add some basic styling for the error text on the address-form. r=MattN
The commits from comment 34 were backed out in https://hg.mozilla.org/integration/autoland/rev/4c47479352e2453b2a86db47911bfea6c4a84eaa due to an intermittent failure in browser_payments_onboarding_wizard.js. I have fixed the issue and will re-push.
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/248aac6c4bd6 Clean up some tests WebPayments UI tests. r=MattN https://hg.mozilla.org/integration/autoland/rev/da6e1185a444 Show field-specific shippingaddresschange errors on the add/edit screen. r=MattN https://hg.mozilla.org/integration/autoland/rev/d1d9c187aa7b Add some basic styling for the error text on the address-form. r=MattN
Backed out 3 changesets (bug 1427959) for failing browser-chrome tests on browser/components/payments/test/browser/browser_payments_onboarding_wizard.js Backout revision https://hg.mozilla.org/integration/autoland/rev/4c47479352e2453b2a86db47911bfea6c4a84eaa Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=afaba93c711b8dcfaf4fa8cc0813ff73838816b9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-classifiedState=unclassified Link to the failure loghttps://treeherder.mozilla.org/logviewer.html#?job_id=180296664&repo=autoland&lineNumber=7010 Part of that log:[task 2018-05-25T18:27:19.654Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Got payment widget - [task 2018-05-25T18:27:19.654Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | requestId should be defined - [task 2018-05-25T18:27:19.654Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | dialog should not be closed - [task 2018-05-25T18:27:19.655Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Got payment frame - [task 2018-05-25T18:27:19.656Z] 18:27:19 INFO - dialog ready [task 2018-05-25T18:27:19.656Z] 18:27:19 INFO - helper functions injected into frame [task 2018-05-25T18:27:19.657Z] 18:27:19 INFO - Checking if the basic card has been rendered [task 2018-05-25T18:27:19.659Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Basic card page is rendered - true == true - [task 2018-05-25T18:27:19.659Z] 18:27:19 INFO - Check if the total header is visible on the basic card page during on-boarding [task 2018-05-25T18:27:19.661Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Total Header is visible on the basic card page during on-boarding - true == true - [task 2018-05-25T18:27:19.661Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Total Header contains text - "\n \n \n $60.00\n example.com\n \n \n View All Items\n \n " == true - [task 2018-05-25T18:27:19.661Z] 18:27:19 INFO - TEST-PASS | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Cancel button is visible on the basic card page - true == true - [task 2018-05-25T18:27:19.662Z] 18:27:19 INFO - Buffered messages finished [task 2018-05-25T18:27:19.663Z] 18:27:19 INFO - TEST-UNEXPECTED-FAIL | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Test timed out - [task 2018-05-25T18:27:19.669Z] 18:27:19 INFO - GECKO(2048) | JavaScript error: chrome://payments/content/paymentDialogWrapper.js, line 335: NS_ERROR_NOT_INITIALIZED: [task 2018-05-25T18:27:19.670Z] 18:27:19 INFO - GECKO(2048) | JavaScript error: chrome://payments/content/paymentDialogWrapper.js, line 335: NS_ERROR_NOT_INITIALIZED: [task 2018-05-25T18:27:19.671Z] 18:27:19 INFO - Console message: [JavaScript Error: "NS_ERROR_NOT_INITIALIZED: " {file: "chrome://payments/content/paymentDialogWrapper.js" line: 335}] [task 2018-05-25T18:27:19.672Z] 18:27:19 INFO - Console message: [JavaScript Error: "NS_ERROR_NOT_INITIALIZED: " {file: "chrome://payments/content/paymentDialogWrapper.js" line: 335}] [task 2018-05-25T18:27:19.673Z] 18:27:19 INFO - GECKO(2048) | MEMORY STAT | vsize 933MB | residentFast 255MB | heapAllocated 73MB [task 2018-05-25T18:27:19.674Z] 18:27:19 INFO - TEST-OK | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | took 90093ms [task 2018-05-25T18:27:19.676Z] 18:27:19 INFO - Not taking screenshot here: see the one that was previously logged [task 2018-05-25T18:27:19.680Z] 18:27:19 INFO - TEST-UNEXPECTED-FAIL | browser/components/payments/test/browser/browser_payments_onboarding_wizard.js | Found a tab after previous test timed out: https://example.com/browser/browser/components/payments/test/browser/blank_page.html - [task 2018-05-25T18:27:19.681Z] 18:27:19 INFO - GECKO(2048) | ++DOCSHELL 0xe22b8800 == 1 [pid = 2135] [id = {17c24b24-74fd-482f-b4ce-75249772a35d}] [task 2018-05-25T18:27:19.683Z] 18:27:19 INFO - GECKO(2048) | ++DOMWINDOW == 1 (0xe36fb080) [pid = 2135] [serial = 57] [outer = (nil)] [task 2018-05-25T18:27:19.684Z] 18:27:19 INFO - checking window state [task 2018-05-25T18:27:19.686Z] 18:27:19 INFO - GECKO(2048) | ++DOMWINDOW == 2 (0xe424ba00) [pid = 2135] [serial = 58] [outer = 0xe36fb080]
https://hg.mozilla.org/mozilla-central/rev/248aac6c4bd6 https://hg.mozilla.org/mozilla-central/rev/da6e1185a444 https://hg.mozilla.org/mozilla-central/rev/d1d9c187aa7b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: