Set shippingOption during construction only if options.requestShipping is true.

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: edenchuang, Assigned: edenchuang)

Tracking

({dev-doc-complete})

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

According to the spec change 

https://github.com/w3c/payment-request/issues/609

shippingOption should always be null when options.requestShipping is false, even there is a shippingOption is set as selected.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
This patch updates the PaymentRequest.shippingOption setting implementation according to the spec changes.

1. Return TypeError when creating PaymentRequest with duplicate shippingOption id.
2. Setting the default selected shippingOption only if PaymentOptions.requestShipping is true, otherwise, set the default shippingOption with null.
Attachment #8908527 - Flags: review?(amarchesini)
This patch updates the tests related to PaymentRequest.shippingOption setting. 
It changes the test_construction.html with following modifications.
   
    1. Modify the test testWithDuplicateShippingOptionsParameters to expect a
       TypeError when constructing with duplicate shippingOption ids.
    2. Add test testShippingOtpionAttribute for shippingOption setting checking
       with following conditions
       1. No selected shippingOption and PaymentOptions.requestShipping is false
       2. One selected shippingOption and PaymentOptions.requestShipping is false
       3. One selected shippingOption and PaymentOptions.requestShipping is true
       4. Multiple selected shippingOptions and PaymentOptions.requestShipping is
          true.
Attachment #8908531 - Flags: review?(amarchesini)
Attachment #8908527 - Flags: review?(amarchesini) → review+
Attachment #8908531 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/080349a1a621
https://hg.mozilla.org/mozilla-central/rev/26289fa4d71a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
The 

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/shippingOption

page already describes this behaviour (I edited it a little bit), so I've marked this as ddc. Let me know if you think the description is ok!
Thanks for the documentation, I am ok with the description. 

Marcos, how do you think?
Flags: needinfo?(mcaceres)
I replaced the example with the one from the spec, as it was not using promises correctly and did some questionable things (sorry, Chris!). 

cmills - few critical things: avoid calling `reject()` with text, always use new Error("message")... otherwise, the developer doesn't get a stack trace if things go bad. 

Also, avoid ever passing around resolve()/reject() to another function. That should never ever ever ever be necessary - if that happens, then that means there is a serious bug in the code. 

Remember all also that APIs methods that expect a promise (e.g., `updateWith()`) will always "auto-wrap" things in promises, so these are all equivalent:

```JS
// just an object - auto-wrap
ev.updateWith({foo: "bar"});

// resolving but sync. 
ev.updateWith(new Promise(resolve => resolve({foo: "bar"}));

// calling with a resolved promise. 
ev.updateWith(Promise.resolve({foo: "bar"}));

```

Generally, it's best to now avoid using Promises entirely and just use async/await - treat `.then()` or `.catch()` as a code smell.
Flags: needinfo?(mcaceres)
You need to log in before you can comment on or make changes to this bug.