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

Fix stock configuration examples in documentation #3487

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

gugaiz
Copy link
Contributor

@gugaiz gugaiz commented Jan 24, 2020

Fix an issue with the documentation related to the custom configuration
of the stock location filter and sorter. The way it was documented
didn't work.

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)

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 for your contribution! Maybe Spree.config.spree.stock.location_filter_class = 'Spree::Stock::LocationFilter::SameOrderCountry' without the block is enough?

@gugaiz
Copy link
Contributor Author

gugaiz commented Jan 24, 2020

@kennyadsl, I tried that but didn't work for me neither.
getting 'config': no block given (yield) (LocalJumpError)

@kennyadsl
Copy link
Member

Just to be sure, are you restarting the rails server after these changes?

@gugaiz
Copy link
Contributor Author

gugaiz commented Jan 24, 2020

yes

@kennyadsl
Copy link
Member

Sorry, my fault, it should be:

Spree::Config.stock.location_filter_class = 'Spree::Stock::LocationFilter::SameOrderCountry'

@gugaiz
Copy link
Contributor Author

gugaiz commented Jan 24, 2020

Ok, that worked. I can change it with a new commit if you prefer that way

@kennyadsl
Copy link
Member

Yes, I think it's better. And please, can you also squash commits into a single one before pushing? Thanks again! 🙂

@gugaiz gugaiz force-pushed the fix_documentation branch from 97be617 to 62c9777 Compare January 24, 2020 16:28
@gugaiz
Copy link
Contributor Author

gugaiz commented Jan 24, 2020

Done

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!

@kennyadsl kennyadsl changed the title Fix issue on documentation related with the custom configuration of the stock location filter and sorter Fix stock configuration examples in documentation Jan 24, 2020
@spaghetticode
Copy link
Member

Out of curiosity, I tried to verify the issue locally.

So, I generated a sandbox app with rake sandbox, then edited the config/initializers/spree.rb like this:

class CustomAllocator
end
class CustomFilter
end
class CustomSorter
end

Spree.config do |config|
  config.stock.allocator_class = 'CustomAllocator'
  config.stock.location_filter_class = 'CustomFilter'
  config.stock.location_sorter_class = 'CustomSorter'
  # ...
end

then starting the sandbox console with rails c I got the expected custom configuration values:

Loading development environment (Rails 6.0.2.1)
2.6.3 :001 > Spree::Config.stock.allocator_class
 => CustomAllocator 
2.6.3 :002 > Spree::Config.stock.location_filter_class
 => CustomFilter 
2.6.3 :003 > Spree::Config.stock.location_sorter_class
 => CustomSorter 

So, in this case it seems that following the existing documentation work properly. @gugaiz can you explain in detail what kind of issues you experienced?

@gugaiz
Copy link
Contributor Author

gugaiz commented Jan 31, 2020

@spaghetticode that works and that was my first proposal, but @kennyadsl preferred the way is documented on this PR ( Spree::Config.stock.location_filter_class )
The current documentation suggest to use Rails.application.config.spree.stock.location_filter_class which does not work.

@spaghetticode
Copy link
Member

spaghetticode commented Jan 31, 2020

@gugaiz thanks for the clarification. So, the documentation is wrong only for the two files that used Rails.application.config. I verified in the sandbox console that the correct way to access/set that configuration via Rails.application.config would be with:

Rails.application.config.spree.preferences.stock.location_filter_class

and

Rails.application.config.spree.preferences.stock.location_sorter_class

I have no idea if we should prefer one way or the other(s) for this case, maybe we should document them all?

@gugaiz
Copy link
Contributor Author

gugaiz commented Jan 31, 2020

@spaghetticode I guess someone else might answer that. If you ask me, I prefer the way below because is what you find when you open the config/initializer/spree.rb file

Spree.config do |config|
  config.stock.allocator_class = 'CustomAllocator'
  config.stock.location_filter_class = 'CustomFilter'
  config.stock.location_sorter_class = 'CustomSorter'
  # ...
end

@tvdeyen
Copy link
Member

tvdeyen commented Feb 5, 2020

@spaghetticode I guess someone else might answer that. If you ask me, I prefer the way below because is what you find when you open the config/initializer/spree.rb file

Spree.config do |config|
  config.stock.allocator_class = 'CustomAllocator'
  config.stock.location_filter_class = 'CustomFilter'
  config.stock.location_sorter_class = 'CustomSorter'
  # ...
end

I also prefer that way

@kennyadsl
Copy link
Member

I'm fine with both, so 👍 if @gugaiz has time to change it.

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.

@gugaiz thanks 👍

Fix an issue with the documentation related to the custom configuration
of the stock location filter and sorter. The way it was documented
didn't work.
@gugaiz
Copy link
Contributor Author

gugaiz commented Feb 25, 2020

@kennyadsl I think is done

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.

@gugaiz 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!

@spaghetticode spaghetticode merged commit 0fbeac4 into solidusio:master Feb 25, 2020
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.

4 participants