Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(620)

Issue 2972973002: [Payments] When saving an edited address in editor, clear data first. (Closed)

Created:
3 years, 5 months ago by Mathieu
Modified:
3 years, 5 months ago
CC:
chromium-reviews, rouslan+autofill_chromium.org, tfarina, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, mathp+autofillwatch_chromium.org, rogerm+autofillwatch_chromium.org, mahmadi+paymentswatch_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, sebsg+paymentswatch_chromium.org, Moe
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Payments] When saving an edited address in editor, clear data first. The original bug was that some information was wrongfully being retained in the edited profile when saved, because some fields were absent in the newly selected country (say you edited from US to Albania, which has no states). We fix this by clearing all the address fields from the profile being edited, before replacing with the values in the editor. BUG=723610 TEST=browser_tests Review-Url: https://codereview.chromium.org/2972973002 Cr-Commit-Position: refs/heads/master@{#484756} Committed: https://chromium.googlesource.com/chromium/src/+/ee7829e4607ec6e4b27efbfdf4212c8e447bde13

Patch Set 1 #

Total comments: 4

Patch Set 2 : addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+101 lines, -18 lines) Patch
M chrome/browser/ui/views/payments/shipping_address_editor_view_controller.cc View 1 1 chunk +9 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc View 1 5 chunks +83 lines, -4 lines 0 comments Download
M components/autofill/core/browser/autofill_profile.h View 2 chunks +1 line, -3 lines 0 comments Download
M components/autofill/core/browser/autofill_profile.cc View 2 chunks +8 lines, -8 lines 0 comments Download

Messages

Total messages: 23 (13 generated)
sebsg
LGTM % some comments Nice find! https://codereview.chromium.org/2972973002/diff/1/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc File chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2972973002/diff/1/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc#newcode40 chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:40: Nit: remove line? ...
3 years, 5 months ago (2017-07-06 14:55:19 UTC) #7
please use gerrit instead
FYI, that's how we do this on Android: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java?rcl=5b87618142279f2c84dd02bc21cf58e970413373&l=252
3 years, 5 months ago (2017-07-06 15:03:06 UTC) #8
please use gerrit instead
Moe: Can you verify iOS behavior?
3 years, 5 months ago (2017-07-06 15:03:48 UTC) #10
Mathieu
On 2017/07/06 15:03:48, rouslan wrote: > Moe: Can you verify iOS behavior? Does Android have ...
3 years, 5 months ago (2017-07-06 15:14:47 UTC) #11
please use gerrit instead
On 2017/07/06 15:14:47, Mathieu wrote: > On 2017/07/06 15:03:48, rouslan wrote: > > Moe: Can ...
3 years, 5 months ago (2017-07-06 15:18:49 UTC) #12
Mathieu
Thanks https://codereview.chromium.org/2972973002/diff/1/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc File chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc (right): https://codereview.chromium.org/2972973002/diff/1/chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc#newcode40 chrome/browser/ui/views/payments/shipping_address_editor_view_controller_browsertest.cc:40: On 2017/07/06 14:55:19, sebsg wrote: > Nit: remove ...
3 years, 5 months ago (2017-07-06 15:43:43 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2972973002/20001
3 years, 5 months ago (2017-07-06 15:44:37 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/422119)
3 years, 5 months ago (2017-07-06 16:48:45 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2972973002/20001
3 years, 5 months ago (2017-07-06 21:19:31 UTC) #20
commit-bot: I haz the power
3 years, 5 months ago (2017-07-06 22:31:05 UTC) #23
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ee7829e4607ec6e4b27efbfdf421...

Powered by Google App Engine
This is Rietveld 408576698