From 25f34b4db7f7269447bc565926d7a6fe60298218 Mon Sep 17 00:00:00 2001
From: Daniela Soares Feitosa <danielafeitosa@colivre.coop.br>
Date: Sun, 19 Jan 2014 16:19:58 +0000
Subject: [PATCH] article_versions: list of versions on a new page

---
 app/controllers/public/content_viewer_controller.rb | 18 ++++++++++++------
 app/helpers/article_helper.rb                       |  4 ++--
 app/models/article.rb                               |  2 +-
 app/views/content_viewer/_article_toolbar.rhtml     |  4 ++++
 app/views/content_viewer/_article_versions.rhtml    |  6 ------
 app/views/content_viewer/article_versions.rhtml     | 16 ++++++++++++++++
 app/views/content_viewer/versioned_article.rhtml    | 14 +++++---------
 app/views/content_viewer/view_page.rhtml            |  7 -------
 config/routes.rb                                    |  4 +++-
 features/article_versioning.feature                 | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------
 public/stylesheets/application.css                  | 13 +------------
 test/functional/content_viewer_controller_test.rb   | 23 ++++++++++++++++-------
 test/integration/routing_test.rb                    | 15 +++++++++++++++
 test/unit/article_test.rb                           |  2 +-
 14 files changed, 130 insertions(+), 76 deletions(-)
 delete mode 100644 app/views/content_viewer/_article_versions.rhtml
 create mode 100644 app/views/content_viewer/article_versions.rhtml

diff --git a/app/controllers/public/content_viewer_controller.rb b/app/controllers/public/content_viewer_controller.rb
index 047ce38..0a2987c 100644
--- a/app/controllers/public/content_viewer_controller.rb
+++ b/app/controllers/public/content_viewer_controller.rb
@@ -7,7 +7,7 @@ class ContentViewerController < ApplicationController
 
   def view_page
     path = params[:page].join('/')
-    @version = params[:version]
+    @version = params[:version].to_i
 
     if path.blank?
       @page = profile.home_page
@@ -46,7 +46,7 @@ class ContentViewerController < ApplicationController
 
     if @version
       @versioned_article = @page.versions.find_by_version(@version)
-      unless @versioned_article == @page.versions.latest
+      if @versioned_article && @page.versions.latest.version != @versioned_article.version
         render :template => 'content_viewer/versioned_article.rhtml'
         return
       end
@@ -137,10 +137,16 @@ class ContentViewerController < ApplicationController
     end
   end
 
-#  def article_versions
-#    @page = profile.articles.find(params[:page])
-#    @versions = @page.versions
-#  end
+  def article_versions
+    path = params[:page].join('/')
+    @page = profile.articles.find_by_path(path)
+    unless @page
+      render_not_found(@page)
+      return
+    end
+    render_access_denied unless @page.display_versions?
+    @versions = @page.versions.paginate(:per_page => per_page, :page => params[:npage])
+  end
 
   protected
 
diff --git a/app/helpers/article_helper.rb b/app/helpers/article_helper.rb
index 417b41b..ea9046d 100644
--- a/app/helpers/article_helper.rb
+++ b/app/helpers/article_helper.rb
@@ -28,7 +28,7 @@ module ArticleHelper
         'div',
         check_box(:article, :notify_comments) +
         content_tag('label', _('I want to receive a notification of each comment written by e-mail'), :for => 'article_notify_comments') +
