From b8d557a1ea3e0ab1536d1bbef69e96ab2aad5ea5 Mon Sep 17 00:00:00 2001 From: Marc Anguera Insa Date: Wed, 16 Jun 2021 01:36:27 +0200 Subject: [PATCH 1/2] Add avatar file size validation (max 5 MB) --- app/assets/javascripts/application/avatar.js | 8 +++--- .../stylesheets/application/avatar.scss | 8 +++--- app/controllers/users_controller.rb | 20 +++++++++++---- app/models/user.rb | 2 ++ app/views/layouts/_messages.html.erb | 2 +- app/views/users/_avatar.html.erb | 17 ++++++------- config/locales/ca.yml | 1 + config/locales/en.yml | 7 +++--- config/locales/es.yml | 1 + config/locales/eu.yml | 1 + config/locales/gl.yml | 1 + config/locales/pt-BR.yml | 1 + spec/controllers/users_controller_spec.rb | 25 ++++++++++++++----- 13 files changed, 62 insertions(+), 32 deletions(-) diff --git a/app/assets/javascripts/application/avatar.js b/app/assets/javascripts/application/avatar.js index 3f4b127bf..c6034c88c 100644 --- a/app/assets/javascripts/application/avatar.js +++ b/app/assets/javascripts/application/avatar.js @@ -38,14 +38,14 @@ $(function () { // on submit take the parameters of the box to crop the avatar $('#form_photo').on("submit", () => { - let total_width = parseInt(getComputedStyle(document.getElementById("containerCrop")).width); - let photo_width = parseInt(getComputedStyle(document.getElementById("foto")).width); + let total_width = parseInt(getComputedStyle(document.getElementById("container_crop")).width); + let photo_width = parseInt(getComputedStyle(document.getElementById("original_img")).width); let left_displacement = total_width - photo_width; $('#height_offset').val(parseInt(panel.css('top')) - 15); $('#width_offset').val(parseInt(panel.css('left')) - 15 - (left_displacement/2)); $('#height_width').val(parseInt(panel.css('width'))); - $('#original_width').val($('#foto').width()); + $('#original_width').val($('#original_img').width()); }); function resize(e) { @@ -93,7 +93,7 @@ $(function () { } function preview_image_modal() { - var preview = document.querySelector('#foto'); + var preview = document.querySelector('#original_img'); var file = document.querySelector('#avatar-js').files[0]; var reader = new FileReader(); diff --git a/app/assets/stylesheets/application/avatar.scss b/app/assets/stylesheets/application/avatar.scss index d87ab1d6b..7ed2d7c8d 100644 --- a/app/assets/stylesheets/application/avatar.scss +++ b/app/assets/stylesheets/application/avatar.scss @@ -6,8 +6,8 @@ #crop_panel { position: absolute; - width: 140px; - height: 140px; + width: 200px; + height: 200px; border-left: 4px dashed black; cursor: move; touch-action: none; @@ -21,7 +21,7 @@ right: 0; width: 16px; height: 100%; - cursor: w-resize; + cursor: ew-resize; } #crop_panel::after { @@ -32,5 +32,5 @@ bottom: 0; width: 100%; height: 16px; - cursor: n-resize; + cursor: ns-resize; } diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index eded58ee7..44529a9b2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -67,13 +67,13 @@ def update def update_avatar avatar = params[:avatar] + errors = validate_avatar(avatar) - if avatar && content_type_permitted(avatar.content_type) + if errors.blank? current_user.avatar.purge if current_user.avatar.attached? - crop_image_and_save(current_user, avatar) else - flash[:error] = t 'users.show.invalid_format' + flash[:error] = errors.join("
") end redirect_to current_user @@ -151,7 +151,17 @@ def crop_image_and_save(user, avatar) ) end - def content_type_permitted(avatar_content_type) - %w[image/jpeg image/pjpeg image/png image/x-png].include? avatar_content_type + def validate_avatar(file) + errors = [] + + if User::AVATAR_CONTENT_TYPES.exclude?(file.content_type) + errors << t("users.show.invalid_format") + end + + if file.size.to_f > User::AVATAR_MAX_SIZE.megabytes + errors << t("users.avatar.max_size_warning", size: User::AVATAR_MAX_SIZE) + end + + errors end end diff --git a/app/models/user.rb b/app/models/user.rb index 4f3e006c8..a0eb4bc9a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,6 +9,8 @@ class User < ApplicationRecord :timeoutable ] + AVATAR_MAX_SIZE = 5 + AVATAR_CONTENT_TYPES = %w[image/jpeg image/pjpeg image/png image/x-png] GENDERS = %w( female male diff --git a/app/views/layouts/_messages.html.erb b/app/views/layouts/_messages.html.erb index 0222a2c35..d1db057d0 100644 --- a/app/views/layouts/_messages.html.erb +++ b/app/views/layouts/_messages.html.erb @@ -5,7 +5,7 @@ diff --git a/app/views/users/_avatar.html.erb b/app/views/users/_avatar.html.erb index bd792a6be..df80232cf 100644 --- a/app/views/users/_avatar.html.erb +++ b/app/views/users/_avatar.html.erb @@ -16,25 +16,24 @@
-

- <%= t ".crop_the_image" %> -

+

<%= t ".crop_the_image" %>

-
+
- +
- - <%= f.hidden_field :height_width, value: 140 %> + <%= f.hidden_field :height_width, value: 200 %> <%= f.hidden_field :width_offset, value: 1 %> <%= f.hidden_field :height_offset, value: 1 %> <%= f.hidden_field :original_width, value: 1 %> + + <%= f.button :submit, class: "btn btn-default", data: { dismiss: "modal" } do %> + <%= t("users.new.cancel") %> + <% end %> <%= f.button :submit, class: "btn btn-primary" do %> <%= t("global.save") %> <% end %> diff --git a/config/locales/ca.yml b/config/locales/ca.yml index 978268a0e..8f67de85f 100644 --- a/config/locales/ca.yml +++ b/config/locales/ca.yml @@ -505,6 +505,7 @@ ca: avatar: change_your_image: Canvia la teva imatge crop_the_image: Retalla la imatge + max_size_warning: La imatge és massa gran, el màxim permès és de %{size}MB edit: edit_user: Canviar usuari form: diff --git a/config/locales/en.yml b/config/locales/en.yml index abe673a18..96f498108 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -502,6 +502,10 @@ en: new: error_amount: Time must be greater than 0 users: + avatar: + change_your_image: Change your image + crop_the_image: Crop the image + max_size_warning: Image is too big, max allowed is %{size}MB edit: edit_user: Update user form: @@ -526,9 +530,6 @@ en: create_more_users_button: Create and add another user new_user: New user user_created_add: User %{uid} %{name} saved, now create next one - avatar: - change_your_image: Change your image - crop_the_image: Crop the image show: account: Last transactions accounts: Accounts diff --git a/config/locales/es.yml b/config/locales/es.yml index 5e1a4c0be..30353ba1b 100644 --- a/config/locales/es.yml +++ b/config/locales/es.yml @@ -505,6 +505,7 @@ es: avatar: change_your_image: Cambia tu imagen crop_the_image: Recortar la imagen + max_size_warning: La imagen es demasiado grande, el máximo permitido es %{size}MB edit: edit_user: Cambiar usuario form: diff --git a/config/locales/eu.yml b/config/locales/eu.yml index c0f81c63c..d5672e39f 100644 --- a/config/locales/eu.yml +++ b/config/locales/eu.yml @@ -505,6 +505,7 @@ eu: avatar: change_your_image: crop_the_image: + max_size_warning: edit: edit_user: Erabiltzailea aldatu form: diff --git a/config/locales/gl.yml b/config/locales/gl.yml index bcd1d3c60..71e4d60bb 100644 --- a/config/locales/gl.yml +++ b/config/locales/gl.yml @@ -505,6 +505,7 @@ gl: avatar: change_your_image: crop_the_image: + max_size_warning: edit: edit_user: Actualizar persoa usuaria form: diff --git a/config/locales/pt-BR.yml b/config/locales/pt-BR.yml index 101268c3e..9a8b9edf3 100644 --- a/config/locales/pt-BR.yml +++ b/config/locales/pt-BR.yml @@ -505,6 +505,7 @@ pt-BR: avatar: change_your_image: crop_the_image: + max_size_warning: edit: edit_user: Trocar usuário form: diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 4a0978cc9..aac98519f 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -409,18 +409,31 @@ context "with a normal logged user" do before { login(user) } - it "change the photo attached @user" do - path = Rails.root.join("docs/images/ofertas.png") - uploaded_file = Rack::Test::UploadedFile.new(path, "image/png") + let(:uploaded_file) do + Rack::Test::UploadedFile.new( + Rails.root.join("docs/images/ofertas.png"), + "image/png" + ) + end + it "change the photo attached @user" do put :update_avatar, params: { avatar: uploaded_file, original_width: 500, height_width: 140 } expect(user.avatar.attached?).to eq true end - it "don't change the photo attached if the format isn't valid" do - path = Rails.root.join("spec/controllers/users_controller_spec.rb") - uploaded_file = Rack::Test::UploadedFile.new(path) + it "don't change the photo attached if the mime_type isn't valid" do + uploaded_file = Rack::Test::UploadedFile.new( + Rails.root.join("spec/controllers/users_controller_spec.rb") + ) + + put :update_avatar, params: { avatar: uploaded_file } + + expect(flash[:error]).not_to be_empty + end + + it "don't change the photo attached if the file size it too big" do + allow_any_instance_of(ActionDispatch::Http::UploadedFile).to receive(:size) { User::AVATAR_MAX_SIZE.megabytes + 1.megabyte } put :update_avatar, params: { avatar: uploaded_file } From 2daded2c91699ebaa71f0ad6a489415142cc3d5a Mon Sep 17 00:00:00 2001 From: Marc Anguera Insa Date: Wed, 16 Jun 2021 21:08:33 +0200 Subject: [PATCH 2/2] move avatar logic out of controller to an own PORO: AvatarGenerator --- app/controllers/users_controller.rb | 44 ++----------------- app/models/user.rb | 2 - app/services/avatar_generator.rb | 51 +++++++++++++++++++++++ spec/controllers/users_controller_spec.rb | 2 +- 4 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 app/services/avatar_generator.rb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 44529a9b2..79e48b807 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -66,14 +66,12 @@ def update end def update_avatar - avatar = params[:avatar] - errors = validate_avatar(avatar) + operation = AvatarGenerator.new(current_user, params) - if errors.blank? - current_user.avatar.purge if current_user.avatar.attached? - crop_image_and_save(current_user, avatar) + if operation.errors.empty? + operation.call else - flash[:error] = errors.join("
") + flash[:error] = operation.errors.join("
") end redirect_to current_user @@ -130,38 +128,4 @@ def redirect_to_after_create name: @user.username) end end - - def crop_image_and_save(user, avatar) - orig_width = params[:original_width].to_i - width = params[:height_width].to_i - left = params[:width_offset].to_i - top = params[:height_offset].to_i - - image_processed = ImageProcessing::MiniMagick. - source(avatar.tempfile). - resize_to_fit(orig_width, nil). - crop("#{width}x#{width}+#{left}+#{top}!"). - convert("png"). - call - - user.avatar.attach( - io: image_processed, - filename: user.username, - content_type: avatar.content_type - ) - end - - def validate_avatar(file) - errors = [] - - if User::AVATAR_CONTENT_TYPES.exclude?(file.content_type) - errors << t("users.show.invalid_format") - end - - if file.size.to_f > User::AVATAR_MAX_SIZE.megabytes - errors << t("users.avatar.max_size_warning", size: User::AVATAR_MAX_SIZE) - end - - errors - end end diff --git a/app/models/user.rb b/app/models/user.rb index a0eb4bc9a..4f3e006c8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,8 +9,6 @@ class User < ApplicationRecord :timeoutable ] - AVATAR_MAX_SIZE = 5 - AVATAR_CONTENT_TYPES = %w[image/jpeg image/pjpeg image/png image/x-png] GENDERS = %w( female male diff --git a/app/services/avatar_generator.rb b/app/services/avatar_generator.rb new file mode 100644 index 000000000..d04187f4c --- /dev/null +++ b/app/services/avatar_generator.rb @@ -0,0 +1,51 @@ +class AvatarGenerator + MAX_SIZE = 5 + CONTENT_TYPES = %w[image/jpeg image/pjpeg image/png image/x-png] + + attr_accessor :errors + + def initialize(user, params) + @user = user + @params = params + @avatar = params[:avatar] + @errors = [] + + validate_avatar + end + + def call + @user.avatar.purge if @user.avatar.attached? + + @user.avatar.attach( + io: process_image, + filename: @user.username, + content_type: @avatar.content_type + ) + end + + private + + def process_image + orig_width = @params[:original_width].to_i + width = @params[:height_width].to_i + left = @params[:width_offset].to_i + top = @params[:height_offset].to_i + + ImageProcessing::MiniMagick. + source(@avatar.tempfile). + resize_to_fit(orig_width, nil). + crop("#{width}x#{width}+#{left}+#{top}!"). + convert("png"). + call + end + + def validate_avatar + if CONTENT_TYPES.exclude?(@avatar.content_type) + @errors << I18n.t("users.show.invalid_format") + end + + if @avatar.size.to_f > MAX_SIZE.megabytes + @errors << I18n.t("users.avatar.max_size_warning", size: MAX_SIZE) + end + end +end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index aac98519f..16676d9fb 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -433,7 +433,7 @@ end it "don't change the photo attached if the file size it too big" do - allow_any_instance_of(ActionDispatch::Http::UploadedFile).to receive(:size) { User::AVATAR_MAX_SIZE.megabytes + 1.megabyte } + allow_any_instance_of(ActionDispatch::Http::UploadedFile).to receive(:size) { AvatarGenerator::MAX_SIZE.megabytes + 1.megabyte } put :update_avatar, params: { avatar: uploaded_file }