From 65c46c334b097816cdc92348a8a390680c4e5a7f Mon Sep 17 00:00:00 2001 From: Skud Date: Mon, 29 Sep 2014 09:18:28 +1000 Subject: [PATCH] Delete unused photos Make sure the models are setup so that if a photo is not used for anything, it's removed from the system. Also wrote a rake task (which should be run on deploy) to remove older unused photos. --- app/models/harvest.rb | 10 +++++- app/models/photo.rb | 9 ++++- app/models/planting.rb | 10 +++++- db/schema.rb | 4 ++- lib/tasks/growstuff.rake | 9 ++++- spec/models/photo_spec.rb | 76 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 113 insertions(+), 5 deletions(-) diff --git a/app/models/harvest.rb b/app/models/harvest.rb index 8cc54e043..68afb2ee5 100644 --- a/app/models/harvest.rb +++ b/app/models/harvest.rb @@ -10,7 +10,15 @@ class Harvest < ActiveRecord::Base belongs_to :plant_part has_and_belongs_to_many :photos - before_destroy {|harvest| harvest.photos.clear} + + before_destroy do |harvest| + photolist = harvest.photos.to_a # save a temp copy of the photo list + harvest.photos.clear # clear relationship b/w harvest and photo + + photolist.each do |photo| + photo.destroy_if_unused + end + end default_scope order('created_at DESC') diff --git a/app/models/photo.rb b/app/models/photo.rb index 4f0a0160f..32679eeed 100644 --- a/app/models/photo.rb +++ b/app/models/photo.rb @@ -5,13 +5,20 @@ class Photo < ActiveRecord::Base has_and_belongs_to_many :plantings has_and_belongs_to_many :harvests - before_destroy do |photo| + before_destroy do |photo| photo.plantings.clear photo.harvests.clear end default_scope order("created_at desc") + # remove photos that aren't used by anything + def destroy_if_unused + unless plantings.size > 0 and harvests.size > 0 + self.destroy + end + end + # This is split into a side-effect free method and a side-effecting method # for easier stubbing and testing. def flickr_metadata diff --git a/app/models/planting.rb b/app/models/planting.rb index 270a827d2..8d0a5227a 100644 --- a/app/models/planting.rb +++ b/app/models/planting.rb @@ -11,7 +11,15 @@ class Planting < ActiveRecord::Base belongs_to :crop, :counter_cache => true has_and_belongs_to_many :photos - before_destroy {|planting| planting.photos.clear} + + before_destroy do |planting| + photolist = planting.photos.to_a # save a temp copy of the photo list + planting.photos.clear # clear relationship b/w planting and photo + + photolist.each do |photo| + photo.destroy_if_unused + end + end default_scope order("created_at desc") scope :finished, where(:finished => true) diff --git a/db/schema.rb b/db/schema.rb index 5ad7d3192..3790e9340 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20140909001730) do +ActiveRecord::Schema.define(:version => 20140928085713) do create_table "account_types", :force => true do |t| t.string "name", :null => false @@ -144,6 +144,7 @@ ActiveRecord::Schema.define(:version => 20140909001730) do t.text "bio" t.integer "plantings_count" t.boolean "newsletter" + t.boolean "send_planting_reminder", :default => true end add_index "members", ["confirmation_token"], :name => "index_users_on_confirmation_token", :unique => true @@ -243,6 +244,7 @@ ActiveRecord::Schema.define(:version => 20140909001730) do t.datetime "updated_at", :null => false t.string "slug" t.integer "forum_id" + t.integer "parent_id" end add_index "posts", ["created_at", "author_id"], :name => "index_updates_on_created_at_and_user_id" diff --git a/lib/tasks/growstuff.rake b/lib/tasks/growstuff.rake index 34739217b..0ae08a699 100644 --- a/lib/tasks/growstuff.rake +++ b/lib/tasks/growstuff.rake @@ -249,13 +249,20 @@ namespace :growstuff do desc "August 2014: fix ping to pint in database" task :ping_to_pint => :environment do Harvest.find_each do |h| - if h.unit == "ping" + if h.unit == "ping" h.unit = "pint" h.save end end end + desc "October 2014: remove unused photos" + task :remove_unused_photos => :environment do + Photo.find_each do |p| + p.destroy_if_unused + end + end + end # end oneoff section end diff --git a/spec/models/photo_spec.rb b/spec/models/photo_spec.rb index 8ac067ab7..2a0b1d5ff 100644 --- a/spec/models/photo_spec.rb +++ b/spec/models/photo_spec.rb @@ -2,6 +2,82 @@ require 'spec_helper' describe Photo do + describe 'add/delete functionality' do + let(:photo) { FactoryGirl.create(:photo) } + let(:planting) { FactoryGirl.create(:planting) } + let(:harvest) { FactoryGirl.create(:harvest) } + + context "adds photos" do + it 'to a planting' do + planting.photos << photo + expect(planting.photos.count).to eq 1 + expect(planting.photos.first).to eq photo + end + + it 'to a harvest' do + harvest.photos << photo + expect(harvest.photos.count).to eq 1 + expect(harvest.photos.first).to eq photo + end + end + + context "removing photos" do + it 'from a planting' do + planting.photos << photo + photo.destroy + expect(planting.photos.count).to eq 0 + end + + it 'from a harvest' do + harvest.photos << photo + photo.destroy + expect(harvest.photos.count).to eq 0 + end + + it "automatically if unused" do + photo.destroy_if_unused + expect(lambda { photo.reload }).to raise_error ActiveRecord::RecordNotFound + end + + it 'they are no longer used by plantings' do + planting.photos << photo + planting.destroy # photo is now no longer used by anything + expect(lambda { photo.reload }).to raise_error ActiveRecord::RecordNotFound + end + + it 'they are no longer used by harvests' do + harvest.photos << photo + harvest.destroy # photo is now no longer used by anything + expect(lambda { photo.reload }).to raise_error ActiveRecord::RecordNotFound + end + + it 'they are no longer used by anything' do + planting.photos << photo + harvest.photos << photo + expect(photo.plantings.size).to eq 1 + expect(photo.harvests.size).to eq 1 + + planting.destroy # photo is still used by harvest + photo.reload + expect(photo).to be_an_instance_of Photo + expect(photo.plantings.size).to eq 0 + expect(photo.harvests.size).to eq 1 + + harvest.destroy # photo is now no longer used by anything + expect(lambda { photo.reload }).to raise_error ActiveRecord::RecordNotFound + end + + it 'does not occur when a photo is still in use' do + planting.photos << photo + harvest.photos << photo + planting.destroy # photo is still used by the harvest + expect(photo).to be_an_instance_of Photo + end + + end # removing photos + + end # add/delete functionality + describe 'flickr_metadata' do # Any further tests led to us MOCKING ALL THE THINGS # which was epistemologically unsatisfactory.