feat: put request id in details dictionary #426

rvm4 · 2017-02-22T20:01:57Z

closes Providing/Generating paymentRequestID during construction #388, PaymentRequestID initialization #412, It is never stated how the payment request ID is used #415.

rsolomakhin · 2017-02-22T20:44:34Z

Looks like you're passing in a paymentRequestID to the constructor, but remove its accessors and don't mention where this identifier is going.

If you expect the merchant website to read this identifier, please expose it through an accessor in PaymentResponse interface. Depending on your use-case, you may want to expose the identifier through an accessor in PaymentRequest interface as well.

If you expect the payment app to read the identifier, please add a note somewhere that it should be sent to the payment app.

marcoscaceres · 2017-02-23T01:19:42Z

index.html

@@ -301,6 +300,7 @@
          </p>
          <pre class="example" title="The 'details' argument">
            const details = {
+              paymentRequestID: "1234567890123",


s/paymentRequestID/paymentRequestId

marcoscaceres · 2017-02-23T01:20:16Z

index.html

@@ -1014,6 +1014,7 @@
      </h2>
      <pre class="idl">
        dictionary PaymentDetails {
+          DOMString paymentRequestID;


s/paymentRequestID/paymentRequestId/g

marcoscaceres · 2017-02-23T01:20:35Z

index.html

@@ -1033,6 +1034,17 @@
      </p>
      <dl>
        <dt>
+          <dfn>paymentRequestID</dfn>


s/paymentRequestID/paymentRequestId

marcoscaceres · 2017-02-23T01:26:39Z

index.html

+        </dt>
+        <dd>
+          <a>paymentRequestId</a> is a freeform identifier for this
+          payment request. This field is intended to hold a unique


Re: "intended", as it reads, it could be confused that the browser is supposed to use it for error recovery. I would remove the second sentence.

marcoscaceres · 2017-02-23T01:27:19Z

index.html

+          payment request. This field is intended to hold a unique
+          identifier for the purposes of recovering from failures and/or as a
+          foreign key into other systems. If a <a>paymentRequestId</a>
+          is not provided by the client then a UUID will be generated and stored


The third sentence should be a non-normative note. Also, there is no requirement that this shall be a UUID in the payment request constructor.

marcoscaceres · 2017-02-23T01:29:03Z

index.html

@@ -204,7 +204,6 @@
          Promise&lt;void&gt; abort();
          Promise&lt;boolean&gt; canMakePayment();

-          readonly attribute DOMString? paymentRequestId;


Did you delete this by accident from the interface?

Also, the constructor algorithm doesn't ever set this to null - so maybe it should not be nullable. It's also unclear what happens if paymentRequestId is set to just "" (the empty string).

Should setting it to null be treated as a special case (i.e., treat null as undefined instead of being converted to the word "null")?

We should not treat null as a special case. It is no more special than false or 0 (all of them are "things that are vaguely negative, but are not the default-triggering special value of undefined").

marcoscaceres · 2017-02-23T01:46:25Z

index.html

@@ -460,6 +457,9 @@
            </ol>
          </li>
          <li>Let <var>serializedModifierData</var> be an empty list.
+          <li>If <var>details.paymentRequestId</var> was not provided during
+          construction, populate the field with an [[RFC4122]] UUID.


Oh, I see. I don't think we have consensus to make this a hard requirement. Also, avoid modifying the incoming data, create a new variable instead.

Drafting some alternative text...

Sent some text for the comments above.

marcoscaceres · 2017-02-23T02:02:00Z

index.html

@@ -333,9 +333,6 @@
          The <a>PaymentRequest</a> constructor MUST act as follows:
        </p>
        <ol data-link-for="PaymentDetails" class="algorithm">
-          <li>If a <code>paymentRequestId</code> was not provided during


Here, I think we want this instead:

<li>Establish the request's id: <ol> <li>Let <var>requestId</var> be the empty string. </li> <li>If <var>details.id</var> is present, set <var> requestId</var> to the value of <var>details.paymentRequestId</var>. Otherwise, generate a string to uniquely identify this payment request and set <var>requestId</var> to that value. It is RECOMMENDED that the generated string be a <abbr title= "Universally Unique Identifier">UUID</abbr> as per [[!RFC4122]]. </li> </ol> </li>

Then, after the object is actually constructed:

<li>Set <var>request</var>.<a>paymentRequestId</a> to <var>requestId</var>.

marcoscaceres · 2017-02-23T02:04:59Z

Fixed the nits via a commit. Let me know if you agree with the changes I proposed. I can add them in.

domenic · 2017-02-23T03:27:04Z

I realize this is a bit late in the process, so feel free to ignore me, but is there a reason it's paymentRequest.paymentRequestId instead of paymentRequest.id? Same for the dictionary member I guess.

marcoscaceres · 2017-02-23T03:27:46Z

I was thinking the same. It's not too late to change it.

marcoscaceres · 2017-02-23T03:28:03Z

(no one shipped this, so we can change still)

marcoscaceres · 2017-02-23T03:59:43Z

@domenic, so, in practice you would end up with (not ideal, IMO):

const request = new PaymentRequest(x, {paymentRequestId: "123121"},z);
const response = await request.show();
// stuff happens... 
if (request.id === response.paymentRequestId ) {

}

What might be best is just call it requestId across all three:

const request = new PaymentRequest(x, {requestId: "123312"}, z);
const response = await request.show();
// stuff happens... 
if (request.requestId === response.requestId ) {

}

WDYT?

domenic · 2017-02-23T04:00:51Z

requestId could be reasonable, although I was thinking something more like

const request = new PaymentRequest(x, {id: "123121"},z);
const response = await request.show();
// stuff happens... 
if (request.id === response.requestId) {

}

marcoscaceres · 2017-02-23T04:05:41Z

@domenic, yeah, that works well actually... it's only the response that needs the qualifier.

marcoscaceres · 2017-02-23T04:55:43Z

I've updated the PR to reflect discussion above:

PaymentRequest gains "id" attribute.
PaymentResponse gains "requestId" attribute.
PaymentDetailsBase gains "id" member.
PaymentRequest constructor algo gains "Establish the request's id:" sub-algo.
UUID is RECOMMENDED.
PaymentResponse's .requestId gets set on construction.

I made changes, so my changes now need review 🤔

marcoscaceres · 2017-02-23T05:27:33Z

Question: do we want an internal slot for the [id] for when it's copied over from Request to Response? Can it be forged if the attribute value is just copied over? I guess having the internal slot is nicer - but I don't have an strong opinion.

marcoscaceres · 2017-02-23T06:07:43Z

so, decided internal slots are fun - so added one. PR now reflects that.

rsolomakhin · 2017-02-23T14:50:22Z

index.html

+              <dfn>[[\requestId]]</dfn>
+            </td>
+            <td>
+              A string, which can be empty, that identifies the a payment request.


s/the a/the/

rsolomakhin

LGTM

domenic

Processing model looks good, just a few nits and the issue of where to store it.

domenic · 2017-02-23T19:35:32Z

index.html

@@ -344,6 +342,17 @@
          </li>
          <li>Let <var>serializedMethodData</var> be an empty list.
          </li>
+          <li>Establish the request's id:
+            <ol>
+              <li>If <var>details.id</var> is present, set <a>[[\requestId]]</a>


Elsewhere (e.g. for details.shippingOptions) we've used the approach of normalizing the details.whatever, instead of creating a separate slot. I think that'd work best here too, so no separate internal slot for the ID. A separate slot leads to potential confusion where algorithms could accidentally reach into both places and get different values.

domenic · 2017-02-23T19:37:17Z

index.html

+        </h2>
+        <p>
+          When getting, the <a>id</a> attribute reflects the value of
+          the payment request's <a>[[\requestId]]</a> internal slot.


"Reflects" is a term of art that isn't quite appropriate here. You want "When getting, returns this PaymentRequest's [[requestId]] internal slot" (or, per the above advice, "this PaymentRequest's [[details]].id")

domenic · 2017-02-23T19:37:46Z

index.html

+        <dd>
+          <a>id</a> is a free-form identifier for this payment request.
+          <p class="note">
+            If a <a>id</a> member is not present, then the UA will generate a


s/a/an; maybe link "user agent" instead of using "UA"

domenic · 2017-02-23T19:37:53Z

index.html

+          <p class="note">
+            If a <a>id</a> member is not present, then the UA will generate a
+            unique identifier for the payment request during <a data-lt=
+            "PaymentRequest.PaymentRequest()">construction</a>.


This is a broken link according to ReSpec.

Yep, I've got a patch to fix that in ReSpec. I'll try to land that soon.

domenic · 2017-02-23T19:40:03Z

index.html

@@ -204,7 +204,6 @@
          Promise&lt;void&gt; abort();
          Promise&lt;boolean&gt; canMakePayment();

-          readonly attribute DOMString? paymentRequestId;


We should not treat null as a special case. It is no more special than false or 0 (all of them are "things that are vaguely negative, but are not the default-triggering special value of undefined").

The payment request Id is either provided by the developer or automagically generated by the UA - hence not nullable right now.

marcoscaceres · 2017-03-06T21:22:32Z

Rebased

marcoscaceres · 2017-03-06T21:25:20Z

@domenic, sorry, can you recheck this one (request.id)? I'm not if everything was addressed or not.

domenic

Bad rebase but overall looking good to me.

domenic · 2017-03-06T21:24:29Z

index.html

+        </h2>
+        <p>
+          When getting, the <a>id</a> attribute returns this
+          <a>PaymentRequest</a> <a>[[\details]]</a>.<a data-lt=


Nit: missing 's after PaymentRequest

domenic · 2017-03-06T21:25:05Z

index.html

@@ -1035,7 +1057,9 @@
        <dfn>PaymentDetailsBase</dfn> dictionary
      </h2>
      <pre class="idl">
-        dictionary PaymentDetailsBase {
+        dictionary PaymentDetails {


Rebasing seems to have gone bad here.

Oh, we'll spotted. How odd :(

domenic · 2017-03-06T21:25:12Z

index.html

+          </p>
+        </dd>
+        <dt>
+          <dfn>total</dfn>


marcoscaceres · 2017-03-06T21:33:57Z

Ok, I think I fixed the rebase issues.

domenic · 2017-03-06T21:34:35Z

Oh, I think this also needs handling in updateWith()? Unless it should move to PaymentDetailsInit...

marcoscaceres · 2017-03-06T21:36:34Z

I'm not sure it makes sense in .updateWith(), so PaymentDetailsInit would be the right place.

marcoscaceres · 2017-03-06T21:45:59Z

index.html

+          <p class="note">
+            If an <a>id</a> member is not present, then the <a>user agent</a>
+            will generate a unique identifier for the payment request during
+            <a data-lt="PaymentRequest.PaymentRequest()">construction</a>.


ReSpec will link this soon...

Follows w3c/payment-request#426 w3c/payment-request#453

Test update for w3c/payment-request#419 w3c/payment-request#426 w3c/payment-request#453

marcoscaceres previously requested changes Feb 23, 2017

View reviewed changes

marcoscaceres changed the title ~~put request id in details dictionary~~ put request id in details dictionary (closes #388) Feb 23, 2017

marcoscaceres changed the title ~~put request id in details dictionary (closes #388)~~ feat: put request id in details dictionary (closes #388, #412) Feb 23, 2017

marcoscaceres changed the title ~~feat: put request id in details dictionary (closes #388, #412)~~ feat: put request id in details dictionary Feb 23, 2017

marcoscaceres requested a review from domenic February 23, 2017 04:58

rsolomakhin reviewed Feb 23, 2017

View reviewed changes

rsolomakhin approved these changes Feb 23, 2017

View reviewed changes

domenic requested changes Feb 23, 2017

View reviewed changes

marcoscaceres force-pushed the request_id_construct branch from 43a81d7 to 756d338 Compare February 28, 2017 00:53

rvm4 and others added 9 commits March 7, 2017 08:20

put request id in details dictionary

94a4396

Lowercase d in Id, as per platform conventions

9095b7f

Add paymentRequestId back in as "id"

32b698f

Feat: Add PaymentResponse.requestId

0b8352e

fix: id is not nullable

3efc14e

The payment request Id is either provided by the developer or automagically generated by the UA - hence not nullable right now.

rename paymentRequestId to just id

6456c61

fix: use id instead of paymentRequestId

a475df5

update text to match discussion

9d3e59f

Set the response id attribute

d46f8f7

marcoscaceres added 3 commits March 7, 2017 08:21

use [[requestId]] internal slot instead

009f63a

Editorial: improve custom details.id usage example

aacea2d

editorial: make it clear we are adding an .id member

ef429cb

marcoscaceres force-pushed the request_id_construct branch from cda253a to ef429cb Compare March 6, 2017 21:22

domenic requested changes Mar 6, 2017

View reviewed changes

fix rebase issue

8d79319

transplant id to PaymentDetailsInit

113fb60

marcoscaceres reviewed Mar 6, 2017

View reviewed changes

domenic approved these changes Mar 6, 2017

View reviewed changes

marcoscaceres merged commit 712e10b into gh-pages Mar 6, 2017

marcoscaceres deleted the request_id_construct branch March 6, 2017 21:51

This was referenced Mar 6, 2017

It is never stated how the payment request ID is used #415

Closed

PaymentRequestID initialization #412

Closed

PaymentDetails should include a transaction ID #287

Closed

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Mar 10, 2017

Update to latest

970a58f

Follows w3c/payment-request#426 w3c/payment-request#453

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Mar 10, 2017

Update to latest

2e4713f

Follows w3c/payment-request#426 w3c/payment-request#453

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Mar 17, 2017

Update IDL for Payment Request and add a historical.html

ee94762

Test update for w3c/payment-request#419 w3c/payment-request#426 w3c/payment-request#453

marcoscaceres added the cr-substantive label May 3, 2017

feat: put request id in details dictionary #426

feat: put request id in details dictionary #426

Conversation

rvm4 commented Feb 22, 2017 • edited by pr-preview bot Loading

rsolomakhin commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

marcoscaceres commented Feb 23, 2017

domenic commented Feb 23, 2017 • edited Loading

marcoscaceres commented Feb 23, 2017

marcoscaceres commented Feb 23, 2017

marcoscaceres commented Feb 23, 2017

domenic commented Feb 23, 2017

marcoscaceres commented Feb 23, 2017

marcoscaceres commented Feb 23, 2017

marcoscaceres commented Feb 23, 2017

marcoscaceres commented Feb 23, 2017

Choose a reason for hiding this comment

rsolomakhin left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Mar 6, 2017

marcoscaceres commented Mar 6, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Mar 6, 2017

domenic commented Mar 6, 2017

marcoscaceres commented Mar 6, 2017

Choose a reason for hiding this comment

rvm4 commented Feb 22, 2017 •

edited by pr-preview bot

Loading

marcoscaceres Feb 23, 2017 •

edited

Loading

domenic commented Feb 23, 2017 •

edited

Loading