Skip to content
This repository

Model display name fixes #319

Closed
wants to merge 6 commits into from

4 participants

Wolfram Arnold Petteri Kääpä Erik Michaels-Ober Darren Shafae
Wolfram Arnold

When the model name has been modified with:

RailsAdmin.config do |config|
  config.model Team do
    label_for_navigation "List of teams"
  end
end

that name isn't used when it should be used for the dashboard table, the breadcrumbs, page name, history entries and delete messages.

This fixes issue: https://github.com/sferik/rails_admin/issues/#issue/318

and others added some commits March 10, 2011
Darren Shafae Fix for asset loading issue:
When assets have been copied with rake admin:copy_assets, then rails_admin should NOT insert
another ActionDispatch::Static middleware, because it eclipses Rails's native middleware and assets
are loaded from rails_admin's public folder instead of the application's public folder.
Fixed by checking if any of the rails_admin subdirectories exist in any of the static asset classes
(images, javascripts, stylesheets).
Also, if the middleware is inserted, it should be done AFTER, not IN FRONT of Rails's own static asset
middleware, to permit precedence to the application's public folder over the gem's.
1cd52f3
Wolfram Arnold Merge in sferik/master which had the same fix for static asset loadin…
…g, but didn't check for the files to be present,

which is cool.
dd46110
Wolfram Arnold Merge remote branch 'sferik/master' 51d73f3
Wolfram Arnold Fix display bug on dashboard page:
Model name in table is now using label override if present.
e88e7ea
Wolfram Arnold Fix display bug on list page:
@page_name should use overridden model name.
Same for breadcrumbs.
Spec coverage added.
aedb196
Wolfram Arnold Fixed several other references to model name for delete and history,
where @model_config.list.label was called, but @model_config.navigation.label should
have been called.
cad2d02
Petteri Kääpä
Collaborator
kaapa commented March 15, 2011

Hi Wolfram and thank you for the fix!

Your approach is on the right track, but I think it should be taken bit further. The old code allowed to define label per section (navigation, update, create, list), but that was indeed an overkill and wasn't even implemented in the dashboard. I think simplifying the label definition to a single point of entry is definitely a good idea, but it should not be contained in the navigation section, but on a higher level.

Therefore I'd propose we'd move RailsAdmin::Config::Labelable module's methods to RailsAdmin::Config::Model, remove inclusion of Labelable in RailsAdmin::Config::Sections::* and reflect that in docs & code. That way there'd be less confusion how a model's label is accessed.

In configuration one would use:

RailsAdmin.config do |config|
  config.model Team do
    label "List of teams"
  end
end

And in code:

@model_config.label

Do you find this proposal acceptable and change the pull request accordingly?

PS. I'm also thinking we should do same kind of move with RailsAdmin::Config::Hideable as authorization hooks serve the use cases I had in mind when it was designed, but that's a different topic .

Wolfram Arnold

Hi Petteri,

Thanks for your reply. To be honest, I hadn't dove deep enough into the architecture to fully understand the bigger picture. Conceptually what you're proposing makes sense, but I'll have to get my head wrapped around the configuration layer in more detail than I had previously. I'll give it a try.

Best,
Wolf

Erik Michaels-Ober
Owner

Can this pull request be close since I've merged #322?

Wolfram Arnold

Yes this can be closed. It was submitted from my co-worker's account at Papercheck. I'll do it. Sorry I forgot. I appreciate your diligence.

Best,
Wolf

Wolfram Arnold wolframarnold closed this March 19, 2011
Justin Brown jbrown referenced this pull request from a commit March 15, 2011
Wolfram Arnold Update the configuration of model label as per discussion here: #319
The model label is now configurable at the model level only, and is no longer configurable
at the section level (list, navigate, update,...), as this was considered overkill.
This refactoring was in part motivated by issue #319 which reported that display of labels was very inconsistent across
various screens, and the label configuration, if given, was not consistently effective.
The Labelable module was removed, and the methods model into config/model.rb
All references to label across the code have been updated to use the model configuration.
Specs updated and passing. Readme also updated accordingly.
c67eccc
Jay Fredlund jayfredlund referenced this pull request from a commit March 16, 2011
Erik Michaels-Ober Revert "Update the configuration of model label as per discussion here:
#319"

This reverts commit c67eccc.

Conflicts:

	README.mkd
a8f20fa
Jay Fredlund jayfredlund referenced this pull request from a commit March 17, 2011
Erik Michaels-Ober Revert "Revert "Update the configuration of model label as per discus…
…sion here: #319""

This reverts commit a8f20fa.
73df396
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 6 unique commits by 2 authors.

Mar 10, 2011
Darren Shafae Fix for asset loading issue:
When assets have been copied with rake admin:copy_assets, then rails_admin should NOT insert
another ActionDispatch::Static middleware, because it eclipses Rails's native middleware and assets
are loaded from rails_admin's public folder instead of the application's public folder.
Fixed by checking if any of the rails_admin subdirectories exist in any of the static asset classes
(images, javascripts, stylesheets).
Also, if the middleware is inserted, it should be done AFTER, not IN FRONT of Rails's own static asset
middleware, to permit precedence to the application's public folder over the gem's.
1cd52f3
Mar 11, 2011
Wolfram Arnold Merge in sferik/master which had the same fix for static asset loadin…
…g, but didn't check for the files to be present,

