Make PaymentsHelper available to Order views #1496
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `confirm` screen shows the payments info[1]. This uses[2] the helper `payment_method_name`[3] without explicitly indicating that controller is using that helper. In many Rails apps, this may be ok due to `config.action_controller.include_all_helpers` defaulting to `true`. But if the application has configured this to be `false` the confirm screen breaks. I am personally a fan of the old Rails behavior (i.e. don't put all helpers in a big pile but make separate helper modules actually mean something). Even though it is the old behavior it is still supported behavior by Rails so it seems an effort to work either way should be made. 1. https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/orders/confirm.html.erb#L54 2. https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/payments/_list.html.erb#L22 3. https://github.com/solidusio/solidus/blob/master/backend/app/helpers/spree/admin/payments_helper.rb#L4
|
👍 for explicitness. I agree about not wanting to include all helpers in all controllers -- things get very messy on large apps when you do that. |
jhawthorn
approved these changes
Oct 4, 2016
|
Agreed. Unfortunately my upstream commits are limited to just stuff I hit while working on my primary goal. Systematically going through all screens to make sure everything is |
gmacdougall
approved these changes
Oct 13, 2016
eric1234
added a commit
to eric1234/solidus
that referenced
this pull request
Feb 16, 2017
The [backend/app/views/spree/admin/variants/_form.html.erb](https://github.com/solidusio/solidus/blob/master/backend/app/views/spree/admin/variants/_form.html.erb#L73) now uses a helper method called `show_rebuild_vat_checkbox?` which is defined in [Spree::Admin::ProductsHelper](https://github.com/solidusio/solidus/blob/master/backend/app/helpers/spree/admin/products_helper.rb#L28). This helper is not automatically included if `config.action_controller.include_all_helpers` is configured to false. See [PR solidusio#1496](solidusio#1496) for rational and previous discussion on this topic.
eric1234
added a commit
to eric1234/solidus
that referenced
this pull request
Mar 14, 2017
Another fix to make Solidus `config.action_controller.include_all_helpers = false` compatible. See PR solidusio#1496 and PR solidusio#1714 for background.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
confirmscreen shows the payments info. This uses the helperpayment_method_namewithout explicitly indicating that controller is using that helper.In many Rails apps, this may be ok due to
config.action_controller.include_all_helpersdefaulting totrue. But if the application has configured this to befalsethe confirm screen breaks. I am personally a fan of the old Rails behavior (i.e. don't put all helpers in a big pile but make separate helper modules actually mean something). Even though it is the old behavior it is still supported behavior by Rails so it seems an effort to work either way should be made.