Closed Bug 1440499 Opened 6 years ago Closed 6 years ago

Implement the payerName/payerEmail/payerPhone contact picker

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: MattN, Assigned: sfoster)

References

(
URL
)

Details

(Whiteboard: [webpayments])

Attachments

(3 files, 2 obsolete files)

UX Spec for Contact Information
227.33 KB, image/png
Details
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1440499 - Add debug options for the payer details and shipping requested.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
We need to handle requestPayerName/requestPayerEmail/requestPayerPhone by providing UI to pick a profile for these. We can re-use our address-picker but we should only show the relevant fields.
Blocks: 1440504
Comment on attachment 8954496 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker

https://reviewboard.mozilla.org/r/223564/#review229672

Here are comments of things to fix on my WIP patch.

::: toolkit/components/payments/res/components/address-option.css:13
(Diff revision 1)
>  rich-select[open] > .rich-select-popup-box > address-option {
>    grid-template-areas:
>      "name           name          "
>      "street-address street-address"
>      "email          tel           ";

I think `email` shouldn't be in the default layout since it's only part of the payer/contact picker, not shipping or billing address.

::: toolkit/components/payments/res/components/address-option.css:20
(Diff revision 1)
> +address-option[hide-address] {
> +  grid-template-areas:
> +    "name name"
> +    "tel  email";
> +}

The @hide-address approach was a hacky temporary method for a demo but I don't think it's scalable to handle the 4 boolean:
* requestShipping
* requestPayerName
* requestPayerEmail
* requestPayerPhone

Jared mentioned maybe we shouldn't use `grid-template-areas`. I wondered if we can have an optional @fields attribute with space-separated field categories (e.g. name, address, email, phone) that affect the visibility of each type of data. We can maybe use the `E[field~="email"]` selector[1].

[1] https://www.w3.org/TR/selectors-3/#attribute-selectors

::: toolkit/components/payments/res/containers/payment-dialog.js:215
(Diff revision 1)
> +    let payerRequested = paymentOptions.requestPayerName || paymentOptions.requestPayerEmail || paymentOptions.requestPayerPhone;
> +    for (let element of this._payerRelatedEls) {
> +      element.hidden = !payerRequested;
> +    }

This would need to be smarter and set separate attribute values on the picker so that we only show the requested types in the picker e.g. don't show the payer name if requestPayerName is false but requestPayerEmail is true.
Comment on attachment 8954496 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker

https://reviewboard.mozilla.org/r/223564/#review229678


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


::: toolkit/components/payments/res/containers/payment-dialog.js:215
(Diff revision 1)
>      this._errorText.textContent = paymentDetails.error;
>      let paymentOptions = request.paymentOptions;
>      for (let element of this._shippingRelatedEls) {
>        element.hidden = !paymentOptions.requestShipping;
>      }
> +    let payerRequested = paymentOptions.requestPayerName || paymentOptions.requestPayerEmail || paymentOptions.requestPayerPhone;

Error: Line 215 exceeds the maximum line length of 100. [eslint: max-len]
Assignee: MattN+bmo → sfoster
Matt: This is far enough along for an initial review. I've not squashed the commits - I hope that's not too confusing. So its your initial patch, a patch to add debugging options, then the functional changes. 

This seems to do what we want so its good checkpoint. I've not polished on the CSS - I can do more here, or leave this for another bug. Likewise I've not worked on the tests yet, but my intention is to do a couple simple mochitests that assert the expected elements show up un-hidden based on the paymentOption values we pass in.
Comment on attachment 8955344 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker.

https://reviewboard.mozilla.org/r/224516/#review230462


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/res/containers/payment-dialog.js:165
(Diff revision 1)
> +      this.requestStore.setState({
> +        selectedPayerAddress: Object.keys(savedAddresses)[0] || null,
> +      });
> +    }
> +
>    }

Error: Block must not be padded by blank lines. [eslint: padded-blocks]

::: toolkit/components/payments/res/containers/payment-dialog.js:215
(Diff revision 1)
>      this._errorText.textContent = paymentDetails.error;
>      let paymentOptions = request.paymentOptions;
>      for (let element of this._shippingRelatedEls) {
>        element.hidden = !paymentOptions.requestShipping;
>      }
> +    let payerRequested = paymentOptions.requestPayerName || paymentOptions.requestPayerEmail || paymentOptions.requestPayerPhone;