which is cool.
dd46110
Mar 14, 2011
Wolfram Arnold Merge remote branch 'sferik/master' 51d73f3
Wolfram Arnold Fix display bug on dashboard page:
Model name in table is now using label override if present.
e88e7ea
Wolfram Arnold Fix display bug on list page:
@page_name should use overridden model name.
Same for breadcrumbs.
Spec coverage added.
aedb196
Wolfram Arnold Fixed several other references to model name for delete and history,
where @model_config.list.label was called, but @model_config.navigation.label should
have been called.
cad2d02
This page is out of date. Refresh to see the latest.
2  app/controllers/rails_admin/history_controller.rb
@@ -24,7 +24,7 @@ def slider
24 24
 
25 25
     def for_model
26 26
       @page_type = @abstract_model.pretty_name.downcase
27  
-      @page_name = t("admin.history.page_name", :name => @model_config.list.label)
  27
+      @page_name = t("admin.history.page_name", :name => @model_config.navigation.label)
28 28
       @general = true
29 29
 
30 30
       @page_count, @history = AbstractHistory.history_for_model @abstract_model, params[:query], params[:sort], params[:sort_reverse], params[:all], params[:page]
6  app/controllers/rails_admin/main_controller.rb
@@ -111,7 +111,7 @@ def update
111 111
     def delete
112 112
       @authorization_adapter.authorize(:delete, @abstract_model, @object) if @authorization_adapter
113 113
 
114  
-      @page_name = t("admin.actions.delete").capitalize + " " + @model_config.list.label.downcase
  114
+      @page_name = t("admin.actions.delete").capitalize + " " + @model_config.navigation.label.downcase
115 115
       @page_type = @abstract_model.pretty_name.downcase
116 116
 
117 117
       render :layout => 'rails_admin/delete'
@@ -131,7 +131,7 @@ def destroy
131 131
     def bulk_delete
132 132
       @authorization_adapter.authorize(:bulk_delete, @abstract_model) if @authorization_adapter
133 133
 
134  
-      @page_name = t("admin.actions.delete").capitalize + " " + @model_config.list.label.downcase
  134
+      @page_name = t("admin.actions.delete").capitalize + " " + @model_config.navigation.label.downcase
135 135
       @page_type = @abstract_model.pretty_name.downcase
136 136
 
137 137
       render :layout => 'rails_admin/delete'
@@ -301,7 +301,7 @@ def list_entries(other = {})
301 301
       @record_count = @abstract_model.count(options, scope)
302 302
 
303 303
       @page_type = @abstract_model.pretty_name.downcase
304  
-      @page_name = t("admin.list.select", :name => @model_config.list.label.downcase)
  304
+      @page_name = t("admin.list.select", :name => @model_config.navigation.label.downcase)
305 305
     end
306 306
 
307 307
     def associations_hash
4  app/views/layouts/rails_admin/list.html.erb
@@ -43,7 +43,7 @@
43 43
           <% if @history %>
44 44
           <li>
45 45
             &rsaquo;
46  
-            <%= link_to(@model_config.list.label, rails_admin_list_path(:model_name => @abstract_model.to_param)) %>
  46
+            <%= link_to(@model_config.navigation.label, rails_admin_list_path(:model_name => @abstract_model.to_param)) %>
47 47
           </li>
48 48
           <li>
49 49
             &rsaquo;
@@ -52,7 +52,7 @@
52 52
           <% else %>
53 53
            <li>
54 54
              &rsaquo;
55  
-            <span><%= @model_config.list.label %></span>
  55
+            <span><%= @model_config.navigation.label %></span>
56 56
            </li>
57 57
           <% end%>
58 58
         </ul>
2  app/views/rails_admin/main/index.html.erb
@@ -17,7 +17,7 @@
17 17
               <% if authorized? :list, abstract_model %>
18 18
                 <tr class="<%= cycle 'odd', 'even' %>">
19 19
                   <td class="modelNameRow">
20  
-                    <%= link_to(RailsAdmin.config(abstract_model).list.label, rails_admin_list_path(:model_name => abstract_model.to_param), :class => "show") %>
  20
+                    <%= link_to(RailsAdmin.config(abstract_model).navigation.label, rails_admin_list_path(:model_name => abstract_model.to_param), :class => "show") %>
21 21
                   </td>
22 22
                   <td>
23 23
                   <% if (last_used = @most_recent_changes[abstract_model.pretty_name]) %>
28  spec/requests/config/navigation/rails_admin_config_navigation_spec.rb
@@ -46,6 +46,34 @@
46 46
       end
47 47
     end
48 48
 
  49
+    it 'should display edited model name in model name column' do
  50
+      RailsAdmin.config Fan do
  51
+        label_for_navigation "NewFan"
  52
+      end
  53
+      get rails_admin_dashboard_path
  54
+      response.should have_tag("td.modelNameRow") do |model_name_cells|
  55
+        model_name_cells.should have_tag("a[href='/admin/fans']", :content => "NewFan")
  56
+      end
  57
+    end
  58
+
  59
+    it 'should use edited model name in breadcrumbs' do
  60
+      RailsAdmin.config Fan do
  61
+        label_for_navigation "NewFan"
  62
+      end
  63
+      get rails_admin_list_path(:model_name => 'fans')
  64
+      response.should have_tag(".breadcrumb li") do |model_name_cells|
  65
+        model_name_cells.should have_tag("span", :content => "NewFan")
  66
+      end
  67
+    end
  68
+
  69
+    it 'should use edited model name in @page_name' do
  70
+      RailsAdmin.config Fan do
  71
+        label_for_navigation "NewFan"
  72
+      end
  73
+      get rails_admin_list_path(:model_name => 'fans')
  74
+      assigns[:page_name].should =~ /NewFan/i
  75
+    end
  76
+
49 77
     it "should be editable via shortcut" do
50 78
       RailsAdmin.config Fan do
51 79
         label_for_navigation "Fan test 2"
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.