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

Constructor should not include "total" in list of items #18

Closed
adrianba opened this issue Mar 3, 2016 · 13 comments
Closed

Constructor should not include "total" in list of items #18

adrianba opened this issue Mar 3, 2016 · 13 comments

Comments

@adrianba
Copy link
Contributor

adrianba commented Mar 3, 2016

This issue comes from WICG/paymentrequest#68.

From the spec:

There is an open issue about whether the items sequence should special-case the last item in the sequence to represent the total.

@adrianhopebailie
Copy link
Collaborator

In TAG review @triblondon said:

[This issue] discusses the format of the PaymentDetails dictionary and the special case treatment of the final 'sum' item. I'm very strongly in favour of breaking this out into a separate element. To me it has no business being in that sequence. In fact I'm dubious of the need for an explicit total at all.

@adrianhopebailie
Copy link
Collaborator

There is a PR #111 which deals with this but also another issue (#15) which has a proposal to change the whole shape of the constructor parameters.

Proposal

In support of the proposal in #15 create a new amount member on PaymentDetails which holds the requested amount. This should be a sequence of CurrencyAmount objects which would address #3.

We could optionally support having an amount member for each PaymentMethod which would address #4

@msporny
Copy link
Member

msporny commented Apr 8, 2016

@adrianhopebailie could you provide some code? I'm finding it hard to parse what your proposal is (I think I get it, but want to be sure).

adrianhopebailie added a commit to adrianhopebailie/browser-payment-api that referenced this issue Apr 10, 2016
Merged PaymentRequest constructor params to allow for options (only
processed by the UA) and details (passed to the payment app).
This commit also proposes solutions to:
w3c#4, w3c#15 and w3c#18
@adrianhopebailie
Copy link
Collaborator

@msporny see PR #133. The solution to #4 is not included but could be easily added.

@adrianhopebailie adrianhopebailie modified the milestones: Priority: High, Priority: Low Apr 10, 2016
@msporny
Copy link
Member

msporny commented Apr 11, 2016

@adrianhopebailie just reviewed the PR - +1 to merge PR #133 (with a few reservations I put into the PR)

@ianbjacobs
Copy link
Collaborator

Here's a proposal:

dictionary PaymentDetails {
sequence items;
sequence shippingOptions;
optional total;
};

And define total in the spec as something like this:

"This field is used to communicate the total amount of the transaction. Processors are not required
to validate that subtotals add up to the total amount."

Then delete this:

"The last PaymentItem in the sequence represents the total amount of the payment request. It is the responsibility of the calling code to ensure that the total amount is the sum of the preceding items. The user agent MAY not validate that this is true. "

(In any case "MAY not" needs to be fixed.)

Ian

@kirkalx
Copy link

kirkalx commented Apr 22, 2016

Am I the only one who has some misgivings about a payments spec that would not enforce that the total amount quoted is the sum of the subtotals? Or will this be enforced elsewhere?

@adrianhopebailie adrianhopebailie modified the milestone: Priority: High Apr 22, 2016
@halindrome
Copy link
Contributor

@kirkalx there isn't anything in there that actually says the "items" need to be comprehensive, so there is no way to enforce the condition even if we wanted to.

@nickjshearer
Copy link

Am I the only one who has some misgivings about a payments spec that would not enforce that the total amount quoted is the sum of the subtotals? Or will this be enforced elsewhere?

There are a bunch of very real use cases where the subtotals won't sum to the total amount you want to actually authorize payment for.

@kirkalx
Copy link

kirkalx commented Apr 25, 2016

Can you give an example please? I struggle to see the point of the subtotals if they can't be tied to the total in an obvious fashion.

@nickjshearer
Copy link

If a merchant doesn't know the final cost of the service they're providing - for example, a ride sharing company may use the sub-totals to indicate the per-mile or per-minute cost of the service, but the total will be the amount to be initially pre-authorized (in such scenarios you also benefit from being able to mark sub-total or total items as pending rather than actual, but that's a matter for another issue). Or, as @halindrome says, where the sub-total list isn't comprehensive.

Putting that aside, if you don't have an explicit total then you're relying on the merchant not to make a mistake in the construction of their sub-totals (or the user-agent to not make a mistake summing them. Neither of these are a given). Having an implicit rather than explicit total introduces several potential failure points for no real benefit. It is much better for the merchant to be explicit about the amount they wish to charge.

@kirkalx
Copy link

kirkalx commented Apr 25, 2016

Perhaps part of the problem is the use of the term 'subtotal', it doesn't seem appropriate if not part of a complete sum. I am also coming from a bitcoin POV, where there is no concept of pre-authorizing (unless overlaid by some intermediary).

@adrianhopebailie
Copy link
Collaborator

@kirkalx The labels on the items in the items list are entirely free.

The issue at hand here is whether the item list should special-case the last item as the implied total which is the case today.

The assertion by most on this thread to date is "No" but there are only two proposals right for how to address this:

  1. @ianbjacobs made a proposal inline to simply add a total member to the details object
  2. I included a fix for this in my PR Merged PaymentRequest params and tweaked to address some issues #133 but this is a much more comprehensive change so thats not all that changes.

adrianba added a commit to adrianba/browser-payment-api that referenced this issue Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants