Fixing regression in phone number collapse on detail page.
The case of collapsing phone numbers with different formats was broken due to
another bug fix. e.g...
(555) 555-5555
555-555-5555
would not collapse when they should.
The second problem was that an inconsistency was inadvertently introduced when
deciding what numbers are equal.
rule 1)
+14155551212 == 4155551212
rule 2)
14155551212 != 4155551212
implicitly a third rule was in effect....
rule 3)
+14155551212 == 14155551212
By transitive equality, then all version of the number are equal which broke
rule 2. This could be seen in the UI when all 3 version of the number exists
and they are all collapsed into 1.
Bug: 8621998
Bug: 7519057
Change-Id: Iafb36fbdc72f9a76d9313811894b57aafebb4f35
diff --git a/src/com/android/contacts/common/MoreContactUtils.java b/src/com/android/contacts/common/MoreContactUtils.java
index fe414e1..32a9d7c 100644
--- a/src/com/android/contacts/common/MoreContactUtils.java
+++ b/src/com/android/contacts/common/MoreContactUtils.java
@@ -82,7 +82,8 @@
if (TextUtils.equals(dataPart1, dataPart2)) continue;
// do a full parse of the numbers
- switch (util.isNumberMatch(dataPart1, dataPart2)) {
+ final PhoneNumberUtil.MatchType result = util.isNumberMatch(dataPart1, dataPart2);
+ switch (result) {
case NOT_A_NUMBER:
// don't understand the numbers? let's play it safe
return false;
@@ -95,10 +96,61 @@
// For NANP phone numbers, match when one has +1 and the other does not.
// In this case, prefer the +1 version.
if (util.parse(dataPart1, null).getCountryCode() == 1) {
+ // At this point, the numbers can be either case 1 or 2 below....
+ //
+ // case 1)
+ // +14155551212 <--- country code 1
+ // 14155551212 <--- 1 is trunk prefix, not country code
+ //
+ // and
+ //
+ // case 2)
+ // +14155551212
+ // 4155551212
+ //
+ // From b/7519057, case 2 needs to be equal. But also that bug, case 3
+ // below should not be equal.
+ //
+ // case 3)
+ // 14155551212
+ // 4155551212
+ //
+ // So in order to make sure transitive equality is valid, case 1 cannot
+ // be equal. Otherwise, transitive equality breaks and the following
+ // would all be collapsed:
+ // 4155551212 |
+ // 14155551212 |----> +14155551212
+ // +14155551212 |
+ //
+ // With transitive equality, the collapsed values should be:
+ // 4155551212 | 14155551212
+ // 14155551212 |----> +14155551212
+ // +14155551212 |
+
+ // Distinguish between case 1 and 2 by checking for trunk prefix '1'
+ // at the start of number 2.
+ if (dataPart2.trim().charAt(0) == '1') {
+ // case 1
+ return false;
+ }
break;
}
} catch (NumberParseException e) {
- // Ignore
+ // This is the case where the first number does not have a country code.
+ // examples:
+ // (123) 456-7890 & 123-456-7890 (collapse)
+ // 0049 (8092) 1234 & +49/80921234 (unit test says do not collapse)
+
+ // Check the second number. If it also does not have a country code, then
+ // we should collapse. If it has a country code, then it's a different
+ // number and we should not collapse (this conclusion is based on an
+ // existing unit test).
+ try {
+ util.parse(dataPart2, null);
+ } catch (NumberParseException e2) {
+ // Number 2 also does not have a country. Collapse.
+ break;
+ }
}
return false;
case SHORT_NSN_MATCH:
diff --git a/tests/src/com/android/contacts/common/MoreContactUtilsTest.java b/tests/src/com/android/contacts/common/MoreContactUtilsTest.java
new file mode 100644
index 0000000..8d74455
--- /dev/null
+++ b/tests/src/com/android/contacts/common/MoreContactUtilsTest.java
@@ -0,0 +1,176 @@
+package com.android.contacts.common;
+
+import android.provider.ContactsContract.CommonDataKinds.Phone;
+import android.test.suitebuilder.annotation.SmallTest;
+
+import junit.framework.TestCase;
+
+/**
+ * Tests for MoreContactsUtils.
+ */
+@SmallTest
+public class MoreContactUtilsTest extends TestCase {
+
+ public void testShouldCollapse() throws Exception {
+ assertCollapses("1", true, null, null, null, null);
+ assertCollapses("2", true, "a", "b", "a", "b");
+
+ assertCollapses("11", false, "a", null, null, null);
+ assertCollapses("12", false, null, "a", null, null);
+ assertCollapses("13", false, null, null, "a", null);
+ assertCollapses("14", false, null, null, null, "a");
+
+ assertCollapses("21", false, "a", "b", null, null);
+ assertCollapses("22", false, "a", "b", "a", null);
+ assertCollapses("23", false, "a", "b", null, "b");
+ assertCollapses("24", false, "a", "b", "a", "x");
+ assertCollapses("25", false, "a", "b", "x", "b");
+
+ assertCollapses("31", false, null, null, "a", "b");
+ assertCollapses("32", false, "a", null, "a", "b");
+ assertCollapses("33", false, null, "b", "a", "b");
+ assertCollapses("34", false, "a", "x", "a", "b");
+ assertCollapses("35", false, "x", "b", "a", "b");
+
+ assertCollapses("41", true, Phone.CONTENT_ITEM_TYPE, null, Phone.CONTENT_ITEM_TYPE, null);
+ assertCollapses("42", true, Phone.CONTENT_ITEM_TYPE, "1", Phone.CONTENT_ITEM_TYPE, "1");
+
+ assertCollapses("51", false, Phone.CONTENT_ITEM_TYPE, "1", Phone.CONTENT_ITEM_TYPE, "2");
+ assertCollapses("52", false, Phone.CONTENT_ITEM_TYPE, "1", Phone.CONTENT_ITEM_TYPE, null);
+ assertCollapses("53", false, Phone.CONTENT_ITEM_TYPE, null, Phone.CONTENT_ITEM_TYPE, "2");
+
+ // Test phone numbers
+ assertCollapses("60", true, Phone.CONTENT_ITEM_TYPE, "1234567", Phone.CONTENT_ITEM_TYPE,
+ "1234567");
+ assertCollapses("61", false, Phone.CONTENT_ITEM_TYPE, "1234567", Phone.CONTENT_ITEM_TYPE,
+ "1234568");
+ assertCollapses("62", true, Phone.CONTENT_ITEM_TYPE, "1234567;0", Phone.CONTENT_ITEM_TYPE,
+ "1234567;0");
+ assertCollapses("63", false, Phone.CONTENT_ITEM_TYPE, "1234567;89321",
+ Phone.CONTENT_ITEM_TYPE, "1234567;89322");
+ assertCollapses("64", true, Phone.CONTENT_ITEM_TYPE, "1234567;89321",
+ Phone.CONTENT_ITEM_TYPE, "1234567;89321");
+ assertCollapses("65", false, Phone.CONTENT_ITEM_TYPE, "1234567;0111111111",
+ Phone.CONTENT_ITEM_TYPE, "1234567;");
+ assertCollapses("66", false, Phone.CONTENT_ITEM_TYPE, "12345675426;91970xxxxx",
+ Phone.CONTENT_ITEM_TYPE, "12345675426");
+ assertCollapses("67", false, Phone.CONTENT_ITEM_TYPE, "12345675426;23456xxxxx",
+ Phone.CONTENT_ITEM_TYPE, "12345675426;234567xxxx");
+ assertCollapses("68", true, Phone.CONTENT_ITEM_TYPE, "1234567;1234567;1234567",
+ Phone.CONTENT_ITEM_TYPE, "1234567;1234567;1234567");
+ assertCollapses("69", false, Phone.CONTENT_ITEM_TYPE, "1234567;1234567;1234567",
+ Phone.CONTENT_ITEM_TYPE, "1234567;1234567");
+
+ // test some numbers with country and area code
+ assertCollapses("70", true, Phone.CONTENT_ITEM_TYPE, "+49 (89) 12345678",
+ Phone.CONTENT_ITEM_TYPE, "+49 (89) 12345678");
+ assertCollapses("71", true, Phone.CONTENT_ITEM_TYPE, "+49 (89) 12345678",
+ Phone.CONTENT_ITEM_TYPE, "+49 (89)12345678");
+ assertCollapses("72", true, Phone.CONTENT_ITEM_TYPE, "+49 (8092) 1234",
+ Phone.CONTENT_ITEM_TYPE, "+49 (8092)1234");
+ assertCollapses("73", false, Phone.CONTENT_ITEM_TYPE, "0049 (8092) 1234",
+ Phone.CONTENT_ITEM_TYPE, "+49/80921234");
+ assertCollapses("74", false, Phone.CONTENT_ITEM_TYPE, "+49 (89) 12345678",
+ Phone.CONTENT_ITEM_TYPE, "+49 (89) 12345679");
+
+ // test special handling of collapsing country code for NANP region only
+ // This is non symmetrical, because we prefer the number with the +1.
+ assertEquals("100", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "+1 (415) 555-1212", Phone.CONTENT_ITEM_TYPE, "(415) 555-1212"));
+ assertEquals("101", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "+14155551212", Phone.CONTENT_ITEM_TYPE, "4155551212"));
+ assertEquals("102", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "(415) 555-1212", Phone.CONTENT_ITEM_TYPE, "+1 (415) 555-1212"));
+ assertEquals("103", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "4155551212", Phone.CONTENT_ITEM_TYPE, "+14155551212"));
+ // Require explicit +1 country code declaration to collapse
+ assertEquals("104", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "1-415-555-1212", Phone.CONTENT_ITEM_TYPE, "415-555-1212"));
+ assertEquals("105", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "14155551212", Phone.CONTENT_ITEM_TYPE, "4155551212"));
+ assertEquals("106", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "+1 (415) 555-1212", Phone.CONTENT_ITEM_TYPE, " 1 (415) 555-1212"));
+ assertEquals("107", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "+14155551212", Phone.CONTENT_ITEM_TYPE, " 14155551212"));
+ assertEquals("108", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "1 (415) 555-1212", Phone.CONTENT_ITEM_TYPE, "+1 (415) 555-1212"));
+ assertEquals("109", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "14155551212", Phone.CONTENT_ITEM_TYPE, "+14155551212"));
+
+ // test some numbers with wait symbol and area code
+ assertCollapses("200", true, Phone.CONTENT_ITEM_TYPE, "+49 (8092) 1234;89321",
+ Phone.CONTENT_ITEM_TYPE, "+49/80921234;89321");
+ assertCollapses("201", false, Phone.CONTENT_ITEM_TYPE, "+49 (8092) 1234;89321",
+ Phone.CONTENT_ITEM_TYPE, "+49/80921235;89321");
+ assertCollapses("202", false, Phone.CONTENT_ITEM_TYPE, "+49 (8092) 1234;89322",
+ Phone.CONTENT_ITEM_TYPE, "+49/80921234;89321");
+ assertCollapses("203", true, Phone.CONTENT_ITEM_TYPE, "1234567;+49 (8092) 1234",
+ Phone.CONTENT_ITEM_TYPE, "1234567;+49/80921234");
+
+ assertCollapses("300", true, Phone.CONTENT_ITEM_TYPE, "", Phone.CONTENT_ITEM_TYPE, "");
+
+ assertCollapses("301", false, Phone.CONTENT_ITEM_TYPE, "1", Phone.CONTENT_ITEM_TYPE, "");
+
+ assertCollapses("302", false, Phone.CONTENT_ITEM_TYPE, "", Phone.CONTENT_ITEM_TYPE, "1");
+
+ assertCollapses("303", true, Phone.CONTENT_ITEM_TYPE, "---", Phone.CONTENT_ITEM_TYPE, "---");
+
+ assertCollapses("304", false, Phone.CONTENT_ITEM_TYPE, "1-/().", Phone.CONTENT_ITEM_TYPE,
+ "--$%1");
+
+ // Test numbers using keypad letters. This is non-symmetrical, because we prefer
+ // the version with letters.
+ assertEquals("400", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "abcdefghijklmnopqrstuvwxyz", Phone.CONTENT_ITEM_TYPE,
+ "22233344455566677778889999"));
+ assertEquals("401", false, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "22233344455566677778889999", Phone.CONTENT_ITEM_TYPE,
+ "abcdefghijklmnopqrstuvwxyz"));
+
+ assertCollapses("402", false, Phone.CONTENT_ITEM_TYPE, "1;2", Phone.CONTENT_ITEM_TYPE,
+ "12");
+
+ assertCollapses("403", false, Phone.CONTENT_ITEM_TYPE, "1,2", Phone.CONTENT_ITEM_TYPE,
+ "12");
+ }
+
+ public void testShouldCollapse_collapsesSameNumberWithDifferentFormats() {
+ assertEquals("1", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "555-1212", Phone.CONTENT_ITEM_TYPE, "5551212"));
+ assertEquals("1", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "415-555-1212", Phone.CONTENT_ITEM_TYPE, "(415) 555-1212"));
+ assertEquals("2", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "4155551212", Phone.CONTENT_ITEM_TYPE, "(415) 555-1212"));
+ assertEquals("3", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "1-415-555-1212", Phone.CONTENT_ITEM_TYPE, "1 (415) 555-1212"));
+ assertEquals("4", true, MoreContactUtils.shouldCollapse(Phone.CONTENT_ITEM_TYPE,
+ "14155551212", Phone.CONTENT_ITEM_TYPE, "1 (415) 555-1212"));
+ }
+
+ private void assertCollapses(String message, boolean expected, CharSequence mimetype1,
+ CharSequence data1, CharSequence mimetype2, CharSequence data2) {
+ assertEquals(message, expected, MoreContactUtils.shouldCollapse(mimetype1, data1, mimetype2,
+ data2));
+ assertEquals(message, expected, MoreContactUtils.shouldCollapse(mimetype2, data2, mimetype1,
+ data1));
+
+ // If data1 and data2 are the same instance, make sure the same test passes with different
+ // instances.
+ if (data1 == data2 && data1 != null) {
+ // Create a different instance
+ final CharSequence data2_newref = new StringBuilder(data2).append("").toString();
+
+ if (data1 == data2_newref) {
+ // In some cases no matter what we do the runtime reuses the same instance, so
+ // we can't do the "different instance" test.
+ return;
+ }
+
+ // we have two different instances, now make sure we get the same result as before
+ assertEquals(message, expected, MoreContactUtils.shouldCollapse(mimetype1, data1,
+ mimetype2, data2_newref));
+ assertEquals(message, expected, MoreContactUtils.shouldCollapse(mimetype2, data2_newref,
+ mimetype1, data1));
+ }
+ }
+}