From d2879e8d3784f2f714a5519fc6f9ca012c54cacf Mon Sep 17 00:00:00 2001 From: Rodrigo Souto Date: Thu, 18 Mar 2010 10:34:38 -0300 Subject: [PATCH] Refactoring the public/private article's logic --- app/controllers/public/content_viewer_controller.rb | 5 ----- app/models/article.rb | 37 +++++++++++++++++++++++++------------ app/models/profile.rb | 4 ++-- db/migrate/084_set_public_article_into_published_attribute.rb | 9 +++++++++ db/migrate/085_remove_public_article.rb | 10 ++++++++++ script/apply-template | 2 +- test/functional/cms_controller_test.rb | 6 +++--- test/functional/content_viewer_controller_test.rb | 36 +++++++++++++----------------------- test/unit/article_test.rb | 65 +++++++++++++++++++---------------------------------------------- test/unit/profile_test.rb | 4 ++-- 10 files changed, 84 insertions(+), 94 deletions(-) create mode 100644 db/migrate/084_set_public_article_into_published_attribute.rb create mode 100644 db/migrate/085_remove_public_article.rb diff --git a/app/controllers/public/content_viewer_controller.rb b/app/controllers/public/content_viewer_controller.rb index 974aa25..3f8c5ce 100644 --- a/app/controllers/public/content_viewer_controller.rb +++ b/app/controllers/public/content_viewer_controller.rb @@ -26,11 +26,6 @@ class ContentViewerController < ApplicationController end end - # only show unpublished articles to those who can edit then - if @page && !@page.published && !@page.allow_post_content?(user) - @page = nil - end - # page not found, give error if @page.nil? render_not_found(@path) diff --git a/app/models/article.rb b/app/models/article.rb index e23ea3d..78ae5dd 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -84,13 +84,6 @@ class Article < ActiveRecord::Base pending_categorizations.clear end - before_save do |article| - if article.parent - article.public_article = article.parent.public_article - end - true - end - acts_as_taggable N_('Tag list') @@ -123,11 +116,10 @@ class Article < ActiveRecord::Base options = { :limit => limit, :conditions => [ "advertise = ? AND - public_article = ? AND published = ? AND profiles.visible = ? AND profiles.public_profile = ? AND - ((articles.type != ? and articles.type != ? and articles.type != ?) OR articles.type is NULL)", true, true, true, true, true, 'UploadedFile', 'RssFeed', 'Blog' + ((articles.type != ? and articles.type != ? and articles.type != ?) OR articles.type is NULL)", true, true, true, true, 'UploadedFile', 'RssFeed', 'Blog' ], :include => 'profile', :order => 'articles.published_at desc, articles.id desc' @@ -221,16 +213,32 @@ class Article < ActiveRecord::Base false end + def published? + if self.published + if self.parent && !self.parent.published? + return false + end + true + else + false + end + end + named_scope :folders, :conditions => { :type => ['Folder', 'Blog'] } + def display_unpublished_article_to?(user) + self.author == user || allow_view_private_content?(user) || user == self.profile || + user.is_admin?(self.profile.environment) || user.is_admin?(self.profile) + end + def display_to?(user) - if self.public_article + if self.published? self.profile.display_info_to?(user) else if user.nil? false else - (user == self.profile) || user.has_permission?('view_private_content', self.profile) + self.display_unpublished_article_to?(user) end end end @@ -243,6 +251,10 @@ class Article < ActiveRecord::Base user && user.has_permission?('publish_content', profile) end + def allow_view_private_content?(user = nil) + user && user.has_permission?('view_private_content', profile) + end + def comments_updated ferret_update end @@ -252,9 +264,10 @@ class Article < ActiveRecord::Base end def public? - profile.visible? && profile.public? && public_article + profile.visible? && profile.public? && published? end + def copy(options) attrs = attributes.reject! { |key, value| article_attr_blacklist.include?(key) } attrs.merge!(options) diff --git a/app/models/profile.rb b/app/models/profile.rb index 461303b..217218a 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -447,7 +447,7 @@ private :generate_url, :url_options # a default private folder if public if self.public? - folder = Folder.new(:name => _("Intranet"), :public_article => false) + folder = Folder.new(:name => _("Intranet"), :published => false) self.articles << folder end end @@ -692,7 +692,7 @@ private :generate_url, :url_options if user.nil? false else - (user == self) || (user.is_admin?(self.environment)) || (user.memberships.include?(self)) + (user == self) || (user.is_admin?(self.environment)) || user.is_admin?(self) || user.memberships.include?(self) end end end diff --git a/db/migrate/084_set_public_article_into_published_attribute.rb b/db/migrate/084_set_public_article_into_published_attribute.rb new file mode 100644 index 0000000..8ede46f --- /dev/null +++ b/db/migrate/084_set_public_article_into_published_attribute.rb @@ -0,0 +1,9 @@ +class SetPublicArticleIntoPublishedAttribute < ActiveRecord::Migration + def self.up + execute('update articles set published=(1!=1) where not public_article') + end + + def self.down + raise "this migration can't be reverted" + end +end diff --git a/db/migrate/085_remove_public_article.rb b/db/migrate/085_remove_public_article.rb new file mode 100644 index 0000000..f9772c7 --- /dev/null +++ b/db/migrate/085_remove_public_article.rb @@ -0,0 +1,10 @@ +class RemovePublicArticle < ActiveRecord::Migration + def self.up + remove_column :articles, :public_article + end + + def self.down + add_column :articles, :public_article, :boolean, :default => true + execute('update articles set public_article = (1>0)') + end +end diff --git a/script/apply-template b/script/apply-template index f3764ba..b1cdab4 100755 --- a/script/apply-template +++ b/script/apply-template @@ -8,7 +8,7 @@ env = Environment.default def move_articles_to_blog(profile) profile.articles.each { |article| - if !article.blog? && !article.is_a?(RssFeed) && article.public_article + if !article.blog? && !article.is_a?(RssFeed) && article.published? puts 'including ' + article.path + ' in the blog' article.parent = profile.blog article.save! diff --git a/test/functional/cms_controller_test.rb b/test/functional/cms_controller_test.rb index a3132f5..b486195 100644 --- a/test/functional/cms_controller_test.rb +++ b/test/functional/cms_controller_test.rb @@ -624,14 +624,14 @@ class CmsControllerTest < Test::Unit::TestCase end should 'create a private article child of private folder' do - folder = Folder.new(:name => 'my intranet', :public_article => false); profile.articles << folder; folder.save! + folder = Folder.new(:name => 'my intranet', :published => false); profile.articles << folder; folder.save! post :new, :profile => profile.identifier, :type => 'TextileArticle', :parent_id => folder.id, :article => { :name => 'new-private-article'} folder.reload - assert !assigns(:article).public? + assert !assigns(:article).published? assert_equal 'new-private-article', folder.children[0].name - assert !folder.children[0].public? + assert !folder.children[0].published? end should 'load communities for that the user belongs' do diff --git a/test/functional/content_viewer_controller_test.rb b/test/functional/content_viewer_controller_test.rb index c815dc6..bf82ee9 100644 --- a/test/functional/content_viewer_controller_test.rb +++ b/test/functional/content_viewer_controller_test.rb @@ -293,10 +293,10 @@ class ContentViewerControllerTest < Test::Unit::TestCase assert_response 404 end - should 'show unpublished articles as unexisting' do + should 'show access denied to unpublished articles' do profile.articles.create!(:name => 'test', :published => false) get :view_page, :profile => profile.identifier, :page => [ 'test' ] - assert_response 404 + assert_response 403 end should 'show unpublished articles to the user himself' do @@ -307,19 +307,9 @@ class ContentViewerControllerTest < Test::Unit::TestCase assert_response :success end - should 'show unpublished articles to members' do - community = Community.create!(:name => 'testcomm') - community.articles.create!(:name => 'test', :published => false) - community.add_member(profile) - - login_as(profile.identifier) - get :view_page, :profile => community.identifier, :page => [ 'test' ] - assert_response :success - end - should 'not show private content to members' do community = Community.create!(:name => 'testcomm') - Folder.create!(:name => 'test', :profile => community, :public_article => false) + Folder.create!(:name => 'test', :profile => community, :published => false) community.add_member(profile) login_as(profile.identifier) @@ -332,7 +322,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'show private content to profile moderators' do community = Community.create!(:name => 'testcomm') - community.articles.create!(:name => 'test', :public_article => false) + community.articles.create!(:name => 'test', :published => false) community.add_moderator(profile) login_as(profile.identifier) @@ -344,7 +334,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'show private content to profile admins' do community = Community.create!(:name => 'testcomm') - community.articles.create!(:name => 'test', :public_article => false) + community.articles.create!(:name => 'test', :published => false) community.add_admin(profile) login_as(profile.identifier) @@ -430,7 +420,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'not give access to private articles if logged off' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) @request.stubs(:ssl?).returns(true) get :view_page, :profile => 'test_profile', :page => [ 'my-intranet' ] @@ -441,7 +431,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'not give access to private articles if logged in but not member' do login_as('testinguser') profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) @request.stubs(:ssl?).returns(true) get :view_page, :profile => 'test_profile', :page => [ 'my-intranet' ] @@ -452,7 +442,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'not give access to private articles if logged in and only member' do person = create_user('test_user').person profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) profile.affiliate(person, Profile::Roles.member(profile.environment.id)) login_as('test_user') @@ -465,7 +455,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'give access to private articles if logged in and moderator' do person = create_user('test_user').person profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) profile.affiliate(person, Profile::Roles.moderator(profile.environment.id)) login_as('test_user') @@ -478,7 +468,7 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'give access to private articles if logged in and admin' do person = create_user('test_user').person profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + intranet = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) profile.affiliate(person, Profile::Roles.admin(profile.environment.id)) login_as('test_user') @@ -507,21 +497,21 @@ class ContentViewerControllerTest < Test::Unit::TestCase should 'require SSL for viewing non-public articles' do Environment.default.update_attribute(:enable_ssl, true) - page = profile.articles.create!(:name => 'myarticle', :body => 'top secret', :public_article => false) + page = profile.articles.create!(:name => 'myarticle', :body => 'top secret', :published => false) get :view_page, :profile => 'testinguser', :page => [ 'myarticle' ] assert_redirected_to :protocol => 'https://', :profile => 'testinguser', :page => [ 'myarticle' ] end should 'avoid SSL for viewing public articles' do @request.expects(:ssl?).returns(true).at_least_once - page = profile.articles.create!(:name => 'myarticle', :body => 'top secret', :public_article => true) + page = profile.articles.create!(:name => 'myarticle', :body => 'top secret', :published => true) get :view_page, :profile => 'testinguser', :page => [ 'myarticle' ] assert_redirected_to :protocol => 'http://', :profile => 'testinguser', :page => [ 'myarticle' ] end should 'not redirect to SSL if already on SSL' do @request.expects(:ssl?).returns(true).at_least_once - page = profile.articles.create!(:name => 'myarticle', :body => 'top secret', :public_article => false) + page = profile.articles.create!(:name => 'myarticle', :body => 'top secret', :published => false) login_as('testinguser') get :view_page, :profile => 'testinguser', :page => [ 'myarticle' ] assert_response :success diff --git a/test/unit/article_test.rb b/test/unit/article_test.rb index c0893a0..452c849 100644 --- a/test/unit/article_test.rb +++ b/test/unit/article_test.rb @@ -160,8 +160,8 @@ class ArticleTest < Test::Unit::TestCase p = create_user('usr1').person Article.destroy_all - first = p.articles.build(:name => 'first', :public_article => true); first.save! - second = p.articles.build(:name => 'second', :public_article => false); second.save! + first = p.articles.build(:name => 'first', :published => true); first.save! + second = p.articles.build(:name => 'second', :published => false); second.save! assert_equal [ first ], Article.recent(nil) end @@ -202,8 +202,8 @@ class ArticleTest < Test::Unit::TestCase now = Time.now - first = p.articles.build(:name => 'first', :public_article => true, :created_at => now, :published_at => now); first.save! - second = p.articles.build(:name => 'second', :public_article => true, :updated_at => now, :published_at => now + 1.second); second.save! + first = p.articles.build(:name => 'first', :published => true, :created_at => now, :published_at => now); first.save! + second = p.articles.build(:name => 'second', :published => true, :updated_at => now, :published_at => now + 1.second); second.save! assert_equal [ second, first ], Article.recent(2) @@ -443,21 +443,21 @@ class ArticleTest < Test::Unit::TestCase assert !Article.new.accept_category?(ProductCategory.new) end - should 'accept public_article attribute' do - assert_respond_to Article.new, :public_article - assert_respond_to Article.new, :public_article= + should 'accept published attribute' do + assert_respond_to Article.new, :published + assert_respond_to Article.new, :published= end should 'say that logged off user cannot see private article' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - article = Article.create!(:name => 'test article', :profile => profile, :public_article => false) + article = Article.create!(:name => 'test article', :profile => profile, :published => false) assert !article.display_to?(nil) end should 'say that not member of profile cannot see private article' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - article = Article.create!(:name => 'test article', :profile => profile, :public_article => false) + article = Article.create!(:name => 'test article', :profile => profile, :published => false) person = create_user('test_user').person assert !article.display_to?(person) @@ -465,7 +465,7 @@ class ArticleTest < Test::Unit::TestCase should 'say that member user can not see private article' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - article = Article.create!(:name => 'test article', :profile => profile, :public_article => false) + article = Article.create!(:name => 'test article', :profile => profile, :published => false) person = create_user('test_user').person profile.affiliate(person, Profile::Roles.member(profile.environment.id)) @@ -474,7 +474,7 @@ class ArticleTest < Test::Unit::TestCase should 'say that profile admin can see private article' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - article = Article.create!(:name => 'test article', :profile => profile, :public_article => false) + article = Article.create!(:name => 'test article', :profile => profile, :published => false) person = create_user('test_user').person profile.affiliate(person, Profile::Roles.admin(profile.environment.id)) @@ -483,7 +483,7 @@ class ArticleTest < Test::Unit::TestCase should 'say that profile moderator can see private article' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - article = Article.create!(:name => 'test article', :profile => profile, :public_article => false) + article = Article.create!(:name => 'test article', :profile => profile, :published => false) person = create_user('test_user').person profile.affiliate(person, Profile::Roles.moderator(profile.environment.id)) @@ -492,7 +492,7 @@ class ArticleTest < Test::Unit::TestCase should 'not show article to non member if article public but profile private' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile', :public_profile => false) - article = Article.create!(:name => 'test article', :profile => profile, :public_article => true) + article = Article.create!(:name => 'test article', :profile => profile, :published => true) person1 = create_user('test_user1').person profile.affiliate(person1, Profile::Roles.member(profile.environment.id)) person2 = create_user('test_user2').person @@ -504,54 +504,27 @@ class ArticleTest < Test::Unit::TestCase should 'make new article private if created inside a private folder' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - folder = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + folder = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) article = Article.create!(:name => 'my private article', :profile => profile, :parent => folder) - assert !article.public_article - end - - should 'respond to public? like public_article if profile is public' do - p = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - a1 = Article.create!(:name => 'test public article', :profile => p) - a2 = Article.create!(:name => 'test private article', :profile => p, :public_article => false) - - assert a1.public? - assert !a2.public? - end - - should 'respond to public? as false if profile is private' do - p = Profile.create!(:name => 'test profile', :identifier => 'test_profile', :public_profile => false) - a1 = Article.create!(:name => 'test public article', :profile => p) - a2 = Article.create!(:name => 'test private article', :profile => p, :public_article => false) - - assert !a1.public? - assert !a2.public? - end - - should 'respond to public? as false if profile is invisible' do - profile = fast_create(Profile, :visible => false) - article1 = fast_create(Article, :profile_id => profile.id) - article2 = fast_create(Article, :profile_id => profile.id, :public_article => false) - - assert !article1.public? - assert !article2.public? + assert !article.published? end should 'save as private' do profile = Profile.create!(:name => 'test profile', :identifier => 'test_profile') - folder = Folder.create!(:name => 'my_intranet', :profile => profile, :public_article => false) + folder = Folder.create!(:name => 'my_intranet', :profile => profile, :published => false) article = TextileArticle.new(:name => 'my private article') article.profile = profile article.parent = folder article.save! article.reload - assert !article.public_article + assert !article.published? end should 'not allow friends of private person see the article' do person = create_user('test_user').person - article = Article.create!(:name => 'test article', :profile => person, :public_article => false) + article = Article.create!(:name => 'test article', :profile => person, :published => false) friend = create_user('test_friend').person person.add_friend(friend) person.save! @@ -562,7 +535,7 @@ class ArticleTest < Test::Unit::TestCase should 'display private articles to people who can view private content' do person = create_user('test_user').person - article = Article.create!(:name => 'test article', :profile => person, :public_article => false) + article = Article.create!(:name => 'test article', :profile => person, :published => false) admin_user = create_user('admin_user').person admin_user.stubs(:has_permission?).with('view_private_content', article.profile).returns('true') diff --git a/test/unit/profile_test.rb b/test/unit/profile_test.rb index e4699c7..283a17b 100644 --- a/test/unit/profile_test.rb +++ b/test/unit/profile_test.rb @@ -930,8 +930,8 @@ class ProfileTest < Test::Unit::TestCase p1 = create(Profile) p2 = create(Profile, :public_profile => false) - assert p1.articles.find(:first, :conditions => {:public_article => false}) - assert !p2.articles.find(:first, :conditions => {:public_article => false}) + assert p1.articles.find(:first, :conditions => {:published => false}) + assert !p2.articles.find(:first, :conditions => {:published => false}) end should 'remove member with many roles' do -- libgit2 0.21.2