Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define PaymentResponse.prototype.retry() method #720

Merged
merged 1 commit into from Jun 21, 2018
Merged

Conversation

@marcoscaceres
Copy link
Member

marcoscaceres commented Jun 7, 2018

part 1 of #705

The following tasks have been completed:

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 marcoscaceres requested a review from domenic Jun 7, 2018
@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jun 7, 2018

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

@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jun 7, 2018

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

@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jun 12, 2018

@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.

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

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Jun 12, 2018

Author Member

@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.

This comment has been minimized.

Copy link
@ianbjacobs

ianbjacobs Jun 12, 2018

Collaborator

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

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Jun 12, 2018

Author Member

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

@marcoscaceres marcoscaceres requested a review from zkoch Jun 12, 2018
@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jun 12, 2018

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

@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jun 12, 2018

@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().

Copy link
Collaborator

domenic left a comment

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().

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

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Jun 12, 2018

Author Member

Great suggestion! I'll make this change.

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

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

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

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

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

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

index.html Outdated
<var>retryPromise</var>.
</li>
<li>
<a>In parallel</a>:

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

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.

index.html Outdated
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:

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

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().

This comment has been minimized.

Copy link
@marcoscaceres

marcoscaceres Jun 18, 2018

Author Member

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

index.html Outdated
"<a>AbortError</a>" <a>DOMException</a>.
</li>
</ol>
<p>

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

This paragraph should be its own standalone step.

index.html Outdated
<dfn>[[\retryPromise]]</dfn>
</td>
<td>
When <a>[[\retrying]]</a> is true, a <a>Promise</a> resolves when

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

A Promise that resolves...

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

This comment has been minimized.

Copy link
@domenic

domenic Jun 12, 2018

Collaborator

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

@marcoscaceres marcoscaceres changed the title define PaymentResponse.retry() method define PaymentResponse.prototype.retry() method Jun 12, 2018
@marcoscaceres marcoscaceres changed the title define PaymentResponse.prototype.retry() method Define PaymentResponse.prototype.retry() method Jun 12, 2018
@marcoscaceres marcoscaceres force-pushed the retry_request branch 2 times, most recently from 8d2345e to 0b629e9 Jun 19, 2018
Sets foundation for the method, which is then expanded upon by
other pull requests.
@marcoscaceres marcoscaceres force-pushed the retry_request branch from 0b629e9 to 9b1e54a Jun 21, 2018
@marcoscaceres marcoscaceres merged commit ee56f25 into gh-pages Jun 21, 2018
1 check passed
1 check passed
ipr PR deemed acceptable.
Details
@marcoscaceres marcoscaceres deleted the retry_request branch Jun 21, 2018
@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jun 28, 2018

@marcoscaceres

This comment has been minimized.

Copy link
Member Author

marcoscaceres commented Jul 9, 2018

@aestes

This comment has been minimized.

Copy link
Collaborator

aestes commented Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.