Skip to content

Commit

Permalink
[payments] Sort payment instruments when multiple exists.
Browse files Browse the repository at this point in the history 
This cl is a follow up for the following cl and includes the listed changes:
https://chromium-review.googlesource.com/c/chromium/src/+/1675343

1- Expired cards are recorded in UMA missingPaymentFields metrics even though
they are considered as complete.
2- Cards with incomplete billing addresses are not considered as complete
for payment.
3- Cases where the saved card does not exactly match the specified card in
payment request are recorded in UMA missingPaymentFields as well.
4- Completeness score is added for autofill payment instruments.
5- CanPreselect is added for SW payment instruments.
6- Payment instruments are sorted on desktop when there are multiple available.

TBR=parastoog@chromium.org

Bug: 978542
Change-Id: I64c3430d3e591fa0bb3bb60f14a6a9972204b32d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1688309
Reviewed-by: Sahel Sharify <sahel@chromium.org>
Reviewed-by: Parastoo Geranmayeh <parastoog@google.com>
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Sahel Sharify <sahel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676679}
  • Loading branch information
Sahel Sharify authored and Commit Bot committed Jul 12, 2019
1 parent 1efb5da commit 13331e8
Show file tree
Hide file tree
Showing 24 changed files with 661 additions and 199 deletions.
12 changes: 6 additions & 6 deletions chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "components/autofill/core/common/autofill_constants.h"
#include "components/payments/content/payment_request_spec.h"
#include "components/payments/content/payment_request_state.h"
#include "components/payments/core/autofill_card_validation.h"
#include "components/payments/core/payment_request_data_util.h"
#include "components/payments/core/strings_util.h"
#include "components/strings/grit/components_strings.h"
Expand Down Expand Up @@ -450,10 +451,10 @@ bool CreditCardEditorViewController::ValidateModelAndSave() {
}

