1427960 - Add a Save the Address to Firefox toggle to the Add Address page

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

Allow the user to choose to not save the address into persistent Firefox storage.

Comment 1

[Matthew N. [:MattN]]

In a private window the toggle should default to being unchecked, otherwise it should default to being checked.

Comment 2

[Sam Foster [:sfoster] (he/him)]

In order to notify the merchant of a change of address, when the state change in the UI includes a change to selectedShippingAddress, we call up to paymentDialogWrapper.onChangeShippingAddress, which looks up the address record using the guid, and passes that record as the argument to the paymentservice's changeShippingAddress method. Having temporary addresses which aren't entered as records in the formAutofillStorage store means that lookup fails.  

Option 1: Send the serialized address up to the parent process for temporary addresses
Option 2: Store the temp address in the autofill store with a flag, and ensure it is deleted between sessions
Option 3: Create a new store for temporary data that is fully wiped in between sessions.

FWIW we will face the same issue with credit cards/payment methods when we add support for the paymentmethodchanged event. 

Option #1 seems least invasive but I suspect is doomed unless we fully emulate the behaviour of the formautofill store when creating transient "records" - as otherwise we don't have the same normalization of field values. Which suggests #2 or #3 as the better plan. Your thoughts Matt?

Comment 3

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] from comment #2)
> Option #1 seems least invasive but I suspect is doomed unless we fully
> emulate the behaviour of the formautofill store when creating transient
> "records" - as otherwise we don't have the same normalization of field
> values. Which suggests #2 or #3 as the better plan. Your thoughts Matt?

Hmm… I was thinking we would do option 1 but didn't think about the normalization behaviour. If that ends up being an issue maybe we could expose and call the normalize methods of from autofill storage and call them on our transient records? Are there specific normalization issues that you already identified that would affect our payment request behaviour? I would probably lean towards doing option 1 and dealing with specific normalization issues when we find them.

