stefanfoulis / django-phonenumber-field

A django model and form field for normalised phone numbers using python-phonenumbers
MIT License
1.48k stars 318 forks source link

Formatting being lost #539

Closed chrisspen closed 1 year ago

chrisspen commented 1 year ago

I recently upgraded to django-phonenumber-field[phonenumbers]==7.0.0 and now all my phone numbers, previously formatted like (800) 123-4567, are being reformatted as +18001234567.

Do you know why this is?

Is there a new flag or formatting template I should set to maintain my preferred format?

francoisfreitag commented 1 year ago

Can you precise where the format is being lost? Is that in your console, in the HTML generated by your template, in your HTML input values?

chrisspen commented 1 year ago

I have a unittest that tests entering an international number, like +18001234567, into a PhoneNumberField via a standard Django ModelAdmin, saving, and then confirms it's re-rendered identically as +18001234567. The test then enters a US number, like 800-123-4567, into the same field, saves, and confirms it's also re-rendered identically as 800-123-4567.

In essence, the default behavior of the field supported both formats in the same field, and this test passed. After upgrading from version 6.3.0 to 7.0.0, this no longer seems to be supported. I can get one of the checks to pass by setting PHONENUMBER_DEFAULT_FORMAT (which I'd previously not defined) to either NATIONAL or INTERNATIONAL, but I can't find a way to fix both.

I imagine the only way this was possible is if PHONENUMBER_DB_FORMAT was blank and/or not being enforced, so the raw yet validated phone number was being stored to the db.

Is it no longer possible to store US and international phone numbers in the same field anymore? If so, that's your design call, but I'll need to fork the project to maintain the previous functionality for my clients, as a ton of people store both formats in the same field.

francoisfreitag commented 1 year ago

I’m confused.

Both 800-123-4567 and +18001234567 are invalid phone numbers, I’m not sure if that’s what you have been using in your test case.

>>> import phonenumbers
>>> x = phonenumbers.parse("+18001234567")
>>> phonenumbers.is_valid_number(x)
False

The library treats invalid phone numbers as just an str (storing and retrieving the raw_input property of the PhoneNumber object). The default widget does provide some formatting for valid numbers, but it does not change invalid numbers. The following test passes:

    def test_invalid_number(self):
        invalid_numbers = ["+18001234567", "800-123-4567"]
        widget = RegionalPhoneNumberWidget()
        for invalid_number in invalid_numbers:
            number = PhoneNumber.from_string(invalid_number, region="US")
            self.assertHTMLEqual(
                widget.render("number", number),
                f'<input name="number" type="tel" value="{invalid_number}" />',
            )

Without customization, the admin should be using that widget, thus your tests should pass, unless other customizations have been applied?

For valid phone numbers, the library by default will:

Here’s the widget implementing the behavior described above: https://github.com/stefanfoulis/django-phonenumber-field/blob/87d113ec35687edf35d0204405206e14fc166f63/phonenumber_field/widgets.py#L125-L158

francoisfreitag commented 1 year ago

Finally got some time to look at your report in a project. Indeed, the RegionalPhoneNumberWidget is not being installed, because the admin provides widgets.AdminTextInputWidget as a widget, which the FormField uses. That’s a change from 7.0.0, in 6.3.0 the formatting was (incorrectly) done by the form field.

That’s why the phone numbers are displayed in E164 and not in the configured region. I’ll try to think of a way to use the RegionalPhoneNumberWidget in the admin by default. In the meantime, the following workaround should restore the previous behavior:

@admin.register(MyModel)
class MyModelAdmin(admin.ModelAdmin):
    formfield_overrides = {
        PhoneNumberField: {"widget": widgets.RegionalPhoneNumberWidget}
    }
francoisfreitag commented 1 year ago

I’ve given this matter some thoughts. I believe we should stick to the AdminTextInputWidget, even though it’s not as nice. The admin can also be used to browse the content of the database, so exposing the stored value “raw” makes sense. Fighting against the admin is probably not worth it, especially since it’s easy to configure otherwise.

What do you think of documenting the above trick (formfield_overrides)?