Rework handling of phone URNs

nyaruka · May 7, 2024 · af69a1d · af69a1d
1 parent 6caa19e
commit af69a1d
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 82 deletions.
diff --git a/urns/phone.go b/urns/phone.go
@@ -1,49 +1,62 @@
 package urns
 
 import (
+	"regexp"
 	"strconv"
 	"strings"
 
+	"github.com/nyaruka/gocommon/i18n"
 	"github.com/nyaruka/phonenumbers"
 	"github.com/pkg/errors"
 )
 
-// FromLocalPhone returns a validated tel URN
-func FromLocalPhone(number string, country string) (URN, error) {
-	path, err := ParsePhone(number, country)
-	if err != nil {
-		return NilURN, err
+var nonTelCharsRegex = regexp.MustCompile(`[^0-9A-Za-z]`)
+
+// ParsePhone returns a validated phone URN. If it can parse a possible number then that is used.. otherwise any value
+// that validates as a phone URN is used.
+func ParsePhone(raw string, country i18n.Country) (URN, error) {
+	// strip all non-tel characters.. only preserving an optional leading +
+	raw = strings.TrimSpace(raw)
+	hasPlus := strings.HasPrefix(raw, "+")
+	raw = nonTelCharsRegex.ReplaceAllString(raw, "")
+	if hasPlus {
+		raw = "+" + raw
 	}
 
-	return NewURNFromParts(Phone, path, "", "")
-}
-
-// ToLocalPhone converts a phone URN to a local number in the given country
-func ToLocalPhone(u URN, country string) string {
-	_, path, _, _ := u.ToParts()
+	number, err := parsePhoneOrShortcode(raw, country)
+	if err != nil {
+		if err == phonenumbers.ErrInvalidCountryCode {
+			return NilURN, errors.New("invalid country code")
+		}
 
-	parsed, err := phonenumbers.Parse(path, country)
-	if err == nil {
-		return strconv.FormatUint(parsed.GetNationalNumber(), 10)
+		return NewURNFromParts(Phone, raw, "", "")
 	}
-	return path
+
+	return NewURNFromParts(Phone, number, "", "")
 }
 
-// ParsePhone tries to parse the given string as a phone number and if successful returns it as E164
-func ParsePhone(s, country string) (string, error) {
-	parsed, err := phonenumbers.Parse(s, country)
+// tries to extract a valid phone number or shortcode from the given string
+func parsePhoneOrShortcode(raw string, country i18n.Country) (string, error) {
+	parsed, err := phonenumbers.Parse(raw, string(country))
 	if err != nil {
-		return "", errors.Wrap(err, "unable to parse number")
+		return "", err
 	}
 
-	if phonenumbers.IsPossibleNumberWithReason(parsed) != phonenumbers.IS_POSSIBLE {
-		// if it's not a possible number, try adding a + and parsing again
-		if !strings.HasPrefix(s, "+") {
-			return ParsePhone("+"+s, country)
-		}
+	if phonenumbers.IsPossibleNumberWithReason(parsed) == phonenumbers.IS_POSSIBLE {
+		return phonenumbers.Format(parsed, phonenumbers.E164), nil
+	}
 
-		return "", errors.New("not a possible number")
+	if phonenumbers.IsPossibleShortNumberForRegion(parsed, string(country)) {
+		return phonenumbers.Format(parsed, phonenumbers.NATIONAL), nil
 	}
 
-	return phonenumbers.Format(parsed, phonenumbers.E164), nil
+	return "", errors.New("unable to parse phone number or shortcode")
+}
+
+func toLocalPhone(path string, country i18n.Country) string {
+	parsed, err := phonenumbers.Parse(path, string(country))
+	if err == nil {
+		return strconv.FormatUint(parsed.GetNationalNumber(), 10)
+	}
+	return path
 }
diff --git a/urns/phone_test.go b/urns/phone_test.go
@@ -1,67 +1,75 @@
-package urns_test
+package urns
 
 import (
 	"testing"
 
-	"github.com/nyaruka/gocommon/urns"
+	"github.com/nyaruka/gocommon/i18n"
 	"github.com/stretchr/testify/assert"
 )
 
-func TestFromLocalPhone(t *testing.T) {
+func TestParsePhone(t *testing.T) {
 	testCases := []struct {
-		number   string
-		country  string
-		expected urns.URN
-		hasError bool
+		input    string
+		country  i18n.Country
+		expected URN
 	}{
-		{"tel:0788383383", "RW", "tel:+250788383383", false},
-		{"tel: +250788383383 ", "KE", "tel:+250788383383", false}, // already has country code
-		{"tel:(917)992-5253", "US", "tel:+19179925253", false},
-		{"tel:800-CABBAGE", "US", "tel:+18002222243", false},
-		{"tel:+62877747666", "ID", "tel:+62877747666", false},
-		{"tel:0877747666", "ID", "tel:+62877747666", false},
-		{"tel:07531669965", "GB", "tel:+447531669965", false},
-		{"tel:263780821000", "ZW", "tel:+263780821000", false},
+		{" 0788383383 ", "RW", "tel:+250788383383"},
+		{"+250788383383 ", "RW", "tel:+250788383383"}, // already has country code and leading +
+		{"250788383383 ", "RW", "tel:+250788383383"},  // already has country code and no leading +
+		{"+250788383383 ", "KE", "tel:+250788383383"}, // already has a different country code
+		{"(917)992-5253", "US", "tel:+19179925253"},
+		{"800-CABBAGE", "US", "tel:+18002222243"},
+		{"+62877747666", "ID", "tel:+62877747666"},
+		{"0877747666", "ID", "tel:+62877747666"},
+		{"07531669965", "GB", "tel:+447531669965"},
+		{"263780821000", "ZW", "tel:+263780821000"},
+
+		{"1", "RW", "tel:1"},
+		{"123456", "RW", "tel:123456"},
+		{"mtn", "RW", "tel:mtn"},
+		{"!mtn!", "RW", "tel:mtn"}, // non tel chars stripped
 
-		{"0788383383", "ZZ", urns.NilURN, true}, // invalid country code
-		{"1", "RW", urns.NilURN, true},
-		{"123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890", "RW", urns.NilURN, true},
+		{"0788383383", "ZZ", NilURN}, // invalid country code
+		{"1234567890123456789012345678901234567890123456789012345678901234567890123456789", "RW", NilURN}, // too long
 	}
 
 	for i, tc := range testCases {
-		urn, err := urns.FromLocalPhone(tc.number, tc.country)
+		urn, err := ParsePhone(tc.input, tc.country)
 
-		if tc.hasError {
-			assert.Error(t, err, "%d: expected error for %s, %s", i, tc.number, tc.country)
+		if tc.expected == NilURN {
+			assert.Error(t, err, "%d: expected error for %s, %s", i, tc.input, tc.country)
 		} else {
-			assert.NoError(t, err, "%d: unexpected error for %s, %s", i, tc.number, tc.country)
-			assert.Equal(t, tc.expected, urn, "%d: created URN mismatch for %s, %s", i, tc.number, tc.country)
+			assert.NoError(t, err, "%d: unexpected error for %s, %s", i, tc.input, tc.country)
+			assert.Equal(t, tc.expected, urn, "%d: created URN mismatch for %s, %s", i, tc.input, tc.country)
 		}
 	}
 }
 
-func TestParsePhone(t *testing.T) {
+func TestParsePhoneOrShortcode(t *testing.T) {
 	tcs := []struct {
-		input   string
-		country string
-		parsed  string
+		input    string
+		country  i18n.Country
+		expected string
 	}{
 		{"+250788123123", "", "+250788123123"},    // international number fine without country
-		{"+250 788 123-123", "", "+250788123123"}, // fine if not E164 formatted
-		{"0788123123", "RW", "+250788123123"},
-		{"206 555 1212", "US", "+12065551212"},
-		{"12065551212", "US", "+12065551212"}, // country code but no +
-		{"5912705", "US", ""},                 // is only possible as a local number so ignored
-		{"10000", "US", ""},
+		{"+250 788 123-123", "", "+250788123123"}, // still fine if not E164 formatted
+
+		{"0788123123", "RW", "+250788123123"},     // country code added
+		{" (206)555-1212 ", "US", "+12065551212"}, // punctiation removed
+		{"800-CABBAGE", "US", "+18002222243"},     // letters converted to numbers
+		{"12065551212", "US", "+12065551212"},     // country code but no +
+		{"10000", "US", "10000"},                  // valid short code for US
+
+		{"5912705", "US", ""}, // is only possible as a local number so ignored
 	}
 
 	for _, tc := range tcs {
-		if tc.parsed != "" {
-			parsed, err := urns.ParsePhone(tc.input, tc.country)
+		parsed, err := parsePhoneOrShortcode(tc.input, tc.country)
+
+		if tc.expected != "" {
 			assert.NoError(t, err, "unexpected error for '%s'", tc.input)
-			assert.Equal(t, parsed, tc.parsed, "result mismatch for '%s'", tc.input)
+			assert.Equal(t, tc.expected, parsed, "result mismatch for '%s'", tc.input)
 		} else {
-			_, err := urns.ParsePhone(tc.input, tc.country)
 			assert.Error(t, err, "expected error for '%s'", tc.input)
 		}
 	}

diff --git a/urns/schemes.go b/urns/schemes.go
@@ -4,11 +4,11 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/nyaruka/gocommon/i18n"
 	"github.com/nyaruka/phonenumbers"
 )
 
 var allDigitsRegex = regexp.MustCompile(`^[0-9]+$`)
-var nonTelCharsRegex = regexp.MustCompile(`[^0-9A-Z]`)
 
 var emailRegex = regexp.MustCompile(`^[^\s@]+@[^\s@]+$`)
 var freshchatRegex = regexp.MustCompile(`^[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}/[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}$`)
@@ -59,6 +59,7 @@ type Scheme struct {
 	Normalize func(string) string
 	Validate  func(string) bool
 	Format    func(string) string
+	Localize  func(string, i18n.Country) string
 }
 
 var Discord = &Scheme{
@@ -115,15 +116,11 @@ var Line = &Scheme{
 var Phone = &Scheme{
 	Prefix: "tel",
 	Normalize: func(path string) string {
-		e164, err := ParsePhone(path, "")
-		if err != nil {
-			// could be a short code so uppercase and remove non alphanumeric characters
-			return nonTelCharsRegex.ReplaceAllString(strings.ToUpper(path), "")
-		}
-
-		return e164
+		// might have alpha characters in it
+		return strings.ToUpper(path)
 	},
 	Validate: func(path string) bool { return telRegex.MatchString(path) },
+	Localize: func(path string, country i18n.Country) string { return toLocalPhone(path, country) },
 	Format: func(path string) string {
 		parsed, err := phonenumbers.Parse(path, "")
 		if err != nil {

diff --git a/urns/urns.go b/urns/urns.go
@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"net/url"
 	"strings"
+
+	"github.com/nyaruka/gocommon/i18n"
 )
 
 const (
@@ -150,6 +152,18 @@ func (u URN) Identity() URN {
 // String returns the string representation of this URN
 func (u URN) String() string { return string(u) }
 
+// Localize returns a localized version of the URN for the given country (only currently used for phone numbers)
+func (u URN) Localize(country i18n.Country) string {
+	scheme, path, _, _ := u.ToParts()
+	s := schemes[scheme]
+
+	if s != nil && s.Localize != nil {
+		return s.Localize(path, country)
+	}
+
+	return path
+}
+
 // Format formats this URN as a human friendly string
 func (u URN) Format() string {
 	scheme, path, _, display := u.ToParts()

diff --git a/urns/urns_test.go b/urns/urns_test.go
@@ -88,24 +88,15 @@ func TestNormalize(t *testing.T) {
 		rawURN   urns.URN
 		expected urns.URN
 	}{
-		// valid tel numbers
-		{"tel: +250788383383 ", "tel:+250788383383"},
+		// valid tel numbers left as they are
 		{"tel:+250788383383", "tel:+250788383383"},
-		{"tel:250788383383", "tel:+250788383383"},
-		{"tel:(917)992-5253", "tel:+19179925253"},
-		{"tel:19179925253", "tel:+19179925253"},
 		{"tel:+62877747666", "tel:+62877747666"},
-		{"tel:62877747666", "tel:+62877747666"},
-		{"tel:0877747666", "tel:+62877747666"},
-		{"tel:07531669965", "tel:+447531669965"},
-		{"tel:22658125926", "tel:+22658125926"},
-		{"tel:263780821000", "tel:+263780821000"},
 		{"tel:+2203693333", "tel:+2203693333"},
 
 		// non-standard phone numbers
 		{"tel:12345", "tel:12345"},
 		{"tel:mtn", "tel:MTN"},
-		{"tel:+12345678901234567890", "tel:12345678901234567890"},
+		{"tel:+12345678901234567890", "tel:+12345678901234567890"},
 
 		// twitter handles remove @
 		{"twitter: @jimmyJO", "twitter:jimmyjo"},