Prompt for expiration date for local cards.

Prompt for expiration date for expired cards.

When PaymentRequest prompts the user for the CVC for an expired local or
full server card, the expiration date should also be updated.

LOCAL CARD: The new expiration date is saved on disk.

FULL SERVER CARD: The new expiration date is sent to the server for
reauthorization. The new expiration date will eventually be synced to
Chrome.

BUG=606930, 606008
Committed: https://crrev.com/1a08353b40578fb410367904472dd98400406dc1
Cr-Commit-Position: refs/heads/master@{#390271}

[please use gerrit instead, 2016-04-27 03:48:47]

Description was changed from

==========
Prompt for expiration date for local cards.

When PaymentRequest prompts the user for the CVC for a local expired\
card, the expiration date should also be updated.

BUG=606930
==========

to

==========
Prompt for expiration date for expired cards.

When PaymentRequest prompts the user for the CVC for an expired local or
full server card, the expiration date should also be updated.

LOCAL CARD: The new expiration date is saved on disk.

FULL SERVER CARD: The new expiration date is sent to the server for
reauthorization. The new expiration date will eventually be synced to
Chrome.

BUG=606930
==========

[please use gerrit instead, 2016-04-27 05:22:42]

Patchset #1 (id:1) has been deleted

[please use gerrit instead, 2016-04-27 05:22:51]

Patchset #1 (id:20001) has been deleted

[please use gerrit instead, 2016-04-27 05:29:26]

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

[please use gerrit instead, 2016-04-27 05:29:27]

mathp@, ptal.

[please use gerrit instead, 2016-04-27 05:34:56]

Patchset #1 (id:40001) has been deleted

[Mathieu, 2016-04-27 12:09:23]

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

[Mathieu, 2016-04-27 12:09:24]

will look soon, also adding sebsg@ who is working on expired cards locally.

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:84: bool IsInThePast(int month,
int year) {
Could you use CreditCard::IsExpired?
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[sebsg, 2016-04-27 14:59:02]

This will be useful thanks!

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:736: // A card that has
expired last year should be updated.
These tests seem to have a lot in common. Do you think you could make one test
and iterate through an array of test cases? Like this for example:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
or
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Thanks!

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/payments/full_card_request.cc (right):

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
components/autofill/core/browser/payments/full_card_request.cc:77: if
(request_->card.record_type() == CreditCard::LOCAL_CARD &&
Quick question for my understanding. In what circumstances do we unmask local
credit cards? I didn't know we did that sometimes. Thanks.

[please use gerrit instead, 2016-04-27 18:01:25]

Description was changed from

==========
Prompt for expiration date for expired cards.

When PaymentRequest prompts the user for the CVC for an expired local or
full server card, the expiration date should also be updated.

LOCAL CARD: The new expiration date is saved on disk.

FULL SERVER CARD: The new expiration date is sent to the server for
reauthorization. The new expiration date will eventually be synced to
Chrome.

BUG=606930
==========

to

==========
Prompt for expiration date for expired cards.

When PaymentRequest prompts the user for the CVC for an expired local or
full server card, the expiration date should also be updated.

LOCAL CARD: The new expiration date is saved on disk.

FULL SERVER CARD: The new expiration date is sent to the server for
reauthorization. The new expiration date will eventually be synced to
Chrome.

BUG=606930, 606008
==========

[please use gerrit instead, 2016-04-27 18:02:28]

Patchset #2 (id:80001) has been deleted

[please use gerrit instead, 2016-04-27 18:05:44]

mathp@, sebsg@, ptal patch 2.

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
components/autofill/core/browser/credit_card.cc:84: bool IsInThePast(int month,
int year) {
On 2016/04/27 12:09:24, Mathieu Perreault wrote:
> Could you use CreditCard::IsExpired?
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Done.

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
components/autofill/core/browser/credit_card_unittest.cc:736: // A card that has
expired last year should be updated.
On 2016/04/27 14:59:02, sebsg wrote:
> These tests seem to have a lot in common. Do you think you could make one test
> and iterate through an array of test cases? Like this for example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> or
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> 
> Thanks!

Done. Sorry for the wall of tests. I attempted to make them as readable as
possible.

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
File components/autofill/core/browser/payments/full_card_request.cc (right):

https://codereview.chromium.org/1926553002/diff/60001/components/autofill/cor...
components/autofill/core/browser/payments/full_card_request.cc:77: if
(request_->card.record_type() == CreditCard::LOCAL_CARD &&
On 2016/04/27 14:59:02, sebsg wrote:
> Quick question for my understanding. In what circumstances do we unmask local
> credit cards? I didn't know we did that sometimes. Thanks.

PaymentRequest uses FullCardRequest to prompt the user for the CVC and
optionally the updated expiration date of the local card. There's no form
filling in PaymentRequest, so the user has to manually type in the CVC
somewhere. I am using AutofillClient::ShowUnmaskPrompt() for this purpose.
"ShowUnmaskPrompt()" should really be renamed into "GetCvcAndExpirationDate()",
because that's what it does, but not in this patch.

Android UI for PaymentRequest landed on trunk only a couple of days ago and is
behind a flag.

Note that the local card's CVC is not sent to the server for the actual
unmasking: line 82.

[Mathieu, 2016-04-27 19:54:50]

Looks good! just checking: are we handling the case where the user enters a
correct CVC but an expired date in the prompt? Say they haven't received their
new expiration date yet.

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
components/autofill/core/browser/credit_card.cc:795: return server_status_ ==
EXPIRED || IsExpired(current_time);
looks like this function could be replaced with

return server_status_ == EXPIRED || IsExpired(current_time);

because a local card will have server_status_ == OK. What do you think?

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:737: class ExpTestCase
{
would a struct simplify things here? possibly less boilerplate and they can also
be instantiated quite simply too
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

[please use gerrit instead, 2016-04-28 00:30:24]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[please use gerrit instead, 2016-04-28 00:30:24]

dfalcantara@, ptal CardUnmaskPrompt.java in patch 3. It was missing a check for
an expiration year in the previous months of the current year. This was causing
a DCHECK() in the controller, because it assumed that to be invalid.

mathp@, sebsg@, ptal patch 3.

On 2016/04/27 19:54:50, Mathieu Perreault wrote:
> are we handling the case where the user enters a
> correct CVC but an expired date in the prompt? Say they haven't received their
> new expiration date yet.

The UI prevents user from submitting an expiration date in the past.

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
File components/autofill/core/browser/credit_card.cc (right):

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
components/autofill/core/browser/credit_card.cc:795: return server_status_ ==
EXPIRED || IsExpired(current_time);
On 2016/04/27 19:54:50, Mathieu Perreault wrote:
> looks like this function could be replaced with
> 
> return server_status_ == EXPIRED || IsExpired(current_time);
> 
> because a local card will have server_status_ == OK. What do you think?

Done.

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
File components/autofill/core/browser/credit_card_unittest.cc (right):

https://codereview.chromium.org/1926553002/diff/100001/components/autofill/co...
components/autofill/core/browser/credit_card_unittest.cc:737: class ExpTestCase
{
On 2016/04/27 19:54:50, Mathieu Perreault wrote:
> would a struct simplify things here? possibly less boilerplate and they can
also
> be instantiated quite simply too
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Done.

[gone, 2016-04-28 00:41:35]

CardUmaskPrompt.java lgtm

[Mathieu, 2016-04-28 00:58:07]

lgtm, thanks!

[please use gerrit instead, 2016-04-28 00:59:33]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2016-04-28 00:59:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1926553002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1926553002/140001

[commit-bot: I haz the power, 2016-04-28 01:30:01]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-04-30 17:15:28]

Patchset 4 (id:??) landed as
https://crrev.com/1a08353b40578fb410367904472dd98400406dc1
Cr-Commit-Position: refs/heads/master@{#390271}

[commit-bot: I haz the power, 2016-04-30 17:53:41]

Description was changed from

==========
Prompt for expiration date for expired cards.

When PaymentRequest prompts the user for the CVC for an expired local or
full server card, the expiration date should also be updated.

LOCAL CARD: The new expiration date is saved on disk.

FULL SERVER CARD: The new expiration date is sent to the server for
reauthorization. The new expiration date will eventually be synced to
Chrome.

BUG=606930, 606008
==========

to

==========
Prompt for expiration date for expired cards.

When PaymentRequest prompts the user for the CVC for an expired local or
full server card, the expiration date should also be updated.

LOCAL CARD: The new expiration date is saved on disk.

FULL SERVER CARD: The new expiration date is sent to the server for
reauthorization. The new expiration date will eventually be synced to
Chrome.

BUG=606930, 606008

Committed: https://crrev.com/1a08353b40578fb410367904472dd98400406dc1
Cr-Commit-Position: refs/heads/master@{#390271}
==========