Closed Bug 1463608 Opened 6 years ago Closed 6 years ago

Required `name` property is missing on temporary PR addresses

Categories

(Firefox :: WebPayments UI, defect, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: MattN, Assigned: sfoster)

References

Details

(Whiteboard: [webpayments])

Attachments

(3 files)

state dump
2.65 KB, application/json
Details
Bug 1463608 - Expose FormAutofill computeFields methods, use them for temporary records in payments.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1463608 - Add test helpers for payment address and cards.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
(In reply to Sam Foster [:sfoster] from bug 1427960 comment #12)
> Our tests pass currently without the address normalizations. So perhaps that
> could be filed as a follow-up?

I found that we do have bugs that apparently our tests didn't catch.

We have many places that rely on the `name` property of addresses and that doesn't exist for temporary addresses since it needs to be computed:

* https://dxr.mozilla.org/mozilla-central/rev/b75acf9652937ce79a9bf02de843c100db0e5ec7/browser/components/payments/content/paymentDialogWrapper.js#95,123
* https://dxr.mozilla.org/mozilla-central/rev/b75acf9652937ce79a9bf02de843c100db0e5ec7/browser/components/payments/res/components/address-option.js#60
* https://dxr.mozilla.org/mozilla-central/rev/b75acf9652937ce79a9bf02de843c100db0e5ec7/browser/components/payments/res/containers/address-picker.js#63,72

I was able to get the UI stuck rendering the payer address picker because an option was supposed to be selected but `filterAddresses` filtered it out since it had no `name`.

> Error: "selectedPayerAddress option temp-1394654167 does not exist in options"
> renderresource://payments/containers/address-picker.js:136:13
> stateChangeCallbackresource://payments/mixins/PaymentStateSubscriberMixin.js:102:7
> setState resource://payments/PaymentsStore.js:70:9

We should be computing fields that are necessary for our UI, perhaps all of them to be consistent with autofill storage. We should also add tests to catch this issue in the future.

Sam, can you take this follow-up bug?
Flags: needinfo?(sfoster)
Attached file state dump — 

    
  

        Attachment #8979783 -
        Attachment mime type: text/plain → application/json

    
    

    
  
yeah I'll take this.
Assignee: nobody → sfoster
Flags: needinfo?(sfoster)
Status: NEW → ASSIGNED
Priority: P2 → P1

    
    

    
  
Comment on attachment 8982101 [details]
Bug 1463608 - Expose FormAutofill computeFields methods, use them for temporary records in payments.

https://reviewboard.mozilla.org/r/248134/#review254504

Thanks

::: browser/components/payments/content/paymentDialogWrapper.js:45
(Diff revision 1)
> -  constructor(data = {}) {
> +  constructor(type, data = {}) {
> +    /**
> +     * The name of the collection. e.g. 'addresses' or 'creditCards'
> +     * Used to access methods from the FormAutofillStorage collections
> +     */
> +    this._type = type;

Is there a reason to not store a reference to the appropriate storage backend in the constructor rather than looking it up each time update/add is called? We could use `defineLazyGetter` to make the lookup lazy if that's the reason for delaying it.

::: browser/components/payments/content/paymentDialogWrapper.js:52
(Diff revision 1)
>    }
>    get(guid) {
>      return this._data[guid];
>    }
>    update(guid, record, preserveOldProperties) {

Nit for a past revision: could you insert new lines between each method… it's kinda hard to read without

::: browser/components/payments/content/paymentDialogWrapper.js:58
(Diff revision 1)
> -      Object.assign(this._data[guid], record);
> -    } else {
> +    let formAutofillCollection = this._type && formAutofillStorage[this._type];
> +    if (formAutofillCollection) {

Nit: I would rather we threw an exception if an invalid type is used rather than check that the collection is truthy. I don't think we should hide typos/errors like you are.

It think it's be fine to do that implicitly via a normal JS error looking for .computeFields even:
```js
formAutofillStorage[this._type].computeFields(recordToSave);
```

::: browser/components/payments/test/browser/browser_address_edit.js:75
(Diff revision 1)
>        is(addressGUIDs.length, 1, "Check there is one address");
>        let savedAddress = state.savedAddresses[addressGUIDs[0]];
>        for (let [key, val] of Object.entries(address)) {
>          is(savedAddress[key], val, "Check " + key);
>        }
> +      ok(savedAddress.guid, "Address got a guid");

I'm pretty sure we have some tests that check the PaymentResponse's shipping address that we should improve to have noticed the missing .name. Can you check if it's easy to get those tests to run in a loop with two modes: saved and temporary addresses?

::: browser/components/payments/test/browser/browser_address_edit.js:76
(Diff revision 1)
>        let savedAddress = state.savedAddresses[addressGUIDs[0]];
>        for (let [key, val] of Object.entries(address)) {
>          is(savedAddress[key], val, "Check " + key);
>        }
> +      ok(savedAddress.guid, "Address got a guid");
> +      ok(savedAddress.name, "Address got a name");

Can we check the value of the name here and in below hunks to ensure it's not truthy garbage?

        Attachment #8982101 -
        Flags: review?(MattN+bmo) → review+

    
    

    
      
  

    
    

    
  
Comment on attachment 8984618 [details]
Bug 1463608 - Add test helpers for payment address and cards.

This patch does some refactoring of the browser_address_edit.js test to reduce some of the duplication, and allow a temporary/not-temporary loop in the add address test. I've thrown these new helpers in head.js, and each does its own spawnPaymentDialogTask. I could probably move these to DialogContentUtils but its 6/half-dozen? 

Regardless, ISTM the end-result is improved, though probably still not ideal. But I need to step back a bit and get some input before I do the same for browser_card_edit.js - and potentially refactor other tests to use the new helpers. 

Jared, you just did some test refactoring so maybe you have opinions?

I had to work around the FTU stuff by starting each test with a single address and card in the formAutofill store. 

There's a surplus of info()s in there I'll clean up before trying to land. 

I may also restructure the test_private_persist_addresses test so it tests opting-in for saving an address from a private window as well as the default behavior.

        Attachment #8984618 -
        Flags: feedback?(jaws)

    
    

    
  
Comment on attachment 8984618 [details]
Bug 1463608 - Add test helpers for payment address and cards.

https://reviewboard.mozilla.org/r/250516/#review256782


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/test/browser/browser_address_edit.js:195
(Diff revision 1)
>    });
>    await cleanupFormAutofillStorage();
>  });
>  
>  add_task(async function test_add_payer_contact_name_email_link() {
> +  let prefilledGuids = await setup();

Error: 'prefilledguids' is assigned a value but never used. [eslint: no-unused-vars]

::: browser/components/payments/test/browser/browser_address_edit.js:271
(Diff revision 1)
>      await BrowserTestUtils.waitForCondition(() => win.closed, "dialog should be closed");
>    });
>  });
>  
>  add_task(async function test_edit_payer_contact_name_email_phone_link() {
> +  let prefilledGuids = await setup();

Error: 'prefilledguids' is assigned a value but never used. [eslint: no-unused-vars]

    
    

    
  
Comment on attachment 8982101 [details]
Bug 1463608 - Expose FormAutofill computeFields methods, use them for temporary records in payments.

https://reviewboard.mozilla.org/r/248134/#review256784


Code analysis found 4 defects in this patch:
 - 4 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/test/browser/browser_address_edit.js:199
(Diff revision 2)
>    });
>    await cleanupFormAutofillStorage();
>  });
>  
>  add_task(async function test_add_payer_contact_name_email_link() {
>    let prefilledGuids = await setup();

Error: 'prefilledguids' is assigned a value but never used. [eslint: no-unused-vars]

::: browser/components/payments/test/browser/browser_address_edit.js:279
(Diff revision 2)
>      await BrowserTestUtils.waitForCondition(() => win.closed, "dialog should be closed");
>    });
>  });
>  
>  add_task(async function test_edit_payer_contact_name_email_phone_link() {
>    let prefilledGuids = await setup();

Error: 'prefilledguids' is assigned a value but never used. [eslint: no-unused-vars]

::: browser/components/payments/test/browser/browser_address_edit.js:437
(Diff revision 2)
>         "Got expected number of pre-filled shipping addresses");
>  
>      await fillInAddressForm(frame, addressToAdd, addOptions);
>      await verifyPersistCheckbox(frame, addOptions);
>      await submitAddressForm(frame, addressToAdd, addOptions);
> +      ok(tempAddress.name, "Address has a name");

Error: Expected indentation of 4 spaces but found 6. [eslint: indent-legacy]

::: browser/components/payments/test/browser/browser_address_edit.js:437
(Diff revision 2)
>         "Got expected number of pre-filled shipping addresses");
>  
>      await fillInAddressForm(frame, addressToAdd, addOptions);
>      await verifyPersistCheckbox(frame, addOptions);
>      await submitAddressForm(frame, addressToAdd, addOptions);
> +      ok(tempAddress.name, "Address has a name");

