Re: [w3c/browser-payment-api] put request id in details dictionary (#426)

marcoscaceres requested changes on this pull request.



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

s/paymentRequestID/paymentRequestId

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

s/paymentRequestID/paymentRequestId/g

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

s/paymentRequestID/paymentRequestId

> @@ -1033,6 +1034,17 @@
       </p>
       <dl>
         <dt>
+          <dfn>paymentRequestId</dfn>
+        </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. 
 

> @@ -1033,6 +1034,17 @@
       </p>
       <dl>
         <dt>
+          <dfn>paymentRequestId</dfn>
+        </dt>
+        <dd>
+          <a>paymentRequestId</a> is a freeform identifier for this
+          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.  

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

> @@ -204,7 +204,6 @@
           Promise&lt;void&gt; abort();
           Promise&lt;boolean&gt; canMakePayment();
 
-          readonly attribute DOMString? paymentRequestId;

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")?

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

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

Drafting some alternative text... 

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

```HTML
          <li>Establish the request's id:
            <ol>
              <li>Let <var>requestId</var> be the empty string.
              </li>
              <li>If <var>details.paymentRequestId</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:

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/browser-payment-api/pull/426#pullrequestreview-23378719