Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514733004:
All billing addresses should be valid/complete (Closed)
|
Created:
4 years, 1 month ago by please use gerrit instead Modified:
4 years, 1 month ago Reviewers:
gogerald1 CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAll billing addresses should be valid/complete
BUG=663041
Committed: https://crrev.com/662a14a241ac885aa219849f2267acb8af0df3e5
Cr-Commit-Position: refs/heads/master@{#433440}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move check to AutofillPaymentApp #Patch Set 3 : Don't pass null to checkComplete #
Messages
Total messages: 21 (12 generated)
The CQ bit was checked by rouslan@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...
rouslan@chromium.org changed reviewers: + gogerald@chromium.org
Ganggui, ptal. This is a part of http://crrev.com/2501593003 that Dan has asked me to split up.
On 2016/11/18 22:53:40, rouslan wrote: > Ganggui, ptal. > > This is a part of http://crrev.com/2501593003 that Dan has asked me to split up. lgtm with comments
lgtm with comments https://codereview.chromium.org/2514733004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2514733004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:222: assert AutofillAddress.checkAddressCompletionStatus(billingAddress) Can't see where the incomplete billing address could come from since we only add and select complete billing addresses in cardEditor and addressEditor only saves the complete address. But could not hurt to do a assert check. https://codereview.chromium.org/2514733004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:253: || AutofillAddress.checkAddressCompletionStatus(mBillingAddress) check address completeness in AutofillPaymentApp.java looks more straightforward to me. In this way, we make sure all addresses in instrument and card editor are complete and no need to store it here but not use it in cardEditor (https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for the review. Addressed your comments and sending to CQ now. https://codereview.chromium.org/2514733004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2514733004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:222: assert AutofillAddress.checkAddressCompletionStatus(billingAddress) On 2016/11/18 23:21:59, gogerald1 wrote: > Can't see where the incomplete billing address could come from since we only add > and select complete billing addresses in cardEditor and addressEditor only saves > the complete address. But could not hurt to do a assert check. Asserts are designed to document expectations and verify those expectations when running tests, for example. As you mentioned, we expect the CardEditor and AddressEditor to only save complete addresses. I'm documenting this expectation here. Note that asserts do not affect production code. https://codereview.chromium.org/2514733004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:253: || AutofillAddress.checkAddressCompletionStatus(mBillingAddress) On 2016/11/18 23:21:59, gogerald1 wrote: > check address completeness in AutofillPaymentApp.java looks more straightforward > to me. In this way, we make sure all addresses in instrument and card editor are > complete and no need to store it here but not use it in cardEditor > (https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...). Done.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2514733004/#ps20001 (title: "Move check to AutofillPaymentApp")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2514733004/#ps40001 (title: "Don't pass null to checkComplete")
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": 40001, "attempt_start_ts": 1479601300613410,
"parent_rev": ["24459813e4c9327f15fe36ddbc17f7934debd4b4", null], "commit_rev":
["0b056b3653a2efcd8c827b39c996066a7a3e1531", null]}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== All billing addresses should be valid/complete BUG=663041 ========== to ========== All billing addresses should be valid/complete BUG=663041 Committed: https://crrev.com/662a14a241ac885aa219849f2267acb8af0df3e5 Cr-Commit-Position: refs/heads/master@{#433440} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/662a14a241ac885aa219849f2267acb8af0df3e5 Cr-Commit-Position: refs/heads/master@{#433440} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
