Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894943002:
Prevent existing card number to be used for new credit cards in the Payment Request (Closed)
|
Created:
3 years, 7 months ago by MAD Modified:
3 years, 7 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, rouslan+autofill_chromium.org, tfarina, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, gogerald+paymentswatch_chromium.org, mahmadi+paymentswatch_chromium.org, mathp+autofillwatch_chromium.org, rogerm+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent existing card number to be used for new credit cards in the Payment Request
BUG=721616
Review-Url: https://codereview.chromium.org/2894943002
Cr-Commit-Position: refs/heads/master@{#474324}
Committed: https://chromium.googlesource.com/chromium/src/+/cf2731fb3a59241511601c1e487814175902fdae
Patch Set 1 #Patch Set 2 : Self CR + test #Patch Set 3 : Rebased and Added a TODO. #Patch Set 4 : Fixed TODO comment. #Patch Set 5 : Merge fluke fix #
Total comments: 4
Patch Set 6 : Moar nits and bots error fix #Patch Set 7 : Rebase #Patch Set 8 : Fixed unit tests crash #
Messages
Total messages: 41 (21 generated)
Patchset #2 (id:20001) has been deleted
mad@chromium.org changed reviewers: + sebsg@chromium.org
Care to take a first look before I ask OWNERs of browser\views\payments to review it? Thanks! BYE MAD...
That looks very good! thanks! LGTM
madecoste@gmail.com changed reviewers: + anthonyvd@chromium.org, madecoste@gmail.com
Thanks Web... +Anthony, ok with you too?
mad@chromium.org changed reviewers: - madecoste@gmail.com
Ping! Anthony, I need an OWNER review for chrome/browser/ui/views/payments/* Thanks... BYE MAD
mathp@chromium.org changed reviewers: + mathp@chromium.org
While this approach works, I wonder if it's not better to just let the card be "added", but actually in the end if it already existed in PersonalDataManager, we only select it. This would only mean changing AddAutofillPaymentInstrument to first check if the instrument was already existing, and if so simply selecting it. Code: https://cs.chromium.org/chromium/src/components/payments/content/payment_requ...
On 2017/05/23 14:58:11, Mathieu wrote: > While this approach works, I wonder if it's not better to just let the card be > "added", but actually in the end if it already existed in PersonalDataManager, > we only select it. This would only mean changing AddAutofillPaymentInstrument to > first check if the instrument was already existing, and if so simply selecting > it. > > Code: > https://cs.chromium.org/chromium/src/components/payments/content/payment_requ... I think given Bruno's comments, we could land this now before branch and have some TODO's to change the experience later? Does that sound good?
Done... Anything else? Thanks! BYE MAD
On 2017/05/23 at 18:58:41, mathp wrote: > On 2017/05/23 14:58:11, Mathieu wrote: > > While this approach works, I wonder if it's not better to just let the card be > > "added", but actually in the end if it already existed in PersonalDataManager, > > we only select it. This would only mean changing AddAutofillPaymentInstrument to > > first check if the instrument was already existing, and if so simply selecting > > it. > > > > Code: > > https://cs.chromium.org/chromium/src/components/payments/content/payment_requ... > > I think given Bruno's comments, we could land this now before branch and have some TODO's to change the experience later? Does that sound good? That SGTM. As discussed, let's optimize for the most likely scenarios for V1. We can revisit this UX interaction later. CL lgtm
The CQ bit was checked by mad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2894943002/#ps80001 (title: "Fixed TODO comment.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by mad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2894943002/#ps100001 (title: "Merge fluke fix")
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
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...)
lgtm with nits https://codereview.chromium.org/2894943002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc (right): https://codereview.chromium.org/2894943002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:476: card_number, supported_card_networks_, error_message)) { can we use spec()->supported_card_networks_set() here? https://codereview.chromium.org/2894943002/diff/100001/components/payments_st... File components/payments_strings.grdp (right): https://codereview.chromium.org/2894943002/diff/100001/components/payments_st... components/payments_strings.grdp:164: <message name="IDS_PAYMENTS_VALIDATION_ALREADY_USED_CREDIT_CARD_NUMBER" desc="Message displayed to user when the credit card number has already been used in this user profile."> would consider a better description for translators, such as: ... when the credit card number they have entered corresponds to an existing credit card number in their user profile.
All done, thanks! Trying the CQ again... BYE MAD https://codereview.chromium.org/2894943002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc (right): https://codereview.chromium.org/2894943002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/payments/credit_card_editor_view_controller.cc:476: card_number, supported_card_networks_, error_message)) { On 2017/05/24 02:40:20, Mathieu wrote: > can we use spec()->supported_card_networks_set() here? Done. https://codereview.chromium.org/2894943002/diff/100001/components/payments_st... File components/payments_strings.grdp (right): https://codereview.chromium.org/2894943002/diff/100001/components/payments_st... components/payments_strings.grdp:164: <message name="IDS_PAYMENTS_VALIDATION_ALREADY_USED_CREDIT_CARD_NUMBER" desc="Message displayed to user when the credit card number has already been used in this user profile."> On 2017/05/24 02:40:20, Mathieu wrote: > would consider a better description for translators, such as: ... when the > credit card number they have entered corresponds to an existing credit card > number in their user profile. Done.
The CQ bit was checked by mad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, sebsg@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2894943002/#ps120001 (title: "Moar nits and bots error fix")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
The CQ bit was checked by mad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, sebsg@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2894943002/#ps140001 (title: "Rebase")
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
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_...)
The CQ bit was checked by mad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, sebsg@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2894943002/#ps160001 (title: "Fixed unit tests crash")
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": 160001, "attempt_start_ts": 1495640981514190,
"parent_rev": "14502d2f4d4cb396b959f824aab00b62f294bd3b", "commit_rev":
"cf2731fb3a59241511601c1e487814175902fdae"}
Message was sent while issue was closed.
Description was changed from ========== Prevent existing card number to be used for new credit cards in the Payment Request BUG=721616 ========== to ========== Prevent existing card number to be used for new credit cards in the Payment Request BUG=721616 Review-Url: https://codereview.chromium.org/2894943002 Cr-Commit-Position: refs/heads/master@{#474324} Committed: https://chromium.googlesource.com/chromium/src/+/cf2731fb3a59241511601c1e4878... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/cf2731fb3a59241511601c1e4878... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