-        observe_field(:article_accept_comments, :function => "$('article_notify_comments').disabled = ! $('article_accept_comments').checked;$('article_moderate_comments').disabled = ! $('article_accept_comments').checked") 
+        observe_field(:article_accept_comments, :function => "$('article_notify_comments').disabled = ! $('article_accept_comments').checked;$('article_moderate_comments').disabled = ! $('article_accept_comments').checked")
       ) +
 
       content_tag(
@@ -42,7 +42,7 @@ module ArticleHelper
         'div',
         check_box(:article, :display_hits) +
         content_tag('label', _('I want this article to display the number of hits it received'), :for => 'article_display_hits')
-      ) : '') + 
+      ) : '') +
 
       (article.can_display_versions? ?
       content_tag(
diff --git a/app/models/article.rb b/app/models/article.rb
index 2a36a3f..1a615ea 100644
--- a/app/models/article.rb
+++ b/app/models/article.rb
@@ -618,7 +618,7 @@ class Article < ActiveRecord::Base
   def author(version_number = nil)
     if version_number
       version = versions.find_by_version(version_number)
-      author_id = version.last_changed_by_id
+      author_id = version.last_changed_by_id if version
       Person.exists?(author_id) ? Person.find(author_id) : nil
     else
       if versions.empty?
diff --git a/app/views/content_viewer/_article_toolbar.rhtml b/app/views/content_viewer/_article_toolbar.rhtml
index 3e9723b..bb76ef4 100644
--- a/app/views/content_viewer/_article_toolbar.rhtml
+++ b/app/views/content_viewer/_article_toolbar.rhtml
@@ -48,6 +48,10 @@
       <%= expirable_button @page, :suggest, content, url, options %>
     <% end %>
 
+    <% if @page.display_versions? %>
+      <%= button(:clock, _('All versions'), {:controller => 'content_viewer', :profile => profile.identifier, :action => 'article_versions'}, :id => 'article-versions-link') %>
+    <% end %>
+
     <%= report_abuse(profile, :link, @page) %>
 
   </div>
diff --git a/app/views/content_viewer/_article_versions.rhtml b/app/views/content_viewer/_article_versions.rhtml
deleted file mode 100644
index 5e7165a..0000000
--- a/app/views/content_viewer/_article_versions.rhtml
+++ /dev/null
@@ -1,6 +0,0 @@
-<h3><%= _('Versions') %></h3>
-<ul class='article-versions'>
-  <% @page.versions.each do |v| %>
-    <li><%= link_to("#{v.version}", @page.url.merge(:version => v.version)) %></li>
-  <% end %>
-<ul>
diff --git a/app/views/content_viewer/article_versions.rhtml b/app/views/content_viewer/article_versions.rhtml
new file mode 100644
index 0000000..3613994
--- /dev/null
+++ b/app/views/content_viewer/article_versions.rhtml
@@ -0,0 +1,16 @@
+<%= article_title(@page, :no_link => true) %>
+
+<p><%= _('This is the list of all versions of this content. Select a version to see it and then revert to it.') %>.</p>
+
+<ul class='article-versions'>
+  <% @versions.each do |v| %>
+    <li>
+      <%= link_to(_("Version #{v.version}"), @page.url.merge(:version => v.version)) %>
+      <%= @page.version == v.version ? _('(current)') : '' %>
+      <span class='updated-by'><%= _('by %{author}') % {:author => link_to(@page.author_name(v.version), @page.author_url)} %></span>
+      <div class='updated-on'><%= show_time(v.updated_at) %></div>
+    </li>
+  <% end %>
+</ul>
+
+<%= pagination_links @versions, :param_name => 'npage' %>
diff --git a/app/views/content_viewer/versioned_article.rhtml b/app/views/content_viewer/versioned_article.rhtml
index 6e626da..130d09b 100644
--- a/app/views/content_viewer/versioned_article.rhtml
+++ b/app/views/content_viewer/versioned_article.rhtml
@@ -1,12 +1,12 @@
 <div id="article" class="<%= @page.css_class_name %>">
 
   <div id="article-actions">
+    <%= button(:clock, _('All versions'), {:controller => 'content_viewer', :profile => profile.identifier, :action => 'article_versions'}, :id => 'article-versions-link') %>
+
     <% if @page.allow_edit?(user) && !remove_content_button(:edit) %>
-      <div id="article-revert-version">
-        <% content = content_tag('span', _('Revert to this version')) %>
-        <% url = profile.admin_url.merge({ :controller => 'cms', :action => 'edit', :id => @page.id, :version => @version }) %>
-        <%= expirable_button @page, :edit, content, url %>
-      </div>
+      <% content = content_tag('span', _('Revert to this version')) %>
+      <% url = profile.admin_url.merge({ :controller => 'cms', :action => 'edit', :id => @page.id, :version => @version }) %>
+      <%= expirable_button @page, :edit, content, url, :id => 'article-revert-version-link' %>
     <% end %>
   </div>
 
@@ -32,10 +32,6 @@
     </div> <!-- end class="article-body" -->
   <% end %>
 
-  <div id="article-versions">
-    <%= render :partial => 'article_versions' %>
-  </div>
-
   <%= display_source_info(@page) %>
 
 </div><!-- end id="article" -->
diff --git a/app/views/content_viewer/view_page.rhtml b/app/views/content_viewer/view_page.rhtml
index 2c9c4a5..be612fc 100644
--- a/app/views/content_viewer/view_page.rhtml
+++ b/app/views/content_viewer/view_page.rhtml
@@ -87,13 +87,6 @@
 
 <%= display_source_info(@page) %>
 
-<% if @page.display_versions? %>
-  <div id="article-versions">
-    <%= render :partial => 'article_versions' %>
-    <%#= button(:clock, _('See older versions'), :controller => 'content_viewer', :profile => profile.identifier, :action => 'article_versions', :page => @page.id) %>
-  </div>
-<% end %>
-
 <div class="comments" id="comments_list">
 
   <% if @page.accept_comments? || @comments_count > 0 %>
diff --git a/config/routes.rb b/config/routes.rb
index fa98554..e4b4f2c 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -126,12 +126,14 @@ ActionController::Routing::Routes.draw do |map|
   # cache stuff - hack
   map.cache 'public/:action/:id', :controller => 'public'
 
+  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]) } }
+  map.connect '*page/versions', :controller => 'content_viewer', :action => 'article_versions'
 
   # match requests for profiles that don't have a custom domain
   map.homepage ':profile/*page', :controller => 'content_viewer', :action => 'view_page', :profile => /#{Noosfero.identifier_format}/, :conditions => { :if => lambda { |env| !Domain.hosting_profile_at(env[:host]) } }
 
