Commit b473c5019edc9123843b7217d1f0ecae11062852
Exists in
staging
and in
29 other branches
Merge branch 'refactor_breadcrumbs_retry' into 'master'
Refactor breadcrumbs plugin The `ContentBreadcrumbsBlock` class HTML generation have been refactored (thanks @joenio) in terms of view and helper, improving MVC compliance but still missed the Controller coupling with the view and helper through the dependency for `params` variable. I'm still not 100% sure if `params` is accessible from views. Some more feedback and review on it would be welcome. Finally the unit tests have been fixed accordingly to the refactor. Travis build: https://travis-ci.org/rafamanzo/noosfero/builds/128041637 See merge request !898
Showing
7 changed files
with
136 additions
and
118 deletions
Show diff stats
plugins/breadcrumbs/lib/breadcrumbs_plugin/content_breadcrumbs_block.rb
@@ -22,70 +22,8 @@ class BreadcrumbsPlugin::ContentBreadcrumbsBlock < Block | @@ -22,70 +22,8 @@ class BreadcrumbsPlugin::ContentBreadcrumbsBlock < Block | ||
22 | _('This block displays breadcrumb trail.') | 22 | _('This block displays breadcrumb trail.') |
23 | end | 23 | end |
24 | 24 | ||
25 | - def page_trail(page, params={}) | ||
26 | - links = [] | ||
27 | - if page | ||
28 | - links = page.ancestors.reverse.map { |p| { :name => p.title, :url => p.url } } | ||
29 | - links << { :name => page.title, :url => page.url } | ||
30 | - elsif params[:controller] == 'cms' | ||
31 | - id = params[:id] || params[:parent_id] | ||
32 | - links = page_trail(Article.find(id)) if id | ||
33 | - links << { :name => cms_action(params[:action]), :url => params } if show_cms_action | ||
34 | - elsif (params[:controller] == 'profile' || params[:controller] == 'events') | ||
35 | - links << { :name => _('Profile'), :url => {:controller=> 'profile', :action =>'index', :profile =>params[:profile]}} | ||
36 | - links << { :name => profile_action(params[:action]), :url => params } unless params[:action] == 'index' | ||
37 | - end | ||
38 | - links | ||
39 | - end | ||
40 | - | ||
41 | - def trail(page, profile=nil, params={}) | ||
42 | - links = page_trail(page, params) | ||
43 | - if profile && !links.empty? && show_profile | ||
44 | - [ {:name => profile.name, :url => profile.url} ] + links | ||
45 | - else | ||
46 | - links | ||
47 | - end | ||
48 | - end | ||
49 | - | ||
50 | - def content(args={}) | ||
51 | - block = self | ||
52 | - ret = (proc do | ||
53 | - trail = block.trail(@page, @profile, params) | ||
54 | - if !trail.empty? | ||
55 | - separator = content_tag('span', ' > ', :class => 'separator') | ||
56 | - | ||
57 | - breadcrumb = trail.map do |t| | ||
58 | - link_to(t[:name], t[:url], :class => 'item') | ||
59 | - end.join(separator) | ||
60 | - | ||
61 | - if block.show_section_name | ||
62 | - section_name = block.show_profile ? trail.second[:name] : trail.first[:name] | ||
63 | - breadcrumb << content_tag('div', section_name, :class => 'section-name') | ||
64 | - end | ||
65 | - | ||
66 | - breadcrumb.html_safe | ||
67 | - else | ||
68 | - '' | ||
69 | - end | ||
70 | - end) | ||
71 | - ret | ||
72 | - end | ||
73 | - | ||
74 | def cacheable? | 25 | def cacheable? |
75 | false | 26 | false |
76 | end | 27 | end |
77 | 28 | ||
78 | - protected | ||
79 | - | ||
80 | - CMS_ACTIONS = {:edit => c_('Edit'), :upload_files => _('Upload Files'), :new => c_('New')} | ||
81 | - PROFILE_ACTIONS = {:members => _('Members'), :events => _('Events')} | ||
82 | - | ||
83 | - def cms_action(action) | ||
84 | - CMS_ACTIONS[action.to_sym] || action | ||
85 | - end | ||
86 | - | ||
87 | - def profile_action(action) | ||
88 | - PROFILE_ACTIONS[action.to_sym] || action | ||
89 | - end | ||
90 | - | ||
91 | end | 29 | end |
@@ -0,0 +1,43 @@ | @@ -0,0 +1,43 @@ | ||
1 | +module BreadcrumbsPluginHelper | ||
2 | + | ||
3 | + def action(action) | ||
4 | + { :edit => c_('Edit'), | ||
5 | + :upload_files => _('Upload Files'), | ||
6 | + :new => c_('New'), | ||
7 | + :members => _('Members'), | ||
8 | + :events => _('Events') | ||
9 | + }[action.to_sym] || action | ||
10 | + end | ||
11 | + | ||
12 | + def page_trail(page) | ||
13 | + links = [] | ||
14 | + page.ancestors.reverse.each do |p| | ||
15 | + links << { :name => p.title, :url => p.url } | ||
16 | + end | ||
17 | + links << { :name => page.title, :url => page.url } | ||
18 | + links | ||
19 | + end | ||
20 | + | ||
21 | + def trail(block, page, profile=nil, params={}) | ||
22 | + links = [] | ||
23 | + if page | ||
24 | + links += page_trail(page) | ||
25 | + elsif params[:controller] == 'cms' && (id = params[:id] || params[:parent_id]) | ||
26 | + links += page_trail(Article.find(id)) | ||
27 | + if block.show_cms_action | ||
28 | + links << { :name => action(params[:action]), :url => params } | ||
29 | + end | ||
30 | + elsif (params[:controller] == 'profile' || params[:controller] == 'events') | ||
31 | + _params = {:controller=> 'profile', :action =>'index', :profile => params[:profile]} | ||
32 | + links << { :name => _('Profile'), :url => _params } | ||
33 | + unless params[:action] == 'index' | ||
34 | + links << { :name => action(params[:action]), :url => params } | ||
35 | + end | ||
36 | + end | ||
37 | + if !links.empty? && profile && block.show_profile | ||
38 | + links.unshift({:name => profile.name, :url => profile.url}) | ||
39 | + end | ||
40 | + links | ||
41 | + end | ||
42 | + | ||
43 | +end |
plugins/breadcrumbs/test/unit/breadcrumbs_plugin_helper_test.rb
0 → 100644
@@ -0,0 +1,66 @@ | @@ -0,0 +1,66 @@ | ||
1 | +require 'test_helper' | ||
2 | + | ||
3 | +class BreadcrumbsPluginHelperTest < ActionView::TestCase | ||
4 | + include BreadcrumbsPluginHelper | ||
5 | + | ||
6 | + def setup | ||
7 | + @block = BreadcrumbsPlugin::ContentBreadcrumbsBlock.new | ||
8 | + @profile = fast_create(Community) | ||
9 | + @folder = fast_create(Folder, :profile_id => @profile.id) | ||
10 | + @article = fast_create(Folder, :profile_id => @profile.id, :parent_id => @folder.id) | ||
11 | + @params = {} | ||
12 | + end | ||
13 | + | ||
14 | + attr_reader :params | ||
15 | + | ||
16 | + should 'return path of links to reach a page' do | ||
17 | + links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
18 | + assert_equal links, page_trail(@article) | ||
19 | + end | ||
20 | + | ||
21 | + should 'return path of links when current page is at cms controller' do | ||
22 | + params = {:controller => 'cms', :action => 'edit', :id => @article.id} | ||
23 | + links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}, {:url=>params, :name=>"Edit"}] | ||
24 | + assert_equal links, trail(@block, nil, nil, params) | ||
25 | + end | ||
26 | + | ||
27 | + should 'not return cms action link when show_cms_action is false' do | ||
28 | + params = {:controller => 'cms', :action => 'edit', :id => @article.id} | ||
29 | + links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
30 | + @block.show_cms_action = false | ||
31 | + assert_equal links, trail(@block, nil, nil, params) | ||
32 | + end | ||
33 | + | ||
34 | + should 'include profile page link on path of links to reach a profile controller page' do | ||
35 | + params = {:controller => 'profile', :action => 'members', :profile => @profile.identifier} | ||
36 | + links = [{:name => 'Profile', :url => {:controller => 'profile', :action => 'index', :profile => @profile.identifier}}, {:name => 'Members', :url => {:controller=>'profile', :action=>'members', :profile=> @profile.identifier}}] | ||
37 | + assert_equal links, trail(@block, nil, nil, params) | ||
38 | + end | ||
39 | + | ||
40 | + should 'include only the profile page link on path links when profile action is index' do | ||
41 | + params = {:controller => 'profile', :action => 'index', :profile => @profile.identifier} | ||
42 | + links = [{:name => 'Profile', :url => {:controller => 'profile', :action => 'index', :profile => @profile.identifier}}] | ||
43 | + assert_equal links, trail(@block, nil, nil, params) | ||
44 | + end | ||
45 | + | ||
46 | + should 'profile page be the ancestor page of event profile page calendar' do | ||
47 | + params = {:controller => 'profile', :action => 'events', :profile => @profile.identifier} | ||
48 | + links = [{:name => 'Profile', :url => {:controller => 'profile', :action => 'index', :profile => @profile.identifier}}, {:name => 'Events', :url => {:controller=>'profile', :action=>'events', :profile=> @profile.identifier}}] | ||
49 | + assert_equal links, trail(@block, nil, nil, params) | ||
50 | + end | ||
51 | + | ||
52 | + should 'include profile link on path of links to reach a page' do | ||
53 | + links = [{:name => @profile.name, :url => @profile.url}, {:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
54 | + assert_equal links, trail(@block, @article, @profile) | ||
55 | + end | ||
56 | + | ||
57 | + should 'not include profile link on path of links when show_profile is false' do | ||
58 | + links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
59 | + @block.show_profile = false | ||
60 | + assert_equal links, trail(@block, @article, @profile) | ||
61 | + end | ||
62 | + | ||
63 | + should 'not include profile link on path of links when trail is empty' do | ||
64 | + assert_equal [], trail(@block, nil, @profile) | ||
65 | + end | ||
66 | +end |
plugins/breadcrumbs/test/unit/content_breadcrumbs_block_test.rb
@@ -6,14 +6,8 @@ class ContentBreadcrumbsBlockTest < ActiveSupport::TestCase | @@ -6,14 +6,8 @@ class ContentBreadcrumbsBlockTest < ActiveSupport::TestCase | ||
6 | 6 | ||
7 | def setup | 7 | def setup |
8 | @block = BreadcrumbsPlugin::ContentBreadcrumbsBlock.new | 8 | @block = BreadcrumbsPlugin::ContentBreadcrumbsBlock.new |
9 | - @profile = fast_create(Community) | ||
10 | - @folder = fast_create(Folder, :profile_id => @profile.id) | ||
11 | - @article = fast_create(Folder, :profile_id => @profile.id, :parent_id => @folder.id) | ||
12 | - @params = {} | ||
13 | end | 9 | end |
14 | 10 | ||
15 | - attr_reader :params | ||
16 | - | ||
17 | should 'has a description' do | 11 | should 'has a description' do |
18 | assert_not_equal Block.description, BreadcrumbsPlugin::ContentBreadcrumbsBlock.description | 12 | assert_not_equal Block.description, BreadcrumbsPlugin::ContentBreadcrumbsBlock.description |
19 | end | 13 | end |
@@ -22,60 +16,28 @@ class ContentBreadcrumbsBlockTest < ActiveSupport::TestCase | @@ -22,60 +16,28 @@ class ContentBreadcrumbsBlockTest < ActiveSupport::TestCase | ||
22 | assert @block.help | 16 | assert @block.help |
23 | end | 17 | end |
24 | 18 | ||
25 | - should 'return path of links to reach a page' do | ||
26 | - links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
27 | - assert_equal links, @block.page_trail(@article) | ||
28 | - end | ||
29 | - | ||
30 | - should 'return path of links when current page is at cms controller' do | ||
31 | - params = {:controller => 'cms', :action => 'edit', :id => @article.id} | ||
32 | - links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}, {:url=>{:controller=>"cms", :action=>"edit", :id=>@article.id}, :name=>"Edit"}] | ||
33 | - assert_equal links, @block.page_trail(nil, params) | ||
34 | - end | ||
35 | - | ||
36 | - should 'not return cms action link when show_cms_action is false' do | ||
37 | - params = {:controller => 'cms', :action => 'edit', :id => @article.id} | ||
38 | - links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
39 | - @block.show_cms_action = false | ||
40 | - assert_equal links, @block.page_trail(nil, params) | ||
41 | - end | ||
42 | - | ||
43 | - should 'include profile page link on path of links to reach a profile controller page' do | ||
44 | - params = {:controller => 'profile', :action => 'members', :profile => @profile.identifier} | ||
45 | - links = [{:name => 'Profile', :url => {:controller => 'profile', :action => 'index', :profile => @profile.identifier}}, {:name => 'Members', :url => {:controller=>'profile', :action=>'members', :profile=> @profile.identifier}}] | ||
46 | - assert_equal links, @block.page_trail(nil, params) | ||
47 | - end | ||
48 | 19 | ||
49 | - should 'include only the profile page link on path links when profile action is index' do | ||
50 | - params = {:controller => 'profile', :action => 'index', :profile => @profile.identifier} | ||
51 | - links = [{:name => 'Profile', :url => {:controller => 'profile', :action => 'index', :profile => @profile.identifier}}] | ||
52 | - assert_equal links, @block.page_trail(nil, params) | 20 | + should 'not be cacheable' do |
21 | + refute @block.cacheable? | ||
53 | end | 22 | end |
54 | 23 | ||
55 | - should 'profile page be the ancestor page of event profile page calendar' do | ||
56 | - params = {:controller => 'profile', :action => 'events', :profile => @profile.identifier} | ||
57 | - links = [{:name => 'Profile', :url => {:controller => 'profile', :action => 'index', :profile => @profile.identifier}}, {:name => 'Events', :url => {:controller=>'profile', :action=>'events', :profile=> @profile.identifier}}] | ||
58 | - assert_equal links, @block.page_trail(nil, params) | ||
59 | - end | 24 | +end |
60 | 25 | ||
61 | - should 'include profile link on path of links to reach a page' do | ||
62 | - links = [{:name => @profile.name, :url => @profile.url}, {:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
63 | - assert_equal links, @block.trail(@article, @profile) | ||
64 | - end | 26 | +require 'boxes_helper' |
65 | 27 | ||
66 | - should 'not include profile link on path of links when show_profile is false' do | ||
67 | - links = [{:name => @folder.name, :url => @folder.url}, {:name => @article.name, :url => @article.url}] | ||
68 | - @block.show_profile = false | ||
69 | - assert_equal links, @block.trail(@article, @profile) | ||
70 | - end | 28 | +class ContentBreadcrumbsBlockViewTest < ActionView::TestCase |
29 | + include BoxesHelper | ||
71 | 30 | ||
72 | - should 'not include profile link on path of links when trail is empty' do | ||
73 | - assert_equal [], @block.trail(nil, @profile) | 31 | + def setup |
32 | + @block = BreadcrumbsPlugin::ContentBreadcrumbsBlock.new | ||
33 | + @profile = fast_create(Community) | ||
34 | + @folder = fast_create(Folder, :profile_id => @profile.id) | ||
35 | + @article = fast_create(Folder, :profile_id => @profile.id, :parent_id => @folder.id) | ||
74 | end | 36 | end |
75 | 37 | ||
76 | should 'render trail if there is links to show' do | 38 | should 'render trail if there is links to show' do |
77 | @page = @article | 39 | @page = @article |
78 | - trail = instance_eval(&@block.content) | 40 | + trail = render_block_content(@block) |
79 | assert_match /#{@profile.name}/, trail | 41 | assert_match /#{@profile.name}/, trail |
80 | assert_match /#{@folder.name}/, trail | 42 | assert_match /#{@folder.name}/, trail |
81 | assert_match /#{@page.name}/, trail | 43 | assert_match /#{@page.name}/, trail |
@@ -83,11 +45,6 @@ class ContentBreadcrumbsBlockTest < ActiveSupport::TestCase | @@ -83,11 +45,6 @@ class ContentBreadcrumbsBlockTest < ActiveSupport::TestCase | ||
83 | 45 | ||
84 | should 'render nothing if there is no links to show' do | 46 | should 'render nothing if there is no links to show' do |
85 | @page = nil | 47 | @page = nil |
86 | - assert_equal '', instance_eval(&@block.content) | ||
87 | - end | ||
88 | - | ||
89 | - should 'not be cacheable' do | ||
90 | - refute @block.cacheable? | 48 | + assert_equal '', render_block_content(@block) |
91 | end | 49 | end |
92 | - | ||
93 | end | 50 | end |
@@ -0,0 +1 @@ | @@ -0,0 +1 @@ | ||
1 | +== link_to link[:name], link[:url], :class => 'item' |
plugins/breadcrumbs/views/blocks/content_breadcrumbs.slim
0 → 100644
@@ -0,0 +1,11 @@ | @@ -0,0 +1,11 @@ | ||
1 | +- extend BreadcrumbsPluginHelper | ||
2 | + | ||
3 | +- links = trail(block, @page, @profile, params).flatten | ||
4 | +- unless links.empty? | ||
5 | + == render :partial => 'blocks/link', :collection => links, :as => 'link', :spacer_template => 'blocks/separator' | ||
6 | + - if block.show_section_name | ||
7 | + div class='section-name' | ||
8 | + - if block.show_profile | ||
9 | + = links.second[:name] | ||
10 | + - else | ||
11 | + = links.first[:name] |