Commit 25f34b4db7f7269447bc565926d7a6fe60298218
1 parent
8e720312
Exists in
master
and in
29 other branches
article_versions: list of versions on a new page
- Added pagination to versions - Treating access denied and not found (ActionItem2822)
Showing
14 changed files
with
130 additions
and
76 deletions
Show diff stats
app/controllers/public/content_viewer_controller.rb
@@ -7,7 +7,7 @@ class ContentViewerController < ApplicationController | @@ -7,7 +7,7 @@ class ContentViewerController < ApplicationController | ||
7 | 7 | ||
8 | def view_page | 8 | def view_page |
9 | path = params[:page].join('/') | 9 | path = params[:page].join('/') |
10 | - @version = params[:version] | 10 | + @version = params[:version].to_i |
11 | 11 | ||
12 | if path.blank? | 12 | if path.blank? |
13 | @page = profile.home_page | 13 | @page = profile.home_page |
@@ -46,7 +46,7 @@ class ContentViewerController < ApplicationController | @@ -46,7 +46,7 @@ class ContentViewerController < ApplicationController | ||
46 | 46 | ||
47 | if @version | 47 | if @version |
48 | @versioned_article = @page.versions.find_by_version(@version) | 48 | @versioned_article = @page.versions.find_by_version(@version) |
49 | - unless @versioned_article == @page.versions.latest | 49 | + if @versioned_article && @page.versions.latest.version != @versioned_article.version |
50 | render :template => 'content_viewer/versioned_article.rhtml' | 50 | render :template => 'content_viewer/versioned_article.rhtml' |
51 | return | 51 | return |
52 | end | 52 | end |
@@ -137,10 +137,16 @@ class ContentViewerController < ApplicationController | @@ -137,10 +137,16 @@ class ContentViewerController < ApplicationController | ||
137 | end | 137 | end |
138 | end | 138 | end |
139 | 139 | ||
140 | -# def article_versions | ||
141 | -# @page = profile.articles.find(params[:page]) | ||
142 | -# @versions = @page.versions | ||
143 | -# end | 140 | + def article_versions |
141 | + path = params[:page].join('/') | ||
142 | + @page = profile.articles.find_by_path(path) | ||
143 | + unless @page | ||
144 | + render_not_found(@page) | ||
145 | + return | ||
146 | + end | ||
147 | + render_access_denied unless @page.display_versions? | ||
148 | + @versions = @page.versions.paginate(:per_page => per_page, :page => params[:npage]) | ||
149 | + end | ||
144 | 150 | ||
145 | protected | 151 | protected |
146 | 152 |
app/helpers/article_helper.rb
@@ -28,7 +28,7 @@ module ArticleHelper | @@ -28,7 +28,7 @@ module ArticleHelper | ||
28 | 'div', | 28 | 'div', |
29 | check_box(:article, :notify_comments) + | 29 | check_box(:article, :notify_comments) + |
30 | content_tag('label', _('I want to receive a notification of each comment written by e-mail'), :for => 'article_notify_comments') + | 30 | content_tag('label', _('I want to receive a notification of each comment written by e-mail'), :for => 'article_notify_comments') + |
31 | - observe_field(:article_accept_comments, :function => "$('article_notify_comments').disabled = ! $('article_accept_comments').checked;$('article_moderate_comments').disabled = ! $('article_accept_comments').checked") | 31 | + observe_field(:article_accept_comments, :function => "$('article_notify_comments').disabled = ! $('article_accept_comments').checked;$('article_moderate_comments').disabled = ! $('article_accept_comments').checked") |
32 | ) + | 32 | ) + |
33 | 33 | ||
34 | content_tag( | 34 | content_tag( |
@@ -42,7 +42,7 @@ module ArticleHelper | @@ -42,7 +42,7 @@ module ArticleHelper | ||
42 | 'div', | 42 | 'div', |
43 | check_box(:article, :display_hits) + | 43 | check_box(:article, :display_hits) + |
44 | content_tag('label', _('I want this article to display the number of hits it received'), :for => 'article_display_hits') | 44 | content_tag('label', _('I want this article to display the number of hits it received'), :for => 'article_display_hits') |
45 | - ) : '') + | 45 | + ) : '') + |
46 | 46 | ||
47 | (article.can_display_versions? ? | 47 | (article.can_display_versions? ? |
48 | content_tag( | 48 | content_tag( |
app/models/article.rb
@@ -618,7 +618,7 @@ class Article < ActiveRecord::Base | @@ -618,7 +618,7 @@ class Article < ActiveRecord::Base | ||
618 | def author(version_number = nil) | 618 | def author(version_number = nil) |
619 | if version_number | 619 | if version_number |
620 | version = versions.find_by_version(version_number) | 620 | version = versions.find_by_version(version_number) |
621 | - author_id = version.last_changed_by_id | 621 | + author_id = version.last_changed_by_id if version |
622 | Person.exists?(author_id) ? Person.find(author_id) : nil | 622 | Person.exists?(author_id) ? Person.find(author_id) : nil |
623 | else | 623 | else |
624 | if versions.empty? | 624 | if versions.empty? |
app/views/content_viewer/_article_toolbar.rhtml
@@ -48,6 +48,10 @@ | @@ -48,6 +48,10 @@ | ||
48 | <%= expirable_button @page, :suggest, content, url, options %> | 48 | <%= expirable_button @page, :suggest, content, url, options %> |
49 | <% end %> | 49 | <% end %> |
50 | 50 | ||
51 | + <% if @page.display_versions? %> | ||
52 | + <%= button(:clock, _('All versions'), {:controller => 'content_viewer', :profile => profile.identifier, :action => 'article_versions'}, :id => 'article-versions-link') %> | ||
53 | + <% end %> | ||
54 | + | ||
51 | <%= report_abuse(profile, :link, @page) %> | 55 | <%= report_abuse(profile, :link, @page) %> |
52 | 56 | ||
53 | </div> | 57 | </div> |
app/views/content_viewer/_article_versions.rhtml
@@ -0,0 +1,16 @@ | @@ -0,0 +1,16 @@ | ||
1 | +<%= article_title(@page, :no_link => true) %> | ||
2 | + | ||
3 | +<p><%= _('This is the list of all versions of this content. Select a version to see it and then revert to it.') %>.</p> | ||
4 | + | ||
5 | +<ul class='article-versions'> | ||
6 | + <% @versions.each do |v| %> | ||
7 | + <li> | ||
8 | + <%= link_to(_("Version #{v.version}"), @page.url.merge(:version => v.version)) %> | ||
9 | + <%= @page.version == v.version ? _('(current)') : '' %> | ||
10 | + <span class='updated-by'><%= _('by %{author}') % {:author => link_to(@page.author_name(v.version), @page.author_url)} %></span> | ||
11 | + <div class='updated-on'><%= show_time(v.updated_at) %></div> | ||
12 | + </li> | ||
13 | + <% end %> | ||
14 | +</ul> | ||
15 | + | ||
16 | +<%= pagination_links @versions, :param_name => 'npage' %> |
app/views/content_viewer/versioned_article.rhtml
1 | <div id="article" class="<%= @page.css_class_name %>"> | 1 | <div id="article" class="<%= @page.css_class_name %>"> |
2 | 2 | ||
3 | <div id="article-actions"> | 3 | <div id="article-actions"> |
4 | + <%= button(:clock, _('All versions'), {:controller => 'content_viewer', :profile => profile.identifier, :action => 'article_versions'}, :id => 'article-versions-link') %> | ||
5 | + | ||
4 | <% if @page.allow_edit?(user) && !remove_content_button(:edit) %> | 6 | <% if @page.allow_edit?(user) && !remove_content_button(:edit) %> |
5 | - <div id="article-revert-version"> | ||
6 | - <% content = content_tag('span', _('Revert to this version')) %> | ||
7 | - <% url = profile.admin_url.merge({ :controller => 'cms', :action => 'edit', :id => @page.id, :version => @version }) %> | ||
8 | - <%= expirable_button @page, :edit, content, url %> | ||
9 | - </div> | 7 | + <% content = content_tag('span', _('Revert to this version')) %> |
8 | + <% url = profile.admin_url.merge({ :controller => 'cms', :action => 'edit', :id => @page.id, :version => @version }) %> | ||
9 | + <%= expirable_button @page, :edit, content, url, :id => 'article-revert-version-link' %> | ||
10 | <% end %> | 10 | <% end %> |
11 | </div> | 11 | </div> |
12 | 12 | ||
@@ -32,10 +32,6 @@ | @@ -32,10 +32,6 @@ | ||
32 | </div> <!-- end class="article-body" --> | 32 | </div> <!-- end class="article-body" --> |
33 | <% end %> | 33 | <% end %> |
34 | 34 | ||
35 | - <div id="article-versions"> | ||
36 | - <%= render :partial => 'article_versions' %> | ||
37 | - </div> | ||
38 | - | ||
39 | <%= display_source_info(@page) %> | 35 | <%= display_source_info(@page) %> |
40 | 36 | ||
41 | </div><!-- end id="article" --> | 37 | </div><!-- end id="article" --> |
app/views/content_viewer/view_page.rhtml
@@ -87,13 +87,6 @@ | @@ -87,13 +87,6 @@ | ||
87 | 87 | ||
88 | <%= display_source_info(@page) %> | 88 | <%= display_source_info(@page) %> |
89 | 89 | ||
90 | -<% if @page.display_versions? %> | ||
91 | - <div id="article-versions"> | ||
92 | - <%= render :partial => 'article_versions' %> | ||
93 | - <%#= button(:clock, _('See older versions'), :controller => 'content_viewer', :profile => profile.identifier, :action => 'article_versions', :page => @page.id) %> | ||
94 | - </div> | ||
95 | -<% end %> | ||
96 | - | ||
97 | <div class="comments" id="comments_list"> | 90 | <div class="comments" id="comments_list"> |
98 | 91 | ||
99 | <% if @page.accept_comments? || @comments_count > 0 %> | 92 | <% if @page.accept_comments? || @comments_count > 0 %> |
config/routes.rb
@@ -126,12 +126,14 @@ ActionController::Routing::Routes.draw do |map| | @@ -126,12 +126,14 @@ ActionController::Routing::Routes.draw do |map| | ||
126 | # cache stuff - hack | 126 | # cache stuff - hack |
127 | map.cache 'public/:action/:id', :controller => 'public' | 127 | map.cache 'public/:action/:id', :controller => 'public' |
128 | 128 | ||
129 | + map.connect ':profile/*page/versions', :controller => 'content_viewer', :action => 'article_versions', :profile => /#{Noosfero.identifier_format}/, :conditions => { :if => lambda { |env| !Domain.hosting_profile_at(env[:host]) } } | ||
130 | + map.connect '*page/versions', :controller => 'content_viewer', :action => 'article_versions' | ||
129 | 131 | ||
130 | # match requests for profiles that don't have a custom domain | 132 | # match requests for profiles that don't have a custom domain |
131 | map.homepage ':profile/*page', :controller => 'content_viewer', :action => 'view_page', :profile => /#{Noosfero.identifier_format}/, :conditions => { :if => lambda { |env| !Domain.hosting_profile_at(env[:host]) } } | 133 | map.homepage ':profile/*page', :controller => 'content_viewer', :action => 'view_page', :profile => /#{Noosfero.identifier_format}/, :conditions => { :if => lambda { |env| !Domain.hosting_profile_at(env[:host]) } } |
132 | 134 | ||
133 | - | ||
134 | # match requests for content in domains hosted for profiles | 135 | # match requests for content in domains hosted for profiles |
135 | map.connect '*page', :controller => 'content_viewer', :action => 'view_page' | 136 | map.connect '*page', :controller => 'content_viewer', :action => 'view_page' |
136 | 137 | ||
138 | + | ||
137 | end | 139 | end |
features/article_versioning.feature
@@ -5,37 +5,67 @@ Feature: article versioning | @@ -5,37 +5,67 @@ Feature: article versioning | ||
5 | 5 | ||
6 | Background: | 6 | Background: |
7 | Given the following users | 7 | Given the following users |
8 | - | login | name | | 8 | + | login | name | |
9 | | joaosilva | Joao Silva | | 9 | | joaosilva | Joao Silva | |
10 | And "joaosilva" has no articles | 10 | And "joaosilva" has no articles |
11 | And the following articles | 11 | And the following articles |
12 | - | owner | name | body | | ||
13 | - | joaosilva | Sample Article | This is the first version of the article | | 12 | + | owner | name | body | display_versions | |
13 | + | joaosilva | Sample Article | This is the first version of the article | true | | ||
14 | And the article "Sample Article" is updated with | 14 | And the article "Sample Article" is updated with |
15 | - | name | body | | 15 | + | name | body | |
16 | | Edited Article | This is the second version of the article | | 16 | | Edited Article | This is the second version of the article | |
17 | And I am logged in as "joaosilva" | 17 | And I am logged in as "joaosilva" |
18 | 18 | ||
19 | - Scenario: display only link for original version | 19 | + @selenium |
20 | + Scenario: enabling visualization of versions | ||
20 | Given the following articles | 21 | Given the following articles |
21 | - | owner | name | body | | ||
22 | - | joaosilva | One version article | This is the first published article | | ||
23 | - And I am on joaosilva's control panel | ||
24 | - And I follow "Manage Content" | ||
25 | - When I follow "One version article" | ||
26 | - Then I should see "r1" within "#article-versions" | ||
27 | - And I should not see "r2" within "#article-versions" | 22 | + | owner | name | body | |
23 | + | joaosilva | New Article | Article to enable versions | | ||
24 | + Given I am on article "New Article" | ||
25 | + Then I should not see "All versions" | ||
26 | + When I follow "Edit" within "#article-actions" | ||
27 | + And I check "I want this article to display a link to older versions" | ||
28 | + And I press "Save" | ||
29 | + Then I should be on article "New Article" | ||
30 | + And I should see "All versions" | ||
28 | 31 | ||
29 | - Scenario: display links for versions | ||
30 | - Given I am on joaosilva's control panel | ||
31 | - And I follow "Manage Content" | ||
32 | - When I follow "Edited Article" | ||
33 | - Then I should see "r1" within "#article-versions" | ||
34 | - And I should see "r2" within "#article-versions" | 32 | + @selenium |
33 | + Scenario: list versions of an article | ||
34 | + Given I am on article "Edited Article" | ||
35 | + When I follow "All versions" | ||
36 | + Then I should be on /joaosilva/edited-article/versions | ||
37 | + And I should see "Version 1" within ".article-versions" | ||
38 | + And I should see "Version 2" within ".article-versions" | ||
35 | 39 | ||
36 | - Scenario: display links for versions | ||
37 | - Given I am on joaosilva's control panel | ||
38 | - And I follow "Manage Content" | ||
39 | - And I follow "Edited Article" | ||
40 | - When I follow "r2" within "#article-versions" | ||
41 | - Then I should see "This is the first version of the article" within ".article-body" | 40 | + @selenium |
41 | + Scenario: see specific version | ||
42 | + Given I go to article "Edited Article" | ||
43 | + Then I should see "Edited Article" within ".title" | ||
44 | + And I should see "This is the second version of the article" within ".article-body" | ||
45 | + When I follow "All versions" | ||
46 | + And I follow "Version 1" | ||
47 | + Then I should see "Sample Article" within ".title" | ||
48 | + And I should see "This is the first version of the article" within ".article-body" | ||
49 | + | ||
50 | + @selenium | ||
51 | + Scenario: revert to a specific version generates a new version | ||
52 | + Given I go to article "Edited Article" | ||
53 | + When I follow "All versions" | ||
54 | + Then I should not see "Version 3" within ".article-versions" | ||
55 | + And I follow "Version 1" | ||
56 | + And I follow "Revert to this version" | ||
57 | + And I press "Save" | ||
58 | + Then I should see "Sample Article" within ".title" | ||
59 | + When I follow "All versions" | ||
60 | + Then I should see "Version 3" within ".article-versions" | ||
61 | + | ||
62 | + Scenario: try to access versions of unexistent article | ||
63 | + Given I go to /joaosilva/unexistent-article/versions | ||
64 | + Then I should see "There is no such page" | ||
65 | + | ||
66 | + Scenario: deny access to versions when disabled on article | ||
67 | + Given the following articles | ||
68 | + | owner | name | body | display_versions | | ||
69 | + | joaosilva | Versions disabled | Versions can't be displayed | false | | ||
70 | + And I go to /joaosilva/versions-disabled/versions | ||
71 | + Then I should see "Access denied" |
public/stylesheets/application.css
@@ -6539,18 +6539,7 @@ li.profile-activity-item.upload_image .activity-gallery-images-count-1 img { | @@ -6539,18 +6539,7 @@ li.profile-activity-item.upload_image .activity-gallery-images-count-1 img { | ||
6539 | z-index: 1; | 6539 | z-index: 1; |
6540 | } | 6540 | } |
6541 | 6541 | ||
6542 | -ul.article-versions { | ||
6543 | - padding: 0px; | ||
6544 | -} | ||
6545 | - | ||
6546 | ul.article-versions li { | 6542 | ul.article-versions li { |
6547 | - display: inline; | ||
6548 | font-size: 13px; | 6543 | font-size: 13px; |
6549 | -} | ||
6550 | - | ||
6551 | -#article ul.article-versions a.link-this-page { | ||
6552 | - text-decoration: none; | ||
6553 | - opacity: 0.5; | ||
6554 | - filter: alpha(opacity=50); | ||
6555 | - | 6544 | + padding: 3px 0px; |
6556 | } | 6545 | } |
test/functional/content_viewer_controller_test.rb
@@ -370,23 +370,32 @@ class ContentViewerControllerTest < ActionController::TestCase | @@ -370,23 +370,32 @@ class ContentViewerControllerTest < ActionController::TestCase | ||
370 | end | 370 | end |
371 | 371 | ||
372 | should "display current article's versions" do | 372 | should "display current article's versions" do |
373 | - page = profile.articles.create!(:name => 'myarticle', :body => 'test article') | 373 | + page = TextArticle.create!(:name => 'myarticle', :body => 'test article', :display_versions => true, :profile => profile) |
374 | page.body = 'test article edited'; page.save | 374 | page.body = 'test article edited'; page.save |
375 | 375 | ||
376 | - get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ] | ||
377 | - assert_tag :tag => 'div', :attributes => { :id => 'article-versions' }, :descendant => { | 376 | + get :article_versions, :profile => profile.identifier, :page => [ 'myarticle' ] |
377 | + assert_tag :tag => 'ul', :attributes => { :class => 'article-versions' }, :descendant => { | ||
378 | :tag => 'a', | 378 | :tag => 'a', |
379 | - :attributes => { :href => "http://#{profile.environment.default_hostname}/#{profile.identifier}/#{page.path}?rev=1" } | 379 | + :attributes => { :href => "http://#{profile.environment.default_hostname}/#{profile.identifier}/#{page.path}?version=1" } |
380 | } | 380 | } |
381 | end | 381 | end |
382 | 382 | ||
383 | should "fetch correct article version" do | 383 | should "fetch correct article version" do |
384 | - page = profile.articles.create!(:name => 'myarticle', :body => 'test article') | ||
385 | - page.body = 'test article edited'; page.save | 384 | + page = profile.articles.create!(:name => 'myarticle', :body => 'original article') |
385 | + page.body = 'edited article'; page.save | ||
386 | 386 | ||
387 | get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ], :version => 1 | 387 | get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ], :version => 1 |
388 | 388 | ||
389 | - assert_equal 1, assigns(:page).version | 389 | + assert_tag :tag => 'div', :attributes => { :class => 'article-body article-body-article' }, :content => /original article/ |
390 | + end | ||
391 | + | ||
392 | + should "display current article if version does not exist" do | ||
393 | + page = profile.articles.create!(:name => 'myarticle', :body => 'original article') | ||
394 | + page.body = 'edited article'; page.save | ||
395 | + | ||
396 | + get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ], :version => 'bli' | ||
397 | + | ||
398 | + assert_tag :tag => 'div', :attributes => { :class => 'article-body article-body-article' }, :content => /edited article/ | ||
390 | end | 399 | end |
391 | 400 | ||
392 | should 'not return an article of a different user' do | 401 | should 'not return an article of a different user' do |
test/integration/routing_test.rb
@@ -256,4 +256,19 @@ class RoutingTest < ActionController::IntegrationTest | @@ -256,4 +256,19 @@ class RoutingTest < ActionController::IntegrationTest | ||
256 | assert_recognizes({:controller => 'not_found', :action => 'nothing', :stuff => ['aksdhf']}, '/user_themes/aksdhf') | 256 | assert_recognizes({:controller => 'not_found', :action => 'nothing', :stuff => ['aksdhf']}, '/user_themes/aksdhf') |
257 | end | 257 | end |
258 | 258 | ||
259 | + should 'have route to versions of an article' do | ||
260 | + | ||
261 | + assert_routing('/ze/work/free-software/versions', :controller => 'content_viewer', :action => 'article_versions', :profile => 'ze', :page => ['work', "free-software"]) | ||
262 | + end | ||
263 | + | ||
264 | + should 'have route to versions of an article if profile has domain' do | ||
265 | + user = create_user('testuser').person | ||
266 | + domain = Domain.create!(:name => 'example.com', :owner => user) | ||
267 | + | ||
268 | + ActionController::TestRequest.any_instance.expects(:host).returns('www.example.com') | ||
269 | + | ||
270 | + assert_routing('/work/free-software/versions', :controller => 'content_viewer', :action => 'article_versions', :page => [ 'work', 'free-software'] ) | ||
271 | + end | ||
272 | + | ||
273 | + | ||
259 | end | 274 | end |
test/unit/article_test.rb
@@ -742,7 +742,7 @@ class ArticleTest < ActiveSupport::TestCase | @@ -742,7 +742,7 @@ class ArticleTest < ActiveSupport::TestCase | ||
742 | 742 | ||
743 | should 'use revision number to compose cache key' do | 743 | should 'use revision number to compose cache key' do |
744 | a = fast_create(Article, :name => 'Versioned article', :profile_id => profile.id) | 744 | a = fast_create(Article, :name => 'Versioned article', :profile_id => profile.id) |
745 | - assert_match(/-rev-2/,a.cache_key(:version => 2)) | 745 | + assert_match(/-version-2/,a.cache_key(:version => 2)) |
746 | end | 746 | end |
747 | 747 | ||
748 | should 'not be highlighted by default' do | 748 | should 'not be highlighted by default' do |