Define PaymentResponse.prototype.retry() method by marcoscaceres · Pull Request #720 · w3c/payment-request

marcoscaceres · 2018-06-07T04:28:51Z

part 1 of #705

The following tasks have been completed:

Confirmed there are no ReSpec errors/warnings.
Added Web platform tests'
MDN Docs

Implementation commitment:

Safari
Chrome (link to issue)
Firefox - currently P3 bug
Edge (public signal)

Impact on Payment Handler spec?

Unknown.

Example

This pull request gets us here... the user doesn't yet know what's actually wrong with the payment, but at least they know something is wrong.

async function doPaymentRequest() {
  const request = new PaymentRequest(methodData, details, options);
  const response = await request.show();
  try {
    await recursiveValidate(request, response);
  } catch (err) { // retry aborted.
    console.error(err);
    return;
  }
  await response.complete("success");
}

async function recursiveValidate(request, response) {
  const promisesToFixThings = [];
  const errors = await validate(request, response);
  if (!errors) {
    return;
  }
  await response.retry();
  return recursiveValidate(request, response);
}

doPaymentRequest();

Preview | Diff

marcoscaceres · 2018-06-07T04:30:03Z

Ok, implemented this and it seems to work for me locally.

marcoscaceres · 2018-06-07T04:30:53Z

(again noting, this doesn't include passing the actual errors yet - that's part 2.)

marcoscaceres · 2018-06-12T07:04:02Z

@domenic, this is ready for review when you have time. Let me know how you want to handler all the parts. I tried to limit the changes to make is easier to review.

marcoscaceres · 2018-06-12T13:48:13Z

index.html

+              <var>request</var>.<a>[[\details]]</a>.<a data-lt=
+              "PaymentDetailsInit.id">id</a>.
+              </li>
+              <li>Set the <a data-lt=


@zkoch, question for you: upon a retry, will it be possible for a user to change payment handlers. For instance, can a user switch from "basic-card" to "https://bob.pay"? Seems like they probably should be able to.

My view is yes, they should be able to do so.

I'll move it down, so it's not part of the initialization.

marcoscaceres · 2018-06-12T13:50:40Z

Seeking additional implementation commitment for .retry() also. Currently, only have commitment from Mozilla.

marcoscaceres · 2018-06-12T13:53:16Z

@ianbjacobs, @adrianhopebailie, could you evaluate if there is any impact on payment handler? I've not given it any thought. If there is, could you add details to the "Impact on Payment Handler spec?" Same applies for the other pull requests relating to retry().

domenic

Mostly looks good, lots of editorial changes... Feel free to merge after fixing if you want to get a move on, or I can take another pass.

Also, as a nit on the commit message, it's PaymentResponse.prototype.retry() or paymentResponse.retry(), not PaymentResponse.retry().

domenic · 2018-06-12T15:18:54Z

index.html

+          <a>DOMException</a>.
+          </li>
+          <li>Set <var>response</var>.<a>[[\retrying]]</a> to true.
+          </li>


You may be able to get away with consolidating [[retryPromise]] and [[retrying]] into just [[retryPromise]], which will be null if you are not retrying. This might also be a good change because then you'd make it clearer that the UA can release the reference to the promise, when you set it to null.

Great suggestion! I'll make this change.

domenic · 2018-06-12T15:19:19Z

index.html

+                </ol>
+                <p>
+                  Finally, when <var>retryPromise</var> settles, set
+                  <var>response</var><a>[[\retrying]]</a> to false.


