[WebPayments] Show labels on incomplete profiles

components/payments/core/profile_util.cc

Index: components/payments/core/profile_util.cc
diff --git a/components/payments/core/profile_util.cc b/components/payments/core/profile_util.cc
index c1149c85ed8bffb1d96a792512f3b4d496bc9010..664be1749666f303f836dcfbecfd65ae8132468b 100644
--- a/components/payments/core/profile_util.cc
+++ b/components/payments/core/profile_util.cc
@@ -5,32 +5,89 @@
 #include "components/payments/core/profile_util.h"
 
 #include <algorithm>
+#include <memory>
 
+#include "base/strings/utf_string_conversions.h"
+#include "components/autofill/core/browser/address_i18n.h"
+#include "components/autofill/core/browser/autofill_country.h"
 #include "components/autofill/core/browser/autofill_profile.h"
 #include "components/autofill/core/browser/field_types.h"
+#include "components/autofill/core/browser/validation.h"
 #include "components/payments/core/payment_options_provider.h"
+#include "components/strings/grit/components_strings.h"
+#include "third_party/libaddressinput/chromium/addressinput_util.h"
+#include "third_party/libaddressinput/src/cpp/include/libaddressinput/address_data.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace payments {
-namespace profile_util {
 
-std::vector<autofill::AutofillProfile*> FilterProfilesForContact(
-    const std::vector<autofill::AutofillProfile*>& profiles,
+PaymentsProfileComparator::PaymentsProfileComparator(
     const std::string& app_locale,
-    const PaymentOptionsProvider& options) {
+    const PaymentOptionsProvider& options)
+    : autofill::AutofillProfileComparator(app_locale), options_(options) {}
+
+PaymentsProfileComparator::~PaymentsProfileComparator() {}
+
+PaymentsProfileComparator::ProfileFields
+PaymentsProfileComparator::GetMissingProfileFields(
+    const autofill::AutofillProfile* profile) const {
+  if (!profile)
+    return kName | kPhone | kEmail | kAddress;
+
+  if (!cache_.count(profile->guid())) {
+    ProfileFields missing = 0;

[Mathieu, 2017/04/27 18:34:01]

Could we put lines 38-68 in a private function, and have

if (!cache_.count(profile->guid())) {
  cache_[profile->guid()] = GetMissingProfileFields(profile);
} else {
  // Profile was in cache. In Debug mode, check that invalidation
  // was properly handled.
  DCHECK_EQ(cache_[profile->guid()], GetMissingProfileFields(profile)) <<
"profile was not invalidated properly!";
}

This is still as efficient because the DCHECK won't run for non-devs, but at
least we will catch dev errors who forget to invalidate?

[tmartino, 2017/04/28 17:10:06]

Great idea! Done.

+
+    if (!profile->HasInfo(autofill::NAME_FULL))
+      missing |= kName;
+
+    // Determine the country code to use when validating the phone number. Use
+    // the profile's country if it has one, or the code for the app locale
+    // otherwise. Note that international format numbers will always work--this
+    // is just the region that will be used to check if the number is
+    // potentially in a local format.
+    std::string country =
+        profile->HasInfo(autofill::AutofillType(
+            autofill::HTML_TYPE_COUNTRY_CODE, autofill::HTML_MODE_NONE))

[Mathieu, 2017/04/27 18:34:01]

Would something like this work?
https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...

If not, we should also fix the other entry

[tmartino, 2017/04/28 17:10:06]

Done

+            ? base::UTF16ToUTF8(profile->GetInfo(
+                  autofill::AutofillType(autofill::HTML_TYPE_COUNTRY_CODE,
+                                         autofill::HTML_MODE_NONE),
+                  app_locale_))
+            : autofill::AutofillCountry::CountryCodeForLocale(app_locale_);
+
+    base::string16 phone = profile->GetInfo(
+        autofill::AutofillType(autofill::PHONE_HOME_WHOLE_NUMBER), app_locale_);
+    if (!autofill::IsValidPhoneNumber(phone, country))
+      missing |= kPhone;
+
+    base::string16 email = profile->GetInfo(
+        autofill::AutofillType(autofill::EMAIL_ADDRESS), app_locale_);
+    if (!autofill::IsValidEmailAddress(email))
+      missing |= kEmail;
+
+    if (!AreRequiredAddressFieldsPresent(*profile))
+      missing |= kAddress;
+
+    cache_[profile->guid()] = missing;
+  }
+
+  return cache_[profile->guid()];
+}
+
+std::vector<autofill::AutofillProfile*>
+PaymentsProfileComparator::FilterProfilesForContact(
+    const std::vector<autofill::AutofillProfile*>& profiles) const {
   // We will be removing profiles, so we operate on a copy.
   std::vector<autofill::AutofillProfile*> processed = profiles;
 
-  PaymentsProfileComparator comparator(app_locale, options);
-
   // Stable sort, since profiles are expected to be passed in frecency order.
   std::stable_sort(
       processed.begin(), processed.end(),
-      std::bind(&PaymentsProfileComparator::IsContactMoreComplete, &comparator,
+      std::bind(&PaymentsProfileComparator::IsContactMoreComplete, this,
                 std::placeholders::_1, std::placeholders::_2));
 
   auto it = processed.begin();
   while (it != processed.end()) {
-    if (comparator.GetContactCompletenessScore(*it) == 0) {
+    if (GetContactCompletenessScore(*it) == 0) {
       // Since profiles are sorted by completeness, this and any further
       // profiles can be discarded.
       processed.erase(it, processed.end());
@@ -42,7 +99,7 @@ std::vector<autofill::AutofillProfile*> FilterProfilesForContact(
     // (< 10), so a more complicated algorithm would be overkill.
     if (std::find_if(processed.begin(), it,
                      [&](autofill::AutofillProfile* prior) {
-                       return comparator.IsContactEqualOrSuperset(*prior, **it);
+                       return IsContactEqualOrSuperset(*prior, **it);
                      }) != it) {
       // Remove the subset profile. |it| will point to the next element after
       // erasure.
@@ -55,16 +112,9 @@ std::vector<autofill::AutofillProfile*> FilterProfilesForContact(
   return processed;
 }
 
-PaymentsProfileComparator::PaymentsProfileComparator(
-    const std::string& app_locale,
-    const PaymentOptionsProvider& options)
-    : autofill::AutofillProfileComparator(app_locale), options_(options) {}
-
-PaymentsProfileComparator::~PaymentsProfileComparator() {}
-
 bool PaymentsProfileComparator::IsContactEqualOrSuperset(
     const autofill::AutofillProfile& super,
-    const autofill::AutofillProfile& sub) {
+    const autofill::AutofillProfile& sub) const {
   if (options_.request_payer_name()) {
     if (sub.HasInfo(autofill::NAME_FULL) &&
         !super.HasInfo(autofill::NAME_FULL)) {
@@ -93,31 +143,107 @@ bool PaymentsProfileComparator::IsContactEqualOrSuperset(
 }
 
 int PaymentsProfileComparator::GetContactCompletenessScore(
-    const autofill::AutofillProfile* profile) {
+    const autofill::AutofillProfile* profile) const {
   if (!profile)
     return 0;
 
-  return (options_.request_payer_name() &&
-          profile->HasInfo(autofill::NAME_FULL)) +
-         (options_.request_payer_phone() &&
-          profile->HasInfo(autofill::PHONE_HOME_WHOLE_NUMBER)) +
-         (options_.request_payer_email() &&
-          profile->HasInfo(autofill::EMAIL_ADDRESS));
+  // Create a bitmask of the fields that are both present and required.
+  ProfileFields present =
+      ~GetMissingProfileFields(profile) & GetRequiredProfileFieldsForContact();
+
+  // Count how many are set.
+  return !!(present & kName) + !!(present & kPhone) + !!(present & kEmail);
 }
 
 bool PaymentsProfileComparator::IsContactInfoComplete(
-    const autofill::AutofillProfile* profile) {
-  int desired_score = options_.request_payer_name() +
-                      options_.request_payer_phone() +
-                      options_.request_payer_email();
-  return GetContactCompletenessScore(profile) == desired_score;
+    const autofill::AutofillProfile* profile) const {
+  // int desired_score = options_.request_payer_name() +

[Mathieu, 2017/04/27 18:34:01]

fix?

[tmartino, 2017/04/28 17:10:06]

Done

+  //                     options_.request_payer_phone() +
+  //                     options_.request_payer_email();
+  // return GetContactCompletenessScore(profile) == desired_score;
+  // Mask the fields that are missing with those that are requried. If any bits
+  // are set (i.e., the result is nonzero), then contact info is incomplete.
+  return !(GetMissingProfileFields(profile) &
+           GetRequiredProfileFieldsForContact());
 }
 
 bool PaymentsProfileComparator::IsContactMoreComplete(
     const autofill::AutofillProfile* p1,
-    const autofill::AutofillProfile* p2) {
+    const autofill::AutofillProfile* p2) const {
   return GetContactCompletenessScore(p1) > GetContactCompletenessScore(p2);
 }
 
-}  // namespace profile_util
+base::string16 PaymentsProfileComparator::GetStringForMissingContactFields(
+    const autofill::AutofillProfile& profile) const {
+  return ProfileFieldsToUIString(GetMissingProfileFields(&profile) &
+                                 GetRequiredProfileFieldsForContact());
+}
+
+bool PaymentsProfileComparator::IsShippingComplete(
+    const autofill::AutofillProfile* profile) const {
+  // Mask the fields that are missing with those that are requried. If any bits
+  // are set (i.e., the result is nonzero), then shipping is incomplete.
+  return !(GetMissingProfileFields(profile) &
+           GetRequiredProfileFieldsForShipping());
+}
+
+base::string16 PaymentsProfileComparator::GetStringForMissingShippingFields(
+    const autofill::AutofillProfile& profile) const {
+  return ProfileFieldsToUIString(GetMissingProfileFields(&profile) &
+                                 GetRequiredProfileFieldsForShipping());
+}
+
+void PaymentsProfileComparator::Invalidate(
+    const autofill::AutofillProfile& profile) {
+  cache_.erase(profile.guid());
+}
+
+PaymentsProfileComparator::ProfileFields
+PaymentsProfileComparator::GetRequiredProfileFieldsForContact() const {
+  ProfileFields required = 0;
+  if (options_.request_payer_name())
+    required |= kName;
+  if (options_.request_payer_phone())
+    required |= kPhone;
+  if (options_.request_payer_email())
+    required |= kEmail;
+  return required;
+}
+
+PaymentsProfileComparator::ProfileFields
+PaymentsProfileComparator::GetRequiredProfileFieldsForShipping() const {
+  return kAddress | kName | kPhone;
+}
+
+base::string16 PaymentsProfileComparator::ProfileFieldsToUIString(

[Mathieu, 2017/04/27 18:34:01]

would recommend a slightly more specific name to indicate you are getting the
string to indicate what is missing

[tmartino, 2017/04/28 17:10:06]

OK, done. Just named it GetStringForMissingFields to be consistent with the
methods it's helping.

+    PaymentsProfileComparator::ProfileFields fields) const {
+  switch (fields) {
+    case 0:
+      // No bits are set, so no fields are missing.
+      return base::string16();
+    case kName:
+      return l10n_util::GetStringUTF16(IDS_PAYMENTS_NAME_REQUIRED);
+    case kPhone:
+      return l10n_util::GetStringUTF16(IDS_PAYMENTS_PHONE_NUMBER_REQUIRED);
+    case kEmail:
+      return l10n_util::GetStringUTF16(IDS_PAYMENTS_EMAIL_REQUIRED);
+    case kAddress:
+      return l10n_util::GetStringUTF16(IDS_PAYMENTS_INVALID_ADDRESS);
+    default:
+      // Either multiple bits are set (likely) or one bit that doesn't
+      // correspond to a named constant is set (shouldn't happen). Return a
+      // generic "More information" message.
+      return l10n_util::GetStringUTF16(IDS_PAYMENTS_MORE_INFORMATION_REQUIRED);
+  }
+}
+
+bool PaymentsProfileComparator::AreRequiredAddressFieldsPresent(
+    const autofill::AutofillProfile& profile) const {
+  std::unique_ptr<::i18n::addressinput::AddressData> data =
+      autofill::i18n::CreateAddressDataFromAutofillProfile(profile,
+                                                           app_locale_);
+
+  return autofill::addressinput::HasAllRequiredFields(*data);
+}
+
 }  // namespace payments