Merge branch 'avatar-cropping' into 'master'

Fix avatar stretching by providing a cropping feature

Originally by @nesk at https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2773

Fixes #7959

See merge request !2951

CHANGELOG

@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.6.0 (unreleased)
   - Improve the formatting for the user page bio (Connor Shea)
+  - Fix avatar stretching by providing a cropping feature (Johann Pardanaud)
 v 8.5.1
   - Fix group projects styles

Gemfile

@@ -77,6 +77,9 @@ gem "haml-rails", '~> 0.9.0'
 # Files attachments
 gem "carrierwave", '~> 0.9.0'
+# Image editing
+gem "mini_magick", '~> 4.4.0'
+
 # Drag and Drop UI
 gem 'dropzonejs-rails', '~> 0.7.1'

Gemfile.lock

@@ -468,6 +468,7 @@ GEM
     method_source (0.8.2)
     mime-types (1.25.1)
     mimemagic (0.3.0)
+    mini_magick (4.4.0)
     mini_portile2 (2.0.0)
     minitest (5.7.0)
     mousetrap-rails (1.4.6)
@@ -955,6 +956,7 @@ DEPENDENCIES
   loofah (~> 2.0.3)
   mail_room (~> 0.6.1)
   method_source (~> 0.8)
+  mini_magick (~> 4.4.0)
   minitest (~> 5.7.0)
   mousetrap-rails (~> 1.4.6)
   mysql2 (~> 0.3.16)

app/assets/javascripts/application.js.coffee

@@ -44,6 +44,7 @@
 #= require jquery.nicescroll
 #= require_tree .
 #= require fuzzaldrin-plus
+#= require cropper.js
 window.slugify = (text) ->
   text.replace(/[^-a-zA-Z0-9]+/g, '_').toLowerCase()

app/assets/javascripts/profile.js.coffee

@@ -16,11 +16,50 @@ class @Profile
     $('.update-notifications').on 'ajax:complete', ->
       $(this).find('.btn-save').enable()
-    $('.js-choose-user-avatar-button').bind "click", ->
-      form = $(this).closest("form")
-      form.find(".js-user-avatar-input").click()
-    $('.js-user-avatar-input').bind "change", ->
+    # Avatar management
+
+    $avatarInput = $('.js-user-avatar-input')
+    $filename = $('.js-avatar-filename')
+    $modalCrop = $('.modal-profile-crop')
+    $modalCropImg = $('.modal-profile-crop-image')
+
+    $('.js-choose-user-avatar-button').on "click", ->
+      $form = $(this).closest("form")
+      $form.find(".js-user-avatar-input").click()
+
+    $modalCrop.on 'shown.bs.modal', ->
+      setTimeout ( -> # The cropper must be asynchronously initialized
+        $modalCropImg.cropper
+          aspectRatio: 1
+          modal: false
+          scalable: false
+          rotatable: false
+          zoomable: false
+
+          crop: (event) ->
+            ['x', 'y'].forEach (key) ->
+              $("#user_avatar_crop_#{key}").val(Math.floor(event[key]))
+            $("#user_avatar_crop_size").val(Math.floor(event.width))
+      ), 0
+
+    $modalCrop.on 'hidden.bs.modal', ->
+      $modalCropImg.attr('src', '').cropper('destroy')
+      $avatarInput.val('')
+      $filename.text($filename.data('label'))
+    $('.js-upload-user-avatar').on 'click', ->
+      $('.edit_user').submit()
+
+    $avatarInput.on "change", ->
       form = $(this).closest("form")
       filename = $(this).val().replace(/^.*[\\\/]/, '')
-      form.find(".js-avatar-filename").text(filename)
+      $filename.data('label', $filename.text()).text(filename)
+
+      reader = new FileReader
+
+      reader.onload = (event) ->
+        $modalCrop.modal('show')
+        $modalCropImg.attr('src', event.target.result)
+
+      fileData = reader.readAsDataURL(this.files[0])
+

app/assets/stylesheets/application.scss

@@ -9,6 +9,7 @@
  *= require_self
  *= require dropzone/basic
  *= require cal-heatmap
