diff --git a/app/models/image.rb b/app/models/image.rb index 57ba0eb..2fa4420 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -1,5 +1,4 @@ class Image < ActiveRecord::Base - belongs_to :owner, :polymorphic => true def self.max_size Image.attachment_options[:max_size] diff --git a/app/models/profile_list_block.rb b/app/models/profile_list_block.rb index 52ff392..9ad6ee0 100644 --- a/app/models/profile_list_block.rb +++ b/app/models/profile_list_block.rb @@ -1,7 +1,7 @@ class ProfileListBlock < Block settings_items :limit, :type => :integer, :default => 6 - settings_items :prioritize_profiles_with_image, :type => :boolean, :default => false + settings_items :prioritize_profiles_with_image, :type => :boolean, :default => true def self.description _('Random profiles') @@ -13,21 +13,21 @@ class ProfileListBlock < Block end def profile_list - profiles.visible.all(:limit => limit, :select => 'DISTINCT profiles.*, ' + image_prioritizer + randomizer, :joins => "LEFT OUTER JOIN images ON images.owner_id = profiles.id", :order => image_prioritizer + randomizer) + result = nil + if !prioritize_profiles_with_image + result = profiles.visible.all(:limit => limit, :order => 'updated_at DESC').sort_by{ rand } + elsif profiles.visible.with_image.count >= limit + result = profiles.visible.with_image.all(:limit => limit * 5, :order => 'updated_at DESC').sort_by{ rand } + else + result = profiles.visible.with_image.sort_by{ rand } + profiles.visible.without_image.all(:limit => limit * 5, :order => 'updated_at DESC').sort_by{ rand } + end + result.slice(0..limit-1) end def profile_count profiles.visible.count('DISTINCT(profiles.id)') end - def randomizer - @randomizer ||= "(profiles.id % #{rand(profile_count) + 1})" - end - - def image_prioritizer - prioritize_profiles_with_image ? '(images.id is null),' : '' - end - # the title of the block. Probably will be overriden in subclasses. def default_title _('{#} People or Groups') diff --git a/db/migrate/20110526201202_move_reference_from_image_to_owners.rb b/db/migrate/20110526201202_move_reference_from_image_to_owners.rb new file mode 100644 index 0000000..8630c6d --- /dev/null +++ b/db/migrate/20110526201202_move_reference_from_image_to_owners.rb @@ -0,0 +1,21 @@ +class MoveReferenceFromImageToOwners < ActiveRecord::Migration + def self.up + %w[ profiles categories products tasks ].each do |table| + type = table.singularize.camelcase + add_column table, :image_id, :integer + update("update #{table} set image_id = (select i.id from images i where i.owner_id = #{table}.id and i.owner_type = '#{type}' limit 1) where id in (select owner_id from images where owner_type = '#{type}' and owner_id is not null)") + end + remove_column :images, :owner_id + remove_column :images, :owner_type + end + + def self.down + add_column :images, :owner_id, :integer + add_column :images, :owner_type, :string + %w[ profiles products categories tasks ].each do |table| + type = table.singularize.camelcase + update("update images set owner_id = (select id from #{table} origin where origin.image_id = images.id), owner_type = '#{type}' where id in (select image_id from #{table} where image_id is not null)") + remove_column table, :image_id + end + end +end diff --git a/db/migrate/20110527042608_remove_unused_column_from_products.rb b/db/migrate/20110527042608_remove_unused_column_from_products.rb new file mode 100644 index 0000000..0404676 --- /dev/null +++ b/db/migrate/20110527042608_remove_unused_column_from_products.rb @@ -0,0 +1,9 @@ +class RemoveUnusedColumnFromProducts < ActiveRecord::Migration + def self.up + remove_column :products, :image + end + + def self.down + add_column :products, :image, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 51c78fc..0deb708 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -9,7 +9,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20110524151137) do +ActiveRecord::Schema.define(:version => 20110527042608) do create_table "action_tracker", :force => true do |t| t.integer "user_id" @@ -165,6 +165,7 @@ ActiveRecord::Schema.define(:version => 20110524151137) do t.boolean "display_in_menu", :default => false t.integer "children_count", :default => 0 t.boolean "accept_products", :default => true + t.integer "image_id" end create_table "categories_profiles", :id => false, :force => true do |t| @@ -270,8 +271,6 @@ ActiveRecord::Schema.define(:version => 20110524151137) do end create_table "images", :force => true do |t| - t.string "owner_type" - t.integer "owner_id" t.integer "parent_id" t.string "content_type" t.string "filename" @@ -339,7 +338,6 @@ ActiveRecord::Schema.define(:version => 20110524151137) do t.string "name" t.decimal "price" t.text "description" - t.string "image" t.datetime "created_at" t.datetime "updated_at" t.float "lat" @@ -348,6 +346,7 @@ ActiveRecord::Schema.define(:version => 20110524151137) do t.boolean "available", :default => true t.boolean "highlighted" t.integer "unit_id" + t.integer "image_id" end add_index "products", ["enterprise_id"], :name => "index_products_on_enterprise_id" @@ -378,6 +377,7 @@ ActiveRecord::Schema.define(:version => 20110524151137) do t.integer "preferred_domain_id" t.datetime "updated_at" t.boolean "visible", :default => true + t.integer "image_id" end add_index "profiles", ["environment_id"], :name => "index_profiles_on_environment_id" @@ -456,6 +456,7 @@ ActiveRecord::Schema.define(:version => 20110524151137) do t.string "type" t.datetime "created_at" t.string "target_type" + t.integer "image_id" end create_table "thumbnails", :force => true do |t| diff --git a/features/step_definitions/noosfero_steps.rb b/features/step_definitions/noosfero_steps.rb index 692e4df..1591c73 100644 --- a/features/step_definitions/noosfero_steps.rb +++ b/features/step_definitions/noosfero_steps.rb @@ -116,8 +116,8 @@ Given /^the following products?$/ do |table| data = item.dup owner = Enterprise[data.delete("owner")] category = Category.find_by_slug(data.delete("category").to_slug) - product = Product.create!(data.merge(:enterprise => owner, :product_category => category)) - image = Image.create!(:owner => product, :uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + product = Product.create!(data.merge(:enterprise => owner, :product_category => category, :image_id => img.id)) end end diff --git a/lib/acts_as_having_image.rb b/lib/acts_as_having_image.rb index 99580df..0ef9fff 100644 --- a/lib/acts_as_having_image.rb +++ b/lib/acts_as_having_image.rb @@ -2,7 +2,9 @@ module ActsAsHavingImage module ClassMethods def acts_as_having_image - has_one :image, :as => 'owner' + belongs_to :image + named_scope :with_image, :conditions => [ "#{table_name}.image_id IS NOT NULL" ] + named_scope :without_image, :conditions => [ "#{table_name}.image_id IS NULL" ] self.send(:include, ActsAsHavingImage) end end diff --git a/test/factories.rb b/test/factories.rb index d1b10c2..0df1bf7 100644 --- a/test/factories.rb +++ b/test/factories.rb @@ -311,6 +311,7 @@ module Noosfero::Factory end alias :defaults_for_blog_archives_block :defaults_for_block + alias :defaults_for_profile_list_block :defaults_for_block ############################################### # Task diff --git a/test/unit/image_test.rb b/test/unit/image_test.rb index bc4175a..17cee4e 100644 --- a/test/unit/image_test.rb +++ b/test/unit/image_test.rb @@ -22,7 +22,8 @@ class ImageTest < Test::Unit::TestCase end should 'create thumbnails after processing jobs' do - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile) + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + profile.update_attribute(:image_id, file.id) process_delayed_job_queue Image.attachment_options[:thumbnails].each do |suffix, size| @@ -32,7 +33,8 @@ class ImageTest < Test::Unit::TestCase end should 'set thumbnails_processed to true after creating thumbnails' do - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile) + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + profile.update_attribute(:image_id, file.id) process_delayed_job_queue @@ -62,7 +64,8 @@ class ImageTest < Test::Unit::TestCase end should 'return image thumbnail if thumbnails were processed' do - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile) + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + profile.update_attribute(:image_id, file.id) process_delayed_job_queue assert_match(/rails_thumb.png/, Image.find(file.id).public_filename(:thumb)) @@ -71,7 +74,8 @@ class ImageTest < Test::Unit::TestCase end should 'store width and height after processing' do - file = Image.create!(:owner => profile, :uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + profile.update_attribute(:image_id, file.id) file.create_thumbnails file = Image.find(file.id) @@ -89,7 +93,7 @@ class ImageTest < Test::Unit::TestCase # this test verifies whether it created background jobs also for the # thumbnails! assert_no_difference Delayed::Job, :count do - image = Image.new(:owner => profile, :uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + image = Image.new(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) image.stubs(:is_thumbnail?).returns(true) image.save! end @@ -97,7 +101,8 @@ class ImageTest < Test::Unit::TestCase should 'upload to a folder with same name as the schema if database is postgresql' do uses_postgresql - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile) + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + profile.update_attribute(:image_id, file.id) process_delayed_job_queue assert_match(/images\/test_schema\/\d{4}\/\d{4}\/rails.png/, Image.find(file.id).public_filename) file.destroy @@ -106,7 +111,8 @@ class ImageTest < Test::Unit::TestCase should 'upload to path prefix folder if database is not postgresql' do uses_sqlite - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile) + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + profile.update_attribute(:image_id, file.id) process_delayed_job_queue assert_match(/images\/\d{4}\/\d{4}\/rails.png/, Image.find(file.id).public_filename) file.destroy diff --git a/test/unit/profile_list_block_test.rb b/test/unit/profile_list_block_test.rb index 9e1f8b9..9e4b2cc 100644 --- a/test/unit/profile_list_block_test.rb +++ b/test/unit/profile_list_block_test.rb @@ -137,50 +137,80 @@ class ProfileListBlockTest < Test::Unit::TestCase should 'list random profiles' do env = fast_create(Environment) - p1 = fast_create(Person, :environment_id => env.id) - p2 = fast_create(Person, :environment_id => env.id) - p3 = fast_create(Person, :environment_id => env.id) + 6.times.each do + fast_create(Person, :environment_id => env.id) + end block = ProfileListBlock.new block.stubs(:owner).returns(env) - # force the "random" function to return something we know - block.stubs(:randomizer).returns('-profiles.id') - - assert_equal [p3.id, p2.id, p1.id], block.profile_list.map(&:id) + assert_not_equal block.profile_list.map(&:id), block.profile_list.map(&:id) end - should 'randomize using modulo operator and random number' do - block = ProfileListBlock.new - block.expects(:profile_count).returns(10) - block.expects(:rand).with(10).returns(5) - assert_match /profiles.id % 6/, block.randomizer + should 'prioritize profiles with image if this option is turned on' do + env = fast_create(Environment) + img1 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + p1 = fast_create(Person, :environment_id => env.id, :image_id => img1.id) + img2 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + p2 = fast_create(Person, :environment_id => env.id, :image_id => img2.id) + + p_without_image = fast_create(Person, :environment_id => env.id) + + block = ProfileListBlock.new(:limit => 2) + block.stubs(:owner).returns(env) + block.stubs(:prioritize_profiles_with_image).returns(true) + + assert_not_includes block.profile_list[0..1], p_without_image end - should 'not divide by zero' do + should 'list profiles without image only if profiles with image arent enought' do + env = fast_create(Environment) + img1 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + p1 = fast_create(Person, :environment_id => env.id, :image_id => img1.id) + img2 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + p2 = fast_create(Person, :environment_id => env.id, :image_id => img2.id) + p_without_image = fast_create(Person, :environment_id => env.id) block = ProfileListBlock.new - block.stubs(:profile_count).returns(0) - block.expects(:rand).returns(0) - assert_no_match /profiles.id % 0/, block.randomizer + block.stubs(:owner).returns(env) + block.stubs(:prioritize_profiles_with_image).returns(true) + + block.limit = 2 + assert_not_includes block.profile_list, p_without_image + + block.limit = 3 + assert_includes block.profile_list, p_without_image end - should 'prioritize profiles with image if this option is turned on' do + should 'list profile with image among profiles without image' do env = fast_create(Environment) - p1 = fast_create(Person, :environment_id => env.id) - img1 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => p1) - p2 = fast_create(Person, :environment_id => env.id) - img2 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => p2) + 5.times do |n| + fast_create(Person, :environment_id => env.id) + end + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + with_image = fast_create(Person, :environment_id => env.id, :image_id => img.id) + block = ProfileListBlock.new(:limit => 3) + block.stubs(:prioritize_profiles_with_image).returns(true) + block.stubs(:owner).returns(env) + assert_includes block.profile_list, with_image + end - p_without_image = fast_create(Person, :environment_id => env.id) + should 'not prioritize profiles with image if this option is turned off' do + env = fast_create(Environment) + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + with_image = fast_create(Person, :environment_id => env.id, :updated_at => DateTime.now, :image_id => img.id) + 5.times do |n| + fast_create(Person, :environment_id => env.id, :updated_at => DateTime.now + 1.day) + end - block = ProfileListBlock.new + block = ProfileListBlock.new(:limit => 3) block.stubs(:owner).returns(env) - block.stubs(:prioritize_profiles_with_image).returns(true) + block.stubs(:prioritize_profiles_with_image).returns(false) - # force the "random" function to return something we know - block.stubs(:randomizer).returns('-profiles.id') + assert_not_includes block.profile_list, with_image + end - assert_not_includes block.profile_list[0..1], p_without_image + should 'prioritize profiles with image by default' do + assert ProfileListBlock.new.prioritize_profiles_with_image end end diff --git a/test/unit/profile_test.rb b/test/unit/profile_test.rb index dc0e657..b1af76e 100644 --- a/test/unit/profile_test.rb +++ b/test/unit/profile_test.rb @@ -1357,12 +1357,6 @@ class ProfileTest < Test::Unit::TestCase assert !profile.folders.include?(child) end - should 'validates profile image when save' do - profile = build(Profile, :image_builder => {:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')}) - profile.image.expects(:valid?).returns(false).at_least_once - assert !profile.valid? - end - should 'profile is invalid when image not valid' do profile = build(Profile, :image_builder => {:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')}) profile.image.expects(:valid?).returns(false).at_least_once @@ -1710,6 +1704,28 @@ class ProfileTest < Test::Unit::TestCase assert profile.is_on_homepage?("/#{profile.identifier}/#{homepage.slug}", homepage) end + should 'find profiles with image' do + env = fast_create(Environment) + 2.times do |n| + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + fast_create(Person, :name => "with_image_#{n}", :environment_id => env.id, :image_id => img.id) + end + without_image = fast_create(Person, :name => 'without_image', :environment_id => env.id) + assert_equal 2, env.profiles.with_image.count + assert_not_includes env.profiles.with_image, without_image + end + + should 'find profiles withouth image' do + env = fast_create(Environment) + 2.times do |n| + fast_create(Person, :name => "without_image_#{n}", :environment_id => env.id) + end + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')) + with_image = fast_create(Person, :name => 'with_image', :environment_id => env.id, :image_id => img.id) + assert_equal 2, env.profiles.without_image.count + assert_not_includes env.profiles.without_image, with_image + end + private def assert_invalid_identifier(id) -- libgit2 0.21.2