Migrate view helpers to app/helpers path#8202
Merged
Merged
Conversation
This is a single method and used in the layout. No need to call with_indifferent_access anymore as it supports that access already.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8202 +/- ##
==========================================
+ Coverage 99.03% 99.12% +0.09%
==========================================
Files 149 144 -5
Lines 4127 4104 -23
==========================================
- Hits 4087 4068 -19
+ Misses 40 36 -4 ☔ View full report in Codecov by Sentry. |
Removing the `require "active_admin/view_helpers/method_or_proc_helper"` in `lib/active_admin/filters.rb` hid a loading bug with `lib/active_admin/resource.rb` since some of its classes use helper methods from method_or_proc_helper (e.g. scopes, action_items, etc.) but included the module without requiring it. We now require it at the top of the resource file. This brings together the original form_helper and fields_for file which the latter oddly didn't declare a different module but used form helper instead. Since the filter form helper spec included a lot of tests, we'll keep this spec separate from form helper despite being in the same one.
No need to include this in the page controller since specific to index view scopes/filters.
2e727b5 to
ab2c074
Compare
Several methods in DisplayHelper are only used internally and private so now marked as private. The pretty_format spec is now included in the display helper spec.
This was updated in https://github.com/activeadmin/activeadmin/pull/5611/files#diff-bafdb16f12eab7769801ce340a4bd6930e82b9cb8546db3f11ec5f7b6e50986e so the module used extend self but we can use helpers here since its in a controller. The helper module was already being included in the base controller. We can't remove the `extend self` portion yet without additional changes.
e3fe688 to
c07ef4b
Compare
The MethodOrProcHelper still has to live in lib for now as having a hard time getting it to work in app/helpers with its use in resource/menu item classes.
This way we remove the only reference to a MethodOrProcHelper method (in this case render_or_call_method_or_proc_on) which is considered a private API from a view template that the host app can copy.
c07ef4b to
2fb62a1
Compare
In Devise templates, the active_admin_namespace helper won't be available so we need to use active_admin_application but in the admin itself, we have to use the namespace to support site title that can be configured by namespace. We didn't have tests beforehand, even previous release, to cover this case so this adds a feature spec that was failing pre-fix and passing post-fix.
These are already covered by feature specs but helps to have some simple unit tests in place.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This migrates the view helpers from lib to the app/helpers directory like done in other gems and for a modern Rails approach. These methods were already being set as view helpers but this makes it a little easier to test through a plain helper spec and gives us autoloading in development. We'll be adding new helper methods for common actions (e.g. can view show resource link, etc.) so these changes will help.