Chrome's Elo card mapping code is incorrect |
|||||||
Issue descriptionThe six-digit ELO card mappings that begin with 4 (link 1) are getting superseded by the initial check that all Visa cards begin with 4 (link 2). It looks like Union Pay's "62" is clobbering the Elo "627780" option as well (link 3). These become a bigger issue in light of http://crbug.com/868552 . Could this be fixed and merged into M69? [1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/credit_card.cc?l=289-290&rcl=9db0a14b711415d66efdcec8eb88971a9b2efdb0 [2] https://cs.chromium.org/chromium/src/components/autofill/core/browser/credit_card.cc?l=215&rcl=9db0a14b711415d66efdcec8eb88971a9b2efdb0 [3] https://cs.chromium.org/chromium/src/components/autofill/core/browser/credit_card.cc?l=237&rcl=9db0a14b711415d66efdcec8eb88971a9b2efdb0
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/314c21b1f6d6b5c676455969bc6b4446b0fd38b8 commit 314c21b1f6d6b5c676455969bc6b4446b0fd38b8 Author: Jared Saul <jsaul@google.com> Date: Thu Aug 02 20:30:09 2018 [Autofill] Fix Elo card mapping (Visa's "4" and Union Pay's "62" were bypassing it) Bug: 868566 Change-Id: Icf41453cc6b4528463e32338d30eef0c037ce6b4 Reviewed-on: https://chromium-review.googlesource.com/1159738 Commit-Queue: Jared Saul <jsaul@google.com> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Commit-Position: refs/heads/master@{#580301} [modify] https://crrev.com/314c21b1f6d6b5c676455969bc6b4446b0fd38b8/components/autofill/core/browser/credit_card.cc [modify] https://crrev.com/314c21b1f6d6b5c676455969bc6b4446b0fd38b8/components/autofill/core/browser/credit_card_unittest.cc
,
Aug 2
Requesting merge to M69. Chrome was incorrectly identifying Elo cards in 3/6 cases, with additional problems in a fourth case if the card number contained spaces or a hyphen. This fix is also important in the context of http://crbug.com/868552 .
,
Aug 2
This bug requires manual review: Less than 29 days to go before AppStore submit on M69 Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 3
Before we approve merge to M69, please answer followings: * Is this M69 regression? Is it critical? * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M69? * Any other important details to justify the merge. Please note M69 is already in Beta, so merge bar is very high. Thank you.
,
Aug 3
Could you pls point the launch bug for this? Also is this feature behind finch?
,
Aug 3
Answering questions in #c5 and #c6: * It's not specifically an M69 regression. It's not critical in terms of preventing crashes, but it does cause Issue 868552 's merge to be much less effective, though, which is preventing a bad user experience. * I have tested the change in Canary 70.0.3511.0 and it works properly. There are a high amount of automated tests for this feature (there were previously some gaps in the test coverage which I have remedied with this CL). * This particular CL was a bugfix that is NOT behind a Finch flag. My Autofill reviewer determined it to be "simple enough for a merge" though. * Still discussing with durgapandey@ if launch bug is necessary
,
Aug 3
Per the new Chrome launch process, https://docs.google.com/document/d/1hJ1U8-7DNa7lGfTJWRgSgqQyNnOFO4Ks5Czr1-3--8I/edit# I'm assuming we don't need a launch bug for this since it's a bug fix. Krishna can you pls confirm?
,
Aug 3
Is this fall under "bug fixes or other trivial changes"? If yes, then no launch bug is needed. +durgapandey@.
,
Aug 3
Yes, it does. Thanks
,
Aug 3
Approving merge to M69 branch 3497 based on comment #7, #8, #10 and per internal email thread and offline chat with durgapandey@. This is small bugfix; self-contained but not flag protected, well-tested with new automated tests.
,
Aug 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9f60ddaa46395b1a7c6e25c3b837495d0001de7 commit f9f60ddaa46395b1a7c6e25c3b837495d0001de7 Author: sebsg <sebsg@chromium.org> Date: Fri Aug 03 21:27:58 2018 Merge-69 [AF] Fix Elo card mapping. Visa's "4" and Union Pay's "62" were bypassing it TBR=jsaul@google.com (cherry picked from commit 314c21b1f6d6b5c676455969bc6b4446b0fd38b8) Bug: 868566 Change-Id: Icf41453cc6b4528463e32338d30eef0c037ce6b4 Reviewed-on: https://chromium-review.googlesource.com/1159738 Commit-Queue: Jared Saul <jsaul@google.com> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#580301} Reviewed-on: https://chromium-review.googlesource.com/1162761 Cr-Commit-Position: refs/branch-heads/3497@{#396} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/f9f60ddaa46395b1a7c6e25c3b837495d0001de7/components/autofill/core/browser/credit_card.cc [modify] https://crrev.com/f9f60ddaa46395b1a7c6e25c3b837495d0001de7/components/autofill/core/browser/credit_card_unittest.cc
,
Aug 23
Closing bug as this was merged into M69. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by jsaul@google.com
, Aug 2Owner: jsaul@google.com