Issue #46 (Wrong result from local number) fix #47

aazhevsky · 2023-11-24T16:38:52Z

Hello,

These changes will fix the Wrong result obtained from local number #46 issue.

After applying the changes, the following local US phone numbers are parsed successfully:

(888) 555-5512
(408) 555-5270
(707) 555-1854

All the interfaces are kept, as well as all tests passed. The major problem is that the code of the fix is a bit clumsy. At best, CountryCodeParser.extractCountryCode should return an optional, as PhoneParser._findDestinationMetadata itself.

I consciously stopped on as it is, not to spread the changes over the whole project.

…ntry code - git ignores .DS_Store files

cedvdb · 2023-11-24T17:19:39Z

Hey, I will take a look this weekend. I agree it is a bit clumsy.

example/lib/main.dart

test/phone_number_test.dart

cedvdb · 2023-11-27T00:53:53Z

lib/src/parsers/phone_parser.dart

+    }
+
+    // rethrow previously got error
+    throw countryCodeException!;


let's not use the !

That is not possible because it leads to compilation issues. This can be avoided with the transformation CountryCodeParser.extractCountryCode and _findDestinationMetadata to return an optional, which causes to more remarkable code changes.

It seems like you were talking about the country_code_parser interface that throws when the country code is not found. Which could instead return a String?. That's a fair observation and I'm okay with that. At first glance this seems to be a small change but it'd be better if it was in a separate PR imo, just to make it easy to review.

Agreed. What do you think is the best to do in this case?

cedvdb · 2023-11-27T00:58:14Z

lib/src/metadata/metadata_matcher.dart

+    return null;
+  }
+
+  static PhoneMetadata getMatchUsingPatterns(


This does not seem to be used anywhere anymore. Therefor this can be removed and the "Strict" can be removed from the new one.

getMatchUsingPatternsStrict was introduced to avoid duplicated code in getMatchUsingPatterns. As I wrote, I tried to localize changes maximally, but I suggest redesigning interfaces to avoid exceptions and non-valid metadata matches.

You misunderstood me. It seems like getMatchUsingPatterns is not used anywhere in the code base except here. It can safely be removed. When getMatchUsingPatterns is removed we will only have getMatchUsingPatternsStrict which can be renamed to getMatchUsingPatterns

but I suggest redesigning interfaces to avoid exceptions and non-valid metadata matches.

MetadataMatcher already returns a nullable. That discussion could be made for the country code parser though

cedvdb · 2023-11-27T01:48:42Z

lib/src/parsers/phone_parser.dart

+    List<PhoneMetadata>? metadatas;
+
+    try {
+      countryCode = CountryCodeParser.extractCountryCode(phoneWithoutExitCode);


I believe there is something unsound here:

Currently the code in the parser assumes that if there was no exit code, then the next part is not a country code. (line 60)

// if there was no exit code then we assume we are dealing with a national number if (containsExitCode) { national = CountryCodeParser.removeCountryCode( withoutExitCode, destinationMetadata.countryCode, ); }

But here this assumption is not taken into account.

However, some people want to be able to parse phone number starting with a country code without the exit code see: #39 . But I believe that's outside the scope of this PR

Are there any tests that cover this case?

Are there any tests that cover this case?

I suggest not handling this case in this PR.

There is a list of test in the issue that I believe would not pass currently without the leading '+'. That is:

var expected = '+64211234567'; expect(sanitizePhoneNumber('640211234567', countryCode: IsoCode.NZ), expected); expect(sanitizePhoneNumber('64211234567', countryCode: IsoCode.NZ), expected);

This is outside the scope of this Pull request though.

Could you explain to me why don't we do (as per my first comment)

// if there was an exit code then we can safely assume that the next part is a country code. If no country code is found then it is invalid. if (phoneHadExitCode) { countryCode = CountryCodeParser.extractCountryCode(phoneWithoutExitCode); }

Earlier, there was no difference in whether the phone had an exit code or not; the parser tried to interpret the first digits as country code. So, my changes were an attempt to preserve the current way to process input.

aazhevsky · 2023-11-27T11:32:10Z

Thanks a lot!

Thank you for your efforts!

Could you also add an entry to the changelog?

Please clarify what I have to do.

- MetadataMatcher.getMatchUsingPatternsStrict renamed to MetadataMatcher.getMatchUsingPatterns

cedvdb

I'm still trying to wrap my head around some of the changes. My main question point is line 113, the last comment I added.

What would really help is if you could give a few example phone numbers in the comments related to some of the big logic blocks you added in find metadata.

Phone parsing can be difficult (but fun).

cedvdb · 2023-11-29T20:44:57Z

lib/src/parsers/phone_parser.dart

+    if (countryCode != null) {
+      metadatas = MetadataFinder.getMetadatasForCountryCode(countryCode);
+      final national = CountryCodeParser.removeCountryCode(phoneWithoutExitCode, countryCode);
+      final metadataMatch = MetadataMatcher.getMatchUsingPatterns(national, metadatas);


getMatchUsingPatternsStrict checks that the metadata matches the phone number and and the phone number is valid. Why do we need to check that it is valid if there is a match ? It could be an incomplete phone number for example. Wouldn't that negatively affect that scenario ?

For example, while trying to process local US numbers like (707) 555-1854 without an exit code, the country code was resolved as 70, but the number is not valid for IsoCode.RU. I added the strict match option to determine if the number truly belongs to the found region.

cedvdb · 2023-11-29T20:48:39Z

lib/src/parsers/phone_parser.dart

+      if (phoneHadExitCode) {
+        // 1.
+        // if phone number has the exit code, we should use it 
+        // despite caller's metadata and phone number validity


This comment is ambiguous. What is "it" in "we should use => it <=". And why "phone number validity", as metadatas you return already check validity ?

That's a part of handling a partial input that starts with an exit code.

I have updated the comment

cedvdb · 2023-11-29T21:01:24Z

lib/src/parsers/phone_parser.dart

+    List<PhoneMetadata>? metadatas;
+
+    try {
+      countryCode = CountryCodeParser.extractCountryCode(phoneWithoutExitCode);


Could you explain to me why don't we do (as per my first comment)

// if there was an exit code then we can safely assume that the next part is a country code. If no country code is found then it is invalid. if (phoneHadExitCode) { countryCode = CountryCodeParser.extractCountryCode(phoneWithoutExitCode); }

cedvdb · 2023-11-29T23:03:11Z

Hello again, sorry for the back and forth, I somewhat misunderstood your issue at first. I believe the part of the code that should be fixed for your actual issue is not exactly the part that you fixed. Correct me if I'm wrong:

This was your issue:

        expect(
            PhoneNumber.parse('(707) 555-1854', callerCountry: IsoCode.US)
              .international,
            equals('+17075551854'));

The actual culprit is this line https://github.com/cedvdb/phone_numbers_parser/blob/dev/lib/src/parsers/phone_parser.dart#L102

    if (callerMetadata != null &&
        callerNationalPrefix != null &&
        phoneWithoutExitCode.startsWith(callerNationalPrefix)) {   // will be false because it is **local** (same area in us)
      return callerMetadata;
    }

where you'd want to enter this condition. You do not enter this because the phone number is in local form. Therefor the condition: phoneWithoutExitCode.startsWith(callerNationalPrefix)) is wrong or incomplete. It should probably be phoneWithoutExitCode.startsWith(callerNationalPrefix)) || checkThatPhoneNumberIsValidForCallerMetadata())

Am I wrong that this would be the only change required to fix your issue ?

aazhevsky · 2023-11-30T10:06:10Z

Hello again, sorry for the back and forth, I somewhat misunderstood your issue at first. I believe the part of the code that should be fixed for your actual issue is not exactly the part that you fixed. Correct me if I'm wrong:

This was your issue:
        expect(
            PhoneNumber.parse('(707) 555-1854', callerCountry: IsoCode.US)
              .international,
            equals('+17075551854'));
The actual culprit is this line https://github.com/cedvdb/phone_numbers_parser/blob/dev/lib/src/parsers/phone_parser.dart#L102
    if (callerMetadata != null &&
        callerNationalPrefix != null &&
        phoneWithoutExitCode.startsWith(callerNationalPrefix)) {   // will be false because it is **local** (same area in us)
      return callerMetadata;
    }
where you'd want to enter this condition. You do not enter this because the phone number is in local form. Therefor the condition: phoneWithoutExitCode.startsWith(callerNationalPrefix)) is wrong or incomplete. It should probably be phoneWithoutExitCode.startsWith(callerNationalPrefix)) || checkThatPhoneNumberIsValidForCallerMetadata())

Am I wrong that this would be the only change required to fix your issue ?

Potentially, yes, but when I replace the block

    if (callerMetadata != null &&
        callerNationalPrefix != null &&
        phoneWithoutExitCode.startsWith(callerNationalPrefix)) {   // will be false because it is **local** (same area in us)
      return callerMetadata;
    }

with

    if (callerMetadata != null &&        
        ((callerNationalPrefix != null && phoneWithoutExitCode.startsWith(callerNationalPrefix)) || 
        Validator.validateWithPattern(PhoneNumber(nsn: phoneWithoutExitCode, isoCode: callerMetadata.isoCode)))) {
      return callerMetadata;
    }

in the initial version of the library, the tests fail on

expect(
            PhoneNumber.parse('+33 655 5705 76', callerCountry: IsoCode.DE)
                .international,
            equals('+33655570576'));

However, the tests I added perform well. I think an exit code check should be mandatory in this method.

cedvdb · 2023-12-07T00:07:43Z

I've been away the past few days. I'll have a look into this tomorrow to see what I can do.

cedvdb · 2023-12-07T13:18:29Z

I refactored the code with some of your input in PR #50

I fixed the issue in PR #51

Thanks a lot for the help in investigating this

aazhevsky · 2023-12-07T13:49:17Z

I see it was a complex solution. Many thanks for your efforts!

cedvdb · 2023-12-07T14:27:50Z

@aazhevsky the solution itself was 3 lines (there is a function rename that should not be part of the PR), the refactor took a bit longer

aazhevsky added 2 commits November 24, 2023 14:18

- phone parser prioritizes validity of number over its potential cou…

8b653cd

…ntry code - git ignores .DS_Store files

- no-country-code workaround

52b9cc1

cedvdb reviewed Nov 24, 2023

View reviewed changes

example/lib/main.dart Show resolved Hide resolved

- local number w/o trunk prefix test added

092d162

cedvdb reviewed Nov 27, 2023

View reviewed changes

test/phone_number_test.dart Outdated Show resolved Hide resolved

cedvdb reviewed Nov 27, 2023

View reviewed changes

aazhevsky added 2 commits November 27, 2023 10:00

- obsolete comment removed

bae0d72

- previous commit build issue code reverted

04b33b0

- MetadataMatcher.getMatchUsingPatterns removed

3a51059

- MetadataMatcher.getMatchUsingPatternsStrict renamed to MetadataMatcher.getMatchUsingPatterns

cedvdb reviewed Nov 29, 2023

View reviewed changes

- comment of "input number has an exit code" case updated

b2c4464

cedvdb closed this Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #46 (Wrong result from local number) fix #47

Issue #46 (Wrong result from local number) fix #47

aazhevsky commented Nov 24, 2023 •

edited

Loading

cedvdb commented Nov 24, 2023 •

edited

Loading

cedvdb Nov 27, 2023

aazhevsky Nov 27, 2023 •

edited

Loading

cedvdb Nov 29, 2023

aazhevsky Nov 29, 2023

cedvdb Nov 27, 2023 •

edited

Loading

aazhevsky Nov 27, 2023

cedvdb Nov 29, 2023

cedvdb Nov 29, 2023

aazhevsky Nov 29, 2023

cedvdb Nov 27, 2023

aazhevsky Nov 27, 2023

cedvdb Nov 29, 2023

aazhevsky Nov 29, 2023

cedvdb Nov 29, 2023 •

edited

Loading

aazhevsky Nov 29, 2023

aazhevsky commented Nov 27, 2023

cedvdb left a comment •

edited

Loading

cedvdb Nov 29, 2023 •

edited

Loading

aazhevsky Nov 29, 2023

cedvdb Nov 29, 2023

aazhevsky Nov 29, 2023

aazhevsky Nov 29, 2023

cedvdb Nov 29, 2023 •

edited

Loading

cedvdb commented Nov 29, 2023 •

edited

Loading

aazhevsky commented Nov 30, 2023 •

edited

Loading

cedvdb commented Dec 7, 2023

cedvdb commented Dec 7, 2023 •

edited

Loading

aazhevsky commented Dec 7, 2023

cedvdb commented Dec 7, 2023 •

edited

Loading

Issue #46 (Wrong result from local number) fix #47

Issue #46 (Wrong result from local number) fix #47

Conversation

aazhevsky commented Nov 24, 2023 • edited Loading

cedvdb commented Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

aazhevsky Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedvdb Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedvdb Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aazhevsky commented Nov 27, 2023

cedvdb left a comment • edited Loading

Choose a reason for hiding this comment

cedvdb Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedvdb Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

cedvdb commented Nov 29, 2023 • edited Loading

aazhevsky commented Nov 30, 2023 • edited Loading

cedvdb commented Dec 7, 2023

cedvdb commented Dec 7, 2023 • edited Loading

aazhevsky commented Dec 7, 2023

cedvdb commented Dec 7, 2023 • edited Loading

aazhevsky commented Nov 24, 2023 •

edited

Loading

cedvdb commented Nov 24, 2023 •

edited

Loading

aazhevsky Nov 27, 2023 •

edited

Loading

cedvdb Nov 27, 2023 •

edited

Loading

cedvdb Nov 29, 2023 •

edited

Loading

cedvdb left a comment •

edited

Loading

cedvdb Nov 29, 2023 •

edited

Loading

cedvdb Nov 29, 2023 •

edited

Loading

cedvdb commented Nov 29, 2023 •

edited

Loading

aazhevsky commented Nov 30, 2023 •

edited

Loading

cedvdb commented Dec 7, 2023 •

edited

Loading

cedvdb commented Dec 7, 2023 •

edited

Loading