pull hotlinked images

app/assets/javascripts/discourse/views/composer/composer_view.js

@@ -282,10 +282,15 @@ Discourse.ComposerView = Discourse.View.extend(Ember.Evented, {
 
     // done
     $uploadTarget.on('fileuploaddone', function (e, data) {
+      // make sure we have a url
+      if (data.result.url) {
         var markdown = Discourse.Utilities.getUploadMarkdown(data.result);
         // appends a space at the end of the inserted markdown
         composerView.addMarkdown(markdown + " ");
         composerView.set('isUploading', false);
+      } else {
+        bootbox.alert(I18n.t('post.errors.upload'));
+      }
     });
 
     // fail

app/assets/javascripts/discourse/views/modal/avatar_selector_view.js

@@ -54,6 +54,8 @@ Discourse.AvatarSelectorView = Discourse.ModalBodyView.extend({
 
     // when the upload is successful
     $upload.on("fileuploaddone", function (e, data) {
+      // make sure we have a url
+      if (data.result.url) {
         // indicates the users is using an uploaded avatar
         self.get("controller").setProperties({
           has_uploaded_avatar: true,
@@ -68,6 +70,9 @@ Discourse.AvatarSelectorView = Discourse.ModalBodyView.extend({
         Discourse.Utilities.cropAvatar(data.result.url, data.files[0].type).then(function(avatarTemplate) {
           self.get("controller").set("uploaded_avatar_template", avatarTemplate);
         });
+      } else {
+        bootbox.alert(I18n.t('post.errors.upload'));
+      }
     });
 
     // when there has been an error with the upload

app/jobs/regular/generate_avatars.rb

@@ -5,15 +5,15 @@ module Jobs
   class GenerateAvatars < Jobs::Base
 
     def execute(args)
-      upload_id = args[:upload_id]
+      raise Discourse::ImageMagickMissing.new unless system("command -v convert >/dev/null;")
-      raise Discourse::InvalidParameters.new(:upload_id) unless upload_id.present?
 
-      user_id = args[:user_id]
+      upload_id, user_id = args[:upload_id], args[:user_id]
-      raise Discourse::InvalidParameters.new(:user_id) unless user_id.present?
+      raise Discourse::InvalidParameters.new(:upload_id) if upload_id.blank?
+      raise Discourse::InvalidParameters.new(:user_id) if user_id.blank?
 
       upload = Upload.where(id: upload_id).first
       user = User.where(id: user_id).first
-      return unless upload.present? || user.present?
+      return if upload.nil? || user.nil?
 
       external_copy = Discourse.store.download(upload) if Discourse.store.external?
       original_path = if Discourse.store.external?
@@ -22,22 +22,34 @@ module Jobs
         Discourse.store.path_for(upload)
       end
 
-      # we'll extract the first frame when it's a gif
       source = original_path
+      # extract the first frame when it's a gif
       source << "[0]" unless SiteSetting.allow_animated_avatars
+      image = ImageSorcery.new(source)
+      extension = File.extname(original_path)
 
       [120, 45, 32, 25, 20].each do |s|
         # handle retina too
         [s, s * 2].each do |size|
+          begin
             # create a temp file with the same extension as the original
-          temp_file = Tempfile.new(["discourse-avatar", File.extname(original_path)])
+            temp_file = Tempfile.new(["discourse-avatar", extension])
-          temp_path = temp_file.path
+            # create a transparent centered square thumbnail
-          # create a centered square thumbnail
+            if image.convert(temp_file.path,
-          if ImageSorcery.new(source).convert(temp_path, gravity: "center", thumbnail: "#{size}x#{size}^", extent: "#{size}x#{size}", background: "transparent")
+                             gravity: "center",
-            Discourse.store.store_avatar(temp_file, upload, size)
+                             thumbnail: "#{size}x#{size}^",
+                             extent: "#{size}x#{size}",
+                             background: "transparent")
+              if Discourse.store.store_avatar(temp_file, upload, size).blank?
+                Rails.logger.error("Failed to store avatar #{size} for #{upload.url} from #{source}")
               end
+            else
+              Rails.logger.error("Failed to create avatar #{size} for #{upload.url} from #{source}")
+            end
+          ensure
             # close && remove temp file
-          temp_file.close!
+            temp_file && temp_file.close!
+          end
         end
       end
 

app/jobs/regular/pull_hotlinked_images.rb

@@ -0,0 +1,100 @@
+module Jobs
+
+  class PullHotlinkedImages < Jobs::Base
+
+    def initialize
+      # maximum size of the file in bytes
+      @max_size = SiteSetting.max_image_size_kb * 1024
+    end
+
+    def execute(args)
+      # we don't want to run the job if we're not allowed to crawl images
+      return unless SiteSetting.crawl_images?
+
+      post_id = args[:post_id]
+      raise Discourse::InvalidParameters.new(:post_id) unless post_id.present?
+
+      post = Post.where(id: post_id).first
+      return unless post.present?
+
+      raw = post.raw.dup
+      downloaded_urls = {}
+
+      extract_images_from(post.cooked).each do |image|
+        src = image['src']
+
+        if is_valid_image_url(src)
+          begin
+            # have we already downloaded that file?
+            if !downloaded_urls.include?(src)
+              hotlinked = download(src)
+              if hotlinked.size <= @max_size
+                filename = File.basename(URI.parse(src).path)
+                file = ActionDispatch::Http::UploadedFile.new(tempfile: hotlinked, filename: filename)
+                upload = Upload.create_for(post.user_id, file, hotlinked.size, src)
+                downloaded_urls[src] = upload.url
+              else
+                Rails.logger.warn("Failed to pull hotlinked image: #{src} - Image is bigger than #{@max_size}")
+              end
+            end
+            # have we successfuly downloaded that file?
+            if downloaded_urls[src].present?
+              url = downloaded_urls[src]
+              escaped_src = src.gsub("?", "\\?").gsub(".", "\\.").gsub("+", "\\+")
+              # there are 5 ways to insert an image in a post
+              # HTML tag - <img src="http://...">
+              raw.gsub!(/src=["']#{escaped_src}["']/i, "src='#{url}'")
+              # BBCode tag - [img]http://...[/img]
+              raw.gsub!(/\[img\]#{escaped_src}\[\/img\]/i, "[img]#{url}[/img]")
+              # Markdown inline - ![alt](http://...)
+              raw.gsub!(/!\[([^\]]*)\]\(#{escaped_src}\)/) { "![#{$1}](#{url})" }
+              # Markdown reference - [x]: http://
+              raw.gsub!(/\[(\d+)\]: #{escaped_src}/) { "[#{$1}]: #{url}" }
+              # Direct link
+              raw.gsub!(src, "<img src='#{url}'>")
+            end
+          rescue => e
+            Rails.logger.error("Failed to pull hotlinked image: #{src}\n" + e.message + "\n" + e.backtrace.join("\n"))
+          ensure
+            # close & delete the temp file
+            hotlinked && hotlinked.close!
+          end
+        end
+
+      end
+
+      # TODO: make sure the post hasn´t changed while we were downloading remote images
+      if raw != post.raw
+        options = { force_new_version: true }
+        post.revise(Discourse.system_user, raw, options)
+      end
+
+    end
+
+    def extract_images_from(html)
+      doc = Nokogiri::HTML::fragment(html)
+      doc.css("img") - doc.css(".onebox-result img") - doc.css("img.avatar")
+    end
+
+    def is_valid_image_url(src)
+      src.present? && !Discourse.store.has_been_uploaded?(src)
+    end
+
+    def download(url)
+      extension = File.extname(URI.parse(url).path)
+      tmp = Tempfile.new(["discourse-hotlinked", extension])
+
+      File.open(tmp.path, "wb") do |f|
+        hotlinked = open(url, "rb", read_timeout: 5)
+        while f.size <= @max_size && data = hotlinked.read(@max_size)
+          f.write(data)
+        end
+        hotlinked.close!
+      end
+
+      tmp
+    end
+
+  end
+
+end

app/models/optimized_image.rb

@@ -4,8 +4,19 @@ class OptimizedImage < ActiveRecord::Base
   belongs_to :upload
 
   def self.create_for(upload, width, height)
-    return unless width && height
+    return unless width > 0 && height > 0
 
+    # do we already have that thumbnail?
+    thumbnail = where(upload_id: upload.id, width: width, height: height).first
+
+    # make sure the previous thumbnail has not failed
+    if thumbnail && thumbnail.url.blank?
+      thumbnail.destroy
+      thumbnail = nil
+    end
+
+    # create the thumbnail otherwise
+    unless thumbnail
       @image_sorcery_loaded ||= require "image_sorcery"
 
       external_copy = Discourse.store.download(upload) if Discourse.store.external?
@@ -16,7 +27,8 @@ class OptimizedImage < ActiveRecord::Base
       end
 
       # create a temp file with the same extension as the original
-    temp_file = Tempfile.new(["discourse-thumbnail", File.extname(original_path)])
+      extension = File.extname(original_path)
+      temp_file = Tempfile.new(["discourse-thumbnail", extension])
       temp_path = temp_file.path
 
       if ImageSorcery.new("#{original_path}[0]").convert(temp_path, resize: "#{width}x#{height}!")
@@ -29,14 +41,22 @@ class OptimizedImage < ActiveRecord::Base
           url: "",
         )
         # store the optimized image and update its url
-      thumbnail.url = Discourse.store.store_optimized_image(temp_file, thumbnail)
+        url = Discourse.store.store_optimized_image(temp_file, thumbnail)
+        if url.present?
+          thumbnail.url = url
           thumbnail.save
+        else
+          Rails.logger.error("Failed to store avatar #{size} for #{upload.url} from #{source}")
+        end
+      else
+        Rails.logger.error("Failed to create optimized image #{width}x#{height} for #{upload.url}")
       end
 
       # close && remove temp file
       temp_file.close!
       # make sure we remove the cached copy from external stores
       external_copy.close! if Discourse.store.external?
+    end
 
     thumbnail
   end

app/models/post_action.rb

@@ -138,8 +138,7 @@ class PostAction < ActiveRecord::Base
   def self.remove_act(user, post, post_action_type_id)
     if action = where(post_id: post.id,
                       user_id: user.id,
-                      post_action_type_id:
+                      post_action_type_id: post_action_type_id).first
-                      post_action_type_id).first
       action.remove_act!(user)
     end
   end

app/models/upload.rb

@@ -1,12 +1,10 @@
-require 'digest/sha1'
+require "digest/sha1"
-require 'image_sizer'
+require "image_sizer"
-require 'tempfile'
-require 'pathname'
 
 class Upload < ActiveRecord::Base
   belongs_to :user
 
-  has_many :post_uploads
+  has_many :post_uploads, dependent: :destroy
   has_many :posts, through: :post_uploads
 
   has_many :optimized_images, dependent: :destroy
@@ -14,19 +12,16 @@ class Upload < ActiveRecord::Base
   validates_presence_of :filesize
   validates_presence_of :original_filename
 
-  def thumbnail(width = nil, height = nil)
+  def thumbnail(width = self.width, height = self.height)
-    width ||= self.width
-    height ||= self.height
     optimized_images.where(width: width, height: height).first
   end
 
-  def has_thumbnail?(width = nil, height = nil)
+  def has_thumbnail?(width, height)
     thumbnail(width, height).present?
   end
 
   def create_thumbnail!(width, height)
     return unless SiteSetting.create_thumbnails?
-    return if has_thumbnail?(width, height)
     thumbnail = OptimizedImage.create_for(self, width, height)
     if thumbnail
       optimized_images << thumbnail
@@ -47,12 +42,19 @@ class Upload < ActiveRecord::Base
     File.extname(original_filename)
   end
 
-  def self.create_for(user_id, file, filesize)
+  def self.create_for(user_id, file, filesize, origin = nil)
     # compute the sha
     sha1 = Digest::SHA1.file(file.tempfile).hexdigest
     # check if the file has already been uploaded
-    unless upload = Upload.where(sha1: sha1).first
+    upload = Upload.where(sha1: sha1).first
-      # deal with width & heights for images
+    # delete the previously uploaded file if there's been an error
+    if upload && upload.url.blank?
+      upload.destroy
+      upload = nil
+    end
+    # create the upload
+    unless upload
+      # deal with width & height for images
       if SiteSetting.authorized_image?(file)
         # retrieve image info
         image_info = FastImage.new(file.tempfile, raise_on_failure: true)
@@ -61,6 +63,8 @@ class Upload < ActiveRecord::Base
         # make sure we're at the beginning of the file (FastImage is moving the pointer)
         file.rewind
       end
+      # trim the origin if any
+      origin = origin[0...1000] if origin
       # create a db record (so we can use the id)
       upload = Upload.create!(
         user_id: user_id,
@@ -70,11 +74,16 @@ class Upload < ActiveRecord::Base
         url: "",
         width: width,
         height: height,
+        origin: origin,
       )
       # store the file and update its url
-      upload.url = Discourse.store.store_upload(file, upload)
+      url = Discourse.store.store_upload(file, upload)
-      # save the url
+      if url.present?
+        upload.url = url
         upload.save
+      else
+        Rails.logger.error("Failed to store upload ##{upload.id} for user ##{user_id}")
+      end
     end
     # return the uploaded file
     upload
@@ -82,8 +91,7 @@ class Upload < ActiveRecord::Base
 
   def self.get_from_url(url)
     # we store relative urls, so we need to remove any host/cdn
-    asset_host = Rails.configuration.action_controller.asset_host
+    url = url.gsub(/^#{Discourse.asset_host}/i, "") if Discourse.asset_host.present?
-    url = url.gsub(/^#{asset_host}/i, "") if asset_host.present?
     Upload.where(url: url).first if Discourse.store.has_been_uploaded?(url)
   end
 

db/migrate/20131105101051_add_origin_to_uploads.rb

@@ -0,0 +1,5 @@
+class AddOriginToUploads < ActiveRecord::Migration
+  def change
+    add_column :uploads, :origin, :string, limit: 1000
+  end
+end

lib/cooked_post_processor.rb

@@ -1,7 +1,7 @@
 # Post processing that we can do after a post has already been cooked.
 # For example, inserting the onebox content, or image sizes/thumbnails.
 
-require_dependency 'oneboxer'
+require_dependency "oneboxer"
 
 class CookedPostProcessor
   include ActionView::Helpers::NumberHelper
@@ -12,27 +12,38 @@ class CookedPostProcessor
     @post = post
     @doc = Nokogiri::HTML::fragment(post.cooked)
     @size_cache = {}
-    @has_been_uploaded_cache = {}
   end
 
   def post_process
-    clean_up_reverse_index
+    keep_reverse_index_up_to_date
-    post_process_attachments
     post_process_images
     post_process_oneboxes
+    optimize_urls
+    pull_hotlinked_images
   end
 
-  def clean_up_reverse_index
+  def keep_reverse_index_up_to_date
-    PostUpload.delete_all(post_id: @post.id)
+    upload_ids = Set.new
-  end
 
-  def post_process_attachments
+    @doc.search("a").each do |a|
-    attachments.each do |attachment|
+      href = a["href"].to_s
-      href = attachment['href']
-      attachment['href'] = relative_to_absolute(href)
-      # update reverse index
       if upload = Upload.get_from_url(href)
-        associate_to_post(upload)
+        upload_ids << upload.id
+      end
+    end
+
+    @doc.search("img").each do |img|
+      src = img["src"].to_s
+      if upload = Upload.get_from_url(src)
+        upload_ids << upload.id
+      end
+    end
+
+    values = upload_ids.map{ |u| "(#{@post.id},#{u})" }.join(",")
+    PostUpload.transaction do
+      PostUpload.delete_all(post_id: @post.id)
+      if upload_ids.length > 0
+        PostUpload.exec_sql("INSERT INTO post_uploads (post_id, upload_id) VALUES #{values}")
       end
     end
   end
@@ -42,78 +53,53 @@ class CookedPostProcessor
     return if images.blank?
 
     images.each do |img|
-      if img['src'].present?
+      src, width, height = img["src"], img["width"], img["height"]
-        # keep track of the original src
+      limit_size!(img)
-        src = img['src']
+      convert_to_link!(img)
-        # make sure the src is absolute (when working with locally uploaded files)
+      @dirty |= (src != img["src"]) || (width.to_i != img["width"].to_i) || (height.to_i != img["height"].to_i)
-        img['src'] = relative_to_absolute(src)
-        # make sure the img has proper width and height attributes
-        update_dimensions!(img)
-        # retrieve the associated upload, if any
-        if upload = Upload.get_from_url(src)
-          # update reverse index
-          associate_to_post(upload)
-        end
-        # lightbox treatment
-        convert_to_link!(img, upload)
-        # mark the post as dirty whenever the src has changed
-        @dirty |= src != img['src']
-      end
     end
 
-    # Extract the first image from the first post and use it as the 'topic image'
+    update_topic_image(images)
-    extract_topic_image(images)
-  end
-
-  def post_process_oneboxes
-    args = { post_id: @post.id }
-    args[:invalidate_oneboxes] = true if @opts[:invalidate_oneboxes]
-    # bake onebox content into the post
-    result = Oneboxer.apply(@doc) do |url, element|
-      Oneboxer.onebox(url, args)
-    end
-    # mark the post as dirty whenever a onebox as been baked
-    @dirty |= result.changed?
   end
 
   def extract_images
-    # do not extract images inside a onebox or a quote
+    # do not extract images inside oneboxes or quotes
     @doc.css("img") - @doc.css(".onebox-result img") - @doc.css(".quote img")
   end
 
-  def relative_to_absolute(src)
+  def limit_size!(img)
-    if src =~ /^\/[^\/]/
+    w, h = get_size_from_image_sizes(img["src"], @opts[:image_sizes]) || get_size(img["src"])
-      Discourse.base_url_no_prefix + src
+    # limit the size of the thumbnail
-    else
+    img["width"], img["height"] = ImageSizer.resize(w, h)
-      src
+  end
+
+  def get_size_from_image_sizes(src, image_sizes)
+    return unless image_sizes.present?
+    image_sizes.each do |image_size|
+      url, size = image_size[0], image_size[1]
+      return [size["width"], size["height"]] if url.include?(src)
     end
   end
 
-  def update_dimensions!(img)
+  def get_size(url)
-    w, h = get_size_from_image_sizes(img['src'], @opts[:image_sizes]) || get_size(img['src'])
+    absolute_url = url
-    # make sure we limit the size of the thumbnail
+    absolute_url = Discourse.base_url_no_prefix + absolute_url if absolute_url =~ /^\/[^\/]/
-    w, h = ImageSizer.resize(w, h)
+    # FastImage fails when there's no scheme
-    # check whether the dimensions have changed
+    absolute_url = (SiteSetting.use_ssl? ? "https:" : "http:") + absolute_url if absolute_url.start_with?("//")
-    @dirty = (img['width'].to_i != w) || (img['height'].to_i != h)
+    return unless is_valid_image_url?(absolute_url)
-    # update the dimensions
+    # we can *always* crawl our own images
-    img['width'] = w
+    return unless SiteSetting.crawl_images? || Discourse.store.has_been_uploaded?(url)
-    img['height'] = h
+    @size_cache[url] ||= FastImage.size(absolute_url)
+  rescue Zlib::BufError # FastImage.size raises BufError for some gifs
   end
 
-  def associate_to_post(upload)
+  def is_valid_image_url?(url)
-    return if PostUpload.where(post_id: @post.id, upload_id: upload.id).count > 0
+    uri = URI.parse(url)
-    PostUpload.create(post_id: @post.id, upload_id: upload.id)
+    %w(http https).include? uri.scheme
-  rescue ActiveRecord::RecordNotUnique
+  rescue URI::InvalidURIError
-    # do not care if it's already associated
   end
 
-  def optimize_image!(img)
+  def convert_to_link!(img)
-    # TODO
-    # 1) optimize using image_optim
-    # 2) .png vs. .jpg (> 1.5x)
-  end
-
-  def convert_to_link!(img, upload=nil)
     src = img["src"]
     return unless src.present?
 
@@ -123,12 +109,10 @@ class CookedPostProcessor
     return if original_width.to_i <= width && original_height.to_i <= height
     return if original_width.to_i <= SiteSetting.max_image_width && original_height.to_i <= SiteSetting.max_image_height
 
-    return if is_a_hyperlink(img)
+    return if is_a_hyperlink?(img)
 
-    if upload
+    if upload = Upload.get_from_url(src)
-      # create a thumbnail
       upload.create_thumbnail!(width, height)
-      # optimize image
       # TODO: optimize_image!(img)
     end
 
@@ -137,7 +121,7 @@ class CookedPostProcessor
     @dirty = true
   end
 
-  def is_a_hyperlink(img)
+  def is_a_hyperlink?(img)
     parent = img.parent
     while parent
       return if parent.name == "a"
@@ -155,21 +139,20 @@ class CookedPostProcessor
     # then, the link to our larger image
     a = Nokogiri::XML::Node.new("a", @doc)
     img.add_next_sibling(a)
-    a["href"] = img['src']
+    a["href"] = img["src"]
     a["class"] = "lightbox"
     a.add_child(img)
 
     # replace the image by its thumbnail
-    w = img["width"]
+    w, h = img["width"].to_i, img["height"].to_i
-    h = img["height"]
+    img["src"] = upload.thumbnail(w, h).url if upload && upload.has_thumbnail?(w, h)
-    img['src'] = relative_to_absolute(upload.thumbnail(w, h).url) if upload && upload.has_thumbnail?(w, h)
 
     # then, some overlay informations
     meta = Nokogiri::XML::Node.new("div", @doc)
     meta["class"] = "meta"
     img.add_next_sibling(meta)
 
-    filename = get_filename(upload, img['src'])
+    filename = get_filename(upload, img["src"])
     informations = "#{original_width}x#{original_height}"
     informations << " #{number_to_human_size(upload.filesize)}" if upload
 
@@ -181,48 +164,70 @@ class CookedPostProcessor
   def get_filename(upload, src)
     return File.basename(src) unless upload
     return upload.original_filename unless upload.original_filename =~ /^blob(\.png)?$/i
-    return I18n.t('upload.pasted_image_filename')
+    return I18n.t("upload.pasted_image_filename")
   end
 
   def create_span_node(klass, content=nil)
     span = Nokogiri::XML::Node.new("span", @doc)
     span.content = content if content
-    span['class'] = klass
+    span["class"] = klass
     span
   end
 
-  def extract_topic_image(images)
+  def update_topic_image(images)
     if @post.post_number == 1
       img = images.first
-      @post.topic.update_column :image_url, img['src'] if img['src'].present?
+      @post.topic.update_column(:image_url, img["src"]) if img["src"].present?
     end
   end
 
-  def get_size_from_image_sizes(src, image_sizes)
+  def post_process_oneboxes
-    [image_sizes[src]["width"], image_sizes[src]["height"]] if image_sizes.present? && image_sizes[src].present?
+    args = {
+      post_id: @post.id,
+      invalidate_oneboxes: !!@opts[:invalidate_oneboxes],
+    }
+
+    result = Oneboxer.apply(@doc) do |url, element|
+      Oneboxer.onebox(url, args)
     end
 
-  def get_size(url)
+    @dirty |= result.changed?
-    uri = url
-    # make sure urls have a scheme (otherwise, FastImage will fail)
-    uri = (SiteSetting.use_ssl? ? "https:" : "http:") + url if url && url.start_with?("//")
-    return unless is_valid_image_uri?(uri)
-    # we can *always* crawl our own images
-    return unless SiteSetting.crawl_images? || Discourse.store.has_been_uploaded?(url)
-    @size_cache[url] ||= FastImage.size(uri)
-  rescue Zlib::BufError # FastImage.size raises BufError for some gifs
   end
 
-  def is_valid_image_uri?(url)
+  def optimize_urls
-    uri = URI.parse(url)
+    @doc.search("a").each do |a|
-    %w(http https).include? uri.scheme
+      href = a["href"].to_s
-  rescue URI::InvalidURIError
+      if Discourse.store.has_been_uploaded?(href)
+        a["href"] = schemaless relative_to_absolute(href)
+      end
     end
 
-  def attachments
+    @doc.search("img").each do |img|
-    attachments = @doc.css("a.attachment[href^=\"#{Discourse.store.absolute_base_url}\"]")
+      src = img["src"].to_s
-    attachments += @doc.css("a.attachment[href^=\"#{Discourse.store.relative_base_url}\"]") if Discourse.store.internal?
+      if Discourse.store.has_been_uploaded?(src)
-    attachments
+        img["src"] = schemaless relative_to_absolute(src)
+      end
+    end
+  end
+
+  def relative_to_absolute(url)
+    url =~ /^\/[^\/]/ ? (Discourse.asset_host || Discourse.base_url_no_prefix) + url : url
+  end
+
+  def schemaless(url)
+    url.gsub(/^https?:/, "")
+  end
+
+  def pull_hotlinked_images
+    # we don't want to run the job if we're not allowed to crawl images
+    return unless SiteSetting.crawl_images?
+    # we only want to run the job whenever it's changed by a user
+    return if @post.updated_by == Discourse.system_user
+    # make sure no other job is scheduled
+    Jobs.cancel_scheduled_job(:pull_hotlinked_images, post_id: @post.id)
+    # schedule the job
+    delay = SiteSetting.ninja_edit_window + 1
+    Jobs.enqueue_in(delay.minutes.to_i, :pull_hotlinked_images, post_id: @post.id)
   end
 
   def dirty?

lib/discourse.rb

@@ -22,6 +22,9 @@ module Discourse
   # When a setting is missing
   class SiteSettingMissing < Exception; end
 
+  # When ImageMagick is missing
+  class ImageMagickMissing < Exception; end
+
   # Cross site request forgery
   class CSRF < Exception; end
 
@@ -72,11 +75,11 @@ module Discourse
     end
   end
 
-  def self.base_uri default_value=""
+  def self.base_uri(default_value = "")
     if !ActionController::Base.config.relative_url_root.blank?
-      return ActionController::Base.config.relative_url_root
+      ActionController::Base.config.relative_url_root
     else
-      return default_value
+      default_value
     end
   end
 
@@ -142,10 +145,10 @@ module Discourse
   def self.store
     if SiteSetting.enable_s3_uploads?
       @s3_store_loaded ||= require 'file_store/s3_store'
-      S3Store.new
+      FileStore::S3Store.new
     else
       @local_store_loaded ||= require 'file_store/local_store'
-      LocalStore.new
+      FileStore::LocalStore.new
     end
   end
 
@@ -157,6 +160,10 @@ module Discourse
     @current_user_provider = val
   end
 
+  def self.asset_host
+    Rails.configuration.action_controller.asset_host
+  end
+
 private
 
   def self.maintenance_mode_key

lib/file_store/base_store.rb

@@ -0,0 +1,46 @@
+module FileStore
+
+  class BaseStore
+
+    def store_upload(file, upload)
+    end
+
+    def store_optimized_image(file, optimized_image)
+    end
+
+    def store_avatar(file, avatar, size)
+    end
+
+    def remove_upload(upload)
+    end
+
+    def remove_optimized_image(optimized_image)
+    end
+
+    def has_been_uploaded?(url)
+    end
+
+    def absolute_base_url
+    end
+
+    def relative_base_url
+    end
+
+    def external?
+    end
+
+    def internal?
+    end
+
+    def path_for(upload)
+    end
+
+    def download(upload)
+    end
+
+    def absolute_avatar_template(avatar)
+    end
+
+  end
+
+end

lib/file_store/local_store.rb

@@ -1,4 +1,8 @@
-class LocalStore
+require 'file_store/base_store'
+
+module FileStore
+
+  class LocalStore < BaseStore
 
     def store_upload(file, upload)
       path = get_path_for_upload(file, upload)
@@ -10,8 +14,8 @@ class LocalStore
       store_file(file, path)
     end
 
-  def store_avatar(file, upload, size)
+    def store_avatar(file, avatar, size)
-    path = get_path_for_avatar(file, upload, size)
+      path = get_path_for_avatar(file, avatar, size)
       store_file(file, path)
     end
 
@@ -23,18 +27,12 @@ class LocalStore
       remove_file(optimized_image.url)
     end
 
-  def remove_avatars(upload)
-    return unless upload.url =~ /avatars/
-    remove_directory(File.dirname(upload.url))
-  end
-
     def has_been_uploaded?(url)
       is_relative?(url) || is_local?(url)
     end
 
     def absolute_base_url
-    url = asset_host.present? ? asset_host : Discourse.base_url_no_prefix
+      "#{Discourse.base_url_no_prefix}#{relative_base_url}"
-    "#{url}#{relative_base_url}"
     end
 
     def relative_base_url
@@ -53,8 +51,8 @@ class LocalStore
       "#{public_dir}#{upload.url}"
     end
 
-  def absolute_avatar_template(upload)
+    def absolute_avatar_template(avatar)
-    avatar_template(upload, absolute_base_url)
+      avatar_template(avatar, absolute_base_url)
     end
 
     private
@@ -74,32 +72,26 @@ class LocalStore
         "_#{optimized_image.width}x#{optimized_image.height}",
         optimized_image.extension,
       ].join
-    # /uploads/<site>/_optimized/<1A3>/<B5C>/<filename>
+      # path
-    File.join(
+      "#{relative_base_url}/_optimized/#{optimized_image.sha1[0..2]}/#{optimized_image.sha1[3..5]}/#{filename}"
-      relative_base_url,
-      "_optimized",
-      optimized_image.sha1[0..2],
-      optimized_image.sha1[3..5],
-      filename
-    )
     end
 
-  def get_path_for_avatar(file, upload, size)
+    def get_path_for_avatar(file, avatar, size)
-    relative_avatar_template(upload).gsub("{size}", size.to_s)
+      relative_avatar_template(avatar).gsub("{size}", size.to_s)
     end
 
-  def relative_avatar_template(upload)
+    def relative_avatar_template(avatar)
-    avatar_template(upload, relative_base_url)
+      avatar_template(avatar, relative_base_url)
     end
 
-  def avatar_template(upload, base_url)
+    def avatar_template(avatar, base_url)
       File.join(
         base_url,
         "avatars",
-      upload.sha1[0..2],
+        avatar.sha1[0..2],
-      upload.sha1[3..5],
+        avatar.sha1[3..5],
-      upload.sha1[6..15],
+        avatar.sha1[6..15],
-      "{size}#{upload.extension}"
+        "{size}#{avatar.extension}"
       )
     end
 
@@ -107,11 +99,11 @@ class LocalStore
       # copy the file to the right location
       copy_file(file, "#{public_dir}#{path}")
       # url
-    Discourse.base_uri + path
+      "#{Discourse.base_uri}#{path}"
     end
 
     def copy_file(file, path)
-    FileUtils.mkdir_p Pathname.new(path).dirname
+      FileUtils.mkdir_p(Pathname.new(path).dirname)
       # move the file to the right location
       # not using cause mv, cause permissions are no good on move
       File.open(path, "wb") do |f|
@@ -120,30 +112,28 @@ class LocalStore
     end
 
     def remove_file(url)
-    File.delete("#{public_dir}#{url}") if has_been_uploaded?(url)
+      File.delete("#{public_dir}#{url}") if is_relative?(url)
     rescue Errno::ENOENT
       # don't care if the file isn't there
     end
 
-  def remove_directory(path)
-    directory = "#{public_dir}/#{path}"
-    FileUtils.rm_rf(directory)
-  end
-
     def is_relative?(url)
       url.start_with?(relative_base_url)
     end
 
     def is_local?(url)
-    url.start_with?(absolute_base_url)
+      absolute_url = url.start_with?("//") ? (SiteSetting.use_ssl? ? "https:" : "http:") + url : url
+      absolute_url.start_with?(absolute_base_url) || absolute_url.start_with?(absolute_base_cdn_url)
+    end
+
+    def absolute_base_cdn_url
+      "#{Discourse.asset_host}#{relative_base_url}"
     end
 
     def public_dir
       "#{Rails.root}/public"
     end
 
-  def asset_host
-    Rails.configuration.action_controller.asset_host
   end
 
 end

lib/file_store/s3_store.rb

@@ -1,49 +1,23 @@
-require 'digest/sha1'
+require 'file_store/base_store'
-require 'open-uri'
 
-class S3Store
+module FileStore
+
+  class S3Store < BaseStore
     @fog_loaded ||= require 'fog'
 
     def store_upload(file, upload)
-    # <id><sha1><extension>
+      path = get_path_for_upload(file, upload)
-    path = "#{upload.id}#{upload.sha1}#{upload.extension}"
+      store_file(file, path, upload.original_filename, file.content_type)
-
-    # if this fails, it will throw an exception
-    upload(file.tempfile, path, upload.original_filename, file.content_type)
-
-    # returns the url of the uploaded file
-    "#{absolute_base_url}/#{path}"
     end
 
     def store_optimized_image(file, optimized_image)
-    # <id><sha1>_<width>x<height><extension>
+      path = get_path_for_optimized_image(file, optimized_image)
-    path = [
+      store_file(file, path)
-      optimized_image.id,
-      optimized_image.sha1,
-      "_#{optimized_image.width}x#{optimized_image.height}",
-      optimized_image.extension
-    ].join
-
-    # if this fails, it will throw an exception
-    upload(file, path)
-
-    # returns the url of the uploaded file
-    "#{absolute_base_url}/#{path}"
     end
 
-  def store_avatar(file, upload, size)
+    def store_avatar(file, avatar, size)
-    # /avatars/<sha1>/200.jpg
+      path = get_path_for_avatar(file, avatar, size)
-    path = File.join(
+      store_file(file, path)
-      "avatars",
-      upload.sha1,
-      "#{size}#{upload.extension}"
-    )
-
-    # if this fails, it will throw an exception
-    upload(file, path)
-
-    # returns the url of the avatar
-    "#{absolute_base_url}/#{path}"
     end
 
     def remove_upload(upload)
@@ -54,14 +28,6 @@ class S3Store
       remove_file(optimized_image.url)
     end
 
-  def remove_avatars(upload)
-
-  end
-
-  def remove_file(url)
-    remove File.basename(url) if has_been_uploaded?(url)
-  end
-
     def has_been_uploaded?(url)
       url.start_with?(absolute_base_url)
     end
@@ -79,18 +45,50 @@ class S3Store
     end
 
     def download(upload)
-    temp_file = Tempfile.new(["discourse-s3", File.extname(upload.original_filename)])
+      @open_uri_loaded ||= require 'open-uri'
+
+      extension = File.extname(upload.original_filename)
+      temp_file = Tempfile.new(["discourse-s3", extension])
       url = (SiteSetting.use_ssl? ? "https:" : "http:") + upload.url
 
       File.open(temp_file.path, "wb") do |f|
-      f.write open(url, "rb", read_timeout: 20).read
+        f.write(open(url, "rb", read_timeout: 5).read)
       end
 
       temp_file
     end
 
+    def absolute_avatar_template(avatar)
+      "#{absolute_base_url}/avatars/#{avatar.sha1}/{size}#{avatar.extension}"
+    end
+
     private
 
+    def get_path_for_upload(file, upload)
+      "#{upload.id}#{upload.sha1}#{upload.extension}"
+    end
+
+    def get_path_for_optimized_image(file, optimized_image)
+      "#{optimized_image.id}#{optimized_image.sha1}_#{optimized_image.width}x#{optimized_image.height}#{optimized_image.extension}"
+    end
+
+    def get_path_for_avatar(file, avatar, size)
+      "avatars/#{avatar.sha1}/#{size}#{avatar.extension}"
+    end
+
+    def store_file(file, path, filename = nil, content_type = nil)
+      # if this fails, it will throw an exception
+      upload(file, path, filename, content_type)
+      # url
+      "#{absolute_base_url}/#{path}"
+    end
+
+    def remove_file(url)
+      return unless has_been_uploaded?(url)
+      filename = File.basename(url)
+      remove(filename)
+    end
+
     def s3_bucket
       SiteSetting.s3_upload_bucket.downcase
     end
@@ -104,7 +102,7 @@ class S3Store
     def get_or_create_directory(bucket)
       check_missing_site_settings
 
-    fog = Fog::Storage.new s3_options
+      fog = Fog::Storage.new(s3_options)
 
       directory = fog.directories.get(bucket)
       directory = fog.directories.create(key: bucket) unless directory
@@ -134,8 +132,13 @@ class S3Store
     end
 
     def remove(unique_filename)
-    fog = Fog::Storage.new s3_options
+      check_missing_site_settings
+
+      fog = Fog::Storage.new(s3_options)
+
       fog.delete_object(s3_bucket, unique_filename)
     end
 
+  end
+
 end

lib/pretty_text.rb

@@ -24,9 +24,7 @@ module PrettyText
       return "" unless username
 
       user = User.where(username_lower: username.downcase).first
-      if user.present?
+      user.avatar_template if user.present?
-        user.avatar_template
-      end
     end
 
     def is_username_valid(username)
@@ -97,7 +95,6 @@ module PrettyText
   end
 
   def self.v8
-
     return @ctx if @ctx
 
     # ensure we only init one of these
@@ -143,8 +140,6 @@ module PrettyText
       baked = context.eval('Discourse.Markdown.markdownConverter(opts).makeHtml(raw)')
     end
 
-    # we need some minimal server side stuff, apply CDN and TODO filter disallowed markup
-    baked = apply_cdn(baked, Rails.configuration.action_controller.asset_host)
     baked
   end
 
@@ -160,35 +155,12 @@ module PrettyText
     r
   end
 
-  def self.apply_cdn(html, url)
-    return html unless url
-
-    image = /\.(png|jpg|jpeg|gif|bmp|tif|tiff)$/i
-    relative = /^\/[^\/]/
-
-    doc = Nokogiri::HTML.fragment(html)
-
-    doc.css("a").each do |l|
-      href = l["href"].to_s
-      l["href"] = url + href if href =~ relative && href =~ image
-    end
-
-    doc.css("img").each do |l|
-      src = l["src"].to_s
-      l["src"] = url + src if src =~ relative
-    end
-
-    doc.to_s
-  end
-
   def self.cook(text, opts={})
     cloned = opts.dup
     # we have a minor inconsistency
     cloned[:topicId] = opts[:topic_id]
     sanitized = markdown(text.dup, cloned)
-    if SiteSetting.add_rel_nofollow_to_user_content
+    sanitized = add_rel_nofollow_to_user_content(sanitized) if SiteSetting.add_rel_nofollow_to_user_content
-      sanitized = add_rel_nofollow_to_user_content(sanitized)
-    end
     sanitized
   end
 
@@ -196,9 +168,7 @@ module PrettyText
     whitelist = []
 
     l = SiteSetting.exclude_rel_nofollow_domains
-    if l.present?
+    whitelist = l.split(",") if l.present?
-      whitelist = l.split(",")
-    end
 
     site_uri = nil
     doc = Nokogiri::HTML.fragment(html)
@@ -245,7 +215,6 @@ module PrettyText
     links
   end
 
-
   def self.excerpt(html, max_length, options={})
     ExcerptParser.get_excerpt(html, max_length, options)
   end

lib/tasks/images.rake

@@ -9,103 +9,3 @@ task "images:compress" => :environment do
     end
   end
 end
-
-desc "download all hotlinked images"
-task "images:pull_hotlinked" => :environment do
-  RailsMultisite::ConnectionManagement.each_connection do |db|
-    # currently only works when using the local storage
-    next if Discourse.store.external?
-
-    puts "Pulling hotlinked images for: #{db}"
-
-    # shorthand to the asset host
-    asset_host = Rails.configuration.action_controller.asset_host
-    # maximum size of the file in bytes
-    max_size = SiteSetting.max_image_size_kb * 1024
-    # will hold the urls of the already downloaded images
-    upload_urls = {}
-
-    Post.find_each do |post|
-      has_changed = false
-
-      extract_images_from(post.cooked).each do |image|
-        src = image['src']
-        if src.present? &&
-           src !~ /^\/[^\/]/ &&
-           !src.starts_with?(Discourse.base_url_no_prefix) &&
-           !(asset_host.present? && src.starts_with?(asset_host))
-          begin
-            # have we already downloaded that file?
-            if !upload_urls.include?(src)
-              # initialize
-              upload_urls[src] = nil
-              # download the file
-              hotlinked = download(src, max_size)
-              # if the hotlinked image is OK
-              if hotlinked.size <= max_size
-                file = ActionDispatch::Http::UploadedFile.new(tempfile: hotlinked, filename: File.basename(URI.parse(src).path))
-                upload_urls[src] = Upload.create_for(post.user_id, file, hotlinked.size).url
-              else
-                puts "\nFailed to pull: #{src} for post ##{post.id} - too large\n"
-              end
-            end
-            # if we have downloaded a file
-            if upload_urls[src].present?
-              src_for_regexp = src.gsub("?", "\\?").gsub(".", "\\.").gsub("+", "\\+")
-              # there are 5 ways to insert an image in a post
-              # HTML tag - <img src="http://...">
-              post.raw.gsub!(/src=["']#{src_for_regexp}["']/i, "src='#{upload_urls[src]}'")
-              # BBCode tag - [img]http://...[/img]
-              post.raw.gsub!(/\[img\]#{src_for_regexp}\[\/img\]/i, "[img]#{upload_urls[src]}[/img]")
-              # Markdown inline - ![alt](http://...)
-              post.raw.gsub!(/!\[([^\]]*)\]\(#{src_for_regexp}\)/) { "![#{$1}](#{upload_urls[src]})" }
-              # Markdown reference - [x]: http://
-              post.raw.gsub!(/\[(\d+)\]: #{src_for_regexp}/) { "[#{$1}]: #{upload_urls[src]}" }
-              # Direct link
-              post.raw.gsub!(src, "<img src='#{upload_urls[src]}'>")
-              # mark the post as changed
-              has_changed = true
-            end
-          rescue => e
-            puts "\nFailed to pull: #{src} for post ##{post.id} - #{e}\n"
-          ensure
-            # close & delete the temporary file
-            hotlinked && hotlinked.close!
-          end
-        end
-      end
-
-      if has_changed
-        # since the raw has changed, we cook the post once again
-        post.cooked = post.cook(post.raw, topic_id: post.topic_id, invalidate_oneboxes: true)
-        # update both raw & cooked version of the post
-        Post.exec_sql('update posts set cooked = ?, raw = ? where id = ?', post.cooked, post.raw, post.id)
-        # trigger the post processing
-        post.trigger_post_process
-        putc "#"
-      else
-        putc "."
-      end
-    end
-  end
-  puts "\ndone."
-end
-
-def extract_images_from(html)
-  doc = Nokogiri::HTML::fragment(html)
-  doc.css("img") - doc.css(".onebox-result img") - doc.css("img.avatar")
-end
-
-def download(url, max_size)
-  # create a temporary file
-  temp_file = Tempfile.new(["discourse-hotlinked", File.extname(URI.parse(url).path)])
-  # download the hotlinked image
-  File.open(temp_file.path, "wb") do |f|
-    hotlinked = open(url, "rb", read_timeout: 5)
-    while f.size <= max_size && data = hotlinked.read(max_size)
-      f.write(data)
-    end
-    hotlinked.close
-  end
-  temp_file
-end

spec/components/cooked_post_processor_spec.rb

@@ -1,106 +1,49 @@
-require 'spec_helper'
+require "spec_helper"
-require 'cooked_post_processor'
+require "cooked_post_processor"
 
 describe CookedPostProcessor do
 
-  context "post_process" do
+  context ".post_process" do
 
     let(:post) { build(:post) }
     let(:cpp) { CookedPostProcessor.new(post) }
     let(:post_process) { sequence("post_process") }
 
     it "post process in sequence" do
-      cpp.expects(:clean_up_reverse_index).in_sequence(post_process)
+      cpp.expects(:keep_reverse_index_up_to_date).in_sequence(post_process)
-      cpp.expects(:post_process_attachments).in_sequence(post_process)
       cpp.expects(:post_process_images).in_sequence(post_process)
       cpp.expects(:post_process_oneboxes).in_sequence(post_process)
+      cpp.expects(:optimize_urls).in_sequence(post_process)
+      cpp.expects(:pull_hotlinked_images).in_sequence(post_process)
       cpp.post_process
     end
 
   end
 
-  context "clean_up_reverse_index" do
+  context ".keep_reverse_index_up_to_date" do
 
-    let(:post) { build(:post) }
+    let(:post) { build(:post_with_uploads, id: 123) }
     let(:cpp) { CookedPostProcessor.new(post) }
 
+    it "finds all the uploads in the post" do
+      Upload.expects(:get_from_url).with("/uploads/default/2/2345678901234567.jpg")
+      Upload.expects(:get_from_url).with("/uploads/default/1/1234567890123456.jpg")
+      cpp.keep_reverse_index_up_to_date
+    end
+
     it "cleans the reverse index up for the current post" do
       PostUpload.expects(:delete_all).with(post_id: post.id)
-      cpp.clean_up_reverse_index
+      cpp.keep_reverse_index_up_to_date
     end
 
   end
 
-  context "post_process_attachments" do
+  context ".post_process_images" do
 
-    context "with attachment" do
+    context "with image_sizes" do
-
-      let(:upload) { Fabricate(:upload) }
-      let(:post) { Fabricate(:post_with_an_attachment) }
-      let(:cpp) { CookedPostProcessor.new(post) }
-
-      # all in one test to speed things up
-      it "works" do
-        Upload.expects(:get_from_url).returns(upload)
-        cpp.post_process_attachments
-        # ensures absolute urls on attachment
-        cpp.html.should =~ /#{Discourse.store.absolute_base_url}/
-        # keeps the reverse index up to date
-        post.uploads.reload
-        post.uploads.count.should == 1
-      end
-
-    end
-
-  end
-
-  context "post_process_images" do
-
-    context "with images in quotes and oneboxes" do
-
-      let(:post) { build(:post_with_images_in_quote_and_onebox) }
-      let(:cpp) { CookedPostProcessor.new(post) }
-      before { cpp.post_process_images }
-
-      it "does not process them" do
-        cpp.html.should match_html post.cooked
-        cpp.should_not be_dirty
-      end
-
-      it "has no topic image if there isn't one in the post" do
-        post.topic.image_url.should be_blank
-      end
-
-    end
-
-    context "with locally uploaded images" do
-
-      let(:upload) { Fabricate(:upload) }
-      let(:post) { Fabricate(:post_with_uploaded_image) }
-      let(:cpp) { CookedPostProcessor.new(post) }
-      before { FastImage.stubs(:size).returns([200, 400]) }
-
-      # all in one test to speed things up
-      it "works" do
-        Upload.expects(:get_from_url).returns(upload)
-        cpp.post_process_images
-        # ensures absolute urls on uploaded images
-        cpp.html.should =~ /#{LocalStore.new.absolute_base_url}/
-        # dirty
-        cpp.should be_dirty
-        # keeps the reverse index up to date
-        post.uploads.reload
-        post.uploads.count.should == 1
-      end
-
-    end
-
-    context "with sized images" do
 
       let(:post) { build(:post_with_image_url) }
-      let(:cpp) { CookedPostProcessor.new(post, image_sizes: {'http://foo.bar/image.png' => {'width' => 111, 'height' => 222}}) }
+      let(:cpp) { CookedPostProcessor.new(post, image_sizes: {"http://foo.bar/image.png" => {"width" => 111, "height" => 222}}) }
-
-      before { FastImage.stubs(:size).returns([150, 250]) }
 
       it "adds the width from the image sizes provided" do
         cpp.post_process_images
@@ -134,7 +77,6 @@ describe CookedPostProcessor do
         SiteSetting.stubs(:max_image_height).returns(2000)
         SiteSetting.stubs(:create_thumbnails?).returns(true)
         Upload.expects(:get_from_url).returns(upload)
-        cpp.stubs(:associate_to_post)
         FastImage.stubs(:size).returns([1000, 2000])
         # optimized_image
         FileUtils.stubs(:mkdir_p)
@@ -144,7 +86,7 @@ describe CookedPostProcessor do
 
       it "generates overlay information" do
         cpp.post_process_images
-        cpp.html.should match_html '<div><a href="http://test.localhost/uploads/default/1/1234567890123456.jpg" class="lightbox"><img src="http://test.localhost/uploads/default/_optimized/da3/9a3/ee5e6b4b0d_690x1380.jpg" width="690" height="1380"><div class="meta">
+        cpp.html.should match_html '<div><a href="/uploads/default/1/1234567890123456.jpg" class="lightbox"><img src="/uploads/default/_optimized/da3/9a3/ee5e6b4b0d_690x1380.jpg" width="690" height="1380"><div class="meta">
 <span class="filename">uploaded.jpg</span><span class="informations">1000x2000 1.21 KB</span><span class="expand"></span>
 </div></a></div>'
         cpp.should be_dirty
@@ -159,37 +101,107 @@ describe CookedPostProcessor do
       let(:cpp) { CookedPostProcessor.new(post) }
 
       it "adds a topic image if there's one in the post" do
-        FastImage.stubs(:size).returns([100, 100])
+        FastImage.stubs(:size)
+        post.topic.image_url.should be_nil
         cpp.post_process_images
         post.topic.reload
-        post.topic.image_url.should == "http://test.localhost/uploads/default/2/3456789012345678.png"
+        post.topic.image_url.should be_present
       end
 
     end
 
   end
 
-  context "post_process_oneboxes" do
+  context ".extract_images" do
 
-    let(:post) { build(:post_with_youtube, id: 123) }
+    let(:post) { build(:post_with_images_in_quote_and_onebox) }
-    let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) }
+    let(:cpp) { CookedPostProcessor.new(post) }
 
-    before do
+    it "does not extract images inside oneboxes or quotes" do
-      Oneboxer.expects(:onebox).with("http://www.youtube.com/watch?v=9bZkp7q19f0", post_id: 123, invalidate_oneboxes: true).returns('<div>GANGNAM STYLE</div>')
+      cpp.extract_images.length.should == 0
-      cpp.post_process_oneboxes
-    end
-
-    it "should be dirty" do
-      cpp.should be_dirty
-    end
-
-    it "inserts the onebox without wrapping p" do
-      cpp.html.should match_html "<div>GANGNAM STYLE</div>"
     end
 
   end
 
-  context "get_filename" do
+  context ".get_size_from_image_sizes" do
+
+    let(:post) { build(:post) }
+    let(:cpp) { CookedPostProcessor.new(post) }
+
+    it "returns the size" do
+      image_sizes = { "http://my.discourse.org/image.png" => { "width" => 111, "height" => 222 } }
+      cpp.get_size_from_image_sizes("/image.png", image_sizes).should == [111, 222]
+    end
+
+  end
+
+  context ".get_size" do
+
+    let(:post) { build(:post) }
+    let(:cpp) { CookedPostProcessor.new(post) }
+
+    it "ensures urls are absolute" do
+      cpp.expects(:is_valid_image_url?).with("http://test.localhost/relative/url/image.png")
+      cpp.get_size("/relative/url/image.png")
+    end
+
+    it "ensures urls have a default scheme" do
+      cpp.expects(:is_valid_image_url?).with("http://schemaless.url/image.jpg")
+      cpp.get_size("//schemaless.url/image.jpg")
+    end
+
+    it "caches the results" do
+      SiteSetting.stubs(:crawl_images?).returns(true)
+      FastImage.expects(:size).returns([200, 400])
+      cpp.get_size("http://foo.bar/image3.png")
+      cpp.get_size("http://foo.bar/image3.png").should == [200, 400]
+    end
+
+    context "when crawl_images is disabled" do
+
+      before { SiteSetting.stubs(:crawl_images?).returns(false) }
+
+      it "doesn't call FastImage" do
+        FastImage.expects(:size).never
+        cpp.get_size("http://foo.bar/image1.png").should == nil
+      end
+
+      it "is always allowed to crawl our own images" do
+        store = stub
+        store.expects(:has_been_uploaded?).returns(true)
+        Discourse.expects(:store).returns(store)
+        FastImage.expects(:size).returns([100, 200])
+        cpp.get_size("http://foo.bar/image2.png").should == [100, 200]
+      end
+
+    end
+
+  end
+
+  context ".is_valid_image_url?" do
+
+    let(:post) { build(:post) }
+    let(:cpp) { CookedPostProcessor.new(post) }
+
+    it "validates HTTP(s) urls" do
+      cpp.is_valid_image_url?("http://domain.com").should == true
+      cpp.is_valid_image_url?("https://domain.com").should == true
+    end
+
+    it "doesn't validate other urls" do
+      cpp.is_valid_image_url?("ftp://domain.com").should == false
+      cpp.is_valid_image_url?("ftps://domain.com").should == false
+      cpp.is_valid_image_url?("/tmp/image.png").should == false
+      cpp.is_valid_image_url?("//domain.com").should == false
+    end
+
+    it "doesn't throw an exception with a bad URI" do
+      cpp.is_valid_image_url?("http://do<main.com").should == nil
+    end
+
+  end
+
+  context ".get_filename" do
 
     let(:post) { build(:post) }
     let(:cpp) { CookedPostProcessor.new(post) }
@@ -210,61 +222,90 @@ describe CookedPostProcessor do
 
   end
 
-  context "get_size" do
+  context ".post_process_oneboxes" do
+
+    let(:post) { build(:post_with_youtube, id: 123) }
+    let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) }
+
+    before do
+      Oneboxer.expects(:onebox)
+              .with("http://www.youtube.com/watch?v=9bZkp7q19f0", post_id: 123, invalidate_oneboxes: true)
+              .returns("<div>GANGNAM STYLE</div>")
+      cpp.post_process_oneboxes
+    end
+
+    it "is dirty" do
+      cpp.should be_dirty
+    end
+
+    it "inserts the onebox without wrapping p" do
+      cpp.html.should match_html "<div>GANGNAM STYLE</div>"
+    end
+
+  end
+
+  context ".optimize_urls" do
+
+    let(:post) { build(:post_with_uploads_and_links) }
+    let(:cpp) { CookedPostProcessor.new(post) }
+
+    it "uses schemaless url for uploads" do
+      cpp.optimize_urls
+      cpp.html.should match_html '<a href="//test.localhost/uploads/default/2/2345678901234567.jpg">Link</a>
+       <img src="//test.localhost/uploads/default/1/1234567890123456.jpg"><a href="http://www.google.com">Google</a>
+       <img src="http://foo.bar/image.png">'
+    end
+
+    context "when CDN is enabled" do
+
+      it "uses schemaless CDN url for uploads" do
+        Rails.configuration.action_controller.stubs(:asset_host).returns("http://my.cdn.com")
+        cpp.optimize_urls
+        cpp.html.should match_html '<a href="//my.cdn.com/uploads/default/2/2345678901234567.jpg">Link</a>
+       <img src="//my.cdn.com/uploads/default/1/1234567890123456.jpg"><a href="http://www.google.com">Google</a>
+       <img src="http://foo.bar/image.png">'
+      end
+
+    end
+
+  end
+
+  context ".pull_hotlinked_images" do
 
     let(:post) { build(:post) }
     let(:cpp) { CookedPostProcessor.new(post) }
 
-    it "ensures s3 urls have a default scheme" do
+    it "does not run when crawl images is disabled" do
-      FastImage.stubs(:size)
+      SiteSetting.stubs(:crawl_images).returns(false)
-      cpp.expects(:is_valid_image_uri?).with("http://bucket.s3.aws.amazon.com/image.jpg")
+      Jobs.expects(:cancel_scheduled_job).never
-      cpp.get_size("//bucket.s3.aws.amazon.com/image.jpg")
+      cpp.pull_hotlinked_images
     end
 
-    context "crawl_images is disabled" do
+    context "when crawl_images? is enabled" do
 
-      before { SiteSetting.stubs(:crawl_images?).returns(false) }
+      before { SiteSetting.stubs(:crawl_images).returns(true) }
 
-      it "doesn't call FastImage" do
+      it "runs only when a user updated the post" do
-        FastImage.expects(:size).never
+        post.updated_by = Discourse.system_user
-        cpp.get_size("http://foo.bar/image1.png").should == nil
+        Jobs.expects(:cancel_scheduled_job).never
+        cpp.pull_hotlinked_images
       end
 
-      it "is always allowed to crawl our own images" do
+      context "and the post has been updated by a user" do
-        store = {}
+
-        Discourse.expects(:store).returns(store)
+        before { post.id = 42 }
-        store.expects(:has_been_uploaded?).returns(true)
+
-        FastImage.expects(:size).returns([100, 200])
+        it "ensures only one job is scheduled right after the ninja_edit_window" do
-        cpp.get_size("http://foo.bar/image2.png").should == [100, 200]
+          Jobs.expects(:cancel_scheduled_job).with(:pull_hotlinked_images, post_id: post.id).once
+
+          delay = SiteSetting.ninja_edit_window + 1
+          Jobs.expects(:enqueue_in).with(delay.minutes, :pull_hotlinked_images, post_id: post.id).once
+
+          cpp.pull_hotlinked_images
         end
 
       end
 
-    it "caches the results" do
-      SiteSetting.stubs(:crawl_images?).returns(true)
-      FastImage.expects(:size).returns([200, 400])
-      cpp.get_size("http://foo.bar/image3.png")
-      cpp.get_size("http://foo.bar/image3.png").should == [200, 400]
-    end
-
-  end
-
-  context "is_valid_image_uri?" do
-
-    let(:post) { build(:post) }
-    let(:cpp) { CookedPostProcessor.new(post) }
-
-    it "needs the scheme to be either http or https" do
-      cpp.is_valid_image_uri?("http://domain.com").should   == true
-      cpp.is_valid_image_uri?("https://domain.com").should  == true
-      cpp.is_valid_image_uri?("ftp://domain.com").should    == false
-      cpp.is_valid_image_uri?("ftps://domain.com").should   == false
-      cpp.is_valid_image_uri?("//domain.com").should        == false
-      cpp.is_valid_image_uri?("/tmp/image.png").should      == false
-    end
-
-    it "doesn't throw an exception with a bad URI" do
-      cpp.is_valid_image_uri?("http://do<main.com").should  == nil
     end
 
   end

spec/components/discourse_spec.rb

@@ -67,12 +67,12 @@ describe Discourse do
   context "#store" do
 
     it "returns LocalStore by default" do
-      Discourse.store.should be_a(LocalStore)
+      Discourse.store.should be_a(FileStore::LocalStore)
     end
 
     it "returns S3Store when S3 is enabled" do
       SiteSetting.expects(:enable_s3_uploads?).returns(true)
-      Discourse.store.should be_a(S3Store)
+      Discourse.store.should be_a(FileStore::S3Store)
     end
 
   end

spec/components/file_store/local_store_spec.rb

@@ -1,9 +1,9 @@
 require 'spec_helper'
 require 'file_store/local_store'
 
-describe LocalStore do
+describe FileStore::LocalStore do
 
-  let(:store) { LocalStore.new }
+  let(:store) { FileStore::LocalStore.new }
 
   let(:upload) { build(:upload) }
   let(:uploaded_file) do
@@ -14,13 +14,9 @@ describe LocalStore do
   end
 
   let(:optimized_image) { build(:optimized_image) }
+  let(:avatar) { build(:upload) }
 
-  it "is internal" do
+  describe ".store_upload" do
-    store.internal?.should == true
-    store.external?.should == false
-  end
-
-  describe "store_upload" do
 
     it "returns a relative url" do
       Time.stubs(:now).returns(Time.utc(2013, 2, 17, 12, 0, 0, 0))
@@ -31,7 +27,7 @@ describe LocalStore do
 
   end
 
-  describe "store_optimized_image" do
+  describe ".store_optimized_image" do
 
     it "returns a relative url" do
       store.expects(:copy_file)
@@ -40,7 +36,16 @@ describe LocalStore do
 
   end
 
-  describe "remove_upload" do
+  describe ".store_avatar" do
+
+    it "returns a relative url" do
+      store.expects(:copy_file)
+      store.store_avatar({}, upload, 100).should == "/uploads/default/avatars/e9d/71f/5ee7c92d6d/100.jpg"
+    end
+
+  end
+
+  describe ".remove_upload" do
 
     it "does not delete non uploaded" do
       File.expects(:delete).never
@@ -58,30 +63,67 @@ describe LocalStore do
 
   end
 
-  describe "remove_optimized_image" do
+  describe ".remove_optimized_image" do
+
+    it "deletes the file locally" do
+      File.expects(:delete)
+      oi = OptimizedImage.new
+      oi.stubs(:url).returns("/uploads/default/_optimized/42/253dc8edf9d4ada1.png")
+      store.remove_upload(upload)
+    end
 
   end
 
-  describe "remove_avatar" do
+  describe ".has_been_uploaded?" do
 
-  end
+    it "identifies relatives urls" do
-
-
-  describe "has_been_uploaded?" do
-
-    it "identifies local or relatives urls" do
-      Discourse.expects(:base_url_no_prefix).returns("http://discuss.site.com")
-      store.has_been_uploaded?("http://discuss.site.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
       store.has_been_uploaded?("/uploads/default/42/0123456789ABCDEF.jpg").should == true
     end
 
+    it "identifies local urls" do
+      Discourse.stubs(:base_url_no_prefix).returns("http://discuss.site.com")
+      store.has_been_uploaded?("http://discuss.site.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
+      store.has_been_uploaded?("//discuss.site.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
+    end
+
     it "identifies local urls when using a CDN" do
       Rails.configuration.action_controller.stubs(:asset_host).returns("http://my.cdn.com")
       store.has_been_uploaded?("http://my.cdn.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
+      store.has_been_uploaded?("//my.cdn.com/uploads/default/42/0123456789ABCDEF.jpg").should == true
     end
 
     it "does not match dummy urls" do
       store.has_been_uploaded?("http://domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false
+      store.has_been_uploaded?("//domain.com/uploads/default/42/0123456789ABCDEF.jpg").should == false
+    end
+
+  end
+
+  describe ".absolute_base_url" do
+
+    it "is present" do
+      store.absolute_base_url.should == "http://test.localhost/uploads/default"
+    end
+
+  end
+
+  describe ".relative_base_url" do
+
+    it "is present" do
+      store.relative_base_url.should == "/uploads/default"
+    end
+
+  end
+
+  it "is internal" do
+    store.internal?.should == true
+    store.external?.should == false
+  end
+
+  describe ".absolute_avatar_template" do
+
+    it "is present" do
+      store.absolute_avatar_template(avatar).should == "http://test.localhost/uploads/default/avatars/e9d/71f/5ee7c92d6d/{size}.jpg"
     end
 
   end

spec/components/file_store/s3_store_spec.rb

@@ -2,9 +2,9 @@ require 'spec_helper'
 require 'fog'
 require 'file_store/s3_store'
 
-describe S3Store do
+describe FileStore::S3Store do
 
-  let(:store) { S3Store.new }
+  let(:store) { FileStore::S3Store.new }
 
   let(:upload) { build(:upload) }
   let(:uploaded_file) do
@@ -22,6 +22,14 @@ describe S3Store do
     })
   end
 
+  let(:avatar) { build(:upload) }
+  let(:avatar_file) do
+    ActionDispatch::Http::UploadedFile.new({
+      filename: 'logo-dev.png',
+      tempfile: File.new("#{Rails.root}/spec/fixtures/images/logo-dev.png")
+    })
+  end
+
   before(:each) do
     SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
     SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
@@ -31,14 +39,9 @@ describe S3Store do
 
   after(:each) { Fog.unmock! }
 
-  it "is internal" do
+  describe ".store_upload" do
-    store.external?.should == true
-    store.internal?.should == false
-  end
 
-  describe "store_upload" do
+    it "returns an absolute schemaless url" do
-
-    it "returns a relative url" do
       upload.stubs(:id).returns(42)
       upload.stubs(:extension).returns(".png")
       store.store_upload(uploaded_file, upload).should == "//s3_upload_bucket.s3.amazonaws.com/42e9d71f5ee7c92d6dc9e92ffdad17b8bd49418f98.png"
@@ -46,50 +49,72 @@ describe S3Store do
 
   end
 
-  describe "store_optimized_image" do
+  describe ".store_optimized_image" do
 
-    it "returns a relative url" do
+    it "returns an absolute schemaless url" do
       optimized_image.stubs(:id).returns(42)
       store.store_optimized_image(optimized_image_file, optimized_image).should == "//s3_upload_bucket.s3.amazonaws.com/4286f7e437faa5a7fce15d1ddcb9eaeaea377667b8_100x200.png"
     end
 
   end
 
-  describe "remove_upload" do
+  describe ".store_avatar" do
 
-    it "does not delete non uploaded file" do
+    it "returns an absolute schemaless url" do
-      store.expects(:remove).never
+      avatar.stubs(:id).returns(42)
-      upload = Upload.new
+      store.store_avatar(avatar_file, avatar, 100).should == "//s3_upload_bucket.s3.amazonaws.com/avatars/e9d71f5ee7c92d6dc9e92ffdad17b8bd49418f98/100.jpg"
-      upload.stubs(:url).returns("//other_bucket.s3.amazonaws.com/42.png")
-      store.remove_upload(upload)
     end
 
-    it "deletes the file on s3" do
+  end
-      store.expects(:remove)
+
-      upload = Upload.new
+  describe ".remove_upload" do
-      upload.stubs(:url).returns("//s3_upload_bucket.s3.amazonaws.com/42.png")
+
+    it "calls remove_file with the url" do
+      store.expects(:remove_file).with(upload.url)
       store.remove_upload(upload)
     end
 
   end
 
-  describe "remove_optimized_image" do
+  describe ".remove_optimized_image" do
+
+    it "calls remove_file with the url" do
+      store.expects(:remove_file).with(optimized_image.url)
+      store.remove_optimized_image(optimized_image)
+    end
 
   end
 
-  describe "remove_avatar" do
+  describe ".has_been_uploaded?" do
-
-  end
-
-  describe "has_been_uploaded?" do
 
     it "identifies S3 uploads" do
-      SiteSetting.stubs(:enable_s3_uploads).returns(true)
       store.has_been_uploaded?("//s3_upload_bucket.s3.amazonaws.com/1337.png").should == true
     end
 
     it "does not match other s3 urls" do
-      store.has_been_uploaded?("//s3.amazonaws.com/Bucket/1337.png").should == false
+      store.has_been_uploaded?("//s3.amazonaws.com/s3_upload_bucket/1337.png").should == false
+      store.has_been_uploaded?("//s4_upload_bucket.s3.amazonaws.com/1337.png").should == false
+    end
+
+  end
+
+  describe ".absolute_base_url" do
+
+    it "returns a lowercase schemaless absolute url" do
+      store.absolute_base_url.should == "//s3_upload_bucket.s3.amazonaws.com"
+    end
+
+  end
+
+  it "is external" do
+    store.external?.should == true
+    store.internal?.should == false
+  end
+
+  describe ".absolute_avatar_template" do
+
+    it "is present" do
+      store.absolute_avatar_template(avatar).should == "//s3_upload_bucket.s3.amazonaws.com/avatars/e9d71f5ee7c92d6dc9e92ffdad17b8bd49418f98/{size}.jpg"
     end
 
   end

spec/components/pretty_text_spec.rb

@@ -177,20 +177,4 @@ describe PrettyText do
     end
   end
 
-  describe "apply cdn" do
-    it "should detect bare links to images and apply a CDN" do
-      PrettyText.apply_cdn("<a href='/hello.png'>hello</a><img src='/a.jpeg'>","http://a.com").should ==
-        "<a href=\"http://a.com/hello.png\">hello</a><img src=\"http://a.com/a.jpeg\">"
-    end
-
-    it "should not touch non images" do
-      PrettyText.apply_cdn("<a href='/hello'>hello</a>","http://a.com").should ==
-        "<a href=\"/hello\">hello</a>"
-    end
-
-    it "should not touch schemaless links" do
-      PrettyText.apply_cdn("<a href='//hello'>hello</a>","http://a.com").should ==
-        "<a href=\"//hello\">hello</a>"
-    end
-  end
 end

spec/fabricators/post_fabricator.rb

@@ -59,13 +59,29 @@ Fabricator(:post_with_unsized_images, from: :post) do
 end
 
 Fabricator(:post_with_image_url, from: :post) do
-  cooked '<img src="http://foo.bar/image.png">'
+  cooked '<img src="http://foo.bar/image.png" width="50" height="42">'
 end
 
 Fabricator(:post_with_large_image, from: :post) do
   cooked '<img src="/uploads/default/1/1234567890123456.jpg">'
 end
 
+Fabricator(:post_with_uploads, from: :post) do
+  cooked '
+<a href="/uploads/default/2/2345678901234567.jpg">Link</a>
+<img src="/uploads/default/1/1234567890123456.jpg">
+'
+end
+
+Fabricator(:post_with_uploads_and_links, from: :post) do
+  cooked '
+<a href="/uploads/default/2/2345678901234567.jpg">Link</a>
+<img src="/uploads/default/1/1234567890123456.jpg">
+<a href="http://www.google.com">Google</a>
+<img src="http://foo.bar/image.png">
+'
+end
+
 Fabricator(:post_with_external_links, from: :post) do
   user
   topic

spec/models/optimized_image_spec.rb

@@ -4,53 +4,88 @@ describe OptimizedImage do
 
   it { should belong_to :upload }
 
-  let(:upload) { Fabricate(:upload) }
+  let(:upload) { build(:upload) }
-  let(:oi) { OptimizedImage.create_for(upload, 100, 200) }
+
+  before { upload.id = 42 }
 
   describe ".create_for" do
 
-    before { ImageSorcery.any_instance.expects(:convert).returns(true) }
+    context "when using an internal store" do
 
-    describe "internal store" do
+      let(:store) { FakeInternalStore.new }
+      before { Discourse.stubs(:store).returns(store) }
+
+      context "when an error happened while generatign the thumbnail" do
+
+        before { ImageSorcery.any_instance.stubs(:convert).returns(false) }
+
+        it "returns nil" do
+          OptimizedImage.create_for(upload, 100, 200).should be_nil
+        end
+
+      end
+
+      context "when the thumbnail is properly generated" do
+
+        before { ImageSorcery.any_instance.stubs(:convert).returns(true) }
+
+        it "does not download a copy of the original image" do
+          store.expects(:download).never
+          OptimizedImage.create_for(upload, 100, 200)
+        end
+
+        it "closes and removes the tempfile" do
+          Tempfile.any_instance.expects(:close!)
+          OptimizedImage.create_for(upload, 100, 200)
+        end
 
         it "works" do
-        Tempfile.any_instance.expects(:close!)
+          oi = OptimizedImage.create_for(upload, 100, 200)
           oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709"
           oi.extension.should == ".jpg"
           oi.width.should == 100
           oi.height.should == 200
-        oi.url.should == "/uploads/default/_optimized/da3/9a3/ee5e6b4b0d_100x200.jpg"
+          oi.url.should == "/internally/stored/optimized/image.jpg"
+        end
+
       end
 
     end
 
     describe "external store" do
 
-      require 'file_store/s3_store'
+      let(:store) { FakeExternalStore.new }
-      require 'fog'
+      before { Discourse.stubs(:store).returns(store) }
 
-      let(:store) { S3Store.new }
+      context "when an error happened while generatign the thumbnail" do
 
-      before do
+        before { ImageSorcery.any_instance.stubs(:convert).returns(false) }
-        Discourse.stubs(:store).returns(store)
+
-        SiteSetting.stubs(:s3_upload_bucket).returns("S3_Upload_Bucket")
+        it "returns nil" do
-        SiteSetting.stubs(:s3_access_key_id).returns("s3_access_key_id")
+          OptimizedImage.create_for(upload, 100, 200).should be_nil
-        SiteSetting.stubs(:s3_secret_access_key).returns("s3_secret_access_key")
+        end
-        Fog.mock!
+
+      end
+
+      context "when the thumbnail is properly generated" do
+
+        before { ImageSorcery.any_instance.stubs(:convert).returns(true) }
+
+        it "downloads a copy of the original image" do
+          Tempfile.any_instance.expects(:close!).twice
+          store.expects(:download).with(upload).returns(Tempfile.new(["discourse-external", ".jpg"]))
+          OptimizedImage.create_for(upload, 100, 200)
         end
 
         it "works" do
-        # fake downloaded file
+          oi = OptimizedImage.create_for(upload, 100, 200)
-        downloaded_file = {}
-        downloaded_file.expects(:path).returns("/path/to/fake.png")
-        downloaded_file.expects(:close!)
-        store.expects(:download).returns(downloaded_file)
-        # assertions
           oi.sha1.should == "da39a3ee5e6b4b0d3255bfef95601890afd80709"
-        oi.extension.should == ".png"
+          oi.extension.should == ".jpg"
           oi.width.should == 100
           oi.height.should == 200
-        oi.url.should =~ /^\/\/s3_upload_bucket.s3.amazonaws.com\/[0-9a-f]+_100x200.png/
+          oi.url.should == "/externally/stored/optimized/image.jpg"
+        end
+
       end
 
     end
@@ -58,3 +93,44 @@ describe OptimizedImage do
   end
 
 end
+
+class FakeInternalStore
+
+  def internal?
+    true
+  end
+
+  def external?
+    !internal?
+  end
+
+  def path_for(upload)
+    upload.url
+  end
+
+  def store_optimized_image(file, optimized_image)
+    "/internally/stored/optimized/image#{optimized_image.extension}"
+  end
+
+end
+
+class FakeExternalStore
+
+  def external?
+    true
+  end
+
+  def internal?
+    !external?
+  end
+
+  def store_optimized_image(file, optimized_image)
+    "/externally/stored/optimized/image#{optimized_image.extension}"
+  end
+
+  def download(upload)
+    extension = File.extname(upload.original_filename)
+    Tempfile.new(["discourse-s3", extension])
+  end
+
+end

spec/models/upload_spec.rb

@@ -46,18 +46,10 @@ describe Upload do
       upload.create_thumbnail!(100, 100)
     end
 
-    it "does not create another thumbnail" do
-      SiteSetting.expects(:create_thumbnails?).returns(true)
-      upload.expects(:has_thumbnail?).returns(true)
-      OptimizedImage.expects(:create_for).never
-      upload.create_thumbnail!(100, 100)
-    end
-
     it "creates a thumbnail" do
       upload = Fabricate(:upload)
       thumbnail = Fabricate(:optimized_image, upload: upload)
       SiteSetting.expects(:create_thumbnails?).returns(true)
-      upload.expects(:has_thumbnail?).returns(false)
       OptimizedImage.expects(:create_for).returns(thumbnail)
       upload.create_thumbnail!(100, 100)
       upload.reload
@@ -66,7 +58,7 @@ describe Upload do
 
   end
 
-  context ".create_for" do
+  context "#create_for" do
 
     it "does not create another upload if it already exists" do
       Upload.expects(:where).with(sha1: image_sha1).returns([upload])

spec/models/user_action_spec.rb

@@ -210,7 +210,6 @@ describe UserAction do
     end
   end
 
-
   describe 'private messages' do
 
     let(:user) do