Download as
Browse Code »

Commit ebbb202a280f1f163a6be2b117492053a6769d7b

Authored by Joenio Costa
Committed by Daniela Feitosa
1 parent 97633c05
Exists in staging and in 26 other branches angular_poc, api-article-archived, api-homepage, api_visitor, article-readonly, caching-rails4, captcha_serpro_plugin, comment_paragraph_api, comments_api_reply, content-manager-hostspot, environment-api, export-comment-paragraph, fix_event_date_issue, fix_multitenancy, fix_profile_wall, fix_string_downcase_and_upcase, fix_upload_files_on_development_environment, json_cookie_serializer, login-captcha, master, new_security, new_video_plugin, pg_search_rank, production, resend_confirmation_email, tasks_keep_filter_params

Using a ruby way to randomize people from ProfileListBlock 

- Added 2 named scopes to select things with and without images
- Priorize profiles with image by defaut in ProfileListBlock
- Migration to remove unused 'image' column from products
- Migration to move image references to owner of image

(ActionItem1971)
Showing 11 changed files with 143 additions and 58 deletions   Show diff stats
app/models/image.rb
  Diff comments   View file @ebbb202
1 1 class Image < ActiveRecord::Base
2   - belongs_to :owner, :polymorphic => true
3 2  
4 3 def self.max_size
5 4 Image.attachment_options[:max_size]
... ...
app/models/profile_list_block.rb
  Diff comments   View file @ebbb202
1 1 class ProfileListBlock < Block
2 2  
3 3 settings_items :limit, :type => :integer, :default => 6
4   - settings_items :prioritize_profiles_with_image, :type => :boolean, :default => false
  4 + settings_items :prioritize_profiles_with_image, :type => :boolean, :default => true
5 5  
6 6 def self.description
7 7 _('Random profiles')
... ... @@ -13,21 +13,21 @@ class ProfileListBlock &lt; Block
13 13 end
14 14  
15 15 def profile_list
16   - 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)
  16 + result = nil
  17 + if !prioritize_profiles_with_image
  18 + result = profiles.visible.all(:limit => limit, :order => 'updated_at DESC').sort_by{ rand }
  19 + elsif profiles.visible.with_image.count >= limit
  20 + result = profiles.visible.with_image.all(:limit => limit * 5, :order => 'updated_at DESC').sort_by{ rand }
  21 + else
  22 + result = profiles.visible.with_image.sort_by{ rand } + profiles.visible.without_image.all(:limit => limit * 5, :order => 'updated_at DESC').sort_by{ rand }
  23 + end
  24 + result.slice(0..limit-1)
17 25 end
18 26  
19 27 def profile_count
20 28 profiles.visible.count('DISTINCT(profiles.id)')
21 29 end
22 30  
23   - def randomizer
24   - @randomizer ||= "(profiles.id % #{rand(profile_count) + 1})"
25   - end
26   -
27   - def image_prioritizer
28   - prioritize_profiles_with_image ? '(images.id is null),' : ''
29   - end
30   -
31 31 # the title of the block. Probably will be overriden in subclasses.
32 32 def default_title
33 33 _('{#} People or Groups')
... ...
db/migrate/20110526201202_move_reference_from_image_to_owners.rb 0 → 100644
  Diff comments   View file @ebbb202
... ... @@ -0,0 +1,21 @@
  1 +class MoveReferenceFromImageToOwners < ActiveRecord::Migration
  2 + def self.up
  3 + %w[ profiles categories products tasks ].each do |table|
  4 + type = table.singularize.camelcase
  5 + add_column table, :image_id, :integer
  6 + 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)")
  7 + end
  8 + remove_column :images, :owner_id
  9 + remove_column :images, :owner_type
  10 + end
  11 +
  12 + def self.down
  13 + add_column :images, :owner_id, :integer
  14 + add_column :images, :owner_type, :string
  15 + %w[ profiles products categories tasks ].each do |table|
  16 + type = table.singularize.camelcase
  17 + 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)")
  18 + remove_column table, :image_id
  19 + end
  20 + end
  21 +end
... ...
db/migrate/20110527042608_remove_unused_column_from_products.rb 0 → 100644
  Diff comments   View file @ebbb202
... ... @@ -0,0 +1,9 @@
  1 +class RemoveUnusedColumnFromProducts < ActiveRecord::Migration
  2 + def self.up
  3 + remove_column :products, :image
  4 + end
  5 +
  6 + def self.down
  7 + add_column :products, :image, :string
  8 + end
  9 +end
... ...
db/schema.rb
  Diff comments   View file @ebbb202
... ... @@ -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 => 20110524151137) do
  12 +ActiveRecord::Schema.define(:version => 20110527042608) do
13 13  
14 14 create_table "action_tracker", :force => true do |t|
15 15 t.integer "user_id"
... ... @@ -165,6 +165,7 @@ ActiveRecord::Schema.define(:version =&gt; 20110524151137) do
165 165 t.boolean "display_in_menu", :default => false
166 166 t.integer "children_count", :default => 0
167 167 t.boolean "accept_products", :default => true
  168 + t.integer "image_id"
168 169 end
169 170  
170 171 create_table "categories_profiles", :id => false, :force => true do |t|
... ... @@ -270,8 +271,6 @@ ActiveRecord::Schema.define(:version =&gt; 20110524151137) do
270 271 end
271 272  
272 273 create_table "images", :force => true do |t|
273   - t.string "owner_type"
274   - t.integer "owner_id"
275 274 t.integer "parent_id"
276 275 t.string "content_type"
277 276 t.string "filename"
... ... @@ -339,7 +338,6 @@ ActiveRecord::Schema.define(:version =&gt; 20110524151137) do
339 338 t.string "name"
340 339 t.decimal "price"
341 340 t.text "description"
342   - t.string "image"
343 341 t.datetime "created_at"
344 342 t.datetime "updated_at"
345 343 t.float "lat"
... ... @@ -348,6 +346,7 @@ ActiveRecord::Schema.define(:version =&gt; 20110524151137) do
348 346 t.boolean "available", :default => true
349 347 t.boolean "highlighted"
350 348 t.integer "unit_id"
  349 + t.integer "image_id"
351 350 end
352 351  
353 352 add_index "products", ["enterprise_id"], :name => "index_products_on_enterprise_id"
... ... @@ -378,6 +377,7 @@ ActiveRecord::Schema.define(:version =&gt; 20110524151137) do
378 377 t.integer "preferred_domain_id"
379 378 t.datetime "updated_at"
380 379 t.boolean "visible", :default => true
  380 + t.integer "image_id"
381 381 end
382 382  
383 383 add_index "profiles", ["environment_id"], :name => "index_profiles_on_environment_id"
... ... @@ -456,6 +456,7 @@ ActiveRecord::Schema.define(:version =&gt; 20110524151137) do
456 456 t.string "type"
457 457 t.datetime "created_at"
458 458 t.string "target_type"
  459 + t.integer "image_id"
459 460 end
460 461  
461 462 create_table "thumbnails", :force => true do |t|
... ...
features/step_definitions/noosfero_steps.rb
  Diff comments   View file @ebbb202
... ... @@ -116,8 +116,8 @@ Given /^the following products?$/ do |table|
116 116 data = item.dup
117 117 owner = Enterprise[data.delete("owner")]
118 118 category = Category.find_by_slug(data.delete("category").to_slug)
119   - product = Product.create!(data.merge(:enterprise => owner, :product_category => category))
120   - image = Image.create!(:owner => product, :uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  119 + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  120 + product = Product.create!(data.merge(:enterprise => owner, :product_category => category, :image_id => img.id))
121 121 end
122 122 end
123 123  
... ...
lib/acts_as_having_image.rb
  Diff comments   View file @ebbb202
... ... @@ -2,7 +2,9 @@ module ActsAsHavingImage
2 2  
3 3 module ClassMethods
4 4 def acts_as_having_image
5   - has_one :image, :as => 'owner'
  5 + belongs_to :image
  6 + named_scope :with_image, :conditions => [ "#{table_name}.image_id IS NOT NULL" ]
  7 + named_scope :without_image, :conditions => [ "#{table_name}.image_id IS NULL" ]
6 8 self.send(:include, ActsAsHavingImage)
7 9 end
8 10 end
... ...
test/factories.rb
  Diff comments   View file @ebbb202
... ... @@ -311,6 +311,7 @@ module Noosfero::Factory
311 311 end
312 312  
313 313 alias :defaults_for_blog_archives_block :defaults_for_block
  314 + alias :defaults_for_profile_list_block :defaults_for_block
314 315  
315 316 ###############################################
316 317 # Task
... ...
test/unit/image_test.rb
  Diff comments   View file @ebbb202
... ... @@ -22,7 +22,8 @@ class ImageTest &lt; Test::Unit::TestCase
22 22 end
23 23  
24 24 should 'create thumbnails after processing jobs' do
25   - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile)
  25 + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  26 + profile.update_attribute(:image_id, file.id)
26 27  
27 28 process_delayed_job_queue
28 29 Image.attachment_options[:thumbnails].each do |suffix, size|
... ... @@ -32,7 +33,8 @@ class ImageTest &lt; Test::Unit::TestCase
32 33 end
33 34  
34 35 should 'set thumbnails_processed to true after creating thumbnails' do
35   - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile)
  36 + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  37 + profile.update_attribute(:image_id, file.id)