// TODO(crbug.com/711365): Display global error message.
if (autofill::GetCompletionStatusForCard(
if (GetCompletionStatusForCard(
credit_card, locale,
state()->GetPersonalDataManager()->GetProfiles()) !=
autofill::CREDIT_CARD_COMPLETE) {
CREDIT_CARD_COMPLETE) {
return false;
}

Expand Down Expand Up @@ -597,10 +598,9 @@ base::string16 CreditCardEditorViewController::GetSheetTitle() {
return l10n_util::GetStringUTF16(IDS_PAYMENTS_ADD_CARD);

// Gets the completion message, or empty if nothing is missing from the card.
base::string16 title = autofill::GetCompletionMessageForCard(
autofill::GetCompletionStatusForCard(
*credit_card_to_edit_, state()->GetApplicationLocale(),
state()->GetPersonalDataManager()->GetProfiles()));
base::string16 title = GetCompletionMessageForCard(GetCompletionStatusForCard(
*credit_card_to_edit_, state()->GetApplicationLocale(),
state()->GetPersonalDataManager()->GetProfiles()));
return title.empty() ? l10n_util::GetStringUTF16(IDS_PAYMENTS_EDIT_CARD)
: title;
}
Expand Down
74 changes: 57 additions & 17 deletions chrome/browser/ui/views/payments/payment_request_missing_fields_metrics_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "components/autofill/core/browser/data_model/autofill_profile.h"
#include "components/autofill/core/browser/data_model/credit_card.h"
#include "components/autofill/core/browser/validation.h"
#include "components/payments/core/autofill_card_validation.h"
#include "components/payments/core/journey_logger.h"
#include "components/payments/core/payments_profile_comparator.h"

Expand Down Expand Up @@ -44,9 +45,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
// Since no card is added to the profile, all payment fields should be
// missing.
int32_t expected_missing_payment_bits =
autofill::CREDIT_CARD_EXPIRED | autofill::CREDIT_CARD_NO_CARDHOLDER |
autofill::CREDIT_CARD_NO_NUMBER |
autofill::CREDIT_CARD_NO_BILLING_ADDRESS;
CREDIT_CARD_EXPIRED | CREDIT_CARD_NO_CARDHOLDER | CREDIT_CARD_NO_NUMBER |
CREDIT_CARD_NO_BILLING_ADDRESS;
histogram_tester.ExpectBucketCount("PaymentRequest.MissingPaymentFields",
expected_missing_payment_bits, 1);

Expand Down Expand Up @@ -88,9 +88,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
histogram_tester.ExpectBucketCount("PaymentRequest.Events",
expected_event_bits, 1);

int32_t expected_missing_payment_bits = autofill::CREDIT_CARD_NO_CARDHOLDER;
histogram_tester.ExpectBucketCount("PaymentRequest.MissingPaymentFields",
expected_missing_payment_bits, 1);
CREDIT_CARD_NO_CARDHOLDER, 1);

// There should be no log for missing shipping address fields since the
// section had one complete suggestion.
Expand Down Expand Up @@ -129,9 +128,10 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
histogram_tester.ExpectBucketCount("PaymentRequest.Events",
expected_event_bits, 1);

// Expired cards are considered as complete according to
// AutofillPaymentInstrument::IsCompleteForPayment().
histogram_tester.ExpectTotalCount("PaymentRequest.MissingPaymentFields", 0);
// Even though expired cards are treated as complete, we still record the
// MissingPaymentFields with expired bit set.
histogram_tester.ExpectBucketCount("PaymentRequest.MissingPaymentFields",
CREDIT_CARD_EXPIRED, 1);

// There should be no log for missing shipping address fields since the
// section had one complete suggestion.
Expand Down Expand Up @@ -159,12 +159,14 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
// Navigate away to abort the Payment Request and trigger the logs.
NavigateTo("/payment_request_email_test.html");

// Make sure the correct events were logged.
// Make sure the correct events were logged. EVENT_NEEDS_COMPLETION_PAYMENT is
// set since billing address of the card is incomplete.
int32_t expected_event_bits =
JourneyLogger::EVENT_SHOWN | JourneyLogger::EVENT_USER_ABORTED |
JourneyLogger::EVENT_HAD_INITIAL_FORM_OF_PAYMENT |
JourneyLogger::EVENT_REQUEST_SHIPPING |
JourneyLogger::EVENT_REQUEST_METHOD_BASIC_CARD |
JourneyLogger::EVENT_NEEDS_COMPLETION_PAYMENT |
JourneyLogger::EVENT_NEEDS_COMPLETION_SHIPPING;
histogram_tester.ExpectBucketCount("PaymentRequest.Events",
expected_event_bits, 1);
Expand All @@ -177,9 +179,9 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
histogram_tester.ExpectBucketCount("PaymentRequest.MissingShippingFields",
expected_missing_shipping_bits, 1);

// There should be no log for missing payment fields since the section had one
// complete suggestion.
histogram_tester.ExpectTotalCount("PaymentRequest.MissingPaymentFields", 0);
// Ensure that the billing address of the card is incomplete.
histogram_tester.ExpectBucketCount("PaymentRequest.MissingPaymentFields",
CREDIT_CARD_NO_BILLING_ADDRESS, 1);
}

// Tests that proper UMA metrics are logged when contacts section is incomplete.
Expand All @@ -203,7 +205,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
// Navigate away to abort the Payment Request and trigger the logs.
NavigateTo("/payment_request_email_test.html");

// Make sure the correct events were logged.
// Make sure the correct events were logged. EVENT_NEEDS_COMPLETION_PAYMENT is
// set since billing address of the card is incomplete.
int32_t expected_event_bits =
JourneyLogger::EVENT_SHOWN | JourneyLogger::EVENT_USER_ABORTED |
JourneyLogger::EVENT_HAD_INITIAL_FORM_OF_PAYMENT |
Expand All @@ -212,7 +215,8 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
JourneyLogger::EVENT_REQUEST_PAYER_PHONE |
JourneyLogger::EVENT_REQUEST_METHOD_BASIC_CARD |
JourneyLogger::EVENT_REQUEST_METHOD_OTHER |
JourneyLogger::EVENT_NEEDS_COMPLETION_CONTACT_INFO;
JourneyLogger::EVENT_NEEDS_COMPLETION_CONTACT_INFO |
JourneyLogger::EVENT_NEEDS_COMPLETION_PAYMENT;
histogram_tester.ExpectBucketCount("PaymentRequest.Events",
expected_event_bits, 1);

Expand All @@ -223,13 +227,49 @@ IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
histogram_tester.ExpectBucketCount("PaymentRequest.MissingContactFields",
expected_missing_contact_bits, 1);

