Commit 80199d3e authored by Caitlin Fischer's avatar Caitlin Fischer Committed by Commit Bot

[Autofill] Fixed logging of network matches and mismatches.

Logging will now only occur when one of the two cards being compared is
a masked card and the two cards' last four digits match.

Previously, logging was occuring regardless of whether or not the last
four digits matched.

Change-Id: I1a0d9d44df52cf9b6853dbaab041d05a1585e230
Reviewed-on: https://chromium-review.googlesource.com/c/1486431Reviewed-by: default avatarTommy Martino <tmartino@chromium.org>
Reviewed-by: default avatarFabio Tirelo <ftirelo@chromium.org>
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Cr-Commit-Position: refs/heads/master@{#635766}
parent 897455e0
...@@ -661,12 +661,16 @@ bool CreditCard::HasSameNumberAs(const CreditCard& other) const { ...@@ -661,12 +661,16 @@ bool CreditCard::HasSameNumberAs(const CreditCard& other) const {
// cards matches. // cards matches.
if (record_type() == MASKED_SERVER_CARD || if (record_type() == MASKED_SERVER_CARD ||
other.record_type() == MASKED_SERVER_CARD) { other.record_type() == MASKED_SERVER_CARD) {
bool last_four_digits_match = LastFourDigits() == other.LastFourDigits();
// The below metric is logged because this function previously compared // The below metric is logged because this function previously compared
// cards' last four digits and networks if one card was masked. It may be // cards' last four digits and networks if one card was masked. It may be
// useful to know how often networks match; however, it is expected that the // useful to know how often networks match when the last four digits match.
// number of discrepanies will be low. // It is expected that when two cards' last four digits are the same, their
AutofillMetrics::LogMaskedCardComparisonNetworksMatch( // networks will almost always match, too.
NetworkForDisplay() == other.NetworkForDisplay()); if (last_four_digits_match) {
AutofillMetrics::LogMaskedCardComparisonNetworksMatch(
NetworkForDisplay() == other.NetworkForDisplay());
}
bool months_match = expiration_month() == other.expiration_month() || bool months_match = expiration_month() == other.expiration_month() ||
expiration_month() == 0 || expiration_month() == 0 ||
...@@ -675,8 +679,7 @@ bool CreditCard::HasSameNumberAs(const CreditCard& other) const { ...@@ -675,8 +679,7 @@ bool CreditCard::HasSameNumberAs(const CreditCard& other) const {
bool years_match = expiration_year() == other.expiration_year() || bool years_match = expiration_year() == other.expiration_year() ||
expiration_year() == 0 || other.expiration_year() == 0; expiration_year() == 0 || other.expiration_year() == 0;
return LastFourDigits() == other.LastFourDigits() && months_match && return last_four_digits_match && months_match && years_match;
years_match;
} }
return StripSeparators(number_) == StripSeparators(other.number_); return StripSeparators(number_) == StripSeparators(other.number_);
......
...@@ -624,12 +624,13 @@ TEST(CreditCardTest, HasSameNumberAs_LogMaskedCardComparisonNetworksMatch) { ...@@ -624,12 +624,13 @@ TEST(CreditCardTest, HasSameNumberAs_LogMaskedCardComparisonNetworksMatch) {
CreditCard b(base::GenerateGUID(), std::string()); CreditCard b(base::GenerateGUID(), std::string());
a.set_record_type(CreditCard::MASKED_SERVER_CARD); a.set_record_type(CreditCard::MASKED_SERVER_CARD);
a.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
a.SetNetworkForMaskedCard(kVisaCard); a.SetNetworkForMaskedCard(kVisaCard);
// CreditCard b's network is set to kVisaCard because it starts with 4, so the // CreditCard b's network is set to kVisaCard because it starts with 4, so the
// two cards have the same network. // two cards have the same network.
b.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111")); b.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
EXPECT_FALSE(a.HasSameNumberAs(b)); EXPECT_TRUE(a.HasSameNumberAs(b));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
"Autofill.MaskedCardComparisonNetworksMatch", true, 1); "Autofill.MaskedCardComparisonNetworksMatch", true, 1);
} }
...@@ -640,11 +641,14 @@ TEST(CreditCardTest, ...@@ -640,11 +641,14 @@ TEST(CreditCardTest,
CreditCard b(base::GenerateGUID(), std::string()); CreditCard b(base::GenerateGUID(), std::string());
a.set_record_type(CreditCard::MASKED_SERVER_CARD); a.set_record_type(CreditCard::MASKED_SERVER_CARD);
a.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
a.SetNetworkForMaskedCard(kDiscoverCard); a.SetNetworkForMaskedCard(kDiscoverCard);
// CreditCard b's network is set to kVisaCard because it starts with 4. // CreditCard b's network is set to kVisaCard because it starts with 4. The
// two cards have the same last four digits, but their networks are different,
// so this discrepancy should be logged.
b.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111")); b.SetRawInfo(CREDIT_CARD_NUMBER, ASCIIToUTF16("4111111111111111"));
base::HistogramTester histogram_tester; base::HistogramTester histogram_tester;
EXPECT_FALSE(a.HasSameNumberAs(b)); EXPECT_TRUE(a.HasSameNumberAs(b));
histogram_tester.ExpectUniqueSample( histogram_tester.ExpectUniqueSample(
"Autofill.MaskedCardComparisonNetworksMatch", false, 1); "Autofill.MaskedCardComparisonNetworksMatch", false, 1);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment