Shaved some yaks to remove a rescue from a controller

app/controllers/photos_controller.rb

@@ -17,10 +17,6 @@ class PhotosController < ApplicationController
   end
 
   def new
-    @id = params[:id]
-    @type = params[:type]
-    redirect_to photos_path if @type.nil?
-
     @photo = Photo.new
     @item = item_to_link_to
     retrieve_from_flickr
@@ -32,11 +28,13 @@ class PhotosController < ApplicationController
   end
 
   def create
-    @photo = find_or_create_photo_from_flickr_photo
-    @item = item_to_link_to
-    raise "Could not find this item owned by you" unless @item
-    collection << @item unless collection.include?(@item)
-    @photo.save! if @photo.present?
+    ActiveRecord::Base.transaction do
+      @photo = find_or_create_photo_from_flickr_photo
+      @item = item_to_link_to
+      raise "Could not find this #{type} owned by you" unless @item
+      collection << @item unless collection.include?(@item)
+      @photo.save! if @photo.present?
+    end
     respond_with @photo
   end
 
@@ -52,12 +50,14 @@ class PhotosController < ApplicationController
 
   private
 
-  def item_id?
-    item_id.present?
+  #
+  # Params
+  def item_id
+    params[:id]
   end
 
-  def item_id
-    params.key? :id
+  def item_type
+    params[:type]
   end
 
   def flickr_photo_id_param
@@ -69,6 +69,26 @@ class PhotosController < ApplicationController
       :license_url, :thumbnail_url, :fullsize_url, :link_url)
   end
 
+  # Item with photos attached
+  #
+  def item_to_link_to
+    raise "No item id provided" if item_id.nil?
+    raise "No item type provided" if item_type.nil?
+    raise "Missing or invalid type provided" unless photos_supported_on_type?(item_type)
+    item_class = Growstuff::Constants::PhotoModels.get_item(item_type)
+    item_class.find_by!(id: params[:id], owner_id: current_member.id)
+  end
+
+  def collection
+    Growstuff::Constants::PhotoModels.get_relation(@photo, item_type)
+  end
+
+  def photos_supported_on_type?(_type)
+    Growstuff::Constants::PhotoModels.types.include?(item_type)
+  end
+
+  #
+  # Flickr retrieval
   def find_or_create_photo_from_flickr_photo
     photo = Photo.find_by(flickr_photo_id: flickr_photo_id_param)
     photo ||= Photo.new(photo_params)
@@ -77,17 +97,6 @@ class PhotosController < ApplicationController
     photo
   end
 
-  def collection
-    raise "Missing or invalid type provided" unless Growstuff::Constants::PhotoModels.types.include?(params[:type])
-    raise "No item id provided" unless item_id?
-    Growstuff::Constants::PhotoModels.get_relation(@photo, params[:type])
-  end
-
-  def item_to_link_to
-    item_class = Growstuff::Constants::PhotoModels.get_item(params[:type])
-    item_class.find_by!(id: params[:id], owner_id: current_member.id)
-  end
-
   def retrieve_from_flickr
     @flickr_auth = current_member.auth('flickr')
     @current_set = params[:set]

spec/controllers/photos_controller_spec.rb

@@ -40,23 +40,20 @@ describe PhotosController do
     describe "planting photos" do
       before(:each) { get :new, type: "planting", id: planting.id }
       it { assigns(:flickr_auth).should be_an_instance_of(Authentication) }
-      it { assigns(:id).should eq planting.id.to_s }
-      it { assigns(:type).should eq "planting" }
+      it { assigns(:item).should eq planting }
       it { expect(flash[:alert]).not_to be_present }
       it { expect(flash[:alert]).not_to be_present }
     end
 
     describe "harvest photos" do
       before { get :new, type: "harvest", id: harvest.id }
-      it { assigns(:id).should eq harvest.id.to_s }
-      it { assigns(:type).should eq "harvest" }
+      it { assigns(:item).should eq harvest }
       it { expect(flash[:alert]).not_to be_present }
     end
 
     describe "garden photos" do
       before { get :new, type: "garden", id: garden.id }
-      it { assigns(:id).should eq garden.id.to_s }
-      it { assigns(:type).should eq "garden" }
+      it { assigns(:item).should eq garden }
       it { expect(flash[:alert]).not_to be_present }
     end
   end
@@ -108,18 +105,20 @@ describe PhotosController do
 
       it "doesn't attach photo to a comment" do
         comment = FactoryBot.create(:comment)
-        post :create, photo: { flickr_photo_id: photo.flickr_photo_id }, type: "comment", id: comment.id
-        expect(flash[:alert]).to be_present
+        expect do
+          post :create, photo: { flickr_photo_id: photo.flickr_photo_id }, type: "comment", id: comment.id
+        end.to raise_error
       end
     end
 
     describe "for the second time" do
+      let(:planting) { FactoryBot.create :planting, owner: member }
       it "does not add a photo twice" do
         expect do
-          post :create, photo: { flickr_photo_id: 1 }
+          post :create, photo: { flickr_photo_id: 1 }, id: planting.id, type: 'planting'
         end.to change(Photo, :count).by(1)
         expect do
-          post :create, photo: { flickr_photo_id: 1 }
+          post :create, photo: { flickr_photo_id: 1 }, id: planting.id, type: 'planting'
         end.to change(Photo, :count).by(0)
       end
     end
@@ -146,16 +145,18 @@ describe PhotosController do
       it "does not create the planting/photo link" do
         # members will be auto-created, and different
         another_planting = FactoryBot.create(:planting)
-        post :create, photo: { flickr_photo_id: photo.flickr_photo_id }, type: "planting", id: another_planting.id
-        expect(flash[:alert]).to be_present
+        expect do
+          post :create, photo: { flickr_photo_id: photo.flickr_photo_id }, type: "planting", id: another_planting.id
+        end.to raise_error(ActiveRecord::RecordNotFound)
         Photo.last.plantings.first.should_not eq another_planting
       end
 
       it "does not create the harvest/photo link" do
         # members will be auto-created, and different
         another_harvest = FactoryBot.create(:harvest)
-        post :create, photo: { flickr_photo_id: photo.flickr_photo_id }, type: "harvest", id: another_harvest.id
-        expect(flash[:alert]).to be_present
+        expect do
+          post :create, photo: { flickr_photo_id: photo.flickr_photo_id }, type: "harvest", id: another_harvest.id
+        end.to raise_error(ActiveRecord::RecordNotFound)
         Photo.last.harvests.first.should_not eq another_harvest
       end
     end