[Payments] Show what's missing for incomplete payment methods.

[Payments] Show what's missing for incomplete payment methods.

Refactored logic to determine the completeness of a payment instrument.

BUG=711358
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2813203004
Cr-Commit-Position: refs/heads/master@{#465348}
Committed: https://chromium.googlesource.com/chromium/src/+/c45bd0f5bfda65c49b54527b994c08e8cf7203b5

[Mathieu, 2017-04-13 17:52:24]

mathp@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[Mathieu, 2017-04-13 17:52:24]

Anthony, PTAL

[anthonyvd, 2017-04-13 19:50:55]

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
components/autofill/core/browser/validation.cc:136: bool
IsCompleteForPaymentRequest(const CreditCard& card,
I think this could return a bitfield enum instead of a bool and remove the
|missing_info| param. This would decouple this from UI strings (so it's easier
to reuse it in a place where we do care about what's missing but not the
specific strings), letting the UI do the enum -> string mapping.

[Mathieu, 2017-04-13 20:00:15]

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
components/autofill/core/browser/validation.cc:136: bool
IsCompleteForPaymentRequest(const CreditCard& card,
On 2017/04/13 19:50:55, anthonyvd wrote:
> I think this could return a bitfield enum instead of a bool and remove the
> |missing_info| param. This would decouple this from UI strings (so it's easier
> to reuse it in a place where we do care about what's missing but not the
> specific strings), letting the UI do the enum -> string mapping.

This is intentional because we want this file to be the state -> string mapping,
so that it can be used on other platforms (mainly iOS but I could see android
also calling this). I had discussed with Moe and it's useful for him to have a
method such as this one. For example, he used IsValidForType from this file to
get the proper error for the credit card editor [1].

[1] https://cs.chromium.org/search/?q=IsValidForType+file:ios&type=cs

[anthonyvd, 2017-04-13 20:48:27]

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
components/autofill/core/browser/validation.cc:136: bool
IsCompleteForPaymentRequest(const CreditCard& card,
On 2017/04/13 at 20:00:15, Mathieu wrote:
> On 2017/04/13 19:50:55, anthonyvd wrote:
> > I think this could return a bitfield enum instead of a bool and remove the
> > |missing_info| param. This would decouple this from UI strings (so it's
easier
> > to reuse it in a place where we do care about what's missing but not the
> > specific strings), letting the UI do the enum -> string mapping.
> 
> This is intentional because we want this file to be the state -> string
mapping, so that it can be used on other platforms (mainly iOS but I could see
android also calling this). I had discussed with Moe and it's useful for him to
have a method such as this one. For example, he used IsValidForType from this
file to get the proper error for the credit card editor [1].
> 
> [1] https://cs.chromium.org/search/?q=IsValidForType+file:ios&type=cs

Totally understand the need to have a common function to get that string but I
still think determining which field is missing should be separate from the one
generating the string. I'd do a function returning an enum and extract the
string generation logic from this function here in a separate one.

I also think the resulting code will be cleaner. This function currently repeats
if (!message.empty()) { if (missing_info) {}} 3 times and potentially overwrites
message a bunch of times, which makes it harder to follow.

Something like:

ValidationResult IsCompleteForPayment(const CreditCard& cc) {
  ValidationResult res = VALID;
  if (cc.IsExpired()) res |= EXPIRED;
  if (/* name is empty */) res |= NAME_MISSING;

  return res;
}

base::string16 GetCCValidationString(ValidationResult res) {
  if (res == VALID) return "";
  if (res == EXPIRED) return "expired";
  if (res == NAME_MISSING) return "name_missing";
  return "Multiple things wrong";
}

[Mathieu, 2017-04-18 17:26:01]

Thanks, PTAL?

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
File components/autofill/core/browser/validation.cc (right):

https://codereview.chromium.org/2813203004/diff/10001/components/autofill/cor...
components/autofill/core/browser/validation.cc:136: bool
IsCompleteForPaymentRequest(const CreditCard& card,
On 2017/04/13 20:48:27, anthonyvd wrote:
> On 2017/04/13 at 20:00:15, Mathieu wrote:
> > On 2017/04/13 19:50:55, anthonyvd wrote:
> > > I think this could return a bitfield enum instead of a bool and remove the
> > > |missing_info| param. This would decouple this from UI strings (so it's
> easier
> > > to reuse it in a place where we do care about what's missing but not the
> > > specific strings), letting the UI do the enum -> string mapping.
> > 
> > This is intentional because we want this file to be the state -> string
> mapping, so that it can be used on other platforms (mainly iOS but I could see
> android also calling this). I had discussed with Moe and it's useful for him
to
> have a method such as this one. For example, he used IsValidForType from this
> file to get the proper error for the credit card editor [1].
> > 
> > [1] https://cs.chromium.org/search/?q=IsValidForType+file:ios&type=cs
> 
> Totally understand the need to have a common function to get that string but I
> still think determining which field is missing should be separate from the one
> generating the string. I'd do a function returning an enum and extract the
> string generation logic from this function here in a separate one.
> 
> I also think the resulting code will be cleaner. This function currently
repeats
> if (!message.empty()) { if (missing_info) {}} 3 times and potentially
overwrites
> message a bunch of times, which makes it harder to follow.
> 
> Something like:
> 
> ValidationResult IsCompleteForPayment(const CreditCard& cc) {
>   ValidationResult res = VALID;
>   if (cc.IsExpired()) res |= EXPIRED;
>   if (/* name is empty */) res |= NAME_MISSING;
> 
>   return res;
> }
> 
> base::string16 GetCCValidationString(ValidationResult res) {
>   if (res == VALID) return "";
>   if (res == EXPIRED) return "expired";
>   if (res == NAME_MISSING) return "name_missing";
>   return "Multiple things wrong";
> }

Cool, it's better now, PTAL

[anthonyvd, 2017-04-18 18:49:42]

lgtm

[Mathieu, 2017-04-18 19:15:43]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2017-04-18 19:16:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 20:24:34]

CQ is committing da patch.

Bot data: {"patchset_id": 30001, "attempt_start_ts": 1492542943885540,
"parent_rev": "4b2894f1267ae935781d02057ab0ac01feae8d7e", "commit_rev":
"c45bd0f5bfda65c49b54527b994c08e8cf7203b5"}

[commit-bot: I haz the power, 2017-04-18 20:24:44]

Description was changed from

==========
[Payments] Show what's missing for incomplete payment methods.

Refactored logic to determine the completeness of a payment instrument.

BUG=711358
TEST=components_unittests
==========

to

==========
[Payments] Show what's missing for incomplete payment methods.

Refactored logic to determine the completeness of a payment instrument.

BUG=711358
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2813203004
Cr-Commit-Position: refs/heads/master@{#465348}
Committed:
https://chromium.googlesource.com/chromium/src/+/c45bd0f5bfda65c49b54527b994c...
==========

[commit-bot: I haz the power, 2017-04-18 20:24:45]

Committed patchset #3 (id:30001) as
https://chromium.googlesource.com/chromium/src/+/c45bd0f5bfda65c49b54527b994c...