Skip to content

Conversation

@MegaphoneJon
Copy link
Contributor

The current normalization code seems geared toward names entered as all lowercase - but doesn't handle the situation of a name entered in all uppercase. This PR addresses the all uppercase situation.

@nganivet
Copy link
Contributor

Jon - thanks for contributing back!

This design is on purpose so we do not normalize the LLC, LLP, LP and other similar acronyms for other countries. If I merge your patch these will be normalized as Llc and Llp, which is incorrect.

The solution might be to set a flag after line 125 and add a test for this flag on line 132.

@MegaphoneJon
Copy link
Contributor Author

Thanks for the test case. Agreed that this is important, and hopefully we also agree that handling the all-uppercase scenario is important. I'll submit a new PR that handles both shortly.

@stevekessler
Copy link

Another thing that I am not sure how to address with this use case is orgs that have all caps for their names.

-Steve

@nganivet
Copy link
Contributor

Good point Steve. Do you have any names that pop in mind?
I definitely would see CiviCRM, LLC ; but also and more importantly all
acronyms for associations, foundations, etc: APA, DCTA, Colorado CPA
Association, UNA, etc.

------ Original Message ------
From: "Steve Kessler" notifications@github.com
To: "cividesk/com.cividesk.normalize"
com.cividesk.normalize@noreply.github.com
Cc: "nganivet" nicolas@cividesk.com
Sent: 8/21/2015 1:10:16 PM
Subject: Re: [com.cividesk.normalize] Normalize names that are all
uppercased (#3)

Another thing that I am not sure how to address with this use case is
orgs that have all caps for their names.

-Steve


Reply to this email directly or view it on GitHub.

@stevekessler
Copy link

Not Civi users but DRCOG, DRMAC - they have full names but they might
enter their acronym.

On Fri, Aug 21, 2015 at 1:41 PM, nganivet notifications@github.com wrote:

Good point Steve. Do you have any names that pop in mind?
I definitely would see CiviCRM, LLC ; but also and more importantly all
acronyms for associations, foundations, etc: APA, DCTA, Colorado CPA
Association, UNA, etc.

------ Original Message ------
From: "Steve Kessler" notifications@github.com
To: "cividesk/com.cividesk.normalize"
com.cividesk.normalize@noreply.github.com
Cc: "nganivet" nicolas@cividesk.com
Sent: 8/21/2015 1:10:16 PM
Subject: Re: [com.cividesk.normalize] Normalize names that are all
uppercased (#3)

Another thing that I am not sure how to address with this use case is
orgs that have all caps for their names.

-Steve


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub
#3 (comment)
.

Steve Kessler
Owner and Lead Consultant
Denver DataMan, LLC
303-587-4428

@MegaphoneJon
Copy link
Contributor Author

Can we just handle all-uppercased first/last names, and not organization names? Folks with all-capital names are still an issue, but hopefully are enough of an edge case? It also seems like of most folks with all-capital names, the most common case is someone whose first name is initials (e.g. "JT Smith"), so we can also not do title case on names with a length < 3.

@nganivet
Copy link
Contributor

OK, sounds reasonable. Can you document this with a comment in the code?
I will also try to add comments on my end.
Thanks Jon.

------ Original Message ------
From: "Jon" notifications@github.com
To: "cividesk/com.cividesk.normalize"
com.cividesk.normalize@noreply.github.com
Cc: "nganivet" nicolas@cividesk.com
Sent: 8/21/2015 3:43:44 PM
Subject: Re: [com.cividesk.normalize] Normalize names that are all
uppercased (#3)

Can we just handle all-uppercased first/last names, and not
organization names? Folks with all-capital names are still an issue,
but hopefully are enough of an edge case? It also seems like of most
folks with all-capital names, the most common case is someone whose
first name is initials (e.g. "JT Smith"), so we can also not do title
case on names with a length < 3.


Reply to this email directly or view it on GitHub.

@MegaphoneJon
Copy link
Contributor Author

OK, I fixed this PR!

@nganivet
Copy link
Contributor

Jon - sorry this is not merged yet. I have doubts as to whether this one line fix does not have other side effects, and almost all my customers have this extension installed so I need to exercise extra caution.

Can you please confirm that you have this modification in production with some of your customers and that it resolves all the use cases described above with no other side effects?

Thanks.

@MegaphoneJon
Copy link
Contributor Author

Hi Nicolas - no worries! I do have a client using this in production - the only one who uses this extension. There aren't any side effects, though note that it doesn't handle the situation I mentioned above ("JT Smith").

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 this pull request may close these issues.

3 participants