Commit 9a33b591 authored by Jinho Bang's avatar Jinho Bang Committed by Commit Bot

PaymentRequest: Make shippingOptions optional in shipping events

Before this patch, when calling updateWith() w/o shippingOptions in
PaymentRequestUpdateEvent, it will be set to empty array by force. This
is a implementation bug and doesn't match behavior with the spec[1].
It cause that calling updateWith() with no shippingOptions resets the
shipping address on payment sheet UI.

To resolve the problem, this patch makes the shippingOptions member
optional. FYI, we already fixed it for payerdetailchange and
paymentmethodchange events[2].

Use count data:
  https://www.chromestatus.com/metrics/feature/timeline/popularity/2622
  https://www.chromestatus.com/metrics/feature/timeline/popularity/2623

[1] https://w3c.github.io/payment-request/#update-a-paymentrequest-s-details-algorithm
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1280563

Bug: 902291
Change-Id: If7b2ba6890ac4905ad1acf79c034fe866ec3beb4
Reviewed-on: https://chromium-review.googlesource.com/c/1438980Reviewed-by: default avatarRouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Jinho Bang <jinho.bang@samsung.com>
Cr-Commit-Position: refs/heads/master@{#628570}
parent 65c08b92
......@@ -2205,6 +2205,7 @@ chrome_test_java_sources = [
"javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShowTwiceTest.java",
"javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTabTest.java",
"javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java",
"javatests/src/org/chromium/chrome/browser/payments/PaymentRequestUpdateWithTest.java",
"javatests/src/org/chromium/chrome/browser/payments/PaymentRequestUseStatsTest.java",
"javatests/src/org/chromium/chrome/browser/permissions/GeolocationTest.java",
"javatests/src/org/chromium/chrome/browser/permissions/MediaTest.java",
......
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.chrome.browser.payments;
import android.support.test.filters.MediumTest;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Feature;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeSwitches;
import org.chromium.chrome.browser.autofill.AutofillTestHelper;
import org.chromium.chrome.browser.autofill.CardType;
import org.chromium.chrome.browser.autofill.PersonalDataManager.AutofillProfile;
import org.chromium.chrome.browser.autofill.PersonalDataManager.CreditCard;
import org.chromium.chrome.browser.payments.PaymentRequestTestRule.MainActivityStartCallback;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.ui.modaldialog.ModalDialogProperties;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeoutException;
/**
* A payment integration test for updateWith().
*/
@RunWith(ChromeJUnit4ClassRunner.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
public class PaymentRequestUpdateWithTest implements MainActivityStartCallback {
@Rule
public PaymentRequestTestRule mRule =
new PaymentRequestTestRule("payment_request_update_with_test.html", this);
@Override
public void onMainActivityStarted()
throws InterruptedException, ExecutionException, TimeoutException {
AutofillTestHelper helper = new AutofillTestHelper();
helper.setProfile(new AutofillProfile("" /* guid */, "https://www.example.com" /* origin */,
"Lisa Simpson", "Acme Inc.", "123 Main", "California", "Los Angeles", "", "90210",
"", "US", "555 123-4567", "lisa@simpson.com", ""));
String billingAddressId = helper.setProfile(
new AutofillProfile("" /* guid */, "https://www.example.com" /* origin */,
"Maggie Simpson", "Acme Inc.", "123 Main", "California", "Los Angeles", "",
"90210", "", "Uzbekistan", "555 123-4567", "maggie@simpson.com", ""));
helper.setCreditCard(new CreditCard("", "https://example.com", true, true, "Jon Doe",
"4111111111111111", "1111", "12", "2050", "visa", R.drawable.visa_card,
CardType.UNKNOWN, billingAddressId, "" /* serverId */));
}
/**
* A merchant that calls updateWith() without shipping options will not cause timeouts in UI.
*/
@Test
@MediumTest
@Feature({"Payments"})
public void testUpdateWithNoShippingOptions() throws Throwable {
mRule.triggerUIAndWait("updateWithNoShippingOptions", mRule.getReadyForInput());
Assert.assertEquals("USD $5.00", mRule.getOrderSummaryTotal());
mRule.clickInShippingAddressAndWait(R.id.payments_section, mRule.getReadyForInput());
mRule.clickOnShippingAddressSuggestionOptionAndWait(1, mRule.getReadyForInput());
Assert.assertEquals("USD $10.00", mRule.getOrderSummaryTotal());
mRule.clickAndWait(R.id.button_primary, mRule.getReadyForUnmaskInput());
mRule.setTextInCardUnmaskDialogAndWait(
R.id.card_unmask_input, "123", mRule.getReadyToUnmask());
mRule.clickCardUnmaskButtonAndWait(
ModalDialogProperties.ButtonType.POSITIVE, mRule.getDismissed());
mRule.expectResultContains(new String[] {"freeShipping"});
}
/** A merchant that calls updateWith() with shipping options will not cause timeouts in UI. */
@Test
@MediumTest
@Feature({"Payments"})
public void testUpdateWithShippingOptions() throws Throwable {
mRule.triggerUIAndWait("updateWithShippingOptions", mRule.getReadyForInput());
Assert.assertEquals("USD $5.00", mRule.getOrderSummaryTotal());
mRule.clickInShippingAddressAndWait(R.id.payments_section, mRule.getReadyForInput());
mRule.clickOnShippingAddressSuggestionOptionAndWait(1, mRule.getReadyForInput());
Assert.assertEquals("USD $10.00", mRule.getOrderSummaryTotal());
mRule.clickAndWait(R.id.button_primary, mRule.getReadyForUnmaskInput());
mRule.setTextInCardUnmaskDialogAndWait(
R.id.card_unmask_input, "123", mRule.getReadyToUnmask());
mRule.clickCardUnmaskButtonAndWait(
ModalDialogProperties.ButtonType.POSITIVE, mRule.getDismissed());
mRule.expectResultContains(new String[] {"updatedShipping"});
}
}
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/ui/views/payments/payment_request_browsertest_base.h"
#include "chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h"
#include "components/autofill/core/browser/autofill_profile.h"
#include "components/autofill/core/browser/autofill_test_utils.h"
#include "content/public/test/browser_test_utils.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace payments {
class PaymentRequestUpdateWithTest : public PaymentRequestBrowserTestBase {
protected:
PaymentRequestUpdateWithTest() {}
void RunJavaScriptFunctionToOpenPaymentRequestUI(
const std::string& function_name) {
ResetEventWaiterForDialogOpened();
content::WebContents* web_contents = GetActiveWebContents();
ASSERT_TRUE(content::ExecuteScript(web_contents, function_name + "();"));
WaitForObservedEvent();
}
private:
DISALLOW_COPY_AND_ASSIGN(PaymentRequestUpdateWithTest);
};
IN_PROC_BROWSER_TEST_F(PaymentRequestUpdateWithTest,
UpdateWithNoShippingOptions) {
NavigateTo("/payment_request_update_with_test.html");
autofill::AutofillProfile billing_address = autofill::test::GetFullProfile();
AddAutofillProfile(billing_address);
AddAutofillProfile(autofill::test::GetFullProfile2());
autofill::CreditCard card = autofill::test::GetCreditCard();
card.set_billing_address_id(billing_address.guid());
AddCreditCard(card);
RunJavaScriptFunctionToOpenPaymentRequestUI("updateWithNoShippingOptions");
OpenOrderSummaryScreen();
EXPECT_EQ(base::ASCIIToUTF16("$5.00"),
GetLabelText(DialogViewID::ORDER_SUMMARY_TOTAL_AMOUNT_LABEL));
ClickOnBackArrow();
OpenShippingAddressSectionScreen();
ResetEventWaiterForSequence({DialogEvent::PROCESSING_SPINNER_SHOWN,
DialogEvent::PROCESSING_SPINNER_HIDDEN,
DialogEvent::SPEC_DONE_UPDATING,
DialogEvent::BACK_NAVIGATION});
ClickOnChildInListViewAndWait(
/* child_index=*/1, /*total_num_children=*/2,
DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW,
/*wait_for_animation=*/false);
// Wait for the animation here explicitly, otherwise
// ClickOnChildInListViewAndWait tries to install an AnimationDelegate before
// the animation is kicked off (since that's triggered off of the spec being
// updated) and this hits a DCHECK.
WaitForAnimation();
OpenOrderSummaryScreen();
EXPECT_EQ(base::ASCIIToUTF16("$10.00"),
GetLabelText(DialogViewID::ORDER_SUMMARY_TOTAL_AMOUNT_LABEL));
ClickOnBackArrow();
PayWithCreditCardAndWait(base::ASCIIToUTF16("123"));
ExpectBodyContains({"freeShipping"});
}
IN_PROC_BROWSER_TEST_F(PaymentRequestUpdateWithTest,
UpdateWithShippingOptions) {
NavigateTo("/payment_request_update_with_test.html");
autofill::AutofillProfile billing_address = autofill::test::GetFullProfile();
AddAutofillProfile(billing_address);
AddAutofillProfile(autofill::test::GetFullProfile2());
autofill::CreditCard card = autofill::test::GetCreditCard();
card.set_billing_address_id(billing_address.guid());
AddCreditCard(card);
RunJavaScriptFunctionToOpenPaymentRequestUI("updateWithShippingOptions");
OpenOrderSummaryScreen();
EXPECT_EQ(base::ASCIIToUTF16("$5.00"),
GetLabelText(DialogViewID::ORDER_SUMMARY_TOTAL_AMOUNT_LABEL));
ClickOnBackArrow();
OpenShippingAddressSectionScreen();
ResetEventWaiterForSequence({DialogEvent::PROCESSING_SPINNER_SHOWN,
DialogEvent::PROCESSING_SPINNER_HIDDEN,
DialogEvent::SPEC_DONE_UPDATING,
DialogEvent::BACK_NAVIGATION});
ClickOnChildInListViewAndWait(
/* child_index=*/1, /*total_num_children=*/2,
DialogViewID::SHIPPING_ADDRESS_SHEET_LIST_VIEW,
/*wait_for_animation=*/false);
// Wait for the animation here explicitly, otherwise
// ClickOnChildInListViewAndWait tries to install an AnimationDelegate before
// the animation is kicked off (since that's triggered off of the spec being
// updated) and this hits a DCHECK.
WaitForAnimation();
OpenOrderSummaryScreen();
EXPECT_EQ(base::ASCIIToUTF16("$10.00"),
GetLabelText(DialogViewID::ORDER_SUMMARY_TOTAL_AMOUNT_LABEL));
ClickOnBackArrow();
PayWithCreditCardAndWait(base::ASCIIToUTF16("123"));
ExpectBodyContains({"updatedShipping"});
}
} // namespace payments
......@@ -1641,6 +1641,7 @@ test("browser_tests") {
"../browser/ui/views/payments/payment_request_payment_app_browsertest.cc",
"../browser/ui/views/payments/payment_request_payment_response_browsertest.cc",
"../browser/ui/views/payments/payment_request_shipping_address_instance_browsertest.cc",
"../browser/ui/views/payments/payment_request_update_with_browsertest.cc",
"../browser/ui/views/payments/payment_request_use_stats_browsertest.cc",
"../browser/ui/views/payments/payment_sheet_view_controller_browsertest.cc",
"../browser/ui/views/payments/profile_list_view_controller_browsertest.cc",
......
<!DOCTYPE html>
<!--
Copyright 2019 The Chromium Authors. All rights reserved.
Use of this source code is governed by a BSD-style license that can be
found in the LICENSE file.
-->
<html>
<head>
<title>updateWith() Test</title>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1">
<link rel="stylesheet" type="text/css" href="style.css">
</head>
<body>
<button onclick="updateWithNoShippingOptions()" id="updateWithNoShippingOptions">updateWithNoShippingOptions</button>
<button onclick="updateWithShippingOptions()" id="updateWithShippingOptions">updateWithShippingOptions</button>
<pre id="result"></pre>
<script src="util.js"></script>
<script src="update_with.js"></script>
</body>
</html>
/*
* Copyright 2019 The Chromium Authors. All rights reserved.
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
*/
/**
* Builds a PaymentRequest that requests a shipping address.
* @return {PaymentRequest} - A new PaymentRequest object.
*/
function buildPaymentRequest() {
try {
return new PaymentRequest(
[{supportedMethods: 'basic-card'}], {
total: {label: 'Total', amount: {currency: 'USD', value: '5.00'}},
shippingOptions: [{
selected: true,
id: 'freeShipping',
label: 'Free shipping',
amount: {currency: 'USD', value: '0.00'},
}],
},
{requestShipping: true});
} catch (error) {
print(error.message);
}
}
/**
* Shows the PaymentRequest.
* @param {PaymentRequest} pr - The PaymentRequest object to show.
*/
function showPaymentRequest(pr) {
pr.show()
.then(function(resp) {
resp.complete('success')
.then(function() {
print(JSON.stringify(resp, undefined, 2));
})
.catch(function(error) {
print(error);
});
})
.catch(function(error) {
print(error);
});
}
/**
* Calls updateWith() with no shipping options
*/
function updateWithNoShippingOptions() { // eslint-disable-line no-unused-vars
var pr = buildPaymentRequest();
var updatedDetails = {
total: {label: 'Updated total', amount: {currency: 'USD', value: '10.00'}},
};
pr.addEventListener('shippingaddresschange', function(e) {
e.updateWith(updatedDetails);
});
pr.addEventListener('shippingoptionchange', function(e) {
e.updateWith(updatedDetails);
});
showPaymentRequest(pr);
}
/**
* Calls updateWith() with shipping options
*/
function updateWithShippingOptions() { // eslint-disable-line no-unused-vars
var pr = buildPaymentRequest();
var updatedDetails = {
total: {label: 'Updated total', amount: {currency: 'USD', value: '10.00'}},
shippingOptions: [{
selected: true,
id: 'updatedShipping',
label: 'Updated shipping',
amount: {currency: 'USD', value: '5.00'},
}],
};
pr.addEventListener('shippingaddresschange', function(e) {
e.updateWith(updatedDetails);
});
pr.addEventListener('shippingoptionchange', function(e) {
e.updateWith(updatedDetails);
});
showPaymentRequest(pr);
}
......@@ -1046,7 +1046,6 @@ ScriptPromise PaymentRequest::Complete(ScriptState* script_state,
}
void PaymentRequest::OnUpdatePaymentDetails(
const AtomicString& event_type,
const ScriptValue& details_script_value) {
if (!GetPendingAcceptPromiseResolver() || !payment_provider_)
return;
......@@ -1083,22 +1082,6 @@ void PaymentRequest::OnUpdatePaymentDetails(
return;
}
// TODO(https://crbug.com/902291): We should make shippingOptions optional.
if (options_->requestShipping() && !details->hasShippingOptions()) {
if (event_type == event_type_names::kShippingaddresschange) {
UseCounter::Count(
GetExecutionContext(),
WebFeature::kUpdateWithoutShippingOptionOnShippingAddressChange);
validated_details->shipping_options = Vector<PaymentShippingOptionPtr>();
}
if (event_type == event_type_names::kShippingoptionchange) {
UseCounter::Count(
GetExecutionContext(),
WebFeature::kUpdateWithoutShippingOptionOnShippingOptionChange);
validated_details->shipping_options = Vector<PaymentShippingOptionPtr>();
}
}
if (!options_->requestShipping())
validated_details->shipping_options = base::nullopt;
......
......@@ -94,8 +94,7 @@ class MODULES_EXPORT PaymentRequest final
ScriptPromise Retry(ScriptState*, const PaymentValidationErrors*) override;
// PaymentUpdater:
void OnUpdatePaymentDetails(const AtomicString& event_type,
const ScriptValue& details_script_value) override;
void OnUpdatePaymentDetails(const ScriptValue& details_script_value) override;
void OnUpdatePaymentDetailsFailure(const String& error) override;
void Trace(blink::Visitor*) override;
......
......@@ -390,7 +390,6 @@ TEST(PaymentRequestTest, IgnoreUpdatePaymentDetailsAfterShowPromiseResolved) {
->OnPaymentResponse(BuildPaymentResponseForTest());
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(scope.GetScriptState(), "foo"));
}
......@@ -407,7 +406,6 @@ TEST(PaymentRequestTest, RejectShowPromiseOnNonPaymentDetailsUpdate) {
.Then(funcs.ExpectNoCall(), funcs.ExpectCall());
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(scope.GetScriptState(), "NotPaymentDetails"));
}
......@@ -424,7 +422,6 @@ TEST(PaymentRequestTest, RejectShowPromiseOnInvalidPaymentDetailsUpdate) {
.Then(funcs.ExpectNoCall(), funcs.ExpectCall());
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(
scope.GetScriptState(),
FromJSONString(scope.GetScriptState()->GetIsolate(),
......@@ -456,7 +453,6 @@ TEST(PaymentRequestTest,
"\"Standard shipping\", \"amount\": {\"currency\": \"USD\", \"value\": "
"\"5.00\"}, \"selected\": true}]}";
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(scope.GetScriptState(),
FromJSONString(scope.GetScriptState()->GetIsolate(),
scope.GetScriptState()->GetContext(),
......@@ -469,7 +465,6 @@ TEST(PaymentRequestTest,
"\"value\": \"5.00\"}}}";
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(scope.GetScriptState(),
FromJSONString(scope.GetScriptState()->GetIsolate(),
scope.GetScriptState()->GetContext(),
......@@ -503,7 +498,6 @@ TEST(
"\"USD\", \"value\": \"50.00\"}}]}";
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(scope.GetScriptState(),
FromJSONString(scope.GetScriptState()->GetIsolate(),
scope.GetScriptState()->GetContext(),
......@@ -534,7 +528,6 @@ TEST(PaymentRequestTest, UseTheSelectedShippingOptionFromPaymentDetailsUpdate) {
"\"USD\", \"value\": \"50.00\"}, \"selected\": true}]}";
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(scope.GetScriptState(),
FromJSONString(scope.GetScriptState()->GetIsolate(),
scope.GetScriptState()->GetContext(),
......@@ -561,7 +554,6 @@ TEST(PaymentRequestTest, NoExceptionWithErrorMessageInUpdate) {
"\"error\": \"This is an error message.\"}";
request->OnUpdatePaymentDetails(
event_type_names::kShippingaddresschange,
ScriptValue::From(
scope.GetScriptState(),
FromJSONString(scope.GetScriptState()->GetIsolate(),
......
......@@ -45,7 +45,7 @@ class UpdatePaymentDetailsFunction : public ScriptFunction {
private:
ScriptValue Call(ScriptValue value) override {
update_event_->OnUpdatePaymentDetails(update_event_->type(), value);
update_event_->OnUpdatePaymentDetails(value);
return ScriptValue();
}
......@@ -134,12 +134,11 @@ void PaymentRequestUpdateEvent::updateWith(ScriptState* script_state,
}
void PaymentRequestUpdateEvent::OnUpdatePaymentDetails(
const AtomicString& event_type,
const ScriptValue& details_script_value) {
if (!updater_)
return;
abort_timer_.Stop();
updater_->OnUpdatePaymentDetails(event_type, details_script_value);
updater_->OnUpdatePaymentDetails(details_script_value);
updater_ = nullptr;
}
......
......@@ -44,8 +44,7 @@ class MODULES_EXPORT PaymentRequestUpdateEvent : public Event,
bool is_waiting_for_update() const { return wait_for_update_; }
// PaymentUpdater:
void OnUpdatePaymentDetails(const AtomicString& event_type,
const ScriptValue& details_script_value) override;
void OnUpdatePaymentDetails(const ScriptValue& details_script_value) override;
void OnUpdatePaymentDetailsFailure(const String& error) override;
void Trace(blink::Visitor*) override;
......
......@@ -28,9 +28,8 @@ class MockPaymentUpdater : public GarbageCollectedFinalized<MockPaymentUpdater>,
MockPaymentUpdater() = default;
~MockPaymentUpdater() override = default;
MOCK_METHOD2(OnUpdatePaymentDetails,
void(const AtomicString& event_type,
const ScriptValue& detailsScriptValue));
MOCK_METHOD1(OnUpdatePaymentDetails,
void(const ScriptValue& detailsScriptValue));
MOCK_METHOD1(OnUpdatePaymentDetailsFailure, void(const String& error));
void Trace(blink::Visitor* visitor) override {}
......@@ -50,9 +49,7 @@ TEST(PaymentRequestUpdateEventTest, OnUpdatePaymentDetailsCalled) {
scope.GetExceptionState());
EXPECT_FALSE(scope.GetExceptionState().HadException());
EXPECT_CALL(*updater,
OnUpdatePaymentDetails(event_type_names::kShippingaddresschange,
testing::_));
EXPECT_CALL(*updater, OnUpdatePaymentDetails(testing::_));
EXPECT_CALL(*updater, OnUpdatePaymentDetailsFailure(testing::_)).Times(0);
payment_details->Resolve("foo");
......@@ -72,10 +69,7 @@ TEST(PaymentRequestUpdateEventTest, OnUpdatePaymentDetailsFailureCalled) {
scope.GetExceptionState());
EXPECT_FALSE(scope.GetExceptionState().HadException());
EXPECT_CALL(*updater,
OnUpdatePaymentDetails(event_type_names::kShippingaddresschange,
testing::_))
.Times(0);
EXPECT_CALL(*updater, OnUpdatePaymentDetails(testing::_)).Times(0);
EXPECT_CALL(*updater, OnUpdatePaymentDetailsFailure(testing::_));
payment_details->Reject("oops");
......
......@@ -16,7 +16,6 @@ class ScriptValue;
class MODULES_EXPORT PaymentUpdater : public GarbageCollectedMixin {
public:
virtual void OnUpdatePaymentDetails(
const AtomicString& event_type,
const ScriptValue& details_script_value) = 0;
virtual void OnUpdatePaymentDetailsFailure(const String& error) = 0;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment