Make Address.find_all_by_name_or_abbr case-insensitive #2043
Conversation
core/app/models/spree/state.rb
Outdated
| @@ -6,7 +6,7 @@ class State < Spree::Base | |||
| validates :country, :name, presence: true | |||
|
|
|||
| def self.find_all_by_name_or_abbr(name_or_abbr) | |||
| where('name = ? OR abbr = ?', name_or_abbr, name_or_abbr) | |||
| where('lower(name) = lower(:name_or_abbr) OR lower(abbr) = lower(:name_or_abbr)', name_or_abbr: name_or_abbr) | |||
There was a problem hiding this comment.
Rather than using lower using arel matches would be preferred. This will use ILIKE in postgres, and LIKE in MySQL.
These types of comparisons can make better use of indexes than relying upon the SQL function return value.
Example:
where(
arel_table[:name].matches(name_or_abbr).or(
arel_table[:abbr].matches(name_or_abbr)
)
)
core/spec/models/spree/state_spec.rb
Outdated
| end | ||
|
|
||
| context 'by abbr' do | ||
| let(:search_term) { 'California' } |
There was a problem hiding this comment.
This doesn't match the context.
core/app/models/spree/state.rb
Outdated
| @@ -6,7 +6,7 @@ class State < Spree::Base | |||
| validates :country, :name, presence: true | |||
|
|
|||
| def self.find_all_by_name_or_abbr(name_or_abbr) | |||
There was a problem hiding this comment.
I'd prefer by_name_or_abbr or with_name_or_abbr. Could just be me, but I'd expect find_all to perform a query (maybe because of old rails 2.x's find(:all)?) and this just builds a scope.
There was a problem hiding this comment.
Completely agree. I'll switch to with_name_or_abbr with a deprecation for the old method.
|
@gmacdougall @jhawthorn updated. If all looks good I'll squash these down. Update: #2045 was causing red specs so I went ahead and squashed and rebased on latest master. |
jordan-brough
force-pushed
the
case-insensitive-state-search
branch
from
67dfa8d
to
3fc42a9
Compare
This is helpful when importing data from third parties. E.g. PayPal.
jordan-brough
force-pushed
the
case-insensitive-state-search
branch
from
3fc42a9
to
67f7752
Compare
This is helpful when importing data from third parties. E.g. PayPal.
Note: I believe the spec failures are due to #2045