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

appRequest attribute should not be a dictionary #111

Closed
rsolomakhin opened this issue Mar 17, 2017 · 11 comments
Closed

appRequest attribute should not be a dictionary #111

rsolomakhin opened this issue Mar 17, 2017 · 11 comments

Comments

@rsolomakhin
Copy link
Collaborator

PaymentRequestEvent.idl contains the following declaration:

readonly attribute PaymentAppRequest appRequest;

According to https://heycam.github.io/webidl/#idl-dictionaries, "Dictionaries must not be used as the type of an attribute or constant." Originally filed in http://crbug.com/702581.

@adamroach
Copy link
Contributor

Thanks; I'm fixing like this in my current round of edits:

interface PaymentAppRequest {
    readonly attribute DOMString                           origin;
    readonly attribute DOMString                           id;
    readonly attribute FrozenArray<PaymentMethodData>      methodData;
    readonly attribute PaymentItem                         total;
    readonly attribute FrozenArray<PaymentDetailsModifier> modifiers;
    readonly attribute DOMString                           optionId;
};

(Needed to change Sequence to FrozenArray since you can't have sequences in interfaces).

adamroach added a commit to adamroach/webpayments-payment-apps-api that referenced this issue Mar 18, 2017
and to flesh out Instrument/Wallet APIs.
These incorporate example code from Marcos'
Payment Handler thumbnail document.
ianbjacobs pushed a commit that referenced this issue Mar 18, 2017
* Proposed resolution to #95 and #96

* Fixing a couple of nits

* Conflict merge

* Changes to resolve issues #99, #109, #111
and to flesh out Instrument/Wallet APIs.
These incorporate example code from Marcos'
Payment Handler thumbnail document.
@marcoscaceres
Copy link
Member

Why not keep this as a dictionary, but just add a .getRequestData() to the event instead? Otherwise, I would expect to see a constructor on this interface so it can be used in the rest of the platform.

@marcoscaceres
Copy link
Member

What is optionId again?

@marcoscaceres
Copy link
Member

This also seems to be missing the shipping options and the selected shipping option?

@marcoscaceres
Copy link
Member

(the selected shipping option can be derived from PaymentShippingOption.selected, so ignore the second part of my question).

@adamroach
Copy link
Contributor

@marcoscaceres --

What is optionId again?

Renamed to instrumentId by subsequent edits, it is described as: This attribute indicates the PaymentInstrument selected by the user. It corresponds to the id field provided during registration.

This also seems to be missing the shipping options and the selected shipping option?

I don't think we've had any conversation about whether those are to be provided to the payment app. My inclination is that they should not be -- we should have a discussion in the WG before making changes.

@marcoscaceres
Copy link
Member

Renamed to instrumentId by subsequent edits, it is described as: This attribute indicates the PaymentInstrument selected by the user. It corresponds to the id field provided during registration.

Sorry, I'm still confused. To which member or attribute does it map in Payment Request?

I don't think we've had any conversation about whether those are to be provided to the payment app. My inclination is that they should not be -- we should have a discussion in the WG before making changes.

Ok, I don't mind. If we reduce this further, it might end up that we don't end up having to spin up a new browsing context at all (and we only show a native payment sheet). That would probably be a good thing.

@adamroach
Copy link
Contributor

@marcoscaceres --

Sorry, I'm still confused. To which member or attribute does it map in Payment Request?

It doesn't. This is part of what we've been trying to explain with regards to it necessarily being a different object type than the PaymentRequest: it's not a subset of the data in PaymentRequest, nor is it a superset. It necessarily lacks some items that appear in PaymentRequest (such as line items), and it necessarily has some items that cannot appear in PaymentRequest, such as implementId.

So, let me try again to explain what the implementId in PaymentAppRequest means. Let's look at this example:

amex

There are three wallets registered: two on www.americanexpress.com, and one on Bank of America's origin (they have an EV cert, so they get a name rather than a domain displayed in my example, but I'll note this is non-normative). When www.americanexpress.com added the "Account ending in 9004", they did so with something like the following:

    registration.paymentManager.instruments.set(
      "dc2de27a-ca5e-4fbd-883e-b6ded6c69d4f", // This is the implementId
      {
        name: "Account ending in 9004",
        enabledMethods: ["basic-card"],
        capabilities: {
          supportedNetworks: ['amex'],
          supportedTypes: ['credit']
        }
      }),

They would have registered the other payment instruments ("Account ending in 7195" and "Account ending in 3015") similarly, although the first parameter to "set" would have been different.

That first parameter, that key, is the instrumentId.

So, when I, as a user, click on the "Account ending in 9004" option, we're going to pass that information (dc2de27a-ca5e-4fbd-883e-b6ded6c69d4f) along to the service worker that is handling the event. We will call it instrumentId. And this will let it know that I have indicated a desire to use that card -- rather than the other two it knows about -- so that it doesn't have to ask me again.

@marcoscaceres
Copy link
Member

ah, got it. Thanks for the clarification. Note that "capabilities" member maps to "data" on the Payment Request side. I wonder if we should change it to data but treat it conceptually as "capability".

@marcoscaceres
Copy link
Member

That is: maybe it's possible to reuse "PaymentMethodData" there, so we don't need define a new thing?

registration.paymentManager.instruments.set(id, <PaymentMethodData> ) 

@rsolomakhin
Copy link
Collaborator Author

I'm satisfied with the outcome (PaymentAppRequest is no longer a dictionary). @marcoscaceres Let's think some more about reusing PaymentMEthodData and circle back to this idea later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@marcoscaceres @rsolomakhin @adamroach and others