CRM-21769: Show 'unsupported locale for parsing' warning only when enabling address parsing #11672
Conversation
|
@michaelmcandrew you were offering to review something - this looks very reviewable - although @omarabuhussein could you please squash the commits? |
|
@eileenmcnaughton - yes I was though @seamuslee001 beat me to it a couple of times :) I can take a look at this one today. |
|
@michaelmcandrew did you close this in favour of another? |
|
no :) gimme one sec... |
|
jenkins is not feeling the love |
|
@Jenkins, can you feel the love tonight? @omarabuhussein - this looks good. Agree that it shouldn't be reported each time that you save a contact. At the same time, since it is a mis-configuration, I think we should report it somewhere less ephemeral than a postProces message so I added a system check. The admin can dismiss this if he knows better. I presume that you have some reason to have it enabled in an unsupported locale (and am interested to know what that is if so). Let me know what you think |
|
@omarabuhussein - any thoughts on this one? |
|
@guanhuan if @omarabuhussein is not about can someone else respond. This seems to need a final re-review |
|
hey guys, thanks both for your review and for your work @michaelmcandrew .. but sorry I am super super busy currently and trying to finish some stuff on my todo list but hopefully I will get back to this before the end of this week. I will just quickly try to respond to this :
I really don't have strong opinion on on this, so I think it is fine.
It was for one of our client sites who requested that, I remember that I asked our project manager who manage that site the same question and telling her that it is just a configuration problem and they have to disable address parsing before doing this, I don't remmber her reply but I think it was a good reason so that's why I did this PR, I will recheck the reason with her again and let u know :) . |
|
@omarabuhussein - thanks for the reponse. @eileenmcnaughton - your call on how much more review we need. |
|
@michaelmcandrew so my take on this is that you reviewed it & thought it made sense as an issue & was good to merge, but added an additional system check, which somewhat confused things (although it was probably a good addition). If you confirm you are comfortable with that I'm OK to merge. In terms of how to keep PRs reviewable/ avoid them getting stalled, I think that trying not to grow the scope of them & instead doing follow up PRs is the special sauce |
|
OK - I will merge. |
|
@eileenmcnaughton fwiw, i didn't think it was quite ready to merge without the additional check and rather than ask the submitter to do it, I thought it would speed things up if I did it myself. Admittedly, that requires an extra review but by thinking was that the review would be quicker than doing the extra work. |
|
well, this get merged pretty quickly before I had the chance to review the extra commits!!! So I think nothing for me todo here anymore :D . Thanks guys |
|
@michaelmcandrew I've been stung quite a few times doing additional fixes or revised fixes & then the original reviewer not re-reviewing them in a timely way - I'm not quite sure the answer. In this case the PR was already kinda cold when you reviewed it. I tend to be annoyed when I review things fairly quickly and then the original submitter doesn't put in the time to re-review |
|
@omarabuhussein It would be good if you would still do a final check now it's merged |
|
@eileenmcnaughton np, will take a look after reviewing this one #11556 |
|
I tested it on http://dmaster.demo.civicrm.org and things are working fine. |
|
Thanks @omarabuhussein |
Overview
Improving "Unsupported locale specified to parseStreetAddress" warning.
Before
1- Go to Administer >> Localization >> Languages, Currency, Locations.
2- Change the default Language to anything other than "English (United States)", "English (Canada)" or "French (Canada)", for example change it to "English (United Kingdom)" and save the settings.
3- Go Administer >> Localization >> Address Settings.
4- At "Address Editing" section, check "Street Address Parsing" option and hit save.
5- Now any time you try to edit a contact with street address, a warning says "Unsupported locale specified to parseStreetAddress en_GB, Proceeding with en_US locale." will appear.
After
The above situation is not ideal for users, the warning now does not appear each time you try to edit a user and it default to en_US silently if the locale is not supported, the warning above only appear when the admin enable "Street Address Parsing" (step 4 above) in case the default locale is not supported.