Merge pull request #218 from lnu-norge/image-uploads-crash

Fix for image uploads crashing the app

app/controllers/space_images_controller.rb

@@ -19,9 +19,16 @@ def destroy
   end
 
   def upload_image
-    params["image"].each do |image|
-      Image.create!(space: @space, image:)
+    images = params["images_to_upload"]&.compact_blank
+    return upload_failed if images.blank?
+
+    # Attempt to upload each image
+    images.each do |image|
+      new_image = Image.new(space: @space, image:)
+
+      return upload_failed unless new_image.save
     end
+
     flash[:notice] = t("images.upload_success")
     redirect_to space_path(params[:id])
   end
@@ -35,4 +42,9 @@ def image_params
   def set_space
     @space = Space.find(params[:id])
   end
+
+  def upload_failed
+    flash[:alert] = t("images.upload_failed")
+    redirect_to space_path(params[:id])
+  end
 end

app/models/image.rb

@@ -2,6 +2,8 @@
 
 class Image < ApplicationRecord
   has_one_attached :image
+  validate :image_is_attached
+
   belongs_to :space
 
   before_destroy :delete_image
@@ -15,6 +17,10 @@ def url
   def delete_image
     image.purge
   end
+
+  def image_is_attached
+    errors.add(:image, "must be attached") unless image.attached?
+  end
 end
 
 # == Schema Information

app/views/spaces/show/_image_header_upload_button.html.erb

@@ -1,7 +1,7 @@
 <%= form_with url: spaces_upload_image_path do |f| %>
   <%= f.hidden_field :id, value: @space.id %>
-  <%= f.file_field :image, accept: 'image/png,image/gif,image/jpeg', multiple: true, class: "hidden", onchange: "this.form.submit()" %>
-  <%= f.label :image, role: "button" do %>
+  <%= f.file_field :images_to_upload, accept: 'image/png,image/gif,image/jpeg', multiple: true, class: "hidden", onchange: "this.form.submit()" %>
+  <%= f.label :images_to_upload, role: "button" do %>
     <div class="edit-button collapsable">
       <span class="text"><%= t('images.upload_image') %></span>
       <%= inline_svg_tag 'camera', class: "text-gray-700", alt: t('images.upload'), title: t('images.upload') %>

config/environments/test.rb

@@ -8,6 +8,9 @@
 # and recreated between test runs. Don't rely on the data there!
 
 Rails.application.configure do
+  # Default host needed for tests:
+  routes.default_url_options[:host] = "localhost:3000"
+
   # Settings specified here will take precedence over those in config/application.rb.
 
   config.cache_classes = false

config/locales/nb.yml

@@ -101,6 +101,7 @@ nb:
   images:
     image_missing: "Bilde mangler"
     upload_success: "Bilde lastet opp"
+    upload_failed: "Fikk ikke lastet opp bilde, noe gikk galt"
     upload: "Last opp"
     upload_image: "Last opp bilde"
     edit_images: "Rediger bilder"

spec/fabricators/image_fabricator.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+Fabricator(:image) do
+  space
+  image { Rack::Test::UploadedFile.new(TestImageHelper.image_path, TestImageHelper.content_type) }
+end

spec/models/space_spec.rb

@@ -107,4 +107,14 @@
       expect(space.versions.last.event).to eq("update")
     end
   end
+
+  describe "Image upload" do
+    let(:space) { Fabricate(:space) }
+    let(:image) { Fabricate(:image, space:) }
+
+    it "allows image to be attached" do
+      expect(image.image).to be_attached
+      expect(space.images.count).to eq(1)
+    end
+  end
 end

spec/rails_helper.rb

@@ -8,6 +8,7 @@
 # Add additional requires below this line. Rails is not loaded until this point!
 require 'selenium-webdriver'
 require 'vcr'
+require 'support/test_image/test_image_helper'
 
 
 # Requires supporting ruby files with custom matchers and macros, etc, in
@@ -119,4 +120,6 @@
         (uri.host == 'localhost' && uri.port != 3001) # Ignore localhost except on port 3001 which is used for recording KRA API cassettes
     end
   end
+
+  config.include TestImageHelper
 end

spec/requests/space_images_spec.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe "SpaceImages", type: :request do
+  let(:user) { Fabricate(:user) }
+
+  before do
+    sign_in user
+  end
+
+  describe "POST /spaces/:id/upload_image" do
+    let(:space) { Fabricate(:space) }
+    let(:image_file) { fixture_file_upload(TestImageHelper.image_path, TestImageHelper.content_type) }
+
+    it "fails to upload an empty image" do
+      expect do
+        post spaces_upload_image_path(id: space.id), params: { image: [""] }
+      end.not_to change(space.images, :count)
+      expect(response).to redirect_to(space_path(space.id))
+      follow_redirect!
+      expect(response.body).to include(I18n.t("images.upload_failed"))
+    end
+
+    it "uploads an image and redirects" do
+      expect do
+        # Rails for some reason adds an empty "" to the start of the array when allowing multiple uploads
+        post spaces_upload_image_path(id: space.id), params: { images_to_upload: ["", image_file] }
+      end.to change(space.images, :count).by(1)
+      expect(response).to redirect_to(space_path(space.id))
+      follow_redirect!
+      expect(response.body).to include(I18n.t("images.upload_success"))
+    end
+  end
+
+  describe "PUT /spaces/:id/images/:image_id" do
+    let(:space) { Fabricate(:space) }
+    let(:image) { Fabricate(:image, space:) }
+    let(:params) { { image: { caption: "New Caption", credits: "New Credits" } } }
+
+    it "updates the image info and redirects" do
+      put(space_image_path(id: space.id, image_id: image.id), params:)
+      expect(response).to redirect_to(space_image_path(space))
+      follow_redirect!
+      expect(response.body).to include(I18n.t("images.update_success"))
+    end
+  end
+
+  describe "DELETE /spaces/:id/images/:image_id" do
+    let(:space) { Fabricate(:space) }
+    let!(:image) { Fabricate(:image, space:) }
+
+    it "deletes the image and redirects" do
+      expect { delete space_image_path(id: space.id, image:) }.to change(Image, :count).by(-1)
+      expect(response).to redirect_to(space_image_path(space))
+      follow_redirect!
+      expect(response.body).to include(I18n.t("images.delete_success"))
+    end
+  end
+end

spec/support/test_image/test_image_helper.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module TestImageHelper
+  def self.image_path
+    Rails.root.join("spec/support/test_image/test_image.jpg")
+  end
+
+  def self.content_type
+    "image/jpeg"
+  end
+
+  def self.filename
+    "test_image.jpg"
+  end
+
+  def self.image_file
+    File.open(image_path)
+  end
+
+  def self.image_upload
+    {
+      io: image_file,
+      filename:,
+      content_type:
+    }
+  end
+end