There are a lot of missing periods in this PR. Search for </var><a>[[ (should be </var>.<a>[[).

domenic · 2018-06-12T15:20:30Z

index.html

+              is wrong with the user-provided data of the payment response.
+              </li>
+              <li>
+                <p>


The first paragraph here doesn't seem like a numbered step. I would put it as a NOTE inside the step 10, "return retryPromise".

domenic · 2018-06-12T15:21:35Z

index.html

+          <var>retryPromise</var>.
+          </li>
+          <li>
+            <a>In parallel</a>:


I don't think any of these steps should be done in parallel (on a background thread). Updating the UI needs to be done from the UI thread. And the other steps are basically just setting up callbacks on main-thread objects.

domenic · 2018-06-12T15:23:43Z

index.html

+                  If <var>document</var> stops being <a data-cite=
+                  "!HTML#fully-active">fully active</a> while the user
+                  interface is being shown, or no longer is by the time this
+                  step is reached, then:


I think you also want a fully active check earlier in the algorithm (before you present the UI in the first place), as is done in show().

Good point. But we still need this for when the UI is being presented (as happens in show() step 19). Adding both checks.

domenic · 2018-06-12T15:24:29Z

index.html

+                  "<a>AbortError</a>" <a>DOMException</a>.
+                  </li>
+                </ol>
+                <p>


This paragraph should be its own standalone step.

domenic · 2018-06-12T15:25:34Z

index.html

+              <dfn>[[\retryPromise]]</dfn>
+            </td>
+            <td>
+              When <a>[[\retrying]]</a> is true, a <a>Promise</a> resolves when


A Promise that resolves...

domenic · 2018-06-12T15:26:12Z

index.html

+              <li>Set <var>response</var>.<a>[[\retrying]]</a> to false.
+              </li>
+              <li>Set <var>response</var>.<a>[[\retryPromise]]</a> to
+              undefined.


null, not undefined, is what you used elsewhere for the default here

Sets foundation for the method, which is then expanded upon by other pull requests.

marcoscaceres · 2018-06-28T23:16:24Z

Added some developer docs:
https://developer.mozilla.org/en-US/docs/Web/API/PaymentResponse/retry

marcoscaceres · 2018-07-09T16:24:31Z

Blink intent to implement https://groups.google.com/a/chromium.org/d/msg/blink-dev/wayZGnuBkrI/SCLOTxCACwAJ

aestes · 2018-11-02T21:57:56Z

WebKit tracking bug: https://bugs.webkit.org/show_bug.cgi?id=190985

marcoscaceres requested a review from domenic June 7, 2018 04:28

This was referenced Jun 7, 2018

teach retry() about payerErrors #721

Merged

Add tests for PaymentResponse.retry() web-platform-tests/wpt#11101

Merged

marcoscaceres commented Jun 12, 2018

View reviewed changes

marcoscaceres requested a review from zkoch June 12, 2018 13:50

domenic approved these changes Jun 12, 2018

View reviewed changes

marcoscaceres changed the title ~~define PaymentResponse.retry() method~~ define PaymentResponse.prototype.retry() method Jun 12, 2018

marcoscaceres changed the title ~~define PaymentResponse.prototype.retry() method~~ Define PaymentResponse.prototype.retry() method Jun 12, 2018

marcoscaceres force-pushed the retry_request branch 2 times, most recently from 8d2345e to 0b629e9 Compare June 19, 2018 18:48

Define PaymentResponse.prototype.retry() method

9b1e54a

Sets foundation for the method, which is then expanded upon by other pull requests.

marcoscaceres force-pushed the retry_request branch from 0b629e9 to 9b1e54a Compare June 21, 2018 16:05

marcoscaceres merged commit ee56f25 into gh-pages Jun 21, 2018

marcoscaceres deleted the retry_request branch June 21, 2018 16:10

Conversation

marcoscaceres commented Jun 7, 2018 • edited by aestes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Uh oh!

marcoscaceres commented Jun 7, 2018

Uh oh!

marcoscaceres commented Jun 7, 2018

Uh oh!

marcoscaceres commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jun 12, 2018

Uh oh!

marcoscaceres commented Jun 12, 2018

Uh oh!

domenic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Jun 28, 2018

Uh oh!

marcoscaceres commented Jul 9, 2018

Uh oh!

aestes commented Nov 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcoscaceres commented Jun 7, 2018 •

edited by aestes

Loading

domenic left a comment •

edited

Loading