Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349033002:
Include addresses with the getdetailsforsavecard Payments RPC. (Closed)
|
Created:
4 years, 3 months ago by Justin Donnelly Modified:
4 years, 2 months ago Reviewers:
Evan Stade CC:
chromium-reviews, rouslan+autofill_chromium.org, jam, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, Jared Saul, zkoch1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude addresses with the getdetailsforsavecard Payments RPC.
These addresses will be used by Payments to determine the correct legal
documents to show the user and to verify that the addresses are valid
for their purposes (and thus won't fail on any subsequent save).
Names and phone numbers are removed from the addresses before uploading
since they are not useful for these purposes.
BUG=535784
Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82
Committed: https://crrev.com/005d6cd78d14cb1d68a92a0d8dd9a98595b54d80
Cr-Original-Commit-Position: refs/heads/master@{#420409}
Cr-Commit-Position: refs/heads/master@{#420893}
Patch Set 1 #Patch Set 2 : Add tests to verify whether or not address names are sent. #
Total comments: 1
Patch Set 3 : Also don't send phone numbers. #Patch Set 4 : Eliminate static initializers, update a comment. #
Messages
Total messages: 53 (33 generated)
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
jdonnelly@chromium.org changed reviewers: + mathp@google.com
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user. BUG=535784 ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user. BUG=535784 ==========
jdonnelly@chromium.org changed reviewers: + mathp@chromium.org - mathp@google.com
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: This issue passed the CQ dry run.
jdonnelly@chromium.org changed reviewers: + estade@chromium.org - mathp@chromium.org
-mathp (paternity leave), +estade
estade: friendly ping If you can't get to this immediately, that's fine. I just want to make sure it's in your queue.
sorry for delay. Why are names stripped? It seems like there's still of PII in the profiles (it's really not a secret who lives at my address). Can we strip more, e.g. just send zip codes or something? Can you add tests to make sure the right data is in or is missing from the request?
On 2016/09/20 20:57:51, Evan Stade wrote: > sorry for delay. Why are names stripped? It seems like there's still of PII in > the profiles (it's really not a secret who lives at my address). Can we strip > more, e.g. just send zip codes or something? The full address is useful to Payments for identifying additional cases that could fail if and when the user tried to upload their card (P.O. boxes, addresses that fail geo validation). This allows them to reject the details RPC so we don't show the bubble in a case we know will fail. Removing the names doesn't make it not-PII, true, but we might as well limit the data to only what we need. > Can you add tests to make sure the right data is in or is missing from the > request? Sure, will do.
The CQ bit was checked by jdonnelly@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...
Added tests for names in addresses. Please take another look.
The CQ bit was checked by jdonnelly@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm (do we need to send phone number? that's another pretty uniquely identifying bit of info it would be nice to strip) https://codereview.chromium.org/2349033002/diff/40001/components/autofill/con... File components/autofill/content/browser/payments/payments_client_unittest.cc (right): https://codereview.chromium.org/2349033002/diff/40001/components/autofill/con... components/autofill/content/browser/payments/payments_client_unittest.cc:207: std::string::npos); nit: verify other fields are present? verify "John" and "Smith" aren't present?
The CQ bit was checked by jdonnelly@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...
On 2016/09/22 16:16:13, Evan Stade wrote: > (do we need to send phone number? that's another pretty uniquely identifying bit > of info it would be nice to strip) Good point. Added removal of phone number as well and tests to verify. > components/autofill/content/browser/payments/payments_client_unittest.cc:207: > std::string::npos); > nit: verify other fields are present? verify "John" and "Smith" aren't present? Done.
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user. BUG=535784 ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses because they are not useful for these purposes. BUG=535784 ==========
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses because they are not useful for these purposes. BUG=535784 ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org Link to the patchset: https://codereview.chromium.org/2349033002/#ps60001 (title: "Also don't send phone numbers.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2358803005/ by gcasto@chromium.org. The reason for reverting is: This change introduced static initializers. See https://uberchromegw.corp.google.com/i/chromium/builders/Linux%20x64/builds/2.... See http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html for more details on why this matters..
Message was sent while issue was closed.
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409} ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409} ==========
The CQ bit was checked by jdonnelly@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...
Replaced std::string static initializers with char[] constants. Can you take another quick look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jdonnelly@chromium.org
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...)
The CQ bit was checked by jdonnelly@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409} ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409} ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Cr-Commit-Position: refs/heads/master@{#420409} ========== to ========== Include addresses with the getdetailsforsavecard Payments RPC. These addresses will be used by Payments to determine the correct legal documents to show the user and to verify that the addresses are valid for their purposes (and thus won't fail on any subsequent save). Names and phone numbers are removed from the addresses before uploading since they are not useful for these purposes. BUG=535784 Committed: https://crrev.com/da7eb69a380d53eec42cc4e620e55b78e5d6fa82 Committed: https://crrev.com/005d6cd78d14cb1d68a92a0d8dd9a98595b54d80 Cr-Original-Commit-Position: refs/heads/master@{#420409} Cr-Commit-Position: refs/heads/master@{#420893} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/005d6cd78d14cb1d68a92a0d8dd9a98595b54d80 Cr-Commit-Position: refs/heads/master@{#420893} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