36 38  
37 39 process_delayed_job_queue
38 40  
... ... @@ -62,7 +64,8 @@ class ImageTest &lt; Test::Unit::TestCase
62 64 end
63 65  
64 66 should 'return image thumbnail if thumbnails were processed' do
65   - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile)
  67 + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  68 + profile.update_attribute(:image_id, file.id)
66 69 process_delayed_job_queue
67 70  
68 71 assert_match(/rails_thumb.png/, Image.find(file.id).public_filename(:thumb))
... ... @@ -71,7 +74,8 @@ class ImageTest &lt; Test::Unit::TestCase
71 74 end
72 75  
73 76 should 'store width and height after processing' do
74   - file = Image.create!(:owner => profile, :uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  77 + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  78 + profile.update_attribute(:image_id, file.id)
75 79 file.create_thumbnails
76 80  
77 81 file = Image.find(file.id)
... ... @@ -89,7 +93,7 @@ class ImageTest &lt; Test::Unit::TestCase
89 93 # this test verifies whether it created background jobs also for the
90 94 # thumbnails!
91 95 assert_no_difference Delayed::Job, :count do
92   - image = Image.new(:owner => profile, :uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  96 + image = Image.new(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
93 97 image.stubs(:is_thumbnail?).returns(true)
94 98 image.save!
95 99 end
... ... @@ -97,7 +101,8 @@ class ImageTest &lt; Test::Unit::TestCase
97 101  
98 102 should 'upload to a folder with same name as the schema if database is postgresql' do
99 103 uses_postgresql
100   - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile)
  104 + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  105 + profile.update_attribute(:image_id, file.id)
101 106 process_delayed_job_queue
102 107 assert_match(/images\/test_schema\/\d{4}\/\d{4}\/rails.png/, Image.find(file.id).public_filename)
103 108 file.destroy
... ... @@ -106,7 +111,8 @@ class ImageTest &lt; Test::Unit::TestCase
106 111  
107 112 should 'upload to path prefix folder if database is not postgresql' do
108 113 uses_sqlite
109   - file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => profile)
  114 + file = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  115 + profile.update_attribute(:image_id, file.id)
110 116 process_delayed_job_queue
111 117 assert_match(/images\/\d{4}\/\d{4}\/rails.png/, Image.find(file.id).public_filename)
112 118 file.destroy
... ...
test/unit/profile_list_block_test.rb
  Diff comments   View file @ebbb202
... ... @@ -137,50 +137,80 @@ class ProfileListBlockTest &lt; Test::Unit::TestCase
137 137  
138 138 should 'list random profiles' do
139 139 env = fast_create(Environment)
140   - p1 = fast_create(Person, :environment_id => env.id)
141   - p2 = fast_create(Person, :environment_id => env.id)
142   - p3 = fast_create(Person, :environment_id => env.id)
  140 + 6.times.each do
  141 + fast_create(Person, :environment_id => env.id)
  142 + end
143 143  
144 144 block = ProfileListBlock.new
145 145 block.stubs(:owner).returns(env)
146 146  
147   - # force the "random" function to return something we know
148   - block.stubs(:randomizer).returns('-profiles.id')
149   -
150   - assert_equal [p3.id, p2.id, p1.id], block.profile_list.map(&:id)
  147 + assert_not_equal block.profile_list.map(&:id), block.profile_list.map(&:id)
151 148 end
152 149  
153   - should 'randomize using modulo operator and random number' do
154   - block = ProfileListBlock.new
155   - block.expects(:profile_count).returns(10)
156   - block.expects(:rand).with(10).returns(5)
157   - assert_match /profiles.id % 6/, block.randomizer
  150 + should 'prioritize profiles with image if this option is turned on' do
  151 + env = fast_create(Environment)
  152 + img1 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  153 + p1 = fast_create(Person, :environment_id => env.id, :image_id => img1.id)
  154 + img2 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  155 + p2 = fast_create(Person, :environment_id => env.id, :image_id => img2.id)
  156 +
  157 + p_without_image = fast_create(Person, :environment_id => env.id)
  158 +
  159 + block = ProfileListBlock.new(:limit => 2)
  160 + block.stubs(:owner).returns(env)
  161 + block.stubs(:prioritize_profiles_with_image).returns(true)
  162 +
  163 + assert_not_includes block.profile_list[0..1], p_without_image
158 164 end
159 165  
160   - should 'not divide by zero' do
  166 + should 'list profiles without image only if profiles with image arent enought' do
  167 + env = fast_create(Environment)
  168 + img1 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  169 + p1 = fast_create(Person, :environment_id => env.id, :image_id => img1.id)
  170 + img2 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  171 + p2 = fast_create(Person, :environment_id => env.id, :image_id => img2.id)
  172 + p_without_image = fast_create(Person, :environment_id => env.id)
161 173 block = ProfileListBlock.new
162   - block.stubs(:profile_count).returns(0)
163   - block.expects(:rand).returns(0)
164   - assert_no_match /profiles.id % 0/, block.randomizer
  174 + block.stubs(:owner).returns(env)
  175 + block.stubs(:prioritize_profiles_with_image).returns(true)
  176 +
  177 + block.limit = 2
  178 + assert_not_includes block.profile_list, p_without_image
  179 +
  180 + block.limit = 3
  181 + assert_includes block.profile_list, p_without_image
165 182 end
166 183  
167   - should 'prioritize profiles with image if this option is turned on' do
  184 + should 'list profile with image among profiles without image' do
168 185 env = fast_create(Environment)
169   - p1 = fast_create(Person, :environment_id => env.id)
170   - img1 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => p1)
171   - p2 = fast_create(Person, :environment_id => env.id)
172   - img2 = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'), :owner => p2)
  186 + 5.times do |n|
  187 + fast_create(Person, :environment_id => env.id)
  188 + end
  189 + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  190 + with_image = fast_create(Person, :environment_id => env.id, :image_id => img.id)
  191 + block = ProfileListBlock.new(:limit => 3)
  192 + block.stubs(:prioritize_profiles_with_image).returns(true)
  193 + block.stubs(:owner).returns(env)
  194 + assert_includes block.profile_list, with_image
  195 + end
173 196  
174   - p_without_image = fast_create(Person, :environment_id => env.id)
  197 + should 'not prioritize profiles with image if this option is turned off' do
  198 + env = fast_create(Environment)
  199 + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  200 + with_image = fast_create(Person, :environment_id => env.id, :updated_at => DateTime.now, :image_id => img.id)
  201 + 5.times do |n|
  202 + fast_create(Person, :environment_id => env.id, :updated_at => DateTime.now + 1.day)
  203 + end
175 204  
176   - block = ProfileListBlock.new
  205 + block = ProfileListBlock.new(:limit => 3)
177 206 block.stubs(:owner).returns(env)
178   - block.stubs(:prioritize_profiles_with_image).returns(true)
  207 + block.stubs(:prioritize_profiles_with_image).returns(false)
179 208  
180   - # force the "random" function to return something we know
181   - block.stubs(:randomizer).returns('-profiles.id')
  209 + assert_not_includes block.profile_list, with_image
  210 + end
182 211  
183   - assert_not_includes block.profile_list[0..1], p_without_image
  212 + should 'prioritize profiles with image by default' do
  213 + assert ProfileListBlock.new.prioritize_profiles_with_image
184 214 end
185 215  
186 216 end
... ...
test/unit/profile_test.rb
  Diff comments   View file @ebbb202
... ... @@ -1357,12 +1357,6 @@ class ProfileTest &lt; Test::Unit::TestCase
1357 1357 assert !profile.folders.include?(child)
1358 1358 end
1359 1359  
1360   - should 'validates profile image when save' do
1361   - profile = build(Profile, :image_builder => {:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')})
1362   - profile.image.expects(:valid?).returns(false).at_least_once
1363   - assert !profile.valid?
1364   - end
1365   -
1366 1360 should 'profile is invalid when image not valid' do
1367 1361 profile = build(Profile, :image_builder => {:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png')})
1368 1362 profile.image.expects(:valid?).returns(false).at_least_once
... ... @@ -1710,6 +1704,28 @@ class ProfileTest &lt; Test::Unit::TestCase
1710 1704 assert profile.is_on_homepage?("/#{profile.identifier}/#{homepage.slug}", homepage)
1711 1705 end
1712 1706  
  1707 + should 'find profiles with image' do
  1708 + env = fast_create(Environment)
  1709 + 2.times do |n|
  1710 + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  1711 + fast_create(Person, :name => "with_image_#{n}", :environment_id => env.id, :image_id => img.id)
  1712 + end
  1713 + without_image = fast_create(Person, :name => 'without_image', :environment_id => env.id)
  1714 + assert_equal 2, env.profiles.with_image.count
  1715 + assert_not_includes env.profiles.with_image, without_image
  1716 + end
  1717 +
  1718 + should 'find profiles withouth image' do
  1719 + env = fast_create(Environment)
  1720 + 2.times do |n|
  1721 + fast_create(Person, :name => "without_image_#{n}", :environment_id => env.id)
  1722 + end
  1723 + img = Image.create!(:uploaded_data => fixture_file_upload('/files/rails.png', 'image/png'))
  1724 + with_image = fast_create(Person, :name => 'with_image', :environment_id => env.id, :image_id => img.id)
  1725 + assert_equal 2, env.profiles.without_image.count
  1726 + assert_not_includes env.profiles.without_image, with_image
  1727 + end
  1728 +
1713 1729 private
1714 1730  
1715 1731 def assert_invalid_identifier(id)
... ...