// There should be no log for missing payment fields since the section had one
// complete suggestion.
histogram_tester.ExpectTotalCount("PaymentRequest.MissingPaymentFields", 0);
// Ensure that the billing address of the card is incomplete.
histogram_tester.ExpectBucketCount("PaymentRequest.MissingPaymentFields",
CREDIT_CARD_NO_BILLING_ADDRESS, 1);

// Even though the profile is incomplete, there should be no log for missing
// shipping fields since shipping was not required.
histogram_tester.ExpectTotalCount("PaymentRequest.MissingShippingFields", 0);
}

// Tests that proper UMA metrics are logged when the available card type does
// not exactly match the specified type in payment request.
IN_PROC_BROWSER_TEST_F(PaymentRequestMissingFieldsMetricsTest,
TestCardWithMismatchedType) {
NavigateTo("/payment_request_debit_test.html");
base::HistogramTester histogram_tester;
// Add a profile for billing address with a visa card type.
autofill::AutofillProfile billing_address = autofill::test::GetFullProfile();
AddAutofillProfile(billing_address);
autofill::CreditCard visa_card = autofill::test::GetCreditCard2();
visa_card.set_billing_address_id(billing_address.guid());
AddCreditCard(visa_card);

// Show a Payment Request with debit card specified.
InvokePaymentRequestUI();

// Navigate away to abort the Payment Request and trigger the logs.
NavigateTo("/payment_request_email_test.html");

// Make sure the correct events were logged. EVENT_NEEDS_COMPLETION_PAYMENT is
// set since the type of the saved card does not match the specified type in
// payment request.
int32_t expected_event_bits =
JourneyLogger::EVENT_SHOWN | JourneyLogger::EVENT_USER_ABORTED |
JourneyLogger::EVENT_HAD_INITIAL_FORM_OF_PAYMENT |
JourneyLogger::EVENT_REQUEST_METHOD_BASIC_CARD |
JourneyLogger::EVENT_NEEDS_COMPLETION_PAYMENT;
histogram_tester.ExpectBucketCount("PaymentRequest.Events",
expected_event_bits, 1);

// Ensure that the card type does not exactly match the payment request
// network type.
histogram_tester.ExpectBucketCount("PaymentRequest.MissingPaymentFields",
CREDIT_CARD_TYPE_MISMATCH, 1);
}

} // namespace payments
65 changes: 0 additions & 65 deletions components/autofill/core/browser/validation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "components/autofill/core/browser/data_model/credit_card.h"
#include "components/autofill/core/browser/geo/phone_number_i18n.h"
#include "components/autofill/core/browser/geo/state_names.h"
#include "components/autofill/core/browser/personal_data_manager.h"
#include "components/autofill/core/common/autofill_clock.h"
#include "components/autofill/core/common/autofill_regex_constants.h"
#include "components/autofill/core/common/autofill_regexes.h"
Expand Down Expand Up @@ -147,70 +146,6 @@ bool IsValidCreditCardNumberForBasicCardNetworks(
return false;
}

CreditCardCompletionStatus GetCompletionStatusForCard(
const CreditCard& card,
const std::string& app_locale,
const std::vector<AutofillProfile*> billing_addresses) {
CreditCardCompletionStatus status = CREDIT_CARD_COMPLETE;
if (card.IsExpired(autofill::AutofillClock::Now()))
status |= CREDIT_CARD_EXPIRED;

if (card.number().empty())
status |= CREDIT_CARD_NO_NUMBER;

if (card.GetInfo(autofill::AutofillType(autofill::CREDIT_CARD_NAME_FULL),
app_locale)
.empty()) {
status |= CREDIT_CARD_NO_CARDHOLDER;
}

if (card.billing_address_id().empty() ||
!autofill::PersonalDataManager::GetProfileFromProfilesByGUID(
card.billing_address_id(), billing_addresses)) {
status |= CREDIT_CARD_NO_BILLING_ADDRESS;
}

return status;
}

base::string16 GetCompletionMessageForCard(CreditCardCompletionStatus status) {
switch (status) {
// No message is shown for complete or expired card (which will be fixable)
// in the CVC screen.
case CREDIT_CARD_COMPLETE:
case CREDIT_CARD_EXPIRED:
return base::string16();
case CREDIT_CARD_NO_CARDHOLDER:
return l10n_util::GetStringUTF16(IDS_PAYMENTS_NAME_ON_CARD_REQUIRED);
case CREDIT_CARD_NO_NUMBER:
return l10n_util::GetStringUTF16(
IDS_PAYMENTS_CARD_NUMBER_INVALID_VALIDATION_MESSAGE);
case CREDIT_CARD_NO_BILLING_ADDRESS:
return l10n_util::GetStringUTF16(
IDS_PAYMENTS_CARD_BILLING_ADDRESS_REQUIRED);
default:
// Multiple things are missing
return l10n_util::GetStringUTF16(IDS_PAYMENTS_MORE_INFORMATION_REQUIRED);
}
}