Comment 4

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3)
> (In reply to Sam Foster [:sfoster] from comment #2)
> > Option #1 seems least invasive but I suspect is doomed unless we fully
> > emulate the behaviour of the formautofill store when creating transient
> > "records" - as otherwise we don't have the same normalization of field
> > values. Which suggests #2 or #3 as the better plan. Your thoughts Matt?
> 
> Hmm… I was thinking we would do option 1 but didn't think about the
> normalization behaviour. If that ends up being an issue maybe we could
> expose and call the normalize methods of from autofill storage and call them
> on our transient records? Are there specific normalization issues that you
> already identified that would affect our payment request behaviour? I would
> probably lean towards doing option 1 and dealing with specific normalization
> issues when we find them.

Also note that paymentmethodchange won't require details about the cardholder name, or expiration AFAIK, only the card type and network at a maximum.

Comment 5

[Peter Saint-Andre [:stpeter]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #4)

> Also note that paymentmethodchange won't require details about the
> cardholder name, or expiration AFAIK, only the card type and network at a
> maximum.

That's my understanding as well. Perhaps :marcosc: can verify our understanding.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

* TODO: Add tests for the handling of temporary addresses in the add/edit and summary UI
* TODO: Add tests to verify temp addresses are temporary, selectable all places we use an address picker, and correctly populate shippingaddresschange event payloads as well as the payment response.
* TODO: Resolve question: Should the properties & params named like addressGUID be renamed using a addressOrGUID kind of convention (and use typeof string to infer which we got). Or use a convention like { $guid: 'asdasd' } when passing guids rather than data so they can be de-referenced/resolved consistently
* TODO: Clean up logging

* Modify methods expecting a guid for the address record to expect either a guid or address data for temporary addreses
* Add the persist checkbox to the add/edit address form. Defaults to unchecked when adding an address in a private session.
* The union of saved and temporary addresses can be retrieved from paymentRequest.getAddresses(). References to state.savedAddresses updated to use this when appropriate

Review commit: https://reviewboard.mozilla.org/r/241802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241802/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

https://reviewboard.mozilla.org/r/241802/#review247658


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


::: browser/components/payments/res/containers/payment-dialog.js:92
(Diff revision 1)
>    pay() {
>      let {
>        selectedPayerAddress,
>        selectedPaymentCard,
>        selectedPaymentCardSecurityCode,
> +      tempAddresses

Error: Missing trailing comma. [eslint: comma-dangle]

::: browser/components/payments/res/containers/payment-dialog.js:225
(Diff revision 1)
>      // if requestShipping is false.
>      if (state.request.paymentOptions.requestShipping) {
>        if (state.selectedShippingAddress != this._cachedState.selectedShippingAddress) {
> -        this.changeShippingAddress(state.selectedShippingAddress);
> +        let tempAddress = state.tempAddresses[state.selectedShippingAddress];
> +        // TODO: rename properties to reflect their guid-or-addressdata nature
> +        this.changeShippingAddress({shippingAddressGUID: tempAddress || state.selectedShippingAddress});

Error: Line 225 exceeds the maximum line length of 100. [eslint: max-len]

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

https://reviewboard.mozilla.org/r/241802/#review248080

::: commit-message-d07a4:5
(Diff revision 1)
> +1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses. r?MattN
> +
> +* TODO: Add tests for the handling of temporary addresses in the add/edit and summary UI
> +* TODO: Add tests to verify temp addresses are temporary, selectable all places we use an address picker, and correctly populate shippingaddresschange event payloads as well as the payment response.
> +* TODO: Resolve question: Should the properties & params named like addressGUID be renamed using a addressOrGUID kind of convention (and use typeof string to infer which we got). Or use a convention like { $guid: 'asdasd' } when passing guids rather than data so they can be de-referenced/resolved consistently

For addresses, not cards, maybe it makes sense to always pass the address data instead of the GUID since the unprivileged UI already has all the properties IIRC? For cards the decrypted # isn't in the unpriv. context.

::: browser/components/payments/content/paymentDialogWrapper.js:112
(Diff revision 1)
>        addressLines: addressData["street-address"].split("\n"),
>        region: addressData["address-level1"],
>        city: addressData["address-level2"],
>        postalCode: addressData["postal-code"],
>        organization: addressData.organization,
>        recipient: addressData.name,

I guess this patch is also not handling normalization of the 3 name fields into one yet either. We talked about exposing autofill normalization code to use for this.

::: browser/components/payments/content/paymentDialogWrapper.js:503
(Diff revision 1)
>    async onChangeShippingAddress({shippingAddressGUID}) {
>      let address = await this._convertProfileAddressToPaymentAddress(shippingAddressGUID);

The arguments here should change if this isn't always a GUID

::: browser/components/payments/res/containers/address-form.js:83
(Diff revision 1)
>    render(state) {
> +    let {
> +      page,
> +    } = state;
> +
> +    if (page && page.id !== this.id) {

Perhaps a stupid question: Where is `this.id` set?

::: browser/components/payments/res/containers/basic-card-form.js:88
(Diff revision 1)
> +    }
> +
> +

Nit: remove the extra new line

::: browser/components/payments/res/paymentRequest.xhtml:44
(Diff revision 1)
>    <!ENTITY basicCardPage.persistCheckbox.label     "Save credit card to Firefox (Security code will not be saved)">
>    <!ENTITY addressPage.error.genericSave      "There was an error saving the address.">
>    <!ENTITY addressPage.backButton.label       "Back">
>    <!ENTITY addressPage.saveButton.label       "Save">
> +  <!ENTITY addressPage.persistCheckbox.label  "Save address to Firefox">

I just realized we shouldn't be hard-coding the Firefox brand in the string. Can you file a bug to fix this once we switch to Fluent? I doubt we can use brand.dtd right now.

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241802/diff/1-2/

Comment 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

https://reviewboard.mozilla.org/r/241802/#review248080

> I guess this patch is also not handling normalization of the 3 name fields into one yet either. We talked about exposing autofill normalization code to use for this.

Not explicitly addressed in the current patch, but by moving the temporary records to the collections in paymentDialogWrapper, we now pass the address data through the `_convert*` and createPaymentAddress methods, where it that normalization could take place when necessary?

> Perhaps a stupid question: Where is `this.id` set?

this.id is assigned in the markup. I can pull this optimization out for now if its distracting, I was just getting confused when debugging render calls for inactive pages/sections. As we tend not to provide these ids in the mochitests, the new patch only returns when this.id is truthy.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241802/diff/2-3/

Comment 12

[Sam Foster [:sfoster] (he/him)]

The current page creates a temporaryStore.addresses and temporaryStore.creditCards property on the paymentDialogWrapper. The collection implementation has the same get/getAll/update/add API - for simplicity's sake. I've just thrown that into paymentDialogWrapper.js for now, but it could get broken out into its own file? 

I also ended up updating the temporary basicCards/creditCards implementation in this patch, to avoid tracking and testing 2 different approaches. 

Let me know if I should break up the patch at all to make it easier to review

Our tests pass currently without the address normalizations. So perhaps that could be filed as a follow-up?

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

https://reviewboard.mozilla.org/r/241802/#review250840

::: browser/components/payments/content/paymentDialogWrapper.js:205
(Diff revision 3)
> +    XPCOMUtils.defineLazyGetter(this, "log", () => {
> +      let {ConsoleAPI} = ChromeUtils.import("resource://gre/modules/Console.jsm", {});
> +      return new ConsoleAPI({
> +        maxLogLevelPref: "dom.payments.loglevel",
> +        prefix: "paymentDialogWrapper",
> +      });
> +    });

Please do a debug try push since last time I tried to add logging here there was a leak I didn't understand so I ended up removing the logging…

::: browser/components/payments/content/paymentDialogWrapper.js:556
(Diff revision 3)
> +    let isTemporary = record.isTemporary ||
> +                      !!this.temporaryStore[collectionName].get(guid);

Hmm… do we need the 2nd check or is that just in case we mess up somewhere? This is a place where a JS assertion would be great if we had them still…

::: browser/components/payments/res/paymentRequest.css:50
(Diff revision 3)
> +#payment-summary > h1 {
> +  margin: 0 0 0.5em;
> +  font-size: large;
> +}

Nit: not sure what this is solving and I have no idea if this is moving us in the direction of the UI spec or not… I would rather not change this if it's not going to be useful when we do final styling.

::: browser/components/payments/test/PaymentTestUtils.jsm:380
(Diff revision 3)
> +    Temp: {
> +      "given-name": "Temperance",

Nit: maybe add a comment that this profile should only be used as a temporary address since some people may think Temp is just short for Temperance and not also temporary

::: browser/components/payments/test/browser/browser_address_edit.js:401
(Diff revision 3)
> +        PaymentTestUtils: PTU,
> +      } = ChromeUtils.import("resource://testing-common/PaymentTestUtils.jsm", {});
> +
> +      info("adding link");
> +      // click through to add/edit address page
> +      let addLink = content.document.querySelector("address-picker a");

Please use `.add-link` (already in m-c) instead of `a` nowadays. I'm fixing this for existing code in the patch you reviewed today.

Comment 14

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

https://reviewboard.mozilla.org/r/241802/#review250840

> Please do a debug try push since last time I tried to add logging here there was a leak I didn't understand so I ended up removing the logging…

I guess I was just using that during development. There's no logging left in the patch so I'll just drop it.

> Hmm… do we need the 2nd check or is that just in case we mess up somewhere? This is a place where a JS assertion would be great if we had them still…

I don't think that second check is necessary, if we say the form (front-end) is the source of truth for temporariness. There's no scenario that I can think of currently where those 2 could get out of sync, as the record and temporaryStore are both alive for the lifetime of the dialog (PR). I'll ditch it (and check that doesnt break anything)

> Nit: not sure what this is solving and I have no idea if this is moving us in the direction of the UI spec or not… I would rather not change this if it's not going to be useful when we do final styling.

Agreed, that was just making interacting with the UI a little less painful while I was working on this, and is orthogonal to this bug.

> Nit: maybe add a comment that this profile should only be used as a temporary address since some people may think Temp is just short for Temperance and not also temporary

I just needed a 3rd address. Depending on how a test is written it may or may not be temporary of course, but adding it to PTU at least makes it available for re-use in others tests using the boilerplate from this one.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241802/diff/3-4/

Comment 16

[Mozilla Autoland]

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 caea3271ef6fc720833fe4ca0b024b1cacd3b79b -d d52c1a18a259: rebasing 464260:caea3271ef6f "Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses. r=MattN" (tip)
merging browser/components/payments/res/containers/address-form.js
merging browser/components/payments/res/containers/basic-card-form.js
merging browser/components/payments/res/paymentRequest.js
merging browser/components/payments/res/paymentRequest.xhtml
warning: conflicts while merging browser/components/payments/res/containers/address-form.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/payments/res/containers/basic-card-form.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8973321 [details]
Bug 1427960 - Add temporary addresses to state, add toggle to enable saving/not new addresses.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241802/diff/4-5/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cb17cd65c9c
Add temporary addresses to state, add toggle to enable saving/not new addresses. r=MattN

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8cb17cd65c9c