Commit 4bbfef7e42eff51025675d1e6f70aee7c26527ff
1 parent
457c9f59
Exists in
master
and in
27 other branches
article: author refactoring
I'm adding the author as a record relation instead of calculating it based on the last_changed_by. This change has a reasonable performance improvement on pages that access this information, like blog page. Still saving author_name in order to deal with the problem of the author being removed. Including new method to facilitate specific article version retrieval. ActionItem3201
Showing
8 changed files
with
82 additions
and
38 deletions
Show diff stats
app/controllers/my_profile/cms_controller.rb
app/controllers/public/content_viewer_controller.rb
| ... | ... | @@ -83,7 +83,7 @@ class ContentViewerController < ApplicationController |
| 83 | 83 | @page.posts |
| 84 | 84 | end |
| 85 | 85 | |
| 86 | - posts = posts.includes(:parent, {:profile => [:domains, :environment]}) | |
| 86 | + posts = posts.includes(:parent, {:profile => [:domains, :environment]}, :author) | |
| 87 | 87 | |
| 88 | 88 | #FIXME Need to run this before the pagination because this version of |
| 89 | 89 | # will_paginate returns a will_paginate collection instead of a | ... | ... |
app/helpers/content_viewer_helper.rb
| ... | ... | @@ -26,7 +26,7 @@ module ContentViewerHelper |
| 26 | 26 | end |
| 27 | 27 | title << content_tag('span', |
| 28 | 28 | content_tag('span', show_date(article.published_at), :class => 'date') + |
| 29 | - content_tag('span', [_(", by %s") % link_to(article.author_name, article.author_url)], :class => 'author') + | |
| 29 | + content_tag('span', [_(", by %s") % (article.author ? link_to(article.author_name, article.author_url) : article.author_name)], :class => 'author') + | |
| 30 | 30 | content_tag('span', comments, :class => 'comments'), |
| 31 | 31 | :class => 'created-at' |
| 32 | 32 | ) | ... | ... |
app/models/article.rb
| ... | ... | @@ -39,8 +39,8 @@ class Article < ActiveRecord::Base |
| 39 | 39 | before_save :sanitize_tag_list |
| 40 | 40 | |
| 41 | 41 | before_create do |article| |
| 42 | - if article.last_changed_by_id | |
| 43 | - article.author_name = Person.find(article.last_changed_by_id).name | |
| 42 | + if article.author | |
| 43 | + article.author_name = article.author.name | |
| 44 | 44 | end |
| 45 | 45 | end |
| 46 | 46 | |
| ... | ... | @@ -52,6 +52,7 @@ class Article < ActiveRecord::Base |
| 52 | 52 | |
| 53 | 53 | 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? } |
| 54 | 54 | |
| 55 | + belongs_to :author, :class_name => 'Person' | |
| 55 | 56 | belongs_to :last_changed_by, :class_name => 'Person', :foreign_key => 'last_changed_by_id' |
| 56 | 57 | |
| 57 | 58 | has_many :comments, :class_name => 'Comment', :foreign_key => 'source_id', :dependent => :destroy, :order => 'created_at asc' |
| ... | ... | @@ -449,7 +450,7 @@ class Article < ActiveRecord::Base |
| 449 | 450 | ['TextArticle', 'TextileArticle', 'TinyMceArticle'] |
| 450 | 451 | end |
| 451 | 452 | |
| 452 | - named_scope :published, :conditions => { :published => true } | |
| 453 | + named_scope :published, :conditions => ['articles.published = ?', true] | |
| 453 | 454 | named_scope :folders, lambda {|profile|{:conditions => ['articles.type IN (?)', profile.folder_types] }} |
| 454 | 455 | named_scope :no_folders, lambda {|profile|{:conditions => ['articles.type NOT IN (?)', profile.folder_types]}} |
| 455 | 456 | named_scope :galleries, :conditions => [ "articles.type IN ('Gallery')" ] |
| ... | ... | @@ -462,7 +463,7 @@ class Article < ActiveRecord::Base |
| 462 | 463 | named_scope :more_recent, :order => "created_at DESC" |
| 463 | 464 | |
| 464 | 465 | def self.display_filter(user, profile) |
| 465 | - return {:conditions => ['published = ?', true]} if !user | |
| 466 | + return {:conditions => ['articles.published = ?', true]} if !user | |
| 466 | 467 | {:conditions => [" articles.published = ? OR |
| 467 | 468 | articles.last_changed_by_id = ? OR |
| 468 | 469 | articles.profile_id = ? OR |
| ... | ... | @@ -621,39 +622,36 @@ class Article < ActiveRecord::Base |
| 621 | 622 | can_display_versions? && display_versions |
| 622 | 623 | end |
| 623 | 624 | |
| 624 | - def author(version_number = nil) | |
| 625 | - if version_number | |
| 626 | - version = versions.find_by_version(version_number) | |
| 627 | - author_id = version.last_changed_by_id if version | |
| 628 | - Person.exists?(author_id) ? Person.find(author_id) : nil | |
| 629 | - else | |
| 630 | - if versions.empty? | |
| 631 | - last_changed_by | |
| 632 | - else | |
| 633 | - author_id = versions.first.last_changed_by_id | |
| 634 | - Person.exists?(author_id) ? Person.find(author_id) : nil | |
| 635 | - end | |
| 636 | - end | |
| 625 | + def get_version(version_number = nil) | |
| 626 | + version_number ? versions.find(:first, :order => 'version', :offset => version_number - 1) : versions.earliest | |
| 627 | + end | |
| 628 | + | |
| 629 | + def author_by_version(version_number = nil) | |
| 630 | + version_number ? profile.environment.people.find_by_id(get_version(version_number).last_changed_by_id) : author | |
| 637 | 631 | end |
| 638 | 632 | |
| 639 | 633 | def author_name(version_number = nil) |
| 640 | - person = author(version_number) | |
| 641 | - person ? person.name : (setting[:author_name] || _('Unknown')) | |
| 634 | + person = author_by_version(version_number) | |
| 635 | + if version_number | |
| 636 | + person ? person.name : _('Unknown') | |
| 637 | + else | |
| 638 | + person ? person.name : (setting[:author_name] || _('Unknown')) | |
| 639 | + end | |
| 642 | 640 | end |
| 643 | 641 | |
| 644 | 642 | def author_url(version_number = nil) |
| 645 | - person = author(version_number) | |
| 643 | + person = author_by_version(version_number) | |
| 646 | 644 | person ? person.url : nil |
| 647 | 645 | end |
| 648 | 646 | |
| 649 | 647 | def author_id(version_number = nil) |
| 650 | - person = author(version_number) | |
| 648 | + person = author_by_version(version_number) | |
| 651 | 649 | person ? person.id : nil |
| 652 | 650 | end |
| 653 | 651 | |
| 654 | 652 | def version_license(version_number = nil) |
| 655 | 653 | return license if version_number.nil? |
| 656 | - profile.environment.licenses.find_by_id(versions.find_by_version(version_number).license_id) | |
| 654 | + profile.environment.licenses.find_by_id(get_version(version_number).license_id) | |
| 657 | 655 | end |
| 658 | 656 | |
| 659 | 657 | alias :active_record_cache_key :cache_key | ... | ... |
db/migrate/20140709224246_create_real_relation_between_article_and_author.rb
0 → 100644
| ... | ... | @@ -0,0 +1,14 @@ |
| 1 | +class CreateRealRelationBetweenArticleAndAuthor < ActiveRecord::Migration | |
| 2 | + def self.up | |
| 3 | + add_column :articles, :author_id, :integer | |
| 4 | + add_column :article_versions, :author_id, :integer | |
| 5 | + | |
| 6 | + # Set article's author as the first version's last_changed_by_id. | |
| 7 | + execute "update articles set author_id = (select article_versions.last_changed_by_id from article_versions where article_versions.article_id = articles.id and article_versions.version = 1 limit 1)" | |
| 8 | + end | |
| 9 | + | |
| 10 | + def self.down | |
| 11 | + remove_column :articles, :author_id | |
| 12 | + remove_column :article_versions, :author_id | |
| 13 | + end | |
| 14 | +end | ... | ... |
db/schema.rb
| ... | ... | @@ -9,7 +9,7 @@ |
| 9 | 9 | # |
| 10 | 10 | # It's strongly recommended to check this file into your version control system. |
| 11 | 11 | |
| 12 | -ActiveRecord::Schema.define(:version => 20140709212646) do | |
| 12 | +ActiveRecord::Schema.define(:version => 20140709224246) do | |
| 13 | 13 | |
| 14 | 14 | create_table "abuse_reports", :force => true do |t| |
| 15 | 15 | t.integer "reporter_id" |
| ... | ... | @@ -94,6 +94,7 @@ ActiveRecord::Schema.define(:version => 20140709212646) do |
| 94 | 94 | t.integer "image_id" |
| 95 | 95 | t.integer "position" |
| 96 | 96 | t.integer "spam_comments_count", :default => 0 |
| 97 | + t.integer "author_id" | |
| 97 | 98 | end |
| 98 | 99 | |
| 99 | 100 | add_index "article_versions", ["article_id"], :name => "index_article_versions_on_article_id" |
| ... | ... | @@ -140,6 +141,7 @@ ActiveRecord::Schema.define(:version => 20140709212646) do |
| 140 | 141 | t.integer "image_id" |
| 141 | 142 | t.integer "position" |
| 142 | 143 | t.integer "spam_comments_count", :default => 0 |
| 144 | + t.integer "author_id" | |
| 143 | 145 | end |
| 144 | 146 | |
| 145 | 147 | add_index "articles", ["comments_count"], :name => "index_articles_on_comments_count" | ... | ... |
test/functional/content_viewer_controller_test.rb
| ... | ... | @@ -767,7 +767,7 @@ class ContentViewerControllerTest < ActionController::TestCase |
| 767 | 767 | c = Community.create!(:name => 'test_com') |
| 768 | 768 | u = create_user_with_permission('test_user', 'publish_content', c) |
| 769 | 769 | login_as u.identifier |
| 770 | - a = c.articles.create!(:name => 'test-article', :last_changed_by => u, :published => false) | |
| 770 | + a = c.articles.create!(:name => 'test-article', :author => u, :published => false) | |
| 771 | 771 | |
| 772 | 772 | get :view_page, :profile => c.identifier, :page => a.explode_path |
| 773 | 773 | |
| ... | ... | @@ -779,7 +779,7 @@ class ContentViewerControllerTest < ActionController::TestCase |
| 779 | 779 | c = Community.create!(:name => 'test_com') |
| 780 | 780 | u = create_user_with_permission('test_user', 'publish_content', c) |
| 781 | 781 | login_as u.identifier |
| 782 | - a = c.articles.create!(:name => 'test-article', :last_changed_by => profile, :published => true) | |
| 782 | + a = c.articles.create!(:name => 'test-article', :author => profile, :published => true) | |
| 783 | 783 | |
| 784 | 784 | xhr :get, :view_page, :profile => c.identifier, :page => a.explode_path, :toolbar => true |
| 785 | 785 | |
| ... | ... | @@ -926,7 +926,7 @@ class ContentViewerControllerTest < ActionController::TestCase |
| 926 | 926 | community.add_member(author) |
| 927 | 927 | |
| 928 | 928 | forum = Forum.create(:profile => community, :name => 'Forum test', :body => 'Forum test') |
| 929 | - post = fast_create(TextileArticle, :name => 'First post', :profile_id => community.id, :parent_id => forum.id, :last_changed_by_id => author.id) | |
| 929 | + post = fast_create(TextileArticle, :name => 'First post', :profile_id => community.id, :parent_id => forum.id, :author_id => author.id) | |
| 930 | 930 | |
| 931 | 931 | login_as(author.identifier) |
| 932 | 932 | get :view_page, :profile => community.identifier, :page => post.path.split('/') |
| ... | ... | @@ -942,7 +942,7 @@ class ContentViewerControllerTest < ActionController::TestCase |
| 942 | 942 | community.add_member(author) |
| 943 | 943 | |
| 944 | 944 | forum = Forum.create(:profile => community, :name => 'Forum test', :body => 'Forum test') |
| 945 | - post = fast_create(TextileArticle, :name => 'First post', :profile_id => community.id, :parent_id => forum.id, :last_changed_by_id => author.id) | |
| 945 | + post = fast_create(TextileArticle, :name => 'First post', :profile_id => community.id, :parent_id => forum.id, :author_id => author.id) | |
| 946 | 946 | |
| 947 | 947 | login_as(author.identifier) |
| 948 | 948 | get :view_page, :profile => community.identifier, :page => post.path.split('/') | ... | ... |
test/unit/article_test.rb
| ... | ... | @@ -802,7 +802,7 @@ class ArticleTest < ActiveSupport::TestCase |
| 802 | 802 | should 'allow author to edit if is publisher' do |
| 803 | 803 | c = fast_create(Community) |
| 804 | 804 | p = create_user_with_permission('test_user', 'publish_content', c) |
| 805 | - a = c.articles.create!(:name => 'a test article', :last_changed_by => p) | |
| 805 | + a = c.articles.create!(:name => 'a test article', :author => p) | |
| 806 | 806 | |
| 807 | 807 | assert a.allow_post_content?(p) |
| 808 | 808 | end |
| ... | ... | @@ -1378,7 +1378,7 @@ class ArticleTest < ActiveSupport::TestCase |
| 1378 | 1378 | |
| 1379 | 1379 | should "the author_name returns the name of the article's author" do |
| 1380 | 1380 | author = fast_create(Person) |
| 1381 | - a = profile.articles.create!(:name => 'a test article', :last_changed_by => author) | |
| 1381 | + a = profile.articles.create!(:name => 'a test article', :author => author) | |
| 1382 | 1382 | assert_equal author.name, a.author_name |
| 1383 | 1383 | author.destroy |
| 1384 | 1384 | a.reload |
| ... | ... | @@ -1388,7 +1388,7 @@ class ArticleTest < ActiveSupport::TestCase |
| 1388 | 1388 | |
| 1389 | 1389 | should 'retrieve latest info from topic when has no comments' do |
| 1390 | 1390 | forum = fast_create(Forum, :name => 'Forum test', :profile_id => profile.id) |
| 1391 | - 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) | |
| 1391 | + post = fast_create(TextileArticle, :name => 'First post', :profile_id => profile.id, :parent_id => forum.id, :updated_at => Time.now, :author_id => profile.id) | |
| 1392 | 1392 | assert_equal post.updated_at, post.info_from_last_update[:date] |
| 1393 | 1393 | assert_equal profile.name, post.info_from_last_update[:author_name] |
| 1394 | 1394 | assert_equal profile.url, post.info_from_last_update[:author_url] |
| ... | ... | @@ -1668,7 +1668,7 @@ class ArticleTest < ActiveSupport::TestCase |
| 1668 | 1668 | author = fast_create(Person) |
| 1669 | 1669 | community.add_member(author) |
| 1670 | 1670 | forum = Forum.create(:profile => community, :name => 'Forum test', :body => 'Forum test') |
| 1671 | - post = fast_create(TextileArticle, :name => 'First post', :profile_id => community.id, :parent_id => forum.id, :last_changed_by_id => author.id) | |
| 1671 | + post = fast_create(TextileArticle, :name => 'First post', :profile_id => community.id, :parent_id => forum.id, :author_id => author.id) | |
| 1672 | 1672 | |
| 1673 | 1673 | assert post.allow_edit?(author) |
| 1674 | 1674 | end |
| ... | ... | @@ -1753,7 +1753,7 @@ class ArticleTest < ActiveSupport::TestCase |
| 1753 | 1753 | |
| 1754 | 1754 | should 'set author_name before creating article if there is an author' do |
| 1755 | 1755 | author = fast_create(Person) |
| 1756 | - article = Article.create!(:name => 'Test', :profile => profile, :last_changed_by => author) | |
| 1756 | + article = Article.create!(:name => 'Test', :profile => profile, :author => author) | |
| 1757 | 1757 | assert_equal author.name, article.author_name |
| 1758 | 1758 | |
| 1759 | 1759 | author_name = author.name |
| ... | ... | @@ -1764,12 +1764,12 @@ class ArticleTest < ActiveSupport::TestCase |
| 1764 | 1764 | |
| 1765 | 1765 | should "author_id return the author id of the article's author" do |
| 1766 | 1766 | author = fast_create(Person) |
| 1767 | - article = Article.create!(:name => 'Test', :profile => profile, :last_changed_by => author) | |
| 1767 | + article = Article.create!(:name => 'Test', :profile => profile, :author => author) | |
| 1768 | 1768 | assert_equal author.id, article.author_id |
| 1769 | 1769 | end |
| 1770 | 1770 | |
| 1771 | 1771 | should "author_id return nil if there is no article's author" do |
| 1772 | - article = Article.create!(:name => 'Test', :profile => profile, :last_changed_by => nil) | |
| 1772 | + article = Article.create!(:name => 'Test', :profile => profile, :author => nil) | |
| 1773 | 1773 | assert_nil article.author_id |
| 1774 | 1774 | end |
| 1775 | 1775 | |
| ... | ... | @@ -1780,8 +1780,8 @@ class ArticleTest < ActiveSupport::TestCase |
| 1780 | 1780 | article.name = 'second version' |
| 1781 | 1781 | article.last_changed_by = author2 |
| 1782 | 1782 | article.save |
| 1783 | - assert_equal author1, article.author(1) | |
| 1784 | - assert_equal author2, article.author(2) | |
| 1783 | + assert_equal author1, article.author_by_version(1) | |
| 1784 | + assert_equal author2, article.author_by_version(2) | |
| 1785 | 1785 | end |
| 1786 | 1786 | |
| 1787 | 1787 | should "return the author_name of a specific version" do |
| ... | ... | @@ -1837,4 +1837,33 @@ class ArticleTest < ActiveSupport::TestCase |
| 1837 | 1837 | assert_equivalent [c3], Article.with_types(['Event']) |
| 1838 | 1838 | end |
| 1839 | 1839 | |
| 1840 | + should 'get specific version' do | |
| 1841 | + article = Article.create!(:name => 'first version', :profile => profile) | |
| 1842 | + article.name = 'second version' | |
| 1843 | + article.save! | |
| 1844 | + article.name = 'third version' | |
| 1845 | + article.save! | |
| 1846 | + | |
| 1847 | + assert_equal 'first version', article.get_version(1).name | |
| 1848 | + assert_equal 'second version', article.get_version(2).name | |
| 1849 | + assert_equal 'third version', article.get_version(3).name | |
| 1850 | + end | |
| 1851 | + | |
| 1852 | + should 'get author by version' do | |
| 1853 | + p1 = fast_create(Person) | |
| 1854 | + p2 = fast_create(Person) | |
| 1855 | + p3 = fast_create(Person) | |
| 1856 | + article = Article.create!(:name => 'first version', :profile => profile, :last_changed_by => p1) | |
| 1857 | + article.name = 'second version' | |
| 1858 | + article.last_changed_by = p2 | |
| 1859 | + article.save! | |
| 1860 | + article.last_changed_by = p3 | |
| 1861 | + article.name = 'third version' | |
| 1862 | + article.save! | |
| 1863 | + | |
| 1864 | + assert_equal p1, article.author_by_version(1) | |
| 1865 | + assert_equal p2, article.author_by_version(2) | |
| 1866 | + assert_equal p3, article.author_by_version(3) | |
| 1867 | + end | |
| 1868 | + | |
| 1840 | 1869 | end | ... | ... |