Error: Line 215 exceeds the maximum line length of 100. [eslint: max-len]
Comment on attachment 8955345 [details]
Bug 1440499 - Add debug options for the payer details and shipping requested.

https://reviewboard.mozilla.org/r/224518/#review230564

::: toolkit/components/payments/res/debugging.js:308
(Diff revision 1)
> +    let req = Object.assign(requestStore.getState().request, {
> +      paymentOptions: options,
> +    });

This is mutating the original request but it should be treated as immutable. You should clone the request  first (with .assign on {}) and the add the paymentOptions to the clone.
Attachment #8955345 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8955346 [details]
Bug 1440499 - Add payerName/Email/Phone contact picker.

https://reviewboard.mozilla.org/r/224520/#review230572

My original thinking was to not teach address-option about the "payer" concept in order to keep it as a generic "address option" so that's why I was saying there are 4 different kinds of fields (counting address as the fourth). Teaching it about the payer concept definitely makes implementation easier but I worry that it will be harder to maintain because we're introducing another mode. Please consider whether there is a way to avoid teaching address-option about the payer concept e.g. including "address" in the fieldNames.
Attachment #8955346 - Flags: review?(MattN+bmo) → review+
Attachment #8955346 - Attachment is obsolete: true
After saying that I could keep Matt's changes and mine in separate patches, when I checked again it would not leave patches that could really stand on their own. So I've folded those two together. I'm not sure how best to manager authorship here, particularly as it ends up with the author of part of the patch also being the reviewer. Let me know if I can change anything there.
Attachment #8954496 - Attachment is obsolete: true
Comment on attachment 8955345 [details]
Bug 1440499 - Add debug options for the payer details and shipping requested.

https://reviewboard.mozilla.org/r/224518/#review231540

::: toolkit/components/payments/res/debugging.html:23
(Diff revision 3)
> +        <label><input type="checkbox" autocomplete="off" name="requestPayerName" id="setRequestPayerName">requestPayerName</label>
> +        <label><input type="checkbox" autocomplete="off" name="requestPayerEmail" id="setRequestPayerEmail">requestPayerEmail</label>
> +        <label><input type="checkbox" autocomplete="off" name="requestPayerPhone" id="setRequestPayerPhone">requestPayerPhone</label>

Please add requestShipping as a checkbox and then we can probably change/remove the "Contact & No Shipping" button. As-is, if you check any of the three, requestShipping will be removed which is frustrating and confusing.
Comment on attachment 8955344 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker.

https://reviewboard.mozilla.org/r/224516/#review231542

