Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Django's "normal" validation, not hardcoded in __init__() #597

Open
nerdoc opened this issue Feb 9, 2024 · 9 comments · May be fixed by #603
Open

Use Django's "normal" validation, not hardcoded in __init__() #597

nerdoc opened this issue Feb 9, 2024 · 9 comments · May be fixed by #603
Assignees

Comments

@nerdoc
Copy link

nerdoc commented Feb 9, 2024

This is a very cool project - but I need to "validate" my prhone numbers and clean them before the PhoneNumerField gets hand on it. It may have to do something with #441 and could be solved together with that issue too IMHO.

Use case: I have an input form that asks for a phone number, and it is likely one of two countries (DE,AT). People tend to fill in , numbers in a local format, and I want to call clean_phone_number() before the PhoneNumberField calls it's validators, and try to convert it into an international format.

ATM this is not possible, as the validation is done more or less hard coded. I can't override the form's clean_phone_number() or clean() methods, as PhoneNumberField does it's own validation stuff already before, and throws an error before my cleaning even starts.

Could you rearrange the code so that PhoneNumberField uses the default Django validation methods in the right order?
See https://docs.djangoproject.com/en/5.0/ref/forms/validation/

The validation should not be done in the __init__() method, but in the field's validate(). This breaks the possibility of "normal" Django usage of this else very cool module.

I think this would close #441 too as we then could use custom validators.

@nerdoc nerdoc changed the title Use "normal" validation, not in __init__: allow cleaning of field before validation Use Django's "normal" validation, not in __init__: allow cleaning of field before validation Feb 9, 2024
@francoisfreitag
Copy link
Collaborator

francoisfreitag commented Feb 18, 2024

Hi, thanks for reporting the issue. There’s indeed an unexpected check for phonenumber.is_valid() in the to_python widget value_from_datadict method (-- not __init__), which job is to convert the input to a PhoneNumber object, not validate it.

A first step seems is to remove it, which should be fine, as the default_validators are verifying the phone number to be valid.

However, handling short codes phone numbers is a bit trickier: the wrapper of phonenubers.PhoneNumber considers phonenumbers.is_valid_number() as the source of truth for all numbers. So if one simply loosens the form validation, they’ll encounter unexpected failures when dealing with phonenumber_field.PhoneNumber instances.

I believe the suggestion is correct, that check should be removed. But we’ll need to adapt the model fields and phonenumber_field.PhoneNumber wrapper to deal with short codes. It may take some time before #441 is fixed, as it’s larger than meets the eyes initially. But I agree it should be done.

Edit: the to_python method on the form field is actually unused. It probably should be, instead of having the widget perform the to_python operation. 🤔

@francoisfreitag
Copy link
Collaborator

Hi @nerdoc,

#603 should solve your issue, your input would be very valuable :)

@francoisfreitag
Copy link
Collaborator

@nerdoc, please feel free to try out the PR, see if it works for your use case?
Otherwise I plan on merging it in the upcoming weeks, but getting feedback earlier will help shipping the release with less bugs.

@francoisfreitag francoisfreitag self-assigned this Apr 5, 2024
@nerdoc
Copy link
Author

nerdoc commented Apr 5, 2024

I'm afraid I don't find time ATM - if it works for you, just merge it, I will test it in a few weeks when it is easier, and reopen if still a problem, ok?

@francoisfreitag
Copy link
Collaborator

I would rather wait for input before merging (and releasing). I’ll wait another month or two, unless I get feedback otherwise.

@nerdoc
Copy link
Author

nerdoc commented Apr 8, 2024

So, I tested the new behaviour a bit. The field now seems to validate the phone number correctly: it is done in the validate_international_phone_number validator.
The only thing I have to admit: Django itself runs the run_validators() method before the clean_<name>() method.
So if the user just enters "0664 12345678" and I want to "clean" it into "+43 664 12345678", the validator always is before and raises a ValidationError before I can clean the number into a correct format.
This may be Django stuff, but prevents django-phonenumber-field being helpful here.

