1432921 - Show an address input form before the summary view for users without a saved address

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

We want to optimize the flow for new users of PaymentRequest to ensure they have a smooth experience. If the user has no saved address in storage (e.g. from Form Autofill), then we don't want to present the user with an empty dropdown, instead we should take the user directly to the "add address" form so the flow is somewhat more like a wizard.

E.g. from a UX proposal:
 FTU (bug 1432912) => Add Address Form => Add Credit Card Form => Usual Summary

Comment 1

[:prathiksha]

Attachment: Bug 1432921 - Show an address input form before the summary view for users without a saved address.

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

Comment 2

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

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

Comment 3

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

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

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

https://reviewboard.mozilla.org/r/242894/#review248768

::: browser/components/payments/res/containers/address-form.js:144
(Diff revision 2)
>        errorStateChange: {
>          page: {
>            id: "address-page",
>            error: this.dataset.errorGenericSave,
>          },
>        },

pageState gets lost here

::: browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js:19
(Diff revision 2)
> -    id: "payment-summary",
> +    id: "",
> +    pageState: "before-summary-view",

I think the `id` should be reverted so that when developing over http the summary page is shown by default. Perhaps we can show what other properties are possible e.g. `pageState` and `error` in a comment instead?

e.g. 
```js
page: {
  id: "payment-summary",
  // pageState: "",
  // error: "My error",

::: browser/components/payments/res/paymentRequest.js:124
(Diff revision 2)
>        request: detail.request,
>        savedAddresses: detail.savedAddresses,
>        savedBasicCards: detail.savedBasicCards,
>        isPrivate: detail.isPrivate,
> -    });
> +      page: {
> +        id: "payment-page",

Is this supposed to be "payment-summary"?

Please add a browser chrome test to ensure the correct initial page is shown for both with and without saved addresses. Also make sure the other tests pass with this new page.

::: browser/components/payments/res/paymentRequest.js:131
(Diff revision 2)
> +    };
> +
> +    if (Object.keys(detail.savedAddresses).length == 0) {
> +      state.page = {
> +        id: "address-page",
> +        pageState: "before-summary-view",

I'm not sure the name `pageState` property is the clearest name for a string like this. Perhaps a boolean like `onboardingWizard: true` would be better as it will affect the total header visibility and the back/cancel buttons.

Comment 5

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

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

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

https://reviewboard.mozilla.org/r/242894/#review249356

Looks great, just some small things to fix.

::: browser/components/payments/res/containers/address-form.js:70
(Diff revision 4)
>        this.appendChild(this.backButton);
>        this.appendChild(this.saveButton);
> +      this.appendChild(this.cancelButton);

Nit: It seems like the order should be Cancel, Back, Save based on UX specs and I would re-order all the code to stay in that order (constructor and render too)

::: browser/components/payments/res/containers/address-form.js:156
(Diff revision 4)
>            id: "address-page",
> +          onboardingWizard: true,
>            error: this.dataset.errorGenericSave,

This shouldn't always be true, only when it was true. I think it should be:
```js
onboardingWizard: page.onboardingWizard,
```

::: browser/components/payments/res/paymentRequest.xhtml:148
(Diff revision 4)
>        <address-form id="address-page"
>                      class="page"
>                      data-error-generic-save="&addressPage.error.genericSave;"
>                      data-back-button-label="&addressPage.backButton.label;"
>                      data-save-button-label="&addressPage.saveButton.label;"
> +                    data-cancel-button-label="&cancelPaymentButton.label;"

Normally we would use a separate string if the same text is used in a differnet context. I'm not sure how I feel in this case since it visually is the same context other than the page. Just to be safe I think we should add `&addressPage.cancelButton.label;`

::: browser/components/payments/test/browser/browser.ini:18
(Diff revision 4)
>  [browser_shippingaddresschange_error.js]
>  [browser_show_dialog.js]
>  skip-if = os == 'win' && debug # bug 1418385
>  [browser_total.js]
> +[browser_payments_onboarding_wizard.js]

Please add this test in alphabetical order

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:10
(Diff revision 4)
> +    gBrowser,
> +    url: BLANK_PAGE_URL,
> +  }, async browser => {
> +    cleanupFormAutofillStorage();
> +
> +    // Open the payment dialog.

Nit: It's better if the comments like this are output via `info` so that it makes debugging failures easier. e.g.
```js
info("Open the payment dialog");
```

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:19
(Diff revision 4)
> +        details: PTU.Details.total60USD,
> +        options: PTU.Options.requestShippingOption,
> +        merchantTaskFn: PTU.ContentTasks.createAndShowRequest,
> +      });
> +
> +    // Check if the address page has been rendered.

Same here

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:20
(Diff revision 4)
> +        options: PTU.Options.requestShippingOption,
> +        merchantTaskFn: PTU.ContentTasks.createAndShowRequest,
> +      });
> +
> +    // Check if the address page has been rendered.
> +    spawnPaymentDialogTask(frame, async function() {

You should `await` on this response

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:29
(Diff revision 4)
> +    spawnPaymentDialogTask(frame, async function() {
> +      content.document.getElementById("address-page-cancel-button").click();
> +    });

FYI: we intentionally don't `await` on clicking cancel since the dialog will close before the content task has a chance to acknowledge the task completion.

::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:52
(Diff revision 4)
> +        details: PTU.Details.total60USD,
> +        merchantTaskFn: PTU.ContentTasks.createAndShowRequest,
> +      });
> +
> +    // Check if the payment summary page is now visible.
> +    spawnPaymentDialogTask(frame, async function() {

`await` this too

::: browser/components/payments/test/browser/head.js:22
(Diff revision 4)
> +const {ContentTaskUtils: CTU} = ChromeUtils.import(
> +  "resource://testing-common/ContentTaskUtils.jsm", {});

It seems like this isn't used and should be removed. It's already loaded in ContentTask scopes and doesn't need to be loaded in the browser-chrome scope.

Comment 7

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

https://reviewboard.mozilla.org/r/242894/#review249374


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


::: browser/components/payments/test/browser/browser_payments_onboarding_wizard.js:19
(Diff revision 5)
> +        details: PTU.Details.total60USD,
> +        options: PTU.Options.requestShippingOption,
> +        merchantTaskFn: PTU.ContentTasks.createAndShowRequest,
> +      });
> +
> +    info("Checking if the address page has been rendered")

Error: Missing semicolon. [eslint: semi]

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

https://reviewboard.mozilla.org/r/242894/#review249380

Thanks! Feel free to land when try is green!

Comment 10

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

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

Comment 11

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242894/diff/6-7/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/513a4aafc42a
Show an address input form before the summary view for users without a saved address. r=MattN

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out changeset for mochitest e-10s failures on test_address_form.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=513a4aafc42a4dee0c219c52910ec42e21f7ef17&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=178390650&filter-searchStr=mochitest-e10s-1%20m-e10s

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178390650&repo=autoland&lineNumber=3487

Backout link: https://hg.mozilla.org/integration/autoland/rev/918da9f12abd77bc4b4c0919ad79e40b8c667c5a

Comment 14

[Cosmin Sabou [:CosminS]]

Also please take a look at this failure:

https://treeherder.mozilla.org/logviewer.html#?job_id=178390733&repo=autoland&lineNumber=7752

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=513a4aafc42a4dee0c219c52910ec42e21f7ef17&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=178390733

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba6eaefad738
Show an address input form before the summary view for users without a saved address. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d7a58a3c8f
Backed out changeset 513a4aafc42a for mochitest e-10s failures on test_address_form.html. CLOSED TREE

Comment 16

[:prathiksha]

Comment on attachment 8974559 [details]
Bug 1432921 - Show an address input form before the summary view for users without a saved address.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242894/diff/7-8/

Comment 17

[:prathiksha]

(In reply to Cosmin Sabou [:CosminS] from comment #14)
> Also please take a look at this failure:
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=178390733&repo=autoland&lineNumber=7752
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=513a4aafc42a4dee0c219c52910ec42e21f7ef17&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&selectedJob=178390733

I don't think this failure is related to my patch. :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75ba8129c687
Show an address input form before the summary view for users without a saved address.r=MattN

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out for turning bug 1455882 into permafailure. 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=85738035d76b555081da1b1a9cc773701c1f68c8&fromchange=aab18ed4a194e3c80b79140265b59ee8a8a8c092&tochange=9d64158bdbf34177b896997ce0a75abfa43c1f7a&selectedJob=178448069

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178448069&repo=autoland&lineNumber=7675

Backout link: https://hg.mozilla.org/integration/autoland/rev/9d64158bdbf34177b896997ce0a75abfa43c1f7a

I've written to you about this in #c14. After your push the above bug turns into perma. Please take a look at this. Thank you.

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ba6eaefad738

Comment 21

[Matthew N. [:MattN]]

This was backed out according to comment 19.

Natalia, why did this get resolved if it was backed out? Please avoid this for the future.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96c52800d2ec
Show an address input form before the summary view for users without a saved address.r=MattN

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/96c52800d2ec

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

What happened here regarding the "FIXED" status:

The push from comment 12 and the backout in comment 13 got accidentally pushed to inbound (comment 15). That they the changes got new hashes. The commit message of the backout also referenced the hash (from autoland) which is the default. And because of the mismatch of the hash in the commit message and the hash for the landing push on inbound bugherder didn't match those and the landing push (which got backed out) was regarded as having landed without a backout and this bug got set to FIXED.