::: toolkit/components/payments/res/components/address-option.css:42
(Diff revision 3)
> +/* Hide all the fields by default, and enable them explicitly for each format case */
>  address-option > .name,
>  address-option > .street-address,
>  address-option > .email,
>  address-option > .tel {
>    white-space: nowrap;
> +  display: none;
>  }
>  
> -.rich-select-selected-clone > .email,
> -.rich-select-selected-clone > .tel {
> -  display: none;
> +address-picker:not([address-fields]) address-option > .name,
> +address-picker:not([address-fields]) address-option > .street-address {
> +  display: inline-block;
> +}

This commit regresses the behaviour to show the telephone number in the dropdown when open but not in the selected option when closed and I believe that is still in the UX/UI spec: https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/280226161
Attachment #8955344 - Flags: review?(MattN+bmo)
(In reply to Sam Foster [:sfoster] from comment #16)
> After saying that I could keep Matt's changes and mine in separate patches,
> when I checked again it would not leave patches that could really stand on
> their own. So I've folded those two together. I'm not sure how best to
> manager authorship here, particularly as it ends up with the author of part
> of the patch also being the reviewer. Let me know if I can change anything
> there.

In the past I've updated the commit message to include "Original patch authored by MattN" for example, thus to give credit where it's due. It is OK to have MattN still be the reviewer, as it is assumed that you, the new author, implicitly reviewed his changes and agreed with them (where they were left untouched or at least not drastically changed).
Comment on attachment 8955344 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker.

https://reviewboard.mozilla.org/r/224516/#review231542

> This commit regresses the behaviour to show the telephone number in the dropdown when open but not in the selected option when closed and I believe that is still in the UX/UI spec: https://mozilla.invisionapp.com/share/SDFY4PA4EQ7#/screens/280226161

It sounds like this may still need final definition from UX. For now I think the new changes should restore the behavior where we show name, email, tel in the dropdown options, but only display the requested fields in the selected option when the dropdown is closed.
Comment on attachment 8955344 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker.

https://reviewboard.mozilla.org/r/224516/#review231884

::: toolkit/components/payments/res/components/address-option.css:21
(Diff revisions 5 - 6)
>    grid-template-areas:
> -    "name name"
> -    "tel  email";
> +    "name"
> +    "email"
> +    "tel";
>  }

Why did you change this now? It matches https://mozilla.invisionapp.com/share/YAFI31XR3KP#/screens/275361869

::: toolkit/components/payments/res/components/address-option.css:51
(Diff revisions 5 - 6)
> -address-picker.shipping-related .rich-select-selected-clone > .tel {
> +address-picker.shipping-related address-option > .tel {
>    display: none;
>  }

You only needed to change the email line, the `tel` line should go back to the previous version. The telephone number should show on the shipping picker when it's open.
Attachment #8955344 - Flags: review?(MattN+bmo)
Comment on attachment 8955344 [details]
Bug 1440499 - Implement the payerName/payerEmail/payerPhone contact picker.

https://reviewboard.mozilla.org/r/224516/#review232210

Thanks! Sorry about the back and forth
Attachment #8955344 - Flags: review?(MattN+bmo) → review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/479b143d8828
Add debug options for the payer details and shipping requested. r=MattN
https://hg.mozilla.org/integration/autoland/rev/4e2e081dad55
Implement the payerName/payerEmail/payerPhone contact picker. r=MattN
Backed out for ESlint failure on paymentDialogWrapper.js:53

backout: https://hg.mozilla.org/integration/autoland/rev/5fb1b1d9d6699eacb2d749edc095d0f5b2d3fe4f 

push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4e2e081dad557a8c060a3751bb1986c9686d5fe2&selectedJob=166878841

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166878841&repo=autoland&lineNumber=254

[task 2018-03-09T00:31:43.805Z] running build_ext
[task 2018-03-09T00:31:43.805Z] building 'psutil._psutil_linux' extension
[task 2018-03-09T00:31:43.805Z] creating build
[task 2018-03-09T00:31:43.805Z] creating build/temp.linux-x86_64-2.7
[task 2018-03-09T00:31:43.805Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-03-09T00:31:43.805Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-03-09T00:31:43.806Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-03-09T00:31:43.806Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-03-09T00:31:43.806Z] creating build/lib.linux-x86_64-2.7
[task 2018-03-09T00:31:43.806Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-03-09T00:31:43.807Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-03-09T00:31:43.807Z] building 'psutil._psutil_posix' extension
[task 2018-03-09T00:31:43.807Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-03-09T00:31:43.808Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-03-09T00:31:43.808Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-03-09T00:31:43.808Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-03-09T00:31:43.808Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-03-09T00:31:43.808Z] 
[task 2018-03-09T00:31:43.808Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-03-09T00:40:18.217Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/payments/content/paymentDialogWrapper.js:53:23 | 'profileStorage' is not defined. (no-undef)
[taskcluster 2018-03-09 00:40:19.102Z] === Task Finished ===
[taskcluster 2018-03-09 00:40:19.103Z] Unsuccessful task run with exit code: 1 completed in 738.268 seconds
Flags: needinfo?(sfoster)
Thanks for the back-out. The patches needed to be rebased on a more up-to-date central to pick up a change that renamed profileStorage to formAutofillStorage.
Flags: needinfo?(sfoster)
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1a65f74a21c
Add debug options for the payer details and shipping requested. r=MattN
https://hg.mozilla.org/integration/autoland/rev/f1965cf7425f
Implement the payerName/payerEmail/payerPhone contact picker. r=MattN
https://hg.mozilla.org/mozilla-central/rev/d1a65f74a21c
https://hg.mozilla.org/mozilla-central/rev/f1965cf7425f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: