Skip to content
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

Updating documentation around ransack #3709

Merged
merged 2 commits into from
Jul 17, 2020

Conversation

tmtrademarked
Copy link
Contributor

This PR updates the documentation around customizing Ransack attributes to recommend the use of a decorator instead of an initializer. This allows the code to reload correctly when using Zeitwerk under Rails 6+.

Addresses #3706

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

This PR updates the documentation around customizing Ransack attributes to recommend the use of a decorator instead of an initializer. This allows the code to reload correctly when using Zeitwerk under Rails 6+.

Addresses solidusio#3706
@tmtrademarked
Copy link
Contributor Author

The test failure seems most likely to be a flake - is there a good way to re-run this via Circle?

@spaghetticode
Copy link
Member

@tmtrademarked no worries, the failure on postgres_rails_master_activestorage is expected.

the `config/initializers/spree.rb` initializer:
[ransack][ransack]. The easiest way to achieve this is by adding a decorator
which will update the attributes. This ensures that the code is reloaded correctly
when needed. (Using an initializer does not work with Rails 6+ and Zeitwerk) For this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving the sentence is the parentheses before the end of the sentence (dot)?
So that it would read like this:
when needed (using an initializer does not work with Rails 6+ and Zeitwerk). For...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing - updated!

module YourApp
module Spree
class OrderDecorator
Spree::Order.whitelisted_ransackable_attributes << 'last_ip_address'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this version that actually leverages the prepend instruction below?

class OrderDecorator
  def self.prepended(base)
     base.whitelisted_ransackable_attributes << 'last_ip_address'
   end
end

It's a bit longer, but it avoids repeating the Spree::Order class name inside the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it - a bit cleaner to read. Updated!

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmtrademarked thank you 👍

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@spaghetticode spaghetticode merged commit c781cf2 into solidusio:master Jul 17, 2020
@tmtrademarked tmtrademarked deleted the update_documentation branch September 22, 2020 23:35
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