+ *= require cropper.css
 */
 /*

app/assets/stylesheets/framework/mixins.scss

@@ -41,6 +41,12 @@
   transition: $transition;
 }
+@mixin transform($transform) {
+  -webkit-transform: $transform;
+  -ms-transform: $transform;
+  transform: $transform;
+}
+
 /**
  * Prefilled mixins
  * Mixins with fixed values

app/assets/stylesheets/pages/profile.scss

@@ -78,3 +78,39 @@
   max-width: 750px;
   margin: auto;
 }
+
+.modal-profile-crop {
+  .modal-dialog {
+    width: 500px;
+  }
+
+  .modal-body {
+    p {
+      display: table;
+      margin: auto;
+      overflow: hidden;
+    }
+
+    img {
+      display: block;
+      max-width: 400px;
+      max-height: 400px;
+    }
+
+    .cropper-bg {
+      background: none;
+    }
+
+    .cropper-crop-box {
+      box-sizing: content-box;
+      border: 999px solid transparentize(#ccc, 0.5);
+      @include transform(translate(-999px, -999px));
+    }
+  }
+}
+
+@media (max-width: 520px) {
+  .modal-profile-crop .modal-dialog {
+    width: auto;
+  }
+}

app/controllers/profiles_controller.rb

@@ -65,6 +65,9 @@ class ProfilesController < Profiles::ApplicationController
   def user_params
     params.require(:user).permit(
+      :avatar_crop_x,
+      :avatar_crop_y,
+      :avatar_crop_size,
       :avatar,
       :bio,
       :email,

app/models/user.rb

@@ -98,6 +98,9 @@ class User < ActiveRecord::Base
   # Virtual attribute for authenticating by either username or email
   attr_accessor :login
+  # Virtual attributes to define avatar cropping
+  attr_accessor :avatar_crop_x, :avatar_crop_y, :avatar_crop_size
+
   #
   # Relations
   #
@@ -163,6 +166,11 @@ class User < ActiveRecord::Base
   validate :owns_public_email, if: ->(user) { user.public_email_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
+  validates :avatar_crop_x, :avatar_crop_y, :avatar_crop_size,
+    numericality: { only_integer: true },
+    presence: true,
+    if: ->(user) { user.avatar? }
+
   before_validation :generate_password, on: :create
   before_validation :restricted_signup_domains, on: :create
   before_validation :sanitize_attrs

app/uploaders/avatar_uploader.rb

@@ -2,11 +2,22 @@
 class AvatarUploader < CarrierWave::Uploader::Base
   include UploaderHelper
+  include CarrierWave::MiniMagick
   storage :file
   after :store, :reset_events_cache
+  process :cropper
+
+  def cropper
+    return unless model.respond_to?(:avatar_crop_size) && model.valid?
+
+    manipulate! do |img|
+      img.crop "#{model.avatar_crop_size}x#{model.avatar_crop_size}+#{model.avatar_crop_x}+#{model.avatar_crop_y}"
+    end
+  end
+
   def store_dir
     "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}"
   end

app/views/profiles/show.html.haml

@@ -90,6 +90,9 @@
              
             %span.file_name.js-avatar-filename File name...
             = f.file_field :avatar, class: "js-user-avatar-input hidden"
+            = f.hidden_field :avatar_crop_x
+            = f.hidden_field :avatar_crop_y
+            = f.hidden_field :avatar_crop_size
             .light The maximum file size allowed is 200KB.
             - if @user.avatar?
               %hr
@@ -99,3 +102,19 @@
   .form-actions
     = f.submit 'Save changes', class: "btn btn-success"
     = link_to "Cancel", user_path(current_user), class: "btn btn-cancel"
+
+.modal.modal-profile-crop
+  .modal-dialog
+    .modal-content
+      .modal-header
+        %button.close{type: 'button', data: {dismiss: 'modal'}}
+          %span
+            ×
+        %h4.modal-title
+          Crop your new profile picture
+      .modal-body
+        %p
+          %img.modal-profile-crop-image
+      .modal-footer
+        %button.btn.btn-primary.js-upload-user-avatar{:type => "button"}
+          Set new profile picture

features/steps/profile/profile.rb

@@ -27,9 +27,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   end
   step 'I change my avatar' do
-    attach_file(:user_avatar, File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif'))
-    click_button "Save changes"
-    @user.reload
+    attach_avatar
   end
   step 'I should see new avatar' do
@@ -42,9 +40,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   end
   step 'I have an avatar' do
-    attach_file(:user_avatar, File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif'))
-    click_button "Save changes"
-    @user.reload
+    attach_avatar
   end
   step 'I remove my avatar' do
@@ -233,4 +229,16 @@ class Spinach::Features::Profile < Spinach::FeatureSteps
   step "I see that application is removed" do
     expect(page.find(".oauth-applications")).not_to have_content "test_changed"
   end
+
+  def attach_avatar
+    attach_file :user_avatar, Rails.root.join(*%w(spec fixtures banana_sample.gif))
+
+    page.find('#user_avatar_crop_x',    visible: false).set('0')
+    page.find('#user_avatar_crop_y',    visible: false).set('0')
+    page.find('#user_avatar_crop_size', visible: false).set('256')
+
+    click_button "Save changes"
+
+    @user.reload
+  end
 end

spec/controllers/namespaces_controller_spec.rb

 require 'spec_helper'
 describe NamespacesController do
-  let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+  let!(:user) { create(:user, :with_avatar) }
   describe "GET show" do
     context "when the namespace belongs to a user" do

spec/controllers/profiles/avatars_controller_spec.rb

 require 'spec_helper'
 describe Profiles::AvatarsController do
-  let(:user)    { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png")) }
+  let(:user)    { create(:user, :with_avatar) }
   before do
     sign_in(user)

spec/controllers/uploads_controller_spec.rb

 require 'spec_helper'
 describe UploadsController do
-  let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+  let!(:user) { create(:user, :with_avatar) }
   describe "GET show" do
     context "when viewing a user avatar" do

spec/factories.rb

@@ -36,6 +36,13 @@ FactoryGirl.define do
       end
     end
+    trait :with_avatar do
+      avatar { fixture_file_upload(Rails.root.join(*%w(spec fixtures dk.png)), 'image/png') }
+      avatar_crop_x 0
+      avatar_crop_y 0
+      avatar_crop_size 256
+    end
+
     factory :omniauth_user do
       ignore do
         extern_uid '123456'

spec/helpers/application_helper_spec.rb

@@ -77,7 +77,7 @@ describe ApplicationHelper do
     let(:avatar_file_path) { File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif') }
     it 'should return an url for the avatar' do
-      user = create(:user, avatar: File.open(avatar_file_path))
+      user = create(:user, :with_avatar, avatar: File.open(avatar_file_path))
       expect(helper.avatar_icon(user.email).to_s).
         to match("/uploads/user/avatar/#{user.id}/banana_sample.gif")
@@ -88,7 +88,7 @@ describe ApplicationHelper do
       # Must be stubbed after the stub above, and separately
       stub_config_setting(url: Settings.send(:build_gitlab_url))
-      user = create(:user, avatar: File.open(avatar_file_path))
+      user = create(:user, :with_avatar, avatar: File.open(avatar_file_path))
       expect(helper.avatar_icon(user.email).to_s).
         to match("/gitlab/uploads/user/avatar/#{user.id}/banana_sample.gif")
@@ -102,7 +102,7 @@ describe ApplicationHelper do
     describe 'using a User' do
       it 'should return an URL for the avatar' do
-        user = create(:user, avatar: File.open(avatar_file_path))
+        user = create(:user, :with_avatar, avatar: File.open(avatar_file_path))
         expect(helper.avatar_icon(user).to_s).
           to match("/uploads/user/avatar/#{user.id}/banana_sample.gif")

spec/models/user_spec.rb

@@ -174,6 +174,18 @@ describe User, models: true do
         end
       end
     end
+
+    describe 'avatar' do
+      it 'only validates when avatar is present' do
+        user = build(:user, :with_avatar)
+
+        user.avatar_crop_x    = nil
+        user.avatar_crop_y    = nil
+        user.avatar_crop_size = nil
+
+        expect(user).not_to be_valid
+      end
+    end
   end
   describe "Respond to" do