Error: 'tempaddress' is not defined. [eslint: no-undef]

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Last updates include lint fixes and hardening the private window address adding test. Attempting to sanity check the shippingAddress in the response uncovered an issue where the test wasn't selecting the new address so shippingAddress wound up as null.

    
    

    
  
Comment on attachment 8982101 [details]
Bug 1463608 - Expose FormAutofill computeFields methods, use them for temporary records in payments.

https://reviewboard.mozilla.org/r/248134/#review254504

> I'm pretty sure we have some tests that check the PaymentResponse's shipping address that we should improve to have noticed the missing .name. Can you check if it's easy to get those tests to run in a loop with two modes: saved and temporary addresses?

Not trivial but done. I did most of this in the " Add test helpers for payment address and cards" patch - introducing some helpers and refactoring this test to hopefully make it at least somewhat legible.

    
    

    
  
Comment on attachment 8984618 [details]
Bug 1463608 - Add test helpers for payment address and cards.

https://reviewboard.mozilla.org/r/250516/#review257176

::: browser/components/payments/test/PaymentTestUtils.jsm:192
(Diff revision 2)
> +      let {requestStore} = Cu.waiveXrays(content.document.querySelector("payment-dialog"));
> +      let {savedBasicCards, tempBasicCards} = requestStore.getState();

