1432952 - Add the ability to associate a saved billing address with a payment card in the add/edit card form

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

An associated billing address is sometimes needed for credit card purchases and ideally a user shouldn't have to re-choose the billing address for a given payment card since it doesn't change often. We should have the user optionally associate a billing address with their payment card from the add/edit payment card forms and save this choice in the payment card storage for future transactions.

For now autofill won't use this information as addresses and credit cards can still be filled separately but the relationship will make the PaymentRequest flow smoother.

I think we should handle this as a nullable one-to-one foreign key relationship from the payment card storage to the existing saved address using the GUID of the address as the key. We shouldn't enforce the constraint that the address GUID exists in storage since the billing address is optional anyways. Perhaps at query time, a postprocessor could return null if the linked address isn't found. It's possible that it will re-appear later if that address later syncs via the address Sync engine. All UI using the billing address needs to handle the billing address being null.

Comment 1

[Matthew N. [:MattN]]

The advantage of referring to the other address (normalizing the data) is that if you move, changing one address changes both your billing and shipping address which may be what you want. It also means we don't need to duplicate some logic between payment card and plain address management UI.

The disadvantage of this approach is that a user may change an address, not realizing it will also change their billing address too. Perhaps this means we should show a separate address form (probably with a way to copy an existing saved address) or we should show a warning on the edit address screen if an address is associated with a credit card.

We would also need to handle updates to the address via the autofill update doorhanger and warning there may be more confusing.

Comment 2

[Matthew N. [:MattN]]

I talked with Jacqueline and Eric today and they agree that normalizing the data is best.

Comment 3

[Matthew N. [:MattN]]

Attachment: Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

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

Comment 4

[Matthew N. [:MattN]]

Attachment: Bug 1432952 - Add a billing address picker to the credit card add/edit form.

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8966360 [details]
Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

https://reviewboard.mozilla.org/r/235048/#review241124

