Feature/ogc 236 phone number type

[Tschuppi81]

Org: Extend phone number validator

TYPE: Feature
LINK: ogc-236

[linear]

OGC-236

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2810	1	2809	17

View the top 1 failed test(s) by shortest run time

tests/onegov/form/test_browser.py::test_formcode_select_empty_radio

Stack Traces | 3.08s run time

browser = <tests.shared.browser.ExtendedBrowser object at 0x7f41b6e47ed0>

    def test_formcode_select_empty_radio(browser: ExtendedBrowser) -> None:
        browser.visit('/formcode-select')
        browser.wait_for_js_variable('initFormcodeSelect')
        browser.execute_script("""
            var watcher = formcodeWatcherRegistry.new();
            var el = document.querySelector('#container');
    
            initFormcodeSelect(
                el, watcher, 'textarea', 'radio', ['text', 'textarea']);
            watcher.update(arguments[0]);
        """, 'A = ___\nB = ...\nC = *.png')
    
>       assert len(browser.find_by_css('.formcode-select input')) == 2
E       AssertionError: assert 0 == 2
E        +  where 0 = len(<tests.shared.browser.PlaywrightElementList object at 0x7f41baf712b0>)
E        +    where <tests.shared.browser.PlaywrightElementList object at 0x7f41baf712b0> = find_by_css('.formcode-select input')
E        +      where find_by_css = <tests.shared.browser.ExtendedBrowser object at 0x7f41b6e47ed0>.find_by_css

.../onegov/form/test_browser.py:129: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[Daverball]

Why are we defining our own type, if we could just reuse phonenumbers.PhoneNumberType? (I know they currently aren't real enums, so you have to use int at the annotation everywhere you accept those values, I opened an issue about that years ago: daviddrysdale/python-phonenumbers#229, so they're not as type-safe, but you also currently allow str instead of PhoneNumberType, so you're not gaining any type-safety anyways, you only get a runtime check, which you could still do if you used phonenumbers.PhoneNumberType)

ANY also seems unnecessary, why not just use None as the sentinel for any phone number type is valid?

In OCQMS I do the following:

            if self.number_type in supported_types_for_region(region):
                # Validate type if we can
                number_type = number_type_for_number(numobj)
                if number_type == PhoneNumberType.FIXED_LINE_OR_MOBILE:
                    if self.number_type in (
                        PhoneNumberType.FIXED_LINE,
                        PhoneNumberType.MOBILE
                    ):
                        # make aggregate types match.
                        number_type = self.number_type
                if number_type != self.number_type:
                    raise ValidationError(_(
                        'Invalid ${number_type}',
                        mapping={'number_type': self.format_number_type()}
                    ))

[Daverball]

Depending on the country not all number types are possible, there's a supported_types_for_region function which tells you which types can be distinguished, if you can't distinguish the types you're asking for, then you shouldn't raise validation errors for them.

[Daverball]

This seems overly complex, I would just special-case FIXED_LINE_OR_MOBILE in the validation function where we check if the types match. Like in the example code from OCQMS.