[Payments] Normalize shipping address change on Desktop

[Payments] Normalize shipping address change on Desktop

Also make sure that the user cannot click pay until we
get the callback from the merchant.

BUG=715170, 715597
Review-Url: https://codereview.chromium.org/2836353002
Cr-Commit-Position: refs/heads/master@{#467698}
Committed: https://chromium.googlesource.com/chromium/src/+/7e61913aaeda21fcfdbefb435d6085efdc1aa9fb

[sebsg, 2017-04-25 20:24:16]

Patchset #1 (id:1) has been deleted

[sebsg, 2017-04-25 20:27:17]

Patchset #1 (id:20001) has been deleted

[sebsg, 2017-04-25 20:28:37]

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

[sebsg, 2017-04-25 20:28:38]

Hi Math, could you please take a look?

Once all of the normalization CLs are landed I want to see if I could move all
the test objects somewhere reusable (eg. FakeAddressNormalizer,
FakePaymentRequestDelegate).

Thanks!

[sebsg, 2017-04-25 20:28:45]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 20:29:49]

Dry run: 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-25 21:35:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 21:35:25]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-04-26 13:09:49]

Thanks

Might as well create the test_payment_request_delegate and other objects now...
You'll have to do it anyway?

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state.cc:53:
UpdateIsReadyToPayAndNotifyObservers();
what could have changed that may affect "is ready to pay"? It's possibly even a
bug to call it in this flow, because the flow is:

Select an address
StartWaitingForUpdateWith (spinner)
Start normalization
Get normalization result
Send address to merchant
Get new details back
Stop spinning.

I would say the Pay button should be updated after all this flow, what do you
think? Can you test this flow?

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state.cc:158: if
(!autofill::data_util::IsValidCountryCode(country_code)) {
no curlies

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
File components/payments/content/payment_request_state_unittest.cc (right):

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state_unittest.cc:25: class
FakeAddressNormalizer : public AddressNormalizer {
nit: I prefer TestAddressNormalizer instead of Fake, same for others

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state_unittest.cc:358:
TEST_F(PaymentRequestStateTest, SelectedShippingAddressMessage_Normalized) {
can you test the is_ready_to_pay state as well

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
File components/payments/content/payment_response_helper.h (right):

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.h:77:
autofill::AutofillProfile shipping_address_;
// A copy of the shipping address, which will be normalized before being
included in the PaymentResponse.

[sebsg, 2017-04-26 15:54:38]

Description was changed from

==========
[Payments] Normalize shipping address change on Desktop

BUG=715170
==========

to

==========
[Payments] Normalize shipping address change on Desktop

BUG=715170,715597
==========

[sebsg, 2017-04-26 15:55:07]

Description was changed from

==========
[Payments] Normalize shipping address change on Desktop

BUG=715170,715597
==========

to

==========
[Payments] Normalize shipping address change on Desktop

Also make sure that the user cannot click pay until we
get the callback from the merchant.

BUG=715170,715597
==========

[sebsg, 2017-04-26 19:28:34]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2017-04-26 19:28:44]

Patchset #2 (id:60001) has been deleted

[sebsg, 2017-04-26 19:28:54]

Another look? Thanks!

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state.cc:53:
UpdateIsReadyToPayAndNotifyObservers();
On 2017/04/26 13:09:49, Mathieu wrote:
> what could have changed that may affect "is ready to pay"? It's possibly even
a
> bug to call it in this flow, because the flow is:
> 
> Select an address
> StartWaitingForUpdateWith (spinner)
> Start normalization
> Get normalization result
> Send address to merchant
> Get new details back
> Stop spinning.
> 
> I would say the Pay button should be updated after all this flow, what do you
> think? Can you test this flow?

I made a bunch of changes to this logic, to make sure that the Pay button is
disabled until we got the details back from the merchant.

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state.cc:158: if
(!autofill::data_util::IsValidCountryCode(country_code)) {
On 2017/04/26 13:09:49, Mathieu wrote:
> no curlies

Done.

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
File components/payments/content/payment_request_state_unittest.cc (right):

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state_unittest.cc:25: class
FakeAddressNormalizer : public AddressNormalizer {
On 2017/04/26 13:09:49, Mathieu wrote:
> nit: I prefer TestAddressNormalizer instead of Fake, same for others

Done.

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_request_state_unittest.cc:358:
TEST_F(PaymentRequestStateTest, SelectedShippingAddressMessage_Normalized) {
On 2017/04/26 13:09:49, Mathieu wrote:
> can you test the is_ready_to_pay state as well

Done.

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
File components/payments/content/payment_response_helper.h (right):

https://codereview.chromium.org/2836353002/diff/40001/components/payments/con...
components/payments/content/payment_response_helper.h:77:
autofill::AutofillProfile shipping_address_;
On 2017/04/26 13:09:49, Mathieu wrote:
> // A copy of the shipping address, which will be normalized before being
> included in the PaymentResponse.

Thanks, changed the comment a bit as this address is a normalized copy.

[commit-bot: I haz the power, 2017-04-26 19:29:51]

Dry run: 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-26 19:36:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 19:36:50]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sebsg, 2017-04-26 20:24:38]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:26:04]

Dry run: 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-26 20:31:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 20:31:14]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sebsg, 2017-04-26 21:04:28]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:05:19]

Dry run: 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-26 21:11:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 21:11:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sebsg, 2017-04-26 21:19:07]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:19:42]

Dry run: 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-26 21:24:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 21:24:43]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sebsg, 2017-04-26 21:30:35]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:31:22]

Dry run: 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-26 21:38:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 21:38:46]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sebsg, 2017-04-26 21:41:48]

Patchset #5 (id:140001) has been deleted

[sebsg, 2017-04-26 21:49:21]

Patchset #5 (id:160001) has been deleted

[Mathieu, 2017-04-27 11:04:01]

lgtm with comments across 2 patchsets

https://codereview.chromium.org/2836353002/diff/100001/components/payments/co...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2836353002/diff/100001/components/payments/co...
components/payments/content/payment_request_state.cc:67:
is_waiting_for_merchant_validation_ = false;
could this be on line 54?

https://codereview.chromium.org/2836353002/diff/100001/components/payments/co...
components/payments/content/payment_request_state.cc:160:
is_waiting_for_merchant_validation_ = true;
another option is to set |selected_shipping_profile_| to nullptr until it's
normalized. Not sure which one I prefer...

https://codereview.chromium.org/2836353002/diff/180001/components/payments/co...
File components/payments/content/payment_request_state_unittest.cc (right):

https://codereview.chromium.org/2836353002/diff/180001/components/payments/co...
components/payments/content/payment_request_state_unittest.cc:28:
test_payment_request_delegate_(nullptr),
/*personal_data_manager=*/

[sebsg, 2017-04-27 14:43:23]

Thanks, sending to CQ

https://codereview.chromium.org/2836353002/diff/100001/components/payments/co...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2836353002/diff/100001/components/payments/co...
components/payments/content/payment_request_state.cc:67:
is_waiting_for_merchant_validation_ = false;
On 2017/04/27 11:04:01, Mathieu wrote:
> could this be on line 54?

Not really, as the merchant has not validated yet.

https://codereview.chromium.org/2836353002/diff/100001/components/payments/co...
components/payments/content/payment_request_state.cc:160:
is_waiting_for_merchant_validation_ = true;
On 2017/04/27 11:04:01, Mathieu wrote:
> another option is to set |selected_shipping_profile_| to nullptr until it's
> normalized. Not sure which one I prefer...

I thought about that but when we get the result from the normalization we get an
object, so we'd have to handle that. That's why I prefered the current way :)

https://codereview.chromium.org/2836353002/diff/180001/components/payments/co...
File components/payments/content/payment_request_state_unittest.cc (right):

https://codereview.chromium.org/2836353002/diff/180001/components/payments/co...
components/payments/content/payment_request_state_unittest.cc:28:
test_payment_request_delegate_(nullptr),
On 2017/04/27 11:04:01, Mathieu wrote:
> /*personal_data_manager=*/

Done.

[sebsg, 2017-04-27 14:47:26]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2017-04-27 14:47:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2836353002/#ps200001
(title: "Addressed Math's comments")

[commit-bot: I haz the power, 2017-04-27 14:47:52]

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-27 16:07:13]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1493304446697750,
"parent_rev": "923beb5a83a9ccc76ac0b14e9dbeace2983ec8b5", "commit_rev":
"7e61913aaeda21fcfdbefb435d6085efdc1aa9fb"}

[commit-bot: I haz the power, 2017-04-27 16:09:01]

Description was changed from

==========
[Payments] Normalize shipping address change on Desktop

Also make sure that the user cannot click pay until we
get the callback from the merchant.

BUG=715170,715597
==========

to

==========
[Payments] Normalize shipping address change on Desktop

Also make sure that the user cannot click pay until we
get the callback from the merchant.

BUG=715170,715597

Review-Url: https://codereview.chromium.org/2836353002
Cr-Commit-Position: refs/heads/master@{#467698}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e61913aaeda21fcfdbefb435d60...
==========

[commit-bot: I haz the power, 2017-04-27 16:09:03]

Committed patchset #6 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/7e61913aaeda21fcfdbefb435d60...