From f98bb32612144eb92ed7c5b8eaabedddd6233d33 Mon Sep 17 00:00:00 2001 From: Brenda Wallace Date: Sat, 18 Nov 2017 17:14:29 +1300 Subject: [PATCH] Shaved some yaks to remove a rescue from a controller --- app/controllers/photos_controller.rb | 57 +++++++++++++--------- spec/controllers/photos_controller_spec.rb | 29 +++++------ 2 files changed, 48 insertions(+), 38 deletions(-) diff --git a/app/controllers/photos_controller.rb b/app/controllers/photos_controller.rb index 5c8dedfd1..2c707f726 100644 --- a/app/controllers/photos_controller.rb +++ b/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] diff --git a/spec/controllers/photos_controller_spec.rb b/spec/controllers/photos_controller_spec.rb index 8788ee09f..3c4f9dacb 100644 --- a/spec/controllers/photos_controller_spec.rb +++ b/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