Add support for merchant validation by marcoscaceres · Pull Request #751 · w3c/payment-request

marcoscaceres · 2018-07-06T21:41:10Z

closes #646

The following tasks have been completed:

Confirmed there are no ReSpec errors/warnings.
Added Web platform tests and Test PaymentRequest onmerchantvalidation attribute web-platform-tests/wpt#12423
added MDN Docs (link)
added MDN compat data

Implementation commitment:

Safari
Chrome
Firefox
Edge (public signal)

Impact on Payment Handler spec?

Preview | Diff

marcoscaceres · 2018-07-10T03:19:00Z

Blocking on @aestes review/feedback. This is primarily to align with Apple Pay so really need someone from Apple to approve it.

marcoscaceres · 2018-07-10T03:29:12Z

Gecko tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1474499

marcoscaceres · 2018-07-10T03:30:14Z

WebKit implementation: https://trac.webkit.org/changeset/226766

adrianhopebailie · 2018-08-07T13:01:15Z

index.html

+          Validate a merchant algorithm
+        </h2>
+        <p>
+          <dfn>Merchant validation</dfn> is the process by which <a>payment


Two missing a's

<dfn>Merchant validation</dfn> is the process by which a <a>payment handler</a> validates the identity of a merchant.

marcoscaceres · 2018-08-13T05:52:53Z

Added tests for constructor and for onmerchantvalidation attribute:
web-platform-tests/wpt#12424 and web-platform-tests/wpt#12423

aestes

This looks good, although I was surprised to see that MerchantValidationEvent.validationURL can be initialized from the eventInitDict. Is that there for payment handlers' sake?

marcoscaceres · 2018-08-21T23:29:42Z

@aestes, wrote:

This looks good, although I was surprised to see that MerchantValidationEvent.validationURL can be initialized from the eventInitDict. Is that there for payment handlers' sake?

It's not handler related - but could be used there too, I guess: It used to be a requirement in the DOM Spec that there be a 1:1 relationship between initialization dictionary members and Event interface attributes.

@domenic, I see this has now changed in the inner event creation steps. It seems that requirement was loosened up a bit:

For each member → value in dictionary, if event has an attribute whose identifier is member, then initialize that attribute to value.

Or does the 1:1 relationship rule still hold?

@aestes, the .complete() still rely on .isTrusted - so IMO it would still be good to define how URLs are resolved when the MerchantValidationEvent is constructed (even if constructed unsafely in JS).

marcoscaceres · 2018-08-21T23:32:59Z

@domenic requesting prioritization on this when you have time, as currently I'm working on an implementation in Gecko - so would really appreciate your review.

domenic · 2018-08-21T23:33:15Z

The idea is still to have 1:1, but there are a few legacy exceptions we worked to accomodate with that clause.

Will do a full review tomorrow!

domenic

Overall makes sense; some things to fix

domenic · 2018-08-23T18:59:28Z

index.html

+            "MerchantValidationEvent.MerchantValidationEvent()"><code>MerchantValidationEvent</code>
+            Constructor</dfn>
+          </h3>
+          <p data-tests=


This may be new technology since we last worked on this, but now we have https://dom.spec.whatwg.org/#concept-event-constructor-ext. You should define "event constructing steps" for MerchantValidationEvent which:

set [[waitForUpdate]] etc.

Validate/normalize/set the default for the validationURL attribute (no internal slot for event attributes).

The new machinery took me a little bit to get going... but it's much nicer 👌 Will eventually need to update the rest of the spec to use it.

domenic · 2018-08-23T19:00:30Z

index.html

+            Constructor</dfn>
+          </h3>
+          <p data-tests=
+          "MerchantValidationEvent/constructor.https.html, MerchantValidationEvent/constructor.http.html">


The internal slots list, both in the constructor and in the table, seem incomplete. The constructor initializes waitForUpdate and validationURL. (validationURL should not be an internal slot.) The table lists request and validationURL. The complete() method operates on request and waitForUpdate.

Oops, copy/pasta. I've added a [[waitForUpdate]] slot and linked it properly. validationURL is now set on the attribute, not a slot.

domenic · 2018-08-23T19:01:04Z

index.html

+          <ol class="algorithm">
+            <li>Let <var>base</var> be the <a data-cite=
+            "dom#context-object">context object</a>’s <a data-cite=
+            "!html/multipage/webappapis.html#relevant-settings-object">relevant


There is no context object in constructors. But once you use event constructing steps you can use event's relevant settings object.

domenic · 2018-08-23T19:02:55Z

index.html

+              </td>
+              <td>
+                The <a>PaymentRequest</a> instance that instantiated this
+                <a>PaymentResponse</a>.


"this" is not a PaymentResponse, so this doesn't seem right...

These slots are not needed anymore. I can get request from target, after isTrusted is checked.

domenic · 2018-08-23T19:04:24Z

index.html

+            </li>
+            <li>Set <var>event</var>.<a>[[\waitForUpdate]]</a> to true.
+            </li>
+            <li>Run the <a>validate a merchant algorithm</a> with


Wrong algorithm

Oh crap. I went all circular (how embarrassing!)... this was supposed to:

disable the UI.

wait for the promise to settle, "abort the update" is it rejects...