This can just call getCurrentState

::: browser/components/payments/test/browser/browser_address_edit.js:8
(Diff revision 2)
>  "use strict";
>  
>  async function setup() {
>    await setupFormAutofillStorage();
> -  // adds 2 addresses and one card
> -  await addSampleAddressesAndBasicCard();
> +  await cleanupFormAutofillStorage();
> +  // add an address and card to avoid the FTU sequence

addSampleAddressesAndBasicCards returns the GUIDs. Can you just return the response from it?

Since it looks like you just want one address added, you could add an argument to addSampleAddressesAndBasicCards so that only one address will get added.

::: browser/components/payments/test/browser/browser_address_edit.js:70
(Diff revision 2)
> -      info("filling fields");
> -      for (let [key, val] of Object.entries(address)) {
> +      await spawnPaymentDialogTask(frame, async (args) => {
> +        let {address, isTemporary, prefilledGuids} = args;

await spawnPaymentDialogTask(frame, async ({address, isTemporary, prefilledGuids}) => {
  ...
}

::: browser/components/payments/test/browser/head.js:310
(Diff revision 2)
> +// test asserts:
> +/*
> +  * No saved addresses when starting test.

Should this comment be broken up to put these assertions at the top of the various add_tasks that confirm these conditions?

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8984618 [details]
Bug 1463608 - Add test helpers for payment address and cards.

https://reviewboard.mozilla.org/r/250516/#review257176

> addSampleAddressesAndBasicCards returns the GUIDs. Can you just return the response from it?
> 
> Since it looks like you just want one address added, you could add an argument to addSampleAddressesAndBasicCards so that only one address will get added.

Turned out I didn't need it at all.

> Should this comment be broken up to put these assertions at the top of the various add_tasks that confirm these conditions?

Was just my todo list while I was writing this. Removed.

    
    

    
  
I ended up making the changes to browser_card_edit.js in the 2nd commit. It was getting a bit gnarly refactoring those tests in a consistent way ahead of the changes in the following patch that actually made the temporary card store behave in an expected way. 

I could keep going with refactoring the tests to use the helpers, and probably extract more.. but it seems like a lot already. This much has made it easier to write private/non-private versions of the same test. I'm happy to keep going in follow-up bugs.

    
    

    
  
> > Since it looks like you just want one address added, you could add an argument to addSampleAddressesAndBasicCards so that only one address will get added.
> 
> Turned out I didn't need it at all.

That comment got added to the wrong issue. I did implement this suggestion. The function name is a bit odd now (plurals), but rather than touch every test I left it as-is for now.

    
    

    
  
Comment on attachment 8984618 [details]
Bug 1463608 - Add test helpers for payment address and cards.

https://reviewboard.mozilla.org/r/250516/#review259344

::: browser/components/payments/test/browser/head.js:177
(Diff revisions 2 - 3)
> +    PTU.BasicCards.JohnDoe,
> +  ]) {
> +  let count = 0;
> +  let guids = {};
> +
> +  for (let address of addresses) {

This would be simpler if you just used:
```js
for (let i = 0; i < addresses.length; i++) {
  guids[`address${i + 1}GUID`] = await addAddressRecord(addresses[i]);
}
```

Then we don't need to worry about what `count` will be used for outside of the loop.

        Attachment #8984618 -
        Flags: review?(jaws) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a7d3beb6afb8
Add test helpers for payment address and cards. r=jaws
https://hg.mozilla.org/integration/autoland/rev/bc7fb8a2e4ce
Expose FormAutofill computeFields methods, use them for temporary records in payments. r=MattN

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/a7d3beb6afb8
https://hg.mozilla.org/mozilla-central/rev/bc7fb8a2e4ce
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox63:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: