Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905283002:
[Payments] Allow international phone from different country in shipping editor. (Closed)
|
Created:
3 years, 7 months ago by sebsg Modified:
3 years, 6 months ago Reviewers:
Mathieu CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Allow international phone from different country in shipping editor.
BUG=723294
Review-Url: https://codereview.chromium.org/2905283002
Cr-Commit-Position: refs/heads/master@{#475383}
Committed: https://chromium.googlesource.com/chromium/src/+/1c933fe531736aaa2c98f160c4c4e810858a6407
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed mathp's comments #Patch Set 3 : Changed test expectations #
Total comments: 2
Patch Set 4 : Addressed suggestion #
Messages
Total messages: 33 (24 generated)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
Description was changed from ========== [Payments] Fix the international phone number to different country bug. BUG=723294 ========== to ========== [Payments] Fix the international phone number to different country bug. BUG=723294 ==========
Description was changed from ========== [Payments] Fix the international phone number to different country bug. BUG=723294 ========== to ========== [Payments] Allow international phone from different country in shipping editor. BUG=723294 ==========
https://codereview.chromium.org/2905283002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2905283002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:630: ResetEventObserver(DialogEvent::EDITOR_VALIDATION_ERROR); we are disabling the done button when there is invalid data, very soon: https://codereview.chromium.org/2905733002/ You will be able to check that the Save button is disabled and will not need the EDITOR_VALIDATION_ERROR event, probably. https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... File components/payments/core/payment_request_data_util.cc (right): https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util.cc:179: } else { no else because you are returning, above https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... File components/payments/core/payment_request_data_util_unittest.cc (right): https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util_unittest.cc:107: const char* locale; are you using this? https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util_unittest.cc:126: INSTANTIATE_TEST_CASE_P( let's have a US phone in MX and MX phone in US block https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util_unittest.cc:131: // US phone in US. US phone in CA? Let's also have a US phone in US block
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Another look? https://codereview.chromium.org/2905283002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2905283002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:630: ResetEventObserver(DialogEvent::EDITOR_VALIDATION_ERROR); On 2017/05/29 16:34:00, Mathieu wrote: > we are disabling the done button when there is invalid data, very soon: > https://codereview.chromium.org/2905733002/ > > You will be able to check that the Save button is disabled and will not need the > EDITOR_VALIDATION_ERROR event, probably. Ok, I'll rebase when it lands. https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... File components/payments/core/payment_request_data_util.cc (right): https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util.cc:179: } else { On 2017/05/29 16:34:00, Mathieu wrote: > no else because you are returning, above Done. https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... File components/payments/core/payment_request_data_util_unittest.cc (right): https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util_unittest.cc:107: const char* locale; On 2017/05/29 16:34:00, Mathieu wrote: > are you using this? Done. https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util_unittest.cc:126: INSTANTIATE_TEST_CASE_P( On 2017/05/29 16:34:00, Mathieu wrote: > let's have a US phone in MX and MX phone in US block Done. https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util_unittest.cc:131: // US phone in US. On 2017/05/29 16:34:00, Mathieu wrote: > US phone in CA? > > Let's also have a US phone in US block Done.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
On 2017/05/29 19:48:40, sebsg wrote: > Thanks! Another look? > > https://codereview.chromium.org/2905283002/diff/60001/chrome/browser/ui/views... > File > chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc > (right): > > https://codereview.chromium.org/2905283002/diff/60001/chrome/browser/ui/views... > chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:630: > ResetEventObserver(DialogEvent::EDITOR_VALIDATION_ERROR); > On 2017/05/29 16:34:00, Mathieu wrote: > > we are disabling the done button when there is invalid data, very soon: > > https://codereview.chromium.org/2905733002/ > > > > You will be able to check that the Save button is disabled and will not need > the > > EDITOR_VALIDATION_ERROR event, probably. > > Ok, I'll rebase when it lands. > > https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... > File components/payments/core/payment_request_data_util.cc (right): > > https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... > components/payments/core/payment_request_data_util.cc:179: } else { > On 2017/05/29 16:34:00, Mathieu wrote: > > no else because you are returning, above > > Done. > > https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... > File components/payments/core/payment_request_data_util_unittest.cc (right): > > https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... > components/payments/core/payment_request_data_util_unittest.cc:107: const char* > locale; > On 2017/05/29 16:34:00, Mathieu wrote: > > are you using this? > > Done. > > https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... > components/payments/core/payment_request_data_util_unittest.cc:126: > INSTANTIATE_TEST_CASE_P( > On 2017/05/29 16:34:00, Mathieu wrote: > > let's have a US phone in MX and MX phone in US block > > Done. > > https://codereview.chromium.org/2905283002/diff/60001/components/payments/cor... > components/payments/core/payment_request_data_util_unittest.cc:131: // US phone > in US. > On 2017/05/29 16:34:00, Mathieu wrote: > > US phone in CA? > > > > Let's also have a US phone in US block > > Done. Just revert the "on editor validation" logic, and in your test you can instead check for the existence of the error label.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, I changed the test expectations to see if the field is valid
lgtm, thanks https://codereview.chromium.org/2905283002/diff/100001/components/payments/co... File components/payments/core/payment_request_data_util_unittest.cc (right): https://codereview.chromium.org/2905283002/diff/100001/components/payments/co... components/payments/core/payment_request_data_util_unittest.cc:107: const char* locale) = "" ?
Thanks, sending to CQ https://codereview.chromium.org/2905283002/diff/100001/components/payments/co... File components/payments/core/payment_request_data_util_unittest.cc (right): https://codereview.chromium.org/2905283002/diff/100001/components/payments/co... components/payments/core/payment_request_data_util_unittest.cc:107: const char* locale) On 2017/05/29 20:29:03, Mathieu wrote: > = "" ? Ah sweet! thanks!
The CQ bit was checked by sebsg@chromium.org
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/2905283002/#ps120001 (title: "Addressed suggestion")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496089951619360,
"parent_rev": "1f3a7e66d633e18507101436900fa98625d8e987", "commit_rev":
"1c933fe531736aaa2c98f160c4c4e810858a6407"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Allow international phone from different country in shipping editor. BUG=723294 ========== to ========== [Payments] Allow international phone from different country in shipping editor. BUG=723294 Review-Url: https://codereview.chromium.org/2905283002 Cr-Commit-Position: refs/heads/master@{#475383} Committed: https://chromium.googlesource.com/chromium/src/+/1c933fe531736aaa2c98f160c4c4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/1c933fe531736aaa2c98f160c4c4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
