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

2 files changed, 230 insertions, 2 deletions

diff --git a/src/com/android/contacts/common/MoreContactUtils.java b/src/com/android/contacts/common/MoreContactUtils.java
index fe414e15..32a9d7ce 100644
--- a/src/com/android/contacts/common/MoreContactUtils.java
+++ b/src/com/android/contacts/common/MoreContactUtils.java
@@ -82,7 +82,8 @@ public class MoreContactUtils {
             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 @@ public class MoreContactUtils {
                         // 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 00000000..8d74455d
--- /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));
+        }
+    }
+}