base::string16 GetEditDialogTitleForCard(CreditCardCompletionStatus status) {
switch (status) {
case CREDIT_CARD_COMPLETE:
case CREDIT_CARD_EXPIRED:
return l10n_util::GetStringUTF16(IDS_PAYMENTS_EDIT_CARD);
case CREDIT_CARD_NO_CARDHOLDER:
return l10n_util::GetStringUTF16(IDS_PAYMENTS_ADD_NAME_ON_CARD);
case CREDIT_CARD_NO_NUMBER:
return l10n_util::GetStringUTF16(IDS_PAYMENTS_ADD_VALID_CARD_NUMBER);
case CREDIT_CARD_NO_BILLING_ADDRESS:
return l10n_util::GetStringUTF16(IDS_PAYMENTS_ADD_BILLING_ADDRESS);
default:
// Multiple things are missing
return l10n_util::GetStringUTF16(IDS_PAYMENTS_ADD_MORE_INFORMATION);
}
}

bool IsValidEmailAddress(const base::string16& text) {
// E-Mail pattern as defined by the WhatWG. (4.10.7.1.5 E-Mail state)
const base::string16 kEmailPattern = base::ASCIIToUTF16(
Expand Down
28 changes: 0 additions & 28 deletions components/autofill/core/browser/validation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,10 @@ class Time;

namespace autofill {

class CreditCard;
class AutofillProfile;

// Constants for the length of a CVC.
static const size_t GENERAL_CVC_LENGTH = 3;
static const size_t AMEX_CVC_LENGTH = 4;

// Used to express the completion status of a credit card.
typedef uint32_t CreditCardCompletionStatus;
static const CreditCardCompletionStatus CREDIT_CARD_COMPLETE = 0;
static const CreditCardCompletionStatus CREDIT_CARD_EXPIRED = 1 << 0;
static const CreditCardCompletionStatus CREDIT_CARD_NO_CARDHOLDER = 1 << 1;
static const CreditCardCompletionStatus CREDIT_CARD_NO_NUMBER = 1 << 2;
static const CreditCardCompletionStatus CREDIT_CARD_NO_BILLING_ADDRESS = 1 << 3;

// Returns true if |year| and |month| describe a date later than |now|.
// |year| must have 4 digits.
bool IsValidCreditCardExpirationDate(int year,
Expand Down Expand Up @@ -67,23 +56,6 @@ bool IsValidCreditCardNumberForBasicCardNetworks(
const std::set<std::string>& supported_basic_card_networks,
base::string16* error_message);

// Returns the credit card's completion status. If equal to
// CREDIT_CARD_COMPLETE, then the card is ready to be used for Payment Request.
CreditCardCompletionStatus GetCompletionStatusForCard(
const CreditCard& credit_card,
const std::string& app_locale,
const std::vector<AutofillProfile*> billing_addresses);

// Return the message to be displayed to the user, indicating what's missing
// to make the credit card complete for payment. If more than one thing is
// missing, the message will be a generic "more information required".
base::string16 GetCompletionMessageForCard(CreditCardCompletionStatus status);

// Returns the title string for a card edit dialog. The title string will
// mention what needs to be added/fixed to make the card valid if it is not
// valid. Otherwise, it will be "Edit card".
base::string16 GetEditDialogTitleForCard(CreditCardCompletionStatus status);

// Returns true if |text| looks like a valid e-mail address.
bool IsValidEmailAddress(const base::string16& text);

Expand Down
1 change: 1 addition & 0 deletions components/payments/content/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ jumbo_source_set("unit_tests") {

if (!is_android) {
sources += [
"payment_instrument_unittest.cc",
"payment_request_spec_unittest.cc",
"payment_request_state_unittest.cc",
"payment_response_helper_unittest.cc",
Expand Down
Loading

0 comments on commit 13331e8

Please sign in to comment.