-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove name column from default ransackable attributes #3180
Remove name column from default ransackable attributes #3180
Conversation
Fixes solidusio#3179. `name` column included by default within default ransackable attributes causes ransack searches with search matchers based on `name` column to crash when appied on models which don’t have a `name` column. This PR removes the `name` column from default ransackable attributes and adds it explicitly on models which support search by name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work @mdesantis, thanks for working on this! 👏
@kennyadsl do you think this is an important change? because previous implementation could expect |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This makes sense
In this PR I fixed the tested models, but if we want to be 💯% sure that the functionality remains the same for the remaining models with a Spree::AdjustmentReason
Spree::BillingIntegration
Spree::CreditCard
Spree::Gateway
Spree::Gateway::Bogus
Spree::Gateway::BogusSimple
Spree::PaymentMethod
Spree::PaymentMethod::BogusCreditCard
Spree::PaymentMethod::Check
Spree::PaymentMethod::CreditCard
Spree::PaymentMethod::SimpleBogusCreditCard
Spree::PaymentMethod::StoreCredit
Spree::PromotionCategory
Spree::RefundReason
Spree::ReimbursementType
Spree::ReimbursementType::Credit
Spree::ReimbursementType::Exchange
Spree::ReimbursementType::OriginalPayment
Spree::ReimbursementType::StoreCredit
Spree::ReturnReason
Spree::Role
Spree::ShippingCategory
Spree::ShippingMethod
Spree::StateChange
Spree::Store
Spree::StoreCreditCategory
Spree::StoreCreditReason
Spree::StoreCreditType
Spree::TaxCategory
Spree::TaxRate I extracted this list with the following script: ActiveRecord::Base.descendants.select do |model|
begin
model.respond_to?(:whitelisted_ransackable_attributes) &&
model.columns.any? { |column| column.name == 'name' } && (
!model.whitelisted_ransackable_attributes ||
!model.whitelisted_ransackable_attributes.include?('name')
)
rescue ActiveRecord::StatementInvalid
false
end
end.map(&:to_s).sort |
Fixes #3179.
name
column included by default within default ransackable attributes causes ransack searches with search matchers based onname
column to crash when appied on models which don’t have aname
column. This PR removes thename
column from default ransackable attributes and adds it explicitly on models which support search by name.Checklist: