Refactor image processing to use 2 uploaders. Federation and s3 need testing

app/models/jobs/process_photo.rb

@@ -4,10 +4,10 @@
 
 
 module Job
-  class ProcessPhoto < Base 
+  class ProcessPhoto < Base
     @queue = :photos
     def self.perform_delegate(photo_id)
-        Photo.find(photo_id).image.post_process
+      Photo.find(photo_id).process
     end
   end
 end

app/models/photo.rb

@@ -4,7 +4,8 @@
 
 class Photo < Post
   require 'carrierwave/orm/activerecord'
-  mount_uploader :image, ImageUploader
+  mount_uploader :processed_image, ProcessedImage
+  mount_uploader :unprocessed_image, UnprocessedImage
 
   xml_attr :remote_photo_path
   xml_attr :remote_photo_name
@@ -33,23 +34,25 @@ class Photo < Post
     photo = super(params)
     image_file = params.delete(:user_file)
     photo.random_string = gen_random_string(10)
-    photo.processed = false
-    photo.image.store! image_file
-    photo.update_photo_remote_path
+    photo.unprocessed_image.store! image_file
     photo
   end
 
   def not_processed?
-    !self.processed
+    processed_image.path.nil?
   end
 
-  def update_photo_remote_path
-    unless self.image.url.match(/^https?:\/\//)
+  def processed?
+    !processed_image.path.nil?
+  end
+
+  def update_remote_path
+    unless self.processed_image.url.match(/^https?:\/\//)
       pod_url = AppConfig[:pod_url].dup
       pod_url.chop! if AppConfig[:pod_url][-1,1] == '/'
-      remote_path = "#{pod_url}#{self.image.url}"
+      remote_path = "#{pod_url}#{self.processed_image.url}"
     else
-      remote_path = self.image.url
+      remote_path = self.processed_image.url
     end
 
     name_start = remote_path.rindex '/'
@@ -70,13 +73,13 @@ class Photo < Post
   end
 
   def url(name = nil)
-    if self.not_processed? || (!self.image.path.blank? && self.image.path.include?('.gif'))
-      image.url
-    elsif remote_photo_path
+    if remote_photo_path
       name = name.to_s + '_' if name
       remote_photo_path + name.to_s + remote_photo_name
+    elsif not_processed?
+      unprocessed_image.url
     else
-      image.url(name)
+      processed_image.url(name)
     end
   end
 
@@ -96,6 +99,13 @@ class Photo < Post
     Resque.enqueue(Job::ProcessPhoto, self.id)
   end
 
+  def process
+    return false if unprocessed_image.path.include?('.gif') || self.processed?
+    processed_image.store!(unprocessed_image) #Ultra naive
+    update_remote_path
+    save!
+  end
+
   def mutable?
     true
   end

app/models/user.rb

@@ -37,7 +37,7 @@ class User < ActiveRecord::Base
   has_many :contacts
   has_many :contact_people, :through => :contacts, :source => :person
   has_many :services
-  has_many :user_preferences  
+  has_many :user_preferences
 
   before_destroy :disconnect_everyone, :remove_mentions, :remove_person
   before_save do
@@ -52,7 +52,7 @@ class User < ActiveRecord::Base
       self.disable_mail = false
       self.save
     end
-    
+
     pref_hash.keys.each do |key|
       if pref_hash[key] == 'true'
         self.user_preferences.find_or_create_by_email_type(key)
@@ -226,6 +226,7 @@ class User < ActiveRecord::Base
   def update_profile(params)
     if photo = params.delete(:photo)
       photo.update_attributes(:pending => false) if photo.pending
+      photo.process
       params[:image_url] = photo.url(:thumb_large)
       params[:image_url_medium] = photo.url(:thumb_medium)
       params[:image_url_small] = photo.url(:thumb_small)

app/uploaders/image_uploader.rb

@@ -1,54 +0,0 @@
-#   Copyright (c) 2010, Diaspora Inc.  This file is
-#   licensed under the Affero General Public License version 3 or later.  See
-#   the COPYRIGHT file.
-
-class ImageUploader < CarrierWave::Uploader::Base
-  include CarrierWave::MiniMagick
-
-  def store_dir
-    "uploads/images"
-  end
-
-  def extension_white_list
-    %w(jpg jpeg png gif)
-  end
-
-  def filename
-    model.random_string + model.id.to_s + File.extname(@filename) if @filename
-  end
-
-  def post_process
-    self.send(:remove_versions!)
-    unless self.path.include?('.gif')
-      ImageUploader.instance_eval do
-        version :thumb_small do
-          process :resize_to_fill => [50,50]
-        end
-
-        version :thumb_medium do
-          process :resize_to_fill => [100,100]
-        end
-
-        version :thumb_large do
-          process :resize_to_fill => [300,300]
-        end
-
-        version :scaled_full do
-          process :resize_to_limit => [700,700]
-        end
-      end
-
-      self.recreate_versions!
-      self.model.processed = true
-      self.model.update_photo_remote_path
-      self.model.save
-    else
-      false
-    end
-  end
-
-  version :scaled_full
-  version :thumb_large
-  version :thumb_medium
-  version :thumb_small
-end

app/uploaders/processed_image.rb

@@ -0,0 +1,33 @@
+#   Copyright (c) 2010, Diaspora Inc.  This file is
+#   licensed under the Affero General Public License version 3 or later.  See
+#   the COPYRIGHT file.
+
+class ProcessedImage < CarrierWave::Uploader::Base
+  include CarrierWave::MiniMagick
+
+  def store_dir
+    "uploads/images"
+  end
+
+  def extension_white_list
+    %w(jpg jpeg png gif)
+  end
+
+  def filename
+    model.random_string + model.id.to_s + File.extname(@filename) if @filename
+  end
+
+  version :thumb_small do
+    process :resize_to_fill => [50,50]
+  end
+  version :thumb_medium do
+    process :resize_to_fill => [100,100]
+  end
+  version :thumb_large do
+    process :resize_to_fill => [300,300]
+  end
+
+  version :scaled_full do
+    process :resize_to_limit => [700,700]
+  end
+end

app/uploaders/unprocessed_image.rb

@@ -0,0 +1,20 @@
+#   Copyright (c) 2010, Diaspora Inc.  This file is
+#   licensed under the Affero General Public License version 3 or later.  See
+#   the COPYRIGHT file.
+
+class UnprocessedImage < CarrierWave::Uploader::Base
+  include CarrierWave::MiniMagick
+
+  def store_dir
+    "uploads/u_images"
+  end
+
+  def extension_white_list
+    %w(jpg jpeg png gif)
+  end
+
+  def filename
+    model.random_string + model.id.to_s + File.extname(@filename) if @filename
+  end
+
+end

app/views/photos/edit.html.haml

@@ -2,7 +2,7 @@
 -#   licensed under the Affero General Public License version 3 or later.  See
 -#   the COPYRIGHT file.
 
-%h2= "#{t('.editing')} #{@photo.image}"
+%h2= "#{t('.editing')} #{@photo.processed_image}"
 
 %div{:id => @photo.id}
   #show_photo

config/environments/production.rb

@@ -35,7 +35,7 @@ Diaspora::Application.configure do
 
   # Disable Rails's static asset server
   # In production, Apache or nginx will already do this
-  config.serve_static_assets = false
+  config.serve_static_assets = true
 
   # Enable serving of images, stylesheets, and javascripts from an asset server
   # config.action_controller.asset_host = "http://assets.example.com"

db/migrate/20110321205715_unprocessed_image_uploader.rb

@@ -0,0 +1,14 @@
+require 'db/migrate/20110319005509_add_processed_to_post'
+class UnprocessedImageUploader < ActiveRecord::Migration
+  def self.up
+    AddProcessedToPost.down
+    rename_column :posts, :image, :processed_image
+    add_column :posts, :unprocessed_image, :string
+  end
+
+  def self.down
+    remove_column :posts, :unprocessed_image
+    rename_column :posts, :processed_image, :image
+    AddProcessedToPost.up
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20110319172136) do
+ActiveRecord::Schema.define(:version => 20110321205715) do
 
   create_table "aspect_memberships", :force => true do |t|
     t.integer  "aspect_id",  :null => false
@@ -220,12 +220,12 @@ ActiveRecord::Schema.define(:version => 20110319172136) do
     t.text     "remote_photo_path"
     t.string   "remote_photo_name"
     t.string   "random_string"
-    t.string   "image"
+    t.string   "processed_image"
     t.text     "youtube_titles"
     t.datetime "created_at"
     t.datetime "updated_at"
     t.string   "mongo_id"
-    t.boolean  "processed",         :default => true
+    t.string   "unprocessed_image"
   end
 
   add_index "posts", ["author_id"], :name => "index_posts_on_person_id"

spec/models/jobs/process_photo_spec.rb

@@ -1,10 +1,9 @@
 require 'spec_helper'
 
 describe Job::ProcessPhoto do
-  it 'calls post_process on an image uploader' do
+  it 'calls process on the photo' do
     photo = mock()
-    photo.should_receive(:image).and_return(photo)
-    photo.should_receive(:post_process)
+    photo.should_receive(:process)
     Photo.should_receive(:find).with(1).and_return(photo)
     Job::ProcessPhoto.perform(1)
   end

spec/models/photo_spec.rb

@@ -14,9 +14,7 @@ describe Photo do
     @fail_fixture_name = File.join(File.dirname(__FILE__), '..', 'fixtures', 'msg.xml')
 
     @photo  = @user.build_post(:photo, :user_file=> File.open(@fixture_name), :to => @aspect.id)
-    @photo.processed = true
     @photo2 = @user.build_post(:photo, :user_file=> File.open(@fixture_name), :to => @aspect.id)
-    @photo2.processed = true 
   end
 
   describe "protected attributes" do
@@ -44,7 +42,7 @@ describe Photo do
       @photo.save!
     end
   end
-  
+
   it 'is mutable' do
     @photo.mutable?.should == true
   end
@@ -54,33 +52,47 @@ describe Photo do
   end
 
   describe '#diaspora_initialize' do
+    before do
+      image = File.open(@fixture_name)
+      @photo = Photo.diaspora_initialize(
+                :author => @user.person, :user_file => image)
+    end
     it 'sets the persons diaspora handle' do
       @photo2.diaspora_handle.should == @user.person.diaspora_handle
     end
-    it 'has a constructor' do
+    it 'builds the photo without saving' do
+      @photo.created_at.nil?.should be_true
+      @photo.unprocessed_image.read.nil?.should be_false
+    end
+  end
+
+  describe '#update_remote_path' do
+    before do
       image = File.open(@fixture_name)
-      photo = Photo.diaspora_initialize(
+      @photo = Photo.diaspora_initialize(
                 :author => @user.person, :user_file => image)
-      photo.created_at.nil?.should be_true
-      photo.image.read.nil?.should be_false
+      @photo.processed_image.store!(@photo.unprocessed_image)
+      @photo.save!
     end
     it 'sets a remote url' do
-      image = File.open(@fixture_name)
-      photo = Photo.diaspora_initialize(
-                :author => @user.person, :user_file => image)
-      photo.remote_photo_path.should include("http")
-      photo.remote_photo_name.should include(".png")
+      @photo.remote_photo_path.should be_nil
+      @photo.remote_photo_name.should be_nil
+
+      @photo.update_remote_path
+
+      @photo.remote_photo_path.should include("http")
+      @photo.remote_photo_name.should include(".png")
     end
   end
 
   it 'should save a photo' do
-    @photo.image.store! File.open(@fixture_name)
+    @photo.unprocessed_image.store! File.open(@fixture_name)
     @photo.save.should == true
     begin
-      binary = @photo.image.read.force_encoding('BINARY')
+      binary = @photo.unprocessed_image.read.force_encoding('BINARY')
       fixture_binary = File.open(@fixture_name).read.force_encoding('BINARY')
     rescue NoMethodError # Ruby 1.8 doesn't have force_encoding
-      binary = @photo.image.read
+      binary = @photo.unprocessed_image.read
       fixture_binary = File.open(@fixture_name).read
     end
     binary.should == fixture_binary
@@ -88,7 +100,7 @@ describe Photo do
 
   context 'with a saved photo' do
     before do
-      @photo.image.store! File.open(@fixture_name)
+      @photo.unprocessed_image.store! File.open(@fixture_name)
     end
     it 'should have text' do
       @photo.text= "cool story, bro"
@@ -105,25 +117,25 @@ describe Photo do
     end
 
     it 'should not use the imported filename as the url' do
-      @photo.image.url.include?(@fixture_filename).should be false
-      @photo.image.url(:thumb_medium).include?("/" + @fixture_filename).should be false
+      @photo.url.include?(@fixture_filename).should be false
+      @photo.url(:thumb_medium).include?("/" + @fixture_filename).should be false
     end
   end
 
   describe 'non-image files' do
     it 'should not store' do
       file = File.open(@fail_fixture_name)
-      @photo.image.should_receive(:check_whitelist!)
       lambda {
-        @photo.image.store! file
-      }.should raise_error
+        @photo.unprocessed_image.store! file
+      }.should raise_error CarrierWave::IntegrityError, 'You are not allowed to upload "xml" files, allowed types: ["jpg", "jpeg", "png", "gif"]'
     end
 
   end
 
   describe 'serialization' do
     before do
-      @photo.image.store! File.open(@fixture_name)
+      @photo.process
+      @photo.save!
       @xml = @photo.to_xml.to_s
     end
     it 'serializes the url' do
@@ -134,11 +146,11 @@ describe Photo do
       @xml.include?(@user.diaspora_handle).should be true
     end
   end
-  
+
   describe 'remote photos' do
     it 'should set the remote_photo on marshalling' do
-      @photo.image.store! File.open(@fixture_name)
-      @photo.save
+      @photo.process
+      @photo.save!
       #security hax
       user2 = Factory.create(:user)
       aspect2 = user2.aspects.create(:name => "foobars")

spec/models/user_spec.rb

@@ -300,6 +300,10 @@ describe User do
         alice.update_profile(@params).should be true
         @photo.reload.pending.should be_false
       end
+      it 'post-processes the photo' do
+        @photo.should_receive(:process)
+        alice.update_profile(@params).should be true
+      end
     end
   end
 

spec/spec_helper.rb

@@ -42,7 +42,7 @@ RSpec.configure do |config|
   end
 end
 
-ImageUploader.enable_processing = false
+ProcessedImage.enable_processing = false
 
 def set_up_friends
   local_luke = Factory(:user_with_aspect, :username => "luke")