::: browser/extensions/formautofill/FormAutofillStorage.jsm:1658
(Diff revision 1)
>  
>      delete creditCard["cc-exp"];
>    }
>  
>    /**
> -   * Normailze the given record and retrun the first matched guid if storage has the same record.
> +   * Normailze the given record and return the first matched guid if storage has the same record.

Normalize (sp)

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8966360 [details]
Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

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

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8966361 [details]
Bug 1432952 - Add a billing address picker to the credit card add/edit form.

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

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8966360 [details]
Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

https://reviewboard.mozilla.org/r/235048/#review241188

Makes sense to me! So, since the field is optional:

* Old clients will round-trip it. We don't need to worry about merge conflicts, since clients can't change fields they don't understand.
* If the address doesn't exist in storage, that's fine; as you said in comment 0, it's optional. If the address record shows up eventually, it'll be the default the next time you make a purchase. If not, the user will need to select one manually. (For that reason, we also don't need to reupload the credit card when we delete the address...its `billingAddressGUID` will just point to a nonexistent address until the user chooses a new default). Did I get that right?
* If there's a merge conflict for the address, and we fork, the modified record with the original GUID sticks around. Clearing the reference *might* make sense here (you changed the address on two different devices to two different values, which one do you want to use for the card?), but we probably don't need to worry about it. Those kinds of changes are unlikely, and, since the billing address is a hint, anyway, the user can manually pick the forked address.

Comment 9

[Matthew N. [:MattN]]

Attachment: Bug 1432952 - Use the testDialog helper in browser_editCreditCardDialog.js.

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

Comment 10

[Matthew N. [:MattN]]

Attachment: Bug 1432952 - Send the basic-card's billing address with the response.

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

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8966360 [details]
Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

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

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8966361 [details]
Bug 1432952 - Add a billing address picker to the credit card add/edit form.

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8966360 [details]
Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

https://reviewboard.mozilla.org/r/235048/#review241538

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8966799 [details]
Bug 1432952 - Use the testDialog helper in browser_editCreditCardDialog.js.

https://reviewboard.mozilla.org/r/235478/#review241540

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8966361 [details]
Bug 1432952 - Add a billing address picker to the credit card add/edit form.

https://reviewboard.mozilla.org/r/235050/#review241542

::: browser/extensions/formautofill/content/autofillEditForms.js:236
(Diff revision 3)
> +    this._elements.billingAddress.appendChild(new Option("", ""));
> +
> +    for (let [guid, address] of Object.entries(this._addresses)) {

I think we should hide the billing address dropdown and label if this._addresses is empty.

::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:115
(Diff revision 3)
> +    if (fieldName === "cc-number") {
> +      fieldValue = "*".repeat(fieldValue.length - 4) + fieldValue.substr(-4);
> +    }
> +    is(creditCards[1][fieldName], fieldValue, "check " + fieldName);
> +  }
> +  ok(creditCards[1].billingAddressGUID, "billingAddressGUID is truthy");

You can compare creditCards[1] to billingAddress.guid

::: browser/extensions/formautofill/test/browser/browser_editCreditCardDialog.js:152
(Diff revision 3)
> +    billingAddressGUID: "unknown-guid",
> +  });
> +  await saveCreditCard(TEST_CREDIT_CARD);
> +
> +  let creditCards = await getCreditCards();
> +  is(creditCards.length, 1, "two credit cards in storage");

the message here doesn't match your assertion

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8966800 [details]
Bug 1432952 - Send the basic-card's billing address with the response.

https://reviewboard.mozilla.org/r/235480/#review241568

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8966361 [details]
Bug 1432952 - Add a billing address picker to the credit card add/edit form.

https://reviewboard.mozilla.org/r/235050/#review241542

> I think we should hide the billing address dropdown and label if this._addresses is empty.

Good idea, it looked a bit weird otherwise but didn't want to block on UX figuring out a placeholder <option>. I think hiding it makes sense for prefs and then we can unhide it in the PR dialog if we end up providing an add/edit button with it.

> You can compare creditCards[1] to billingAddress.guid

That's already done in the for loop 2 lines above. This is just adding extra certainty that we're not comparing some falsy value to another falsy value there.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8966360 [details]
Bug 1432952 - Add a billingAddressGUID property to saved credit cards.

https://reviewboard.mozilla.org/r/235048/#review241188

> Old clients will round-trip it. We don't need to worry about merge conflicts, since clients can't change fields they don't understand.

Yeah, that's the idea but I realized while reading your comment that the tests for new properties in test_reconcile.js and test_creditCardRecords.js aren't testing with fields that werent' considered `VALID_FIELDS` in the old clients. While adding some unit tests for that I realized that this isn't handled properly so I filed bug 1453542. Since we don't currently ship CC autofill I think we can ignore this problem for now.

> If the address doesn't exist in storage, that's fine; as you said in comment 0, it's optional. If the address record shows up eventually, it'll be the default the next time you make a purchase. If not, the user will need to select one manually. (For that reason, we also don't need to reupload the credit card when we delete the address...its billingAddressGUID will just point to a nonexistent address until the user chooses a new default). Did I get that right?

Correct.

> If there's a merge conflict for the address, and we fork, the modified record with the original GUID sticks around. Clearing the reference might make sense here (you changed the address on two different devices to two different values, which one do you want to use for the card?), but we probably don't need to worry about it. Those kinds of changes are unlikely, and, since the billing address is a hint, anyway, the user can manually pick the forked address.

Yeah, I don't think we can be any smarter about this case as we don't know the user's intention with the changes.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8966361 [details]
Bug 1432952 - Add a billing address picker to the credit card add/edit form.

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

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8966800 [details]
Bug 1432952 - Send the basic-card's billing address with the response.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2af67006060b
Add a billingAddressGUID property to saved credit cards. r=jaws,kitcambridge
https://hg.mozilla.org/integration/autoland/rev/0980a649d1d0
Use the testDialog helper in browser_editCreditCardDialog.js. r=jaws
https://hg.mozilla.org/integration/autoland/rev/a6871a7a0aaa
Add a billing address picker to the credit card add/edit form. r=jaws
https://hg.mozilla.org/integration/autoland/rev/83c1d17f2d85
Send the basic-card's billing address with the response. r=jaws

Comment 22

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2af67006060b
https://hg.mozilla.org/mozilla-central/rev/0980a649d1d0
https://hg.mozilla.org/mozilla-central/rev/a6871a7a0aaa
https://hg.mozilla.org/mozilla-central/rev/83c1d17f2d85