Move letter avatars out of upload system

FIX: S3 issues around system avatars
FIX: reduced backup file size

app/assets/javascripts/discourse/helpers/application_helpers.js

@@ -200,7 +200,7 @@ Handlebars.registerHelper('avatar', function(user, options) {
     }
 
     // this is simply done to ensure we cache images correctly
-    var uploadedAvatarId = Em.get(user, 'uploaded_avatar_id') || Em.get(user, 'user.uploaded_avatar_id') || "_1";
+    var uploadedAvatarId = Em.get(user, 'uploaded_avatar_id') || Em.get(user, 'user.uploaded_avatar_id');
     var avatarTemplate = Discourse.User.avatarTemplate(username,uploadedAvatarId);
 
     return new Handlebars.SafeString(Discourse.Utilities.avatarImg({

app/assets/javascripts/discourse/models/user.js

@@ -417,7 +417,24 @@ Discourse.User = Discourse.Model.extend({
 Discourse.User.reopenClass(Discourse.Singleton, {
 
   avatarTemplate: function(username, uploadedAvatarId){
-    var url = Discourse.getURL("/user_avatar/" + Discourse.BaseUrl + "/" + username.toLowerCase() + "/{size}/" + uploadedAvatarId + ".png");
+    var url;
+    if(uploadedAvatarId){
+      url = "/user_avatar/" +
+            Discourse.BaseUrl +
+            "/" +
+            username.toLowerCase() +
+            "/{size}/" +
+            uploadedAvatarId + ".png";
+
+
+    } else {
+      url = "/letter_avatar/" +
+            username.toLowerCase() +
+            "/{size}/" +
+            Discourse.LetterAvatarVersion + ".png";
+    }
+
+    url = Discourse.getURL(url);
     if(Discourse.CDN){
       url = Discourse.CDN + url;
     }

app/controllers/application_controller.rb

@@ -6,6 +6,7 @@ require_dependency 'archetype'
 require_dependency 'rate_limiter'
 require_dependency 'crawler_detection'
 require_dependency 'json_error'
+require_dependency 'letter_avatar'
 
 class ApplicationController < ActionController::Base
   include CurrentUser

app/controllers/user_avatars_controller.rb

@@ -3,7 +3,7 @@ require_dependency 'letter_avatar'
 class UserAvatarsController < ApplicationController
   DOT = Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==")
 
-  skip_before_filter :check_xhr, :verify_authenticity_token, only: :show
+  skip_before_filter :check_xhr, :verify_authenticity_token, only: [:show, :show_letter]
 
   def refresh_gravatar
 
@@ -20,6 +20,20 @@ class UserAvatarsController < ApplicationController
     end
   end
 
+  def show_letter
+    params.require(:username)
+    params.require(:version)
+    params.require(:size)
+
+    if params[:version].to_i > LetterAvatar::VERSION
+      return render_dot
+    end
+
+    image = LetterAvatar.generate(params[:username].to_s, params[:size].to_i)
+    expires_in 1.year, public: true
+    send_file image, disposition: nil
+  end
+
   def show
     # we need multisite support to keep a single origin pull for CDNs
     RailsMultisite::ConnectionManagement.with_hostname(params[:hostname]) do

app/jobs/scheduled/create_missing_avatars.rb

@@ -2,16 +2,6 @@ module Jobs
   class CreateMissingAvatars < Jobs::Scheduled
     every 1.hour
     def execute(args)
-      UserAvatar
-        .where("system_upload_id IS NULL OR system_avatar_version != ?", UserAvatar::SYSTEM_AVATAR_VERSION)
-        .find_each do |a|
-          if a.user
-            a.update_system_avatar!
-          else
-            Rails.logger.warn("Detected stray avatar for avatar_user_id #{a.id}")
-          end
-      end
-
       # backfill in batches 5000 an hour
       UserAvatar.where(last_gravatar_download_attempt: nil).includes(:user)
           .order("users.last_posted_at desc")

app/jobs/scheduled/periodical_updates.rb

@@ -28,10 +28,7 @@ module Jobs
       Topic.auto_close
 
       # Forces rebake of old posts where needed, as long as no system avatars need updating
-      unless UserAvatar
-        .where("last_gravatar_download_attempt IS NULL OR system_upload_id IS NULL OR system_avatar_version != ?", UserAvatar::SYSTEM_AVATAR_VERSION)
-        .limit(1)
-        .first
+      unless UserAvatar.where("last_gravatar_download_attempt IS NULL").limit(1).first
         Post.rebake_old(250)
       end
     end

app/models/optimized_image.rb

@@ -67,8 +67,6 @@ class OptimizedImage < ActiveRecord::Base
     end
   end
 
-  protected
-
   def self.resize(from, to, width, height)
     instructions = %W{
       #{from}

app/models/user.rb

@@ -8,6 +8,7 @@ require_dependency 'post_destroyer'
 require_dependency 'user_name_suggester'
 require_dependency 'pretty_text'
 require_dependency 'url_helper'
+require_dependency 'letter_avatar'
 
 class User < ActiveRecord::Base
   include Roleable
@@ -354,11 +355,16 @@ class User < ActiveRecord::Base
   end
 
   def self.avatar_template(username,uploaded_avatar_id)
-    id = uploaded_avatar_id || -1
+    return letter_avatar_template(username) if !uploaded_avatar_id
+    id = uploaded_avatar_id
     username ||= ""
     "/user_avatar/#{RailsMultisite::ConnectionManagement.current_hostname}/#{username.downcase}/{size}/#{id}.png"
   end
 
+  def self.letter_avatar_template(username)
+    "/letter_avatar/#{username.downcase}/{size}/#{LetterAvatar::VERSION}.png"
+  end
+
   def avatar_template
     self.class.avatar_template(username,uploaded_avatar_id)
   end
@@ -627,16 +633,8 @@ class User < ActiveRecord::Base
       gravatar_downloaded = avatar.gravatar_upload_id
     end
 
-    if SiteSetting.enable_system_avatars?
-      avatar.update_system_avatar! if !avatar.system_upload_id ||
-                                      username_changed? ||
-                                      avatar.system_avatar_version != UserAvatar::SYSTEM_AVATAR_VERSION
-    end
-
-    desired_avatar_id = avatar.gravatar_upload_id || avatar.system_upload_id
-
-    if (!self.uploaded_avatar_id || gravatar_downloaded) && desired_avatar_id
-      self.update_column(:uploaded_avatar_id, desired_avatar_id)
+    if (!self.uploaded_avatar_id && gravatar_downloaded)
+      self.update_column(:uploaded_avatar_id, avatar.gravatar_upload_id)
     end
 
   end

app/models/user_avatar.rb

@@ -2,34 +2,13 @@ require_dependency 'letter_avatar'
 
 class UserAvatar < ActiveRecord::Base
   MAX_SIZE = 240
-  SYSTEM_AVATAR_VERSION = 1
 
   belongs_to :user
-  belongs_to :system_upload, class_name: 'Upload', dependent: :destroy
   belongs_to :gravatar_upload, class_name: 'Upload', dependent: :destroy
   belongs_to :custom_upload, class_name: 'Upload', dependent: :destroy
 
   def contains_upload?(id)
-    system_upload_id == id || gravatar_upload_id == id || custom_upload_id == id
-  end
-
-  # updates the letter based avatar
-  def update_system_avatar!
-    old_id = nil
-    if system_upload
-      old_id = system_upload_id
-      system_upload.destroy!
-    end
-
-    file = File.open(LetterAvatar.generate(user.username, MAX_SIZE, cache: false), "r")
-    self.system_upload = Upload.create_for(user_id, file, "avatar.png", file.size)
-    self.system_avatar_version = SYSTEM_AVATAR_VERSION
-
-    if old_id == user.uploaded_avatar_id
-      user.update_column(:uploaded_avatar_id, system_upload_id)
-    end
-
-    save!
+    gravatar_upload_id == id || custom_upload_id == id
   end
 
   def update_gravatar!
@@ -63,13 +42,11 @@ end
 #
 #  id                             :integer          not null, primary key
 #  user_id                        :integer          not null
-#  system_upload_id               :integer
 #  custom_upload_id               :integer
 #  gravatar_upload_id             :integer
 #  last_gravatar_download_attempt :datetime
 #  created_at                     :datetime
 #  updated_at                     :datetime
-#  system_avatar_version          :integer          default(0)
 #
 # Indexes
 #

app/views/common/_discourse_javascript.html.erb

@@ -35,6 +35,7 @@
   Discourse.BaseUri = '<%= Discourse::base_uri "/" %>';
   Discourse.Environment = '<%= Rails.env %>';
   Discourse.SiteSettings = PreloadStore.get('siteSettings');
+  Discourse.LetterAvatarVersion = <%= LetterAvatar::VERSION %>;
   Discourse.Router = Ember.Router.extend({ location: 'discourse-location' });
   Discourse.Route.mapRoutes();
   Discourse.start();

config/initializers/06-mini_profiler.rb

@@ -23,6 +23,7 @@ if defined?(Rack::MiniProfiler)
     (path !~ /topics\/timings/) &&
     (path !~ /assets/) &&
     (path !~ /\/user_avatar\//) &&
+    (path !~ /\/letter_avatar\//) &&
     (path !~ /qunit/) &&
     (path !~ /srv\/status/) &&
     (path !~ /commits-widget/) &&

config/routes.rb

@@ -212,6 +212,8 @@ Discourse::Application.routes.draw do
   delete "users/:username" => "users#destroy", constraints: {username: USERNAME_ROUTE_FORMAT}
 
   post "user_avatar/:username/refresh_gravatar" => "user_avatars#refresh_gravatar"
+  get "letter_avatar/:username/:size/:version.png" => "user_avatars#show_letter",
+      format: false, constraints: {hostname: /[\w\.-]+/}
   get "user_avatar/:hostname/:username/:size/:version.png" => "user_avatars#show",
       format: false, constraints: {hostname: /[\w\.-]+/}
 

db/migrate/20140530002535_remove_system_avatars_from_user_avatars.rb

@@ -0,0 +1,44 @@
+# This takes all the system avatars out of the upload system, saving us a huge
+# amount of space on backups
+class RemoveSystemAvatarsFromUserAvatars < ActiveRecord::Migration
+  def up
+    execute "UPDATE users SET uploaded_avatar_id = NULL WHERE uploaded_avatar_id IN (
+      SELECT system_upload_id FROM user_avatars
+    )"
+
+    # normally we dont reach into the object model, but we have to here.
+    # otherwise we will wait a real long time for uploads to go away
+    skip = -1
+    while skip=destroy_system_avatar_batch(skip) do
+      puts "Destroyed up to id: #{skip}"
+    end
+
+    remove_column :user_avatars, :system_upload_id
+    remove_column :user_avatars, :system_avatar_version
+  end
+
+  def destroy_system_avatar_batch(skip)
+    initial = skip
+
+    Upload.where('id IN (SELECT system_upload_id FROM user_avatars) AND id > ?', skip)
+        .order(:id)
+        .limit(500)
+        .each do |upload|
+      skip = upload.id
+      begin
+        upload.destroy
+      rescue
+        Rails.logger.warn "Could not destroy system avatar #{upload.id}"
+      end
+    end
+
+    skip == initial ? nil : skip
+  rescue
+    Rails.logger.warn "Could not destroy system avatars, skipping"
+    nil
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/letter_avatar.rb

@@ -1,52 +1,73 @@
 class LetterAvatar
-  class<<self
 
+  # BUMP UP if avatar algorithm changes
+  VERSION = 1
+
+  # Largest avatar generated, one day when pixel ratio hit 3
+  # we will need to change this
   FULLSIZE = 240
 
+  class<<self
+
+    class Identity
+      attr_accessor :color, :letter
+
+      def self.from_username(username)
+        identity = new
+        identity.color = LetterAvatar::COLORS[
+            Digest::MD5.hexdigest(username)[0...15].to_i(16) % LetterAvatar::COLORS.length
+        ]
+        identity.letter = username[0].upcase
+
+        identity
+      end
+    end
+
+
     def cache_path
-      'tmp/letter_avatars'
+      "tmp/letter_avatars/#{VERSION}"
     end
 
     def generate(username, size, opts = nil)
 
+      identity = Identity.from_username(username)
+
       cache = true
       cache = false if opts && opts[:cache] == false
 
       size = FULLSIZE if size > FULLSIZE
-      filename = cached_path(username, size)
+      filename = cached_path(identity, size)
 
       if cache && File.exists?(filename)
         return filename
       end
 
-      fullsize = fullsize_path(username)
+      fullsize = fullsize_path(identity)
       if !cache || !File.exists?(fullsize)
-        generate_fullsize(username)
+        generate_fullsize(identity)
       end
 
-      resize(fullsize, filename, size)
+      OptimizedImage.resize(fullsize, filename, size, size)
+      filename
     end
 
-    def cached_path(username, size)
-      dir = "#{cache_path}/#{username}"
+    def cached_path(identity, size)
+      dir = "#{cache_path}/#{identity.letter}/#{identity.color.join("_")}"
       FileUtils.mkdir_p(dir)
 
       "#{dir}/#{size}.png"
     end
 
-    def fullsize_path(username)
-      cached_path(username, FULLSIZE)
+    def fullsize_path(identity)
+      cached_path(identity, FULLSIZE)
     end
 
-    def resize(from, to, size)
-      `convert #{from} -resize #{size}x#{size} #{to}`
-      to
-    end
+    def generate_fullsize(identity)
 
-    def generate_fullsize(username)
+      color = identity.color
+      letter = identity.letter
 
-      filename = fullsize_path(username)
-      color = colors[Digest::MD5.hexdigest(username)[0...15].to_i(16) % 216]
+      filename = fullsize_path(identity)
       stroke = darken(color, 0.8)
 
       instructions = %W{
@@ -58,7 +79,7 @@ class LetterAvatar
         -font 'Helvetica'
         -stroke #{to_rgb(stroke)}
         -strokewidth 2
-        -annotate -5+25 '#{username[0].upcase}'
+        -annotate -5+25 '#{letter}'
         '#{filename}'
       }
 
@@ -77,15 +98,15 @@ class LetterAvatar
       r,g,b = color
       "'rgb(#{r},#{g},#{b})'"
     end
+  end
 
-    # palette of 216 optimally disctinct colors
+  # palette of optimally disctinct colors
   # cf. http://tools.medialab.sciences-po.fr/iwanthue/index.php
   # parameters used:
   #   - H: 0 - 360
   #   - C: 0 - 2
   #   - L: 0.75 - 1.5
-    def colors
-       [[198,125,40],
+  COLORS = [[198,125,40],
       [61,155,243],
       [74,243,75],
       [238,89,166],
@@ -302,5 +323,3 @@ class LetterAvatar
       [218,105,73],
       [165,241,221]]
 end
-  end
-end

lib/tasks/avatars.rake

@@ -1,15 +1,3 @@
-desc "Rebuild all system avatars"
-task "avatars:rebuild_system" => :environment do
-  i = 0
-  puts "Regenerating system avatars"
-  puts
-  UserAvatar.find_each do |a|
-    a.update_system_avatar!
-    putc "." if (i+=1)%10 == 0
-  end
-  puts
-end
-
 desc "Refresh all avatars (download missing gravatars, refresh system)"
 task "avatars:refresh" => :environment do
   i = 0

lib/tasks/qunit.rake

@@ -25,6 +25,7 @@ task "qunit:test" => :environment do
   end
 
   unless pid = fork
+    Discourse.after_fork
     Rack::Server.start(:config => "config.ru",
                        :AccessLog => [],
                        :Port => port)

spec/models/user_avatar_spec.rb

@@ -6,22 +6,6 @@ describe UserAvatar do
     user.create_user_avatar!
   }
 
-  it 'can generate a system avatar' do
-    avatar.update_system_avatar!
-    avatar.system_upload.should_not be_nil
-  end
-
-  it 'corrects old system avatars on refresh' do
-    upload = Fabricate(:upload)
-    user = Fabricate(:user, uploaded_avatar_id: upload.id)
-    avatar = UserAvatar.create!(user_id: user.id, system_upload_id: upload.id)
-    avatar.update_system_avatar!
-
-    user.reload
-    user.uploaded_avatar_id.should_not == upload.id
-    avatar.system_upload_id.should == user.uploaded_avatar_id
-  end
-
   it 'can update gravatars' do
     temp = Tempfile.new('test')
     temp.binmode

spec/models/user_spec.rb

@@ -978,7 +978,7 @@ describe User do
     let(:user) { build(:user, username: 'Sam') }
 
     it "returns a 45-pixel-wide avatar" do
-      user.small_avatar_url.should == "//test.localhost/user_avatar/test.localhost/sam/45/-1.png"
+      user.small_avatar_url.should == "//test.localhost/letter_avatar/sam/45/#{LetterAvatar::VERSION}.png"
     end
 
   end
@@ -1139,8 +1139,8 @@ describe User do
       SiteSetting.enable_system_avatars = true
       u = User.create!(username: "bob", email: "bob@bob.com")
       u.reload
-      u.user_avatar.system_upload_id.should == u.uploaded_avatar_id
-      u.uploaded_avatar_id.should_not == nil
+      u.uploaded_avatar_id.should == nil
+      u.avatar_template.should == "/letter_avatar/bob/{size}/#{LetterAvatar::VERSION}.png"
     end
   end
 
@@ -1178,9 +1178,6 @@ describe User do
                              body: png )
 
       user = User.create!(username: "bob", name: "bob", email: "a@a.com")
-      user.create_user_avatar
-      user.user_avatar.update_system_avatar!
-      user.save
       user.reload
 
       SiteSetting.automatically_download_gravatars = true
@@ -1190,6 +1187,13 @@ describe User do
       user.reload
 
       user.user_avatar.gravatar_upload_id.should == user.uploaded_avatar_id
+
+      user.uploaded_avatar_id = nil
+      user.save
+      user.refresh_avatar
+
+      user.reload
+      user.uploaded_avatar_id.should == nil
     end
   end