I don't know exactly how to solve this.
There are long standing discussions (with no ral outcome here and on Stackoverflow.

So there are few options.

  • Remove the validator from one field in my application in the form - but then I can just use a CharField and use my own validator - no need for django-phonenumber-field.
  • change the validator to be less strict.

But at least, the behaviour techically is correct now, AFAICT. So my title "allow cleaning of field before validation" is incorrect, as Django runs validation before cleaning. Which is a bit of nonsense IMHO.

But that's not your problem. So I think you can close the issue, unless you have a better idea.

@nerdoc nerdoc changed the title Use Django's "normal" validation, not in __init__: allow cleaning of field before validation Use Django's "normal" validation, not hardcoded in __init__() Apr 8, 2024
@nerdoc
Copy link
Author

nerdoc commented Apr 8, 2024

The only thing I can think of is: Be less strict. If you have a username check, the validator just checks if the username generally is correct. and in my form , I can add a clean_username() method to see if the username follows "my" principles.

I maybe would suggest to follow this road in phonnumbers too.
The validator should not strictly check if the form-entered phone number was filled in in an international format.

What you really want, is:

  • let the user fill in anything he likes, and be smart enough to correctly clean it to a phone number
  • store the phone number in an international format in the database.

So, I would

  • remove the strict validate_international_phone_number validator, just let it there if a dev needs it, and replate it with a validator that just checks if the entered number is "more or less" correct.
  • be strict on the model field - here the validator can be very strict.

The Form._post_clean() (and hence model.clean() method) is called AFTER the validators and form cleaning, so this would be the correct way to go IMHO.

@francoisfreitag
Copy link
Collaborator

Thanks for the feedback!

First thing, now that I know more about your use case, have you seen the region argument to the form field? It is meant to interpret numbers in a national format and convert them to international.
One limitation is that a national phone number could be valid in multiple regions, so we can’t be too smart about it.

I believe the default of validating international phone number makes sense, developers usually store phone numbers in order to be able to reach people, so they’ll want valid phone numbers, and people may very well come from abroad so developers shouldn’t really limit to national phone numbers.

One thing you can easily do for your app is to change the default validator (by subclassing the field class and setting default_validators) to something less strict. That way, you can do the normalization and loose validation in clean_FIELD(). If you’re going that route, you may be interested in phonenumbers.is_possible_number(). The value of django-phonenumber-field is in knowing the national rules for constructing phone number (e.g. the prefixes in the US), and being able to treat a phone number richly (formatting, knowing if it’s a landline or mobile, etc.).

If you have a username check, the validator just checks if the username generally is correct.

One difference is that phone numbers have known rules they follow, they aren’t free-form values.

The Form._post_clean() (and hence model.clean() method) is called AFTER the validators and form cleaning, so this would be the correct way to go IMHO.

I disagree. _post_clean() is an internal hook for django, not for third-parties to rely on. Besides, the cleaning of the field is the expected place to raise ValidationError for the field, _post_clean() is not specific to a single field, it would be surprising.

be strict on the model field - here the validator can be very strict.

The form is usually the best place to be strict, since developers can surface issues to users. A value that passes form validation is expected to pass model validation and save to the database without issues.

@nerdoc
Copy link
Author

nerdoc commented Apr 14, 2024

First thing, now that I know more about your use case, have you seen the region argument to the form field? It is meant to interpret numbers in a national format and convert them to international. One limitation is that a national phone number could be valid in multiple regions, so we can’t be too smart about it.

That's what I mean. When I need people to fill in numbers in various formats, what I need is a default format that is interpreted. So whatever the user fills in, if it's international, it's ok, of it's not international, let it be of that region.
ATM it's only possible AFAIK to either use inernational fill-ins, or regional.
But reality is, people don't do what you tell them. They don't read, but take their usual pattern and just fill in "their" phone number.
In my case, most of the numbers are Austrian, but there are others too, and I want to let both be entered. Default (if no +XX prefixed) it should be interpreted as Austrian, but if anyone has an US number, he should be able to fill in the number in international format. SAVED is ALWAYS as international.

I believe the default of validating international phone number makes sense, developers usually store phone numbers in order to be able to reach people, so they’ll want valid phone numbers, and people may very well come from abroad so developers shouldn’t really limit to national phone numbers.
Yeah, that's what I said. SAVED should be always in int. format. But the entry should be more fault-tolerant.

One thing you can easily do for your app is to change the default validator (by subclassing the field class and setting default_validators) to something less strict. [...] If you’re going that route, you may be interested in phonenumbers.is_possible_number(). The value of django-phonenumber-field is in knowing the national rules for constructing phone number (e.g. the prefixes in the US), and being able to treat a phone number richly (formatting, knowing if it’s a landline or mobile, etc.).
Sure, but what's the sense in PhoneNumberField then? I can use a simple Charfield, and put a Validator (like PhonenUmberValidator etc.) onto it.

If you have a username check, the validator just checks if the username generally is correct.

One difference is that phone numbers have known rules they follow, they aren’t free-form values.

Sure, but there are two steps. One is the form, here should be the validator "can be a phone number". Noone should be allowed to enter "lkjdsjfh" as Phone number, not even "89273465f3", nor "++4366412345678"
But it should be checked generally IF this entered digits could be a phone number, be it national or international.

THEN, dev interaction should be possible, like cleaning. You could provide a default clean method helper, like using the default regional format as matrix to interpret the filled in number.

THIRD the model validator is called anyway if it is a ModelForm. And THERE, it should be checked (as a model validator) that ALL phone numbers are SAVED in international format. It also should be an error when a developer saves a number as non-international, if the field should be international.

The Form._post_clean() (and hence model.clean() method) is called AFTER the validators and form cleaning, so this would be the correct way to go IMHO.

I disagree. _post_clean() is an internal hook for django, not for third-parties to rely on. Besides, the cleaning of the field is the expected place to raise ValidationError for the field, _post_clean() is not specific to a single field, it would be surprising.

No, I didn't mean you should override _post_clean(). I just meant that Django calls Model.full_clean() in _post_clean(), which itself calls the model's clean method...
So IF devs are using a ModelForm, Model.clean() is called anyway.

And there's my only problem: If not using a ModelForm, Model.clean() is not called and the validation is inconsistent...

be strict on the model field - here the validator can be very strict.

The form is usually the best place to be strict, since developers can surface issues to users. A value that passes form validation is expected to pass model validation and save to the database without issues.
Yes, but the whole concept of clean_<name>() methods is useless on PhoneNumberField if it's validator is too strict...

Think about an input field that is validate using AJAX at each keyup. In good UIs, you can enter a phone number and don't get at each keypress the red flag that it is wrong. It should be tolerant until it is clearly wrong (you enter a "F" or "ß"), but as long as the number could be correct (if entered further this way) there shouldn't be an error as response.

So IMHO Form validators should be "more generic" letting clean methods fix common used errors (add international notation, remove whitespace, etc), and Model validators should be very strict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants