Case Insensitivity for id, fixes #785

[ghbooth12]

Hello. This PR is about fixing the first issue of #785 by using id.downcase instead of id in first_by_friendly_id method. In this way if the url is like users/John-Doe or users/JOHN-DOE, it will search by john-doe in database.

Currently, it find_by with id.(case-sensitive)

# finder_methods.rb

def first_by_friendly_id(id)
	# If `id` is "John-Doe" or "JOHN-DOE"
	find_by(friendly_id_config.query_field => id)
	# It raises `ActiveRecord::RecordNotFound`.
end

It find_by with id.downcase.(case-insensitive)

# finder_methods.rb

def first_by_friendly_id(id)
	# If `id` is "John-Doe" or "JOHN-DOE"
	find_by(friendly_id_config.query_field => id.downcase)
	# It finds data by "john-doe".
end

The tests for the change have been added and all have passed.

[coveralls]

Coverage remained the same at 96.333% when pulling 1d7a5d7 on ghbooth12:785-case-insensitivity into dac59ef on norman:master.

[coveralls]

Coverage remained the same at 96.333% when pulling 1d7a5d7 on ghbooth12:785-case-insensitivity into dac59ef on norman:master.

[coveralls]

Coverage remained the same at 96.333% when pulling 1d7a5d7 on ghbooth12:785-case-insensitivity into dac59ef on norman:master.

[coveralls]

Coverage remained the same at 96.333% when pulling 1d7a5d7 on ghbooth12:785-case-insensitivity into dac59ef on norman:master.

[coveralls]

Coverage remained the same at 96.333% when pulling 1d7a5d7 on ghbooth12:785-case-insensitivity into dac59ef on norman:master.

[eric-norcross]

+1

[ducnguyenhuy-agilityio]

Could I know this is merged or not yet?

[vfonic]

I was just wandering around this repo and found this PR, which is identical to the change I have overridden locally. Would be great to see this merged. 👍

[vfonic]

I just realized there could be couple of issues with this PR:

What if id is nil? (or doesn't respond to :downcase?

[parndt]

All the tests passed, so perhaps if we had tests for those cases, we could fix that preemptively. Are you keen @vfonic ?

[coneybeare]

This was a bad merge. It is a breaking change and has. no opt-in or opt-out behavior. There are many valid reasons for allowing a URL to have case-sensitive slugs, such as short URL services. It should, at the very least, use the normalize_friendly_id method instead of simply downcase

[parndt]

@coneybeare thanks, I see what you mean. Do you have a patch in mind that would fix this for you, that you could open a pull request for? Or if you just want to share a snippet I can turn it into a PR.

[coneybeare]

I’m AFK now, but how about adding a method that defaults one way or the other, and can be overridden in the model to change if necessary? That is the pattern used by the normalize process

[parndt]

yep, that would make sense to me.. something like parse_friendly_id

[vfonic]

That's a good idea. It could also be a config option: friendly_id [slugged: { case_sensitive: true }] or a global FriendlyId config.

[parndt]

That's a good idea. It could also be a config option: friendly_id [slugged: { case_sensitive: true }] or a global FriendlyId config.

I like that too!

[marckohlbrugge]

FWIW, this broke my implementation. I use FriendlyId for usernames, and some usernames are stored with capitalization in the database. For example MarcKohlbrugge. This change makes makes it so that FriendlyId searches for marckohlbrugge instead, which has no matches.

If we want FriendlyId to be truly case insensitive we should also downcase the attribute we're matching on (e.g. LOWER(username))

[parndt]

Yeah, the comments on this PR didn't result in any patches unfortunately. I'm all for making this work for everyone. For now, you can use 5.3.0 and I still think @vfonic's suggestion is the best one about making this a config option (probably that defaults to false)

[parndt]

Let's continue discussion in #950 and #951