cividesk / com.cividesk.normalize

CiviCRM extension - Normalizes data prior to saving to database.
7 stars 14 forks source link

Zip format for Italy #22

Open masetto opened 2 weeks ago

masetto commented 2 weeks ago

I'd like to add Italy zip format and also other interventions to support Italian telephones and addresses. But first I would like to understand why:

  1. the PRs have not yet been merged from august (smarty3 and civix update). Why? Is this extension supported?
  2. Why in this line is checked if $country == 'CA'? I think it is a typo
  3. Have you ever thought of sending a cumulative email instead of one email per contact with an incorrect zip code?

Thanks

nganivet commented 2 weeks ago

Hi @masetto , here are some answers:

  1. this is done on a volunteering basis, sometimes we just need a little nudge ... will look at these by end of week
  2. yes, this looks like a typo, should probably be replaced with array_key_exists($country, $zip_formats) or similar and do a bit of testing ... patch welcomed!
  3. that would make sense, but how would you trigger the cumulative email? on a schedule? by a certain action/condition (ie. civirule)?
masetto commented 2 weeks ago

@nganivet thank you for your answer.

I worked on that: https://github.com/cividesk/com.cividesk.normalize/commit/9f6652844987a08e1a782d11d7a3204a4be4c2df But before to make a PR I would ask your opinion about the following points.

Now I'm thinking to work about validation of phone and postal code: your extension simply notify with a message and do not block if a user write a wrong postal code or phone number. Is it intentional?

About no. 3: I'm thinking to add in extension setting an option to add the invalid postal code (and why not phone number?) contacts in a group specified by the user. What do you think?

nganivet commented 2 weeks ago

@masetto Yes, it is intentional that we do not block the user. This validation is done from a hook, we do not know if it was triggered by a front-end form, a back-end user, a cron job or an API call, so we cannot 'block' the action. It is a good idea to implement on-screen validation for the CiviCRM zip-code entry field, but that would need to integrate differently in CiviCRM.

It seems like a great idea to have the option to put contacts in a group, that is a good alternative to sending emails.

masetto commented 2 weeks ago

@nganivet I create PR #23

I saw that there is the extension Phone Input Mask and I was thinking of adding an option to validate the phone number and the postal code in forms, like that extension. So we can implement the validateForm hook for these form:

The advantage would be to use a single extension...
My client asks to me to validate postal code and phone (to send SMS), but I think that "background" normalization is however useful.... What do you think?

nganivet commented 2 weeks ago

@masetto I did not know about the Phone Input Mask extension, thanks for pointing it out. I would rather have the option to validate the zip codes be rolled into this other extension than adding it here. This way the Normalize extension stays focused on normalizing/validating data once submitted, and the Phone Input Mask deals with the collection and submission in the front-end. I will link Japp into this discussion to get his take (https://lab.civicrm.org/extensions/phoneinputmask/-/issues/2).

jaapjansma commented 1 week ago

The phone input mask is specifically for formatting phone numbers in the backend. So back office users get a message when they a enter a non-formatted phone number. So it does not do any normalization of data. It also has a configuration screen to configure which formats are valid formats.

masetto commented 1 week ago

Yes, how it works phone input mask was clear. But in my opinion the normalisation process cannot be separated from the validation because by normalising you can have errors and you have to notify them: in fact this extension sends an email or display a message (both not very optimal solutions for me). I think it would be very convenient to add also a validation process (activated by an option) because it already does this using "pre" hook. It would only to implement the validate hook by calling the same normalisation functions, only in this case they would return a message to invalidate the fields.

In any case, I would also need the validation of the postal code and I would not want to create an extension only for that.

nganivet commented 1 week ago

@masetto Not quite sure about what you are suggesting, but you are welcome to do a PR.

@jaapjansma Sorry I misunderstood. I thought your extension provided an input mask on the HTML form widget, similar to when you enter dates or credit card numbers some widget limit accepted chars and insert additional characters as you type.

I see now that this is not the case, so there is substancial overlap between our extensions. My extension mainly does auto-reformatting based on configured policy (ie. enter jajp jansma and it will save Japp JANSMA in the database), but validation features were also added later on, primarily for zip codes.

Ideally we would need the following functions (for US zip code field as example):

Phone numbers will be a bit more complex, but still have the need for the entry mask, validation and normalization features.

These features are vastly different in terms of implementation, so could potentially be delegated in different extensions. @masetto outlines the need for common elements between these features. There certainly is symbiosis between entry mask and validation, but I am not sure there is symbiosis with normalization.

nganivet commented 1 week ago

Actually, there is: normalization should validate the field before even trying to reformat it ... makes sense!