if it fulfills, then value needs to be cryptographically verified.

if the merchant is no good, abort the update.

if merchant is validated, enable the UI.

domenic · 2018-08-23T19:05:12Z

index.html

+        <p>
+          For <a>payment methods</a> that support <a>merchant validation</a>,
+          the user agent runs the <dfn>validate a merchant algorithm</dfn>. The
+          algorithm takes a USVString <var>merchantSpecificURL</var>, provided


Either link/code-ify USVString or use scalar value string.

domenic · 2018-08-23T19:08:04Z

index.html

+              <li>Otherwise, set <var>request</var>.<a>[[\updating]]</a> to
+              false.
+              </li>
+              <li>Enable user interface, allowing the request for payment to


Missing "the"

marcoscaceres · 2018-08-28T03:11:03Z

@domenic, hopefully better now :/

domenic

Only a couple more minor things

domenic · 2018-08-28T15:53:33Z

index.html

+          <h3>
+            <dfn data-lt=
+            "MerchantValidationEvent.MerchantValidationEvent()"><code>MerchantValidationEvent</code>
+            Constructor</dfn>


Lowercase "c"

domenic · 2018-08-28T15:54:30Z

index.html

+          <p data-tests=
+          "MerchantValidationEvent/constructor.https.html, MerchantValidationEvent/constructor.http.html">
+            The <dfn>event constructing steps for a
+            <code>MerchantValidationEvent</code></dfn>, which take a


Needs to link to "event constructing steps" in DOM, not define a new set of steps.

domenic · 2018-08-28T15:54:51Z

index.html

+            <var>base</var>.
+            </li>
+            <li>If <var>validationURL</var> is failure, throw a
+            <a>TypeError</a>.


Needs to initialize event's validationURL attribute to validationURL

D'oh! Added just below.

domenic · 2018-08-28T15:55:26Z

index.html

+            <li>Initialize <var>event</var>.<a data-lt=
+            "mechvalidation.waitForUpdate">[[\waitForUpdate]]</a> to false.
+            </li>
+            <li>Return <var>event</var>.


No need to return anything from these steps.

marcoscaceres · 2018-08-29T04:11:00Z

Third time lucky 🤞

marcoscaceres requested a review from aestes July 6, 2018 21:41

marcoscaceres changed the title ~~Merchantvalidation~~ Add support for merchant validation Jul 6, 2018

marcoscaceres mentioned this pull request Jul 6, 2018

Add support for merchant validation #646

Closed

marcoscaceres requested a review from domenic July 6, 2018 22:09

marcoscaceres added the Blocked label Jul 10, 2018

marcoscaceres added the needs tests label Jul 10, 2018

adrianhopebailie reviewed Aug 7, 2018

View reviewed changes

marcoscaceres force-pushed the merchantvalidation branch from d365cf1 to b6cb08b Compare August 10, 2018 04:34

marcoscaceres removed the Blocked label Aug 10, 2018

marcoscaceres force-pushed the merchantvalidation branch from b6cb08b to 5de9181 Compare August 13, 2018 05:34

aestes approved these changes Aug 21, 2018

View reviewed changes

domenic requested changes Aug 23, 2018

View reviewed changes

marcoscaceres added 6 commits August 27, 2018 14:28

Add support for merchant validation

6397d3e

nits

86e00ab

Define contructor

0f2f708

tests: link to onmerchantvalidation attribute test

24642d4

test: link to test for MerchantValidationEvent.complete()

b9edf1b

Rewiew feedback

e54588b

marcoscaceres force-pushed the merchantvalidation branch from 8cfc91b to e54588b Compare August 27, 2018 10:48

marcoscaceres added 3 commits August 28, 2018 12:24

Set internal slot correctly + fixes

272ca1c

Link to right algorithm

7d19483

Call into validate merchant

b350374

fix: MerchantValidationEventInit extends EventInit

f04fc92

domenic requested changes Aug 28, 2018

View reviewed changes

Review feedback

60bdb3b

This was referenced Aug 30, 2018

Test PaymentRequest onmerchantvalidation attribute web-platform-tests/wpt#12423

Merged

tests for MerchantValidationEvent web-platform-tests/wpt#12424

Merged

domenic approved these changes Aug 30, 2018

View reviewed changes

marcoscaceres merged commit 944f512 into gh-pages Aug 30, 2018

marcoscaceres deleted the merchantvalidation branch August 30, 2018 16:19

marcoscaceres removed the needs tests label Sep 14, 2020

Conversation

marcoscaceres commented Jul 6, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcoscaceres commented Jul 10, 2018

Uh oh!

marcoscaceres commented Jul 10, 2018

Uh oh!

marcoscaceres commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Aug 13, 2018

Uh oh!

aestes left a comment

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Aug 21, 2018

Uh oh!

marcoscaceres commented Aug 21, 2018

Uh oh!

domenic commented Aug 21, 2018

Uh oh!

domenic left a comment

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 Aug 27, 2018 • 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!

marcoscaceres commented Aug 28, 2018

Uh oh!

domenic left a comment

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 Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcoscaceres commented Jul 6, 2018 •

edited by pr-preview bot

Loading

marcoscaceres Aug 27, 2018 •

edited

Loading