-
   # match requests for content in domains hosted for profiles
   map.connect '*page', :controller => 'content_viewer', :action => 'view_page'
 
+
 end
diff --git a/features/article_versioning.feature b/features/article_versioning.feature
index 2d7107a..ecf1a1e 100644
--- a/features/article_versioning.feature
+++ b/features/article_versioning.feature
@@ -5,37 +5,67 @@ Feature: article versioning
 
   Background:
     Given the following users
-      | login | name |
+      | login     | name       |
       | joaosilva | Joao Silva |
     And "joaosilva" has no articles
     And the following articles
-      | owner | name | body |
-      | joaosilva | Sample Article | This is the first version of the article |
+      | owner     | name           | body                                     | display_versions |
+      | joaosilva | Sample Article | This is the first version of the article | true             |
     And the article "Sample Article" is updated with
-      | name | body |
+      | name           | body                                      |
       | Edited Article | This is the second version of the article |
     And I am logged in as "joaosilva"
 
-  Scenario: display only link for original version
+  @selenium
+  Scenario: enabling visualization of versions
     Given the following articles
-      | owner | name | body |
-      | joaosilva | One version article | This is the first published article |
-    And I am on joaosilva's control panel
-    And I follow "Manage Content"
-    When I follow "One version article"
-    Then I should see "r1" within "#article-versions"
-    And I should not see "r2" within "#article-versions"
+      | owner     | name        | body                       |
+      | joaosilva | New Article | Article to enable versions |
+   Given I am on article "New Article"
+    Then I should not see "All versions"
+    When I follow "Edit" within "#article-actions"
+    And I check "I want this article to display a link to older versions"
+    And I press "Save"
+    Then I should be on article "New Article"
+    And I should see "All versions"
 
-  Scenario: display links for versions
-    Given I am on joaosilva's control panel
-    And I follow "Manage Content"
-    When I follow "Edited Article"
-    Then I should see "r1" within "#article-versions"
-    And I should see "r2" within "#article-versions"
+  @selenium
+  Scenario: list versions of an article
+    Given I am on article "Edited Article"
+    When I follow "All versions"
+    Then I should be on /joaosilva/edited-article/versions
+    And I should see "Version 1" within ".article-versions"
+    And I should see "Version 2" within ".article-versions"
 
-  Scenario: display links for versions
-    Given I am on joaosilva's control panel
-    And I follow "Manage Content"
-    And I follow "Edited Article" 
-    When I follow "r2" within "#article-versions"
-    Then I should see "This is the first version of the article" within ".article-body"
+  @selenium
+  Scenario: see specific version
+    Given I go to article "Edited Article"
+    Then I should see "Edited Article" within ".title"
+    And I should see "This is the second version of the article" within ".article-body"
+    When I follow "All versions"
+    And I follow "Version 1"
+    Then I should see "Sample Article" within ".title"
+    And I should see "This is the first version of the article" within ".article-body"
+
+  @selenium
+  Scenario: revert to a specific version generates a new version
+    Given I go to article "Edited Article"
+    When I follow "All versions"
+    Then I should not see "Version 3" within ".article-versions"
+    And I follow "Version 1"
+    And I follow "Revert to this version"
+    And I press "Save"
+    Then I should see "Sample Article" within ".title"
+    When I follow "All versions"
+    Then I should see "Version 3" within ".article-versions"
+
+  Scenario: try to access versions of unexistent article
+    Given I go to /joaosilva/unexistent-article/versions
+    Then I should see "There is no such page"
+
+  Scenario: deny access to versions when disabled on article
+    Given the following articles
+      | owner     | name              | body                        | display_versions |
+      | joaosilva | Versions disabled | Versions can't be displayed | false            |
+    And I go to /joaosilva/versions-disabled/versions
+    Then I should see "Access denied"
diff --git a/public/stylesheets/application.css b/public/stylesheets/application.css
index 27f15fd..32b2e76 100644
--- a/public/stylesheets/application.css
+++ b/public/stylesheets/application.css
@@ -6539,18 +6539,7 @@ li.profile-activity-item.upload_image .activity-gallery-images-count-1 img {
   z-index: 1;
 }
 
-ul.article-versions {
-  padding: 0px;
-}
-
 ul.article-versions li {
-  display: inline;
   font-size: 13px;
-}
-
-#article ul.article-versions a.link-this-page {
-  text-decoration: none;
-  opacity: 0.5;
-  filter: alpha(opacity=50);
-
+  padding: 3px 0px;
 }
diff --git a/test/functional/content_viewer_controller_test.rb b/test/functional/content_viewer_controller_test.rb
index 6ba6058..d73984d 100644
--- a/test/functional/content_viewer_controller_test.rb
+++ b/test/functional/content_viewer_controller_test.rb
@@ -370,23 +370,32 @@ class ContentViewerControllerTest < ActionController::TestCase
   end
 
   should "display current article's versions" do
-    page = profile.articles.create!(:name => 'myarticle', :body => 'test article')
+    page = TextArticle.create!(:name => 'myarticle', :body => 'test article', :display_versions => true, :profile => profile)
     page.body = 'test article edited'; page.save
 
-    get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ]
-    assert_tag :tag => 'div', :attributes => { :id => 'article-versions' }, :descendant => {
+    get :article_versions, :profile => profile.identifier, :page => [ 'myarticle' ]
+    assert_tag :tag => 'ul', :attributes => { :class => 'article-versions' }, :descendant => {
       :tag => 'a',
-      :attributes => { :href => "http://#{profile.environment.default_hostname}/#{profile.identifier}/#{page.path}?rev=1" }
+      :attributes => { :href => "http://#{profile.environment.default_hostname}/#{profile.identifier}/#{page.path}?version=1" }
     }
   end
 
   should "fetch correct article version" do
-    page = profile.articles.create!(:name => 'myarticle', :body => 'test article')
-    page.body = 'test article edited'; page.save
+    page = profile.articles.create!(:name => 'myarticle', :body => 'original article')
+    page.body = 'edited article'; page.save
 
     get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ], :version => 1
 
-    assert_equal 1, assigns(:page).version
+    assert_tag :tag => 'div', :attributes => { :class => 'article-body article-body-article' }, :content => /original article/
+  end
+
+  should "display current article if version does not exist" do
+    page = profile.articles.create!(:name => 'myarticle', :body => 'original article')
+    page.body = 'edited article'; page.save
+
+    get :view_page, :profile => profile.identifier, :page => [ 'myarticle' ], :version => 'bli'
+
+    assert_tag :tag => 'div', :attributes => { :class => 'article-body article-body-article' }, :content => /edited article/
   end
 
   should 'not return an article of a different user' do
diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb
index f3b5ff0..2117bac 100644
--- a/test/integration/routing_test.rb
+++ b/test/integration/routing_test.rb
@@ -256,4 +256,19 @@ class RoutingTest < ActionController::IntegrationTest
     assert_recognizes({:controller => 'not_found', :action => 'nothing', :stuff => ['aksdhf']}, '/user_themes/aksdhf')
   end
 
+  should 'have route to versions of an article' do
+
+    assert_routing('/ze/work/free-software/versions', :controller => 'content_viewer', :action => 'article_versions', :profile => 'ze', :page => ['work', "free-software"])
+  end
+
+  should 'have route to versions of an article if profile has domain' do
+    user = create_user('testuser').person
+    domain = Domain.create!(:name => 'example.com', :owner => user)
+
+    ActionController::TestRequest.any_instance.expects(:host).returns('www.example.com')
+
+    assert_routing('/work/free-software/versions', :controller => 'content_viewer', :action =>  'article_versions', :page => [ 'work', 'free-software'] )
+  end
+
+
 end
diff --git a/test/unit/article_test.rb b/test/unit/article_test.rb
index 1ea2c11..180611b 100644
--- a/test/unit/article_test.rb
+++ b/test/unit/article_test.rb
@@ -742,7 +742,7 @@ class ArticleTest < ActiveSupport::TestCase
 
  should 'use revision number to compose cache key' do
     a = fast_create(Article, :name => 'Versioned article', :profile_id => profile.id)
-    assert_match(/-rev-2/,a.cache_key(:version => 2))
+    assert_match(/-version-2/,a.cache_key(:version => 2))
   end
 
   should 'not be highlighted by default' do
--
libgit2 0.21.2