From 9fc96fa97ec1e9473b6f160493eccfc5c100a4e2 Mon Sep 17 00:00:00 2001 From: Leandro Nunes dos Santos Date: Tue, 15 Apr 2014 11:40:39 -0300 Subject: [PATCH] load article's author only when necessary --- app/controllers/my_profile/cms_controller.rb | 1 + app/models/article.rb | 32 +++++++++++++++++--------------- test/functional/cms_controller_test.rb | 27 ++++++++++++++++++++++++++- test/functional/content_viewer_controller_test.rb | 2 +- test/unit/article_test.rb | 35 ++++++++++++++++++++++++----------- 5 files changed, 69 insertions(+), 28 deletions(-) diff --git a/app/controllers/my_profile/cms_controller.rb b/app/controllers/my_profile/cms_controller.rb index 557bd6b..0a00238 100644 --- a/app/controllers/my_profile/cms_controller.rb +++ b/app/controllers/my_profile/cms_controller.rb @@ -144,6 +144,7 @@ class CmsController < MyProfileController @article.profile = profile @article.last_changed_by = user + @article.created_by = user translations if @article.translatable? diff --git a/app/models/article.rb b/app/models/article.rb index d667bce..7980502 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -40,12 +40,6 @@ class Article < ActiveRecord::Base # xss_terminate plugin can't sanitize array fields before_save :sanitize_tag_list - before_create do |article| - if article.last_changed_by_id - article.author_name = Person.find(article.last_changed_by_id).name - end - end - belongs_to :profile validates_presence_of :profile_id, :name validates_presence_of :slug, :path, :if => lambda { |article| !article.name.blank? } @@ -55,6 +49,7 @@ class Article < ActiveRecord::Base validates_uniqueness_of :slug, :scope => ['profile_id', 'parent_id'], :message => N_('The title (article name) is already being used by another article, please use another title.'), :if => lambda { |article| !article.slug.blank? } belongs_to :last_changed_by, :class_name => 'Person', :foreign_key => 'last_changed_by_id' + belongs_to :created_by, :class_name => 'Person', :foreign_key => 'created_by_id' has_many :comments, :class_name => 'Comment', :foreign_key => 'source_id', :dependent => :destroy, :order => 'created_at asc' @@ -89,6 +84,11 @@ class Article < ActiveRecord::Base article.parent = article.profile.blog end end + + if article.created_by + article.author_name = article.created_by.name + end + end after_destroy :destroy_activity @@ -630,18 +630,20 @@ class Article < ActiveRecord::Base def author(version_number = nil) if version_number - version = versions.find_by_version(version_number) + version = self.versions.find_by_version(version_number) author_id = version.last_changed_by_id if version - Person.exists?(author_id) ? Person.find(author_id) : nil else - if versions.empty? - last_changed_by - else - author_id = versions.first.last_changed_by_id - Person.exists?(author_id) ? Person.find(author_id) : nil - end + author_id = self.created_by_id end - end + + begin + @author = Person.find(author_id) if !@author || @author.id != author_id + rescue + @author = nil + end + + @author + end def author_name(version_number = nil) person = author(version_number) diff --git a/test/functional/cms_controller_test.rb b/test/functional/cms_controller_test.rb index 4332a15..9eb1c37 100644 --- a/test/functional/cms_controller_test.rb +++ b/test/functional/cms_controller_test.rb @@ -1228,7 +1228,7 @@ class CmsControllerTest < ActionController::TestCase should 'allow user edit article if he is owner and has publish permission' do c = Community.create!(:name => 'test_comm', :identifier => 'test_comm') u = create_user_with_permission('test_user', 'publish_content', c) - a = create(Article, :profile => c, :name => 'test_article', :last_changed_by => u) + a = create(Article, :profile => c, :name => 'test_article', :created_by => u) login_as :test_user @controller.stubs(:user).returns(u) @@ -1774,6 +1774,31 @@ class CmsControllerTest < ActionController::TestCase assert_equal 'first version', Article.find(article.id).name end + should 'set created_by when creating article' do + login_as(profile.identifier) + + post :new, :type => 'TinyMceArticle', :profile => profile.identifier, :article => { :name => 'changed by me', :body => 'content ...' } + + a = profile.articles.find_by_path('changed-by-me') + assert_not_nil a + assert_equal profile, a.created_by + end + + should 'not change created_by when updating article' do + other_person = create_user('otherperson').person + + a = profile.articles.build(:name => 'my article') + a.created_by = other_person + a.save! + + login_as(profile.identifier) + post :edit, :profile => profile.identifier, :id => a.id, :article => { :body => 'new content for this article' } + + a.reload + + assert_equal other_person, a.created_by + end + protected # FIXME this is to avoid adding an extra dependency for a proper JSON parser. diff --git a/test/functional/content_viewer_controller_test.rb b/test/functional/content_viewer_controller_test.rb index 6aa88e5..03a48aa 100644 --- a/test/functional/content_viewer_controller_test.rb +++ b/test/functional/content_viewer_controller_test.rb @@ -743,7 +743,7 @@ class ContentViewerControllerTest < ActionController::TestCase c = Community.create!(:name => 'test_com') u = create_user_with_permission('test_user', 'publish_content', c) login_as u.identifier - a = create(Article, :profile => c, :name => 'test-article', :last_changed_by => u, :published => false) + a = create(Article, :profile => c, :name => 'test-article', :created_by => u, :published => false) get :view_page, :profile => c.identifier, :page => a.path diff --git a/test/unit/article_test.rb b/test/unit/article_test.rb index 9ef40bc..6d97625 100644 --- a/test/unit/article_test.rb +++ b/test/unit/article_test.rb @@ -154,7 +154,7 @@ class ArticleTest < ActiveSupport::TestCase assert a4.errors[:slug.to_s].present? end - should 'record who did the last change' do + should 'last_changed_by be a person' do a = profile.articles.build(:name => 'test') # must be a person @@ -167,6 +167,19 @@ class ArticleTest < ActiveSupport::TestCase end end + should 'created_by be a person' do + a = profile.articles.build(:name => 'test') + + # must be a person + assert_raise ActiveRecord::AssociationTypeMismatch do + a.created_by = Profile.new + end + assert_nothing_raised do + a.created_by = Person.new + a.save! + end + end + should 'not show private documents as recent' do p = create_user('usr1').person Article.destroy_all @@ -802,7 +815,7 @@ class ArticleTest < ActiveSupport::TestCase should 'allow author to edit if is publisher' do c = fast_create(Community) p = create_user_with_permission('test_user', 'publish_content', c) - a = create(Article, :name => 'a test article', :last_changed_by => p, :profile_id => c.id) + a = create(Article, :name => 'a test article', :created_by => p, :profile_id => c.id) assert a.allow_post_content?(p) end @@ -1380,17 +1393,17 @@ class ArticleTest < ActiveSupport::TestCase should "the author_name returns the name of the article's author" do author = fast_create(Person) - a = create(Article, :name => 'a test article', :last_changed_by => author, :profile_id => profile.id) + a = create(Article, :name => 'a test article', :created_by => author, :profile_id => profile.id) assert_equal author.name, a.author_name author.destroy - a.reload + a = Article.find(a.id) a.author_name = 'some name' assert_equal 'some name', a.author_name end should 'retrieve latest info from topic when has no comments' do forum = fast_create(Forum, :name => 'Forum test', :profile_id => profile.id) - post = fast_create(TextileArticle, :name => 'First post', :profile_id => profile.id, :parent_id => forum.id, :updated_at => Time.now, :last_changed_by_id => profile.id) + post = fast_create(TextileArticle, :name => 'First post', :profile_id => profile.id, :parent_id => forum.id, :updated_at => Time.now, :last_changed_by_id => profile.id, :created_by_id => profile.id) assert_equal post.updated_at, post.info_from_last_update[:date] assert_equal profile.name, post.info_from_last_update[:author_name] assert_equal profile.url, post.info_from_last_update[:author_url] @@ -1744,30 +1757,30 @@ class ArticleTest < ActiveSupport::TestCase should 'set author_name before creating article if there is an author' do author = fast_create(Person) - article = create(Article, :name => 'Test', :profile => profile, :last_changed_by => author) + article = create(Article, :name => 'Test', :profile => profile, :created_by => author) assert_equal author.name, article.author_name author_name = author.name author.destroy - article.reload + article = Article.find(article.id) assert_equal author_name, article.author_name end should "author_id return the author id of the article's author" do author = fast_create(Person) - article = create(Article, :name => 'Test', :profile => profile, :last_changed_by => author) + article = create(Article, :name => 'Test', :profile => profile, :created_by => author) assert_equal author.id, article.author_id end should "author_id return nil if there is no article's author" do - article = create(Article, :name => 'Test', :profile => profile, :last_changed_by => nil) + article = create(Article, :name => 'Test', :profile => profile, :created_by => nil) assert_nil article.author_id end should "return the author of a specific version" do author1 = fast_create(Person) author2 = fast_create(Person) - article = create(Article, :name => 'first version', :profile => profile, :last_changed_by => author1) + article = create(Article, :name => 'first version', :profile => profile, :created_by => author1, :last_changed_by => author1) article.name = 'second version' article.last_changed_by = author2 article.save @@ -1778,7 +1791,7 @@ class ArticleTest < ActiveSupport::TestCase should "return the author_name of a specific version" do author1 = fast_create(Person) author2 = fast_create(Person) - article = create(Article, :name => 'first version', :profile => profile, :last_changed_by => author1) + article = create(Article, :name => 'first version', :profile => profile, :created_by => author1) article.name = 'second version' article.last_changed_by = author2 article.save -- libgit2 0.21.2