Implement PublicPropertyForm validation

[2ynn]

For PublicPropertyForm: https://github.com/LesFruitsDefendus/saskatoon-ng/blob/f6a2a8cdadbcdedd1463837f2017cebfc89a3c1b/saskatoon/harvest/forms.py#L325

• [x] most importantly check for invalid email
• [ ] eventually phone number (there are defined as simple CharFields and aren't used for any automatic task so far)
• [ ] eventually postal code (same)
• [ ] evaluate if Blank=False should be set on neighborhood field

[tristanlatr]

This library should help us: https://validators.readthedocs.io/en/latest/

[2ynn]

thanks @tristanlatr, validators are already available with django form fields https://docs.djangoproject.com/en/4.0/ref/validators/ they just need to be properly implemented, e.g. in https://github.com/LesFruitsDefendus/saskatoon-ng/blob/master/saskatoon/harvest/forms.py

[donaldte]

thanks @tristanlatr, validators are already available with django form fields https://docs.djangoproject.com/en/4.0/ref/validators/ they just need to be properly implemented, e.g. in https://github.com/LesFruitsDefendus/saskatoon-ng/blob/master/saskatoon/harvest/forms.py

Hi I have used this validators available in django to implement email and phone number validation inside the models Please I want you to check and tell if there are something to modify. the branches are under develop branch. thanks

[tristanlatr]

Hello @donaldte,

Please open a pull request with your changes and I'll review them alongside other contributions tonight or tomorrow alongside other contributions.

Thanks

[yokwejuste]

thanks @tristanlatr, validators are already available with django form fields https://docs.djangoproject.com/en/4.0/ref/validators/ they just need to be properly implemented, e.g. in https://github.com/LesFruitsDefendus/saskatoon-ng/blob/master/saskatoon/harvest/forms.py

Hey greetings here! Can I work on this issue? I mean can the issue be assigned to me?

[2ynn]

To be clear on this issue. Do not alter any models.py, we are in the process of migrating databases and cannot afford doing migrations at the moment. The form that needs to be validated more specifically is PublicPropertyForm https://github.com/LesFruitsDefendus/saskatoon-ng/blob/f6a2a8cdadbcdedd1463837f2017cebfc89a3c1b/saskatoon/harvest/forms.py#L325

[yokwejuste]

To be clear on this issue. Do not alter any models.py, we are in the process of migrating databases and cannot afford doing migrations at the moment. The form that needs to be validated more specifically is PublicPropertyForm

https://github.com/LesFruitsDefendus/saskatoon-ng/blob/f6a2a8cdadbcdedd1463837f2017cebfc89a3c1b/saskatoon/harvest/forms.py#L325

Meaning I can start working on the optimization of that form validation right?

[2ynn]

@yokwejuste if you want you can submit a pull request (on develop branch) and we will look at it. thanks

[farhanmasud]

@2ynn @tristanlatr For email field, the form uses the field pending_contact_email from the Property model which uses the django model field EmailField which already has the validations for checking a valid email (Ref). Also the attribute blank for this field is set as False by default so this is field is marked as required on the form.

So, what other possible scenarios should we look for here for validating the form based on the business logic? One possible case can be that the entered email might already exist in the database in AuthUser model. Is that considered as invalid here? What other possible should we look for to implement the email validation here based on the business logic?

[2ynn]

@farhanmasud thanks for checking the pending_contact_email field. No we don't need to check if it already exists in the database for now.

1. pending_contact_phone: please look into implementing a proper phone field (e.g. https://pypi.org/project/django-phone-field/). Ideally we should only store phone numbers formatted as E.164 (which django-phone-field does for us).
2. pending_contact_name should be renamed to pending_contact_first_name in harvest.models.Property (without loosing existing data stored in this field).
3. We should add a pending_contact_family_name field to harvest.models.Property
4. Postal code should be validated and formatted consistently before saving it

Note all this will require migrations but I think it's worth doing it if we are careful. Also some fixtures will need to be updated. Would you feel comfortable tackling this? I would help you. Thanks!

[farhanmasud]

@2ynn thank you very much for your detailed feedback on this. This now gives a clear idea of what needs to be implemented. I'd be very glad tackling this. I'm starting to work on this and will get back to you whenever I need help about anything. Thanks!

[farhanmasud]

@2ynn for implementing the proper phone field with django-phone-field, there is an argument E164_only. If that's set to True it adds a form validator to only accept numbers in the E164 format. Should we put this on to require users to enter the phone number correctly?

It says on the documentation that it only works with US number but I have tested with some Canada numbers it worked with and without the country code. One reason might be that both country numbers start with +1 although I am not sure if it will work with all Canada phone number format.

Setting this argument E164_only to True leads to an issue which is detailed here.

[2ynn]

No I don't think setting E164_only=True should be necessary. We don't want to force people to enter their phone number in E.164 format, what we want is make sure the phone is stored properly in the database. For example if you pass "(514) 123 4567" or "514-123-4567" or "5141234567" or "+15141234567" to the django-phone-field they will all get nicely formatted as "(514) 123-4567" when we display them but they will be stored as E164 and phone.raw_phone will return the E164 formatted string. Btw I've had issues with the is_E164 property before and I don't think we should rely on this.

[farhanmasud]

Got it. Thanks!

[farhanmasud]

@2ynn About cleaning / validating the postal code. I was thinking the following steps -

1. Remove all spaces
2. Check for length 6 since all Canada postal codes are 6 digits long
3. Check if it has only alphanumeric characters
4. Check if the format is A1A1A1 where A being a letter form the alphabet and 1 being a decimal number ref.
5. Finally save it as A1A\<space>1A1 format.

3 & 4 can be combined into one step as well.

Please let me know if we should consider other steps in this validation.

[tristanlatr]

FYI this exists: https://github.com/verhovsky/postalcodes-ca, so let's not reinvent the wheel, the parse_fsa function looks really helpful.

[farhanmasud]

@tristanlatr thanks! I'll look into it

[farhanmasud]

@tristanlatr I went through this package and there is also another function parse_postal_code which validates the whole postal code instead of the first 3 characters from parse_fsa. Could we use that instead of the parse_fsa function? Or should we focus on validating just the first 3 characters for now?

I think we'll need to go through this following step or at least trimming the left spaces since both of parse_fsa and parse_postal_code function is throwing an error if the code is started with a space.

1. Remove all spaces

Also calling the parse_postal_code returns the properly formatted postal code which we could use to save in the database for consistent formatting of postal code stored in the database.

We could edit the model field to have the max_length=7 (currently it's set to 10) which could further help users not to mistakenly input redundant values after the postal code but since we can't modify the model as of now, I think we can work with the above 2 steps mentioned.

Please let me know what you think. Thanks!

[tristanlatr]

Hello @farhanmasud,

Sorry for the late response, there is a lot to process, I'm sure you have looked at this package closer than me, so if you say that the function parse_postal_code better fit our use case, let's go with this.

We could edit the model field to have the max_length=7 (currently it's set to 10)

Would that be necessary if there is proper validation when entering the postal code ?

[farhanmasud]

@tristanlatr thanks a lot for your feedback. I'll make the changes with parse_postal_code for now, I think this will work for our use case. Still I'll keep my eye open if I can find a better solution for this.

With proper validation, we don't need any model changes for now.

I'll finish up the remaining tasks in this issue over this weekend.