diff --git a/app/constants/photo_models.rb b/app/constants/photo_models.rb deleted file mode 100644 index 8f79933e4..000000000 --- a/app/constants/photo_models.rb +++ /dev/null @@ -1,38 +0,0 @@ -module Growstuff - module Constants - class PhotoModels - PLANTING = { type: 'planting', class: 'Planting', relation: 'plantings' }.freeze - HARVEST = { type: 'harvest', class: 'Harvest', relation: 'harvests' }.freeze - GARDEN = { type: 'garden', class: 'Garden', relation: 'gardens' }.freeze - SEED = { type: 'seed', class: 'Seed', relation: 'seeds' }.freeze - - ALL = [PLANTING, HARVEST, GARDEN, SEED].freeze - - def self.types - ALL.map do |model| - model[:type] - end - end - - def self.relations - ALL.map do |model| - model[:relation] - end - end - - def self.get_relation(object, type) - relation = ALL.select do |model| - model[:type] == type - end[0][:relation] - object.send(relation) - end - - def self.get_item(type) - class_name = ALL.select do |model| - model[:type] == type - end[0][:class] - class_name.constantize - end - end - end -end diff --git a/app/controllers/photo_associations_controller.rb b/app/controllers/photo_associations_controller.rb index 3259fe3c8..126aaf9b3 100644 --- a/app/controllers/photo_associations_controller.rb +++ b/app/controllers/photo_associations_controller.rb @@ -3,11 +3,19 @@ class PhotoAssociationsController < ApplicationController respond_to :json, :html def destroy + raise "Photos not supported" unless Photo::PHOTO_CAPABLE.include? item_class @photo = Photo.find_by!(id: params[:photo_id], owner: current_member) - collection = Growstuff::Constants::PhotoModels.get_relation(@photo, params[:type]) - item_class = Growstuff::Constants::PhotoModels.get_item(params[:type]) - @item = item_class.find_by!(id: params[:id], owner_id: current_member.id) - collection.delete(@item) + @item = Photographing.item(item_id, item_class) + @item.photos.delete(@photo) + # @photo.destroy_if_unused respond_with(@photo) end + + def item_class + params[:type].capitalize + end + + def item_id + params[:id] + end end diff --git a/app/controllers/photos_controller.rb b/app/controllers/photos_controller.rb index 18e95302b..2866c1549 100644 --- a/app/controllers/photos_controller.rb +++ b/app/controllers/photos_controller.rb @@ -36,7 +36,7 @@ class PhotosController < ApplicationController @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) + @item.photos << @photo unless @item.photos.include? @photo @photo.save! if @photo.present? end respond_with @photo @@ -74,21 +74,12 @@ class PhotosController < ApplicationController 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) + item_class = item_type.capitalize + raise "Photos not supported" unless Photo::PHOTO_CAPABLE.include? item_class + item_class.constantize.find_by!(id: params[:id], owner_id: current_member.id) end # diff --git a/app/controllers/seeds_controller.rb b/app/controllers/seeds_controller.rb index f7cf530eb..12445877e 100644 --- a/app/controllers/seeds_controller.rb +++ b/app/controllers/seeds_controller.rb @@ -19,6 +19,7 @@ class SeedsController < ApplicationController # GET /seeds/1 # GET /seeds/1.json def show + @photos = @seed.photos.includes(:owner).order(created_at: :desc).paginate(page: params[:page]) respond_with(@seed) end diff --git a/app/models/concerns/photo_capable.rb b/app/models/concerns/photo_capable.rb index 4b1f90821..177a2f832 100644 --- a/app/models/concerns/photo_capable.rb +++ b/app/models/concerns/photo_capable.rb @@ -1,18 +1,10 @@ -require_relative '../../constants/photo_models.rb' module PhotoCapable extend ActiveSupport::Concern included do - has_and_belongs_to_many :photos # rubocop:disable Rails/HasAndBelongsToMany + has_many :photos, through: :photographings, as: :photographable + has_many :photographings, as: :photographable, dependent: :destroy - before_destroy :remove_from_list scope :has_photos, -> { includes(:photos).where.not(photos: { id: nil }) } end - - def remove_from_list - photolist = photos.to_a # save a temp copy of the photo list - photos.clear # clear relationship b/w object and photo - - photolist.each(&:destroy_if_unused) - end end diff --git a/app/models/photo.rb b/app/models/photo.rb index 244687fb5..5800ec6f9 100644 --- a/app/models/photo.rb +++ b/app/models/photo.rb @@ -1,31 +1,19 @@ -require_relative '../constants/photo_models.rb' class Photo < ActiveRecord::Base belongs_to :owner, class_name: 'Member' - Growstuff::Constants::PhotoModels.relations.each do |relation| - has_and_belongs_to_many relation.to_sym # rubocop:disable Rails/HasAndBelongsToMany - end + PHOTO_CAPABLE = %w(Garden Planting Harvest Seed).freeze - before_destroy { all_associations.clear } + has_many :photographings, foreign_key: :photo_id, dependent: :destroy + # creates a relationship for each assignee type + PHOTO_CAPABLE.each do |type| + has_many type.downcase.pluralize.to_s.to_sym, + through: :photographings, + source: :photographable, + source_type: type + end default_scope { joins(:owner) } # Ensures the owner still exists - def associations? - plantings.any? || harvests.any? || gardens.any? || seeds.any? - end - - def all_associations - associations = [] - Growstuff::Constants::PhotoModels.relations.each do |association_name| - associations << send(association_name.to_s).to_a - end - associations.flatten! - end - - def destroy_if_unused - destroy if all_associations.empty? - end - # This is split into a side-effect free method and a side-effecting method # for easier stubbing and testing. def flickr_metadata @@ -43,6 +31,14 @@ class Photo < ActiveRecord::Base } end + def associations? + photographings.size.positive? + end + + def destroy_if_unused + destroy unless associations? + end + def calculate_title(info) if id && title # already has a title saved title diff --git a/app/models/photographing.rb b/app/models/photographing.rb new file mode 100644 index 000000000..3edb78351 --- /dev/null +++ b/app/models/photographing.rb @@ -0,0 +1,12 @@ +class Photographing < ActiveRecord::Base + belongs_to :photo + belongs_to :photographable, polymorphic: true + + def self.item(item_id, item_type) + find_by!(photographable_id: item_id, photographable_type: item_type).photographable + end + + def item + find_by!(photographable_id: photographable_id, photographable_type: photographable_type).photographable + end +end diff --git a/app/views/seeds/_actions.html.haml b/app/views/seeds/_actions.html.haml new file mode 100644 index 000000000..9584182f0 --- /dev/null +++ b/app/views/seeds/_actions.html.haml @@ -0,0 +1,11 @@ +- if can? :edit, @seed + = link_to edit_seed_path(@seed), class: 'btn btn-default btn-xs' do + %span.glyphicon.glyphicon-pencil{ title: "Edit" } + Edit + = link_to new_photo_path(id: seed.id, type: 'seed'), class: 'btn btn-default btn-xs' do + %span.glyphicon.glyphicon-camera{ title: "Add photo" } + Add photo +- if can? :destroy, @seed + = link_to @seed, method: :delete, data: { confirm: 'Are you sure?' }, class: 'btn btn-default btn-xs' do + %span.glyphicon.glyphicon-trash{ title: "Delete" } + Delete diff --git a/app/views/seeds/show.html.haml b/app/views/seeds/show.html.haml index 11579acd0..b1573c7b0 100644 --- a/app/views/seeds/show.html.haml +++ b/app/views/seeds/show.html.haml @@ -58,15 +58,11 @@ subject: "Interested in your #{@seed.crop} seeds"), class: 'btn btn-primary' - else - = render partial: 'shared/signin_signup', locals: { to: 'request seeds' } + = render 'shared/signin_signup', to: 'request seeds' - if can?(:edit, @seed) || can?(:destroy, @seed) %p - - if can? :edit, @seed - = link_to 'Edit', edit_seed_path(@seed), class: 'btn btn-default btn-xs' - - if can? :destroy, @seed - = link_to 'Delete', @seed, method: :delete, data: { confirm: 'Are you sure?' }, class: 'btn btn-default btn-xs' - + = render 'actions', seed: @seed .col-md-6 = render partial: "crops/index_card", locals: { crop: @seed.crop } - if @seed.owner.location @@ -79,3 +75,16 @@ Or = link_to "purchase seeds via Ebay", crop_ebay_seeds_url(@seed.crop), target: "_blank", rel: "noopener noreferrer" + +- if @photos.size.positive? + .row + .pagination + = page_entries_info @photos + = will_paginate @photos +.row + - @photos.each do |p| + .col-md-2.six-across + = render 'photos/thumbnail', photo: p + - if can?(:create, Photo) && can?(:edit, @seed) + .col-md-2 + = link_to "Add photo", new_photo_path(type: "seed", id: @seed.id), class: 'btn btn-primary' diff --git a/db/migrate/20171129041341_create_photographings.rb b/db/migrate/20171129041341_create_photographings.rb new file mode 100644 index 000000000..8054ba9cd --- /dev/null +++ b/db/migrate/20171129041341_create_photographings.rb @@ -0,0 +1,54 @@ +class CreatePhotographings < ActiveRecord::Migration + def change + create_table :photographings do |t| + t.integer :photo_id, null: false + t.integer :photographable_id, null: false + t.string :photographable_type, null: false + t.timestamps null: false + end + + add_foreign_key :photographings, :photos + + add_index :photographings, %i(photographable_id photographable_type photo_id), + unique: true, name: 'items_to_photos_idx' + add_index :photographings, %i(photographable_id photographable_type), + name: 'photographable_idx' + + migrate_data + end + + def migrate_data + say "migrating garden photos" + GardensPhoto.all.each do |g| + Photographing.create! photo_id: g.photo_id, photographable_id: g.garden_id, photographable_type: 'Garden' + end + say "migrating planting photos" + PhotosPlanting.all.each do |p| + Photographing.create! photo_id: p.photo_id, photographable_id: p.planting_id, photographable_type: 'Planting' + end + say "migrating harvest photos" + HarvestsPhoto.all.each do |h| + Photographing.create! photo_id: h.photo_id, photographable_id: h.harvest_id, photographable_type: 'Harvest' + end + say "migrating seed photos" + PhotosSeed.all.each do |s| + Photographing.create! photo_id: s.photo_id, photographable_id: s.seed_id, photographable_type: 'Seed' + end + end + class GardensPhoto < ActiveRecord::Base + belongs_to :photo + belongs_to :garden + end + class PhotosPlanting < ActiveRecord::Base + belongs_to :photo + belongs_to :planting + end + class HarvestsPhoto < ActiveRecord::Base + belongs_to :photo + belongs_to :harvest + end + class PhotosSeed < ActiveRecord::Base + belongs_to :photo + belongs_to :seed + end +end diff --git a/db/schema.rb b/db/schema.rb index 5b11fb94f..473e756fa 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171105011017) do +ActiveRecord::Schema.define(version: 20171129041341) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -383,6 +383,17 @@ ActiveRecord::Schema.define(version: 20171105011017) do t.integer "product_id" end + create_table "photographings", force: :cascade do |t| + t.integer "photo_id", null: false + t.integer "photographable_id", null: false + t.string "photographable_type", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "photographings", ["photographable_id", "photographable_type", "photo_id"], name: "items_to_photos_idx", unique: true, using: :btree + add_index "photographings", ["photographable_id", "photographable_type"], name: "photographable_idx", using: :btree + create_table "photos", force: :cascade do |t| t.integer "owner_id", null: false t.string "thumbnail_url", null: false @@ -498,4 +509,5 @@ ActiveRecord::Schema.define(version: 20171105011017) do add_index "seeds", ["slug"], name: "index_seeds_on_slug", unique: true, using: :btree add_foreign_key "harvests", "plantings" + add_foreign_key "photographings", "photos" end diff --git a/spec/factories/photos.rb b/spec/factories/photos.rb index aef506bf0..455d721c7 100644 --- a/spec/factories/photos.rb +++ b/spec/factories/photos.rb @@ -4,12 +4,12 @@ FactoryBot.define do factory :photo do owner flickr_photo_id 1 - title "Still life with chillies" + title { Faker::HarryPotter.quote } license_name "CC-BY" license_url "http://example.com/license.html" - thumbnail_url "http://example.com/thumb.jpg" - fullsize_url "http://example.com/full.jpg" - link_url "http://example.com/" + thumbnail_url { "http://example.com/#{Faker::File.file_name}.jpg" } + fullsize_url { "http://example.com/#{Faker::File.file_name}.jpg" } + link_url { Faker::Internet.url } factory :unlicensed_photo do license_name "All rights reserved" diff --git a/spec/features/crops/crop_photos_spec.rb b/spec/features/crops/crop_photos_spec.rb index 59eba8f38..260283057 100644 --- a/spec/features/crops/crop_photos_spec.rb +++ b/spec/features/crops/crop_photos_spec.rb @@ -2,27 +2,31 @@ require 'rails_helper' feature "crop detail page", js: true do let(:member) { create :member } + let(:crop) { create :crop, plantings: [planting], harvests: [harvest] } - let(:planting) { create :planting, owner: member, photos: [photo1, photo2] } - let(:harvest) { create :harvest, owner: member, photos: [photo3, photo4] } + let(:planting) { create :planting, owner: member } + let(:harvest) { create :harvest, owner: member } + let(:photo1) do - create(:photo, owner: member, title: 'photo 1', - fullsize_url: 'photo1.jpg', thumbnail_url: 'thumb1.jpg') + create(:photo, owner: member, title: 'photo 1', fullsize_url: 'photo1.jpg', thumbnail_url: 'thumb1.jpg') end let(:photo2) do - create(:photo, owner: member, title: 'photo 2', - fullsize_url: 'photo2.jpg', thumbnail_url: 'thumb2.jpg') + create(:photo, owner: member, title: 'photo 2', fullsize_url: 'photo2.jpg', thumbnail_url: 'thumb2.jpg') end let(:photo3) do - create(:photo, owner: member, title: 'photo 3', - fullsize_url: 'photo3.jpg', thumbnail_url: 'thumb3.jpg') + create(:photo, owner: member, title: 'photo 3', fullsize_url: 'photo3.jpg', thumbnail_url: 'thumb3.jpg') end let(:photo4) do - create(:photo, owner: member, title: 'photo 4', - fullsize_url: 'photo4.jpg', thumbnail_url: 'thumb4.jpg') + create(:photo, owner: member, title: 'photo 4', fullsize_url: 'photo4.jpg', thumbnail_url: 'thumb4.jpg') end - before { visit crop_path(crop) } + before do + planting.photos << photo1 + planting.photos << photo2 + harvest.photos << photo3 + harvest.photos << photo4 + visit crop_path(crop) + end subject { page } shared_examples "shows photos" do diff --git a/spec/features/seeds/seed_photos.rb b/spec/features/seeds/seed_photos.rb new file mode 100644 index 000000000..635b4dc4d --- /dev/null +++ b/spec/features/seeds/seed_photos.rb @@ -0,0 +1,41 @@ +require 'rails_helper' +require 'custom_matchers' + +feature "Seeds", :js do + let(:member) { FactoryBot.create :member } + let!(:seed) { FactoryBot.create :seed, owner: member } + + subject do + login_as member + visit seed_path(seed) + page + end + + it { is_expected.to have_content 'Add photo' } + + # context 'no photos' do + # it { is_expected.to have_content 'no photos' } + # end + context 'has one photo' do + before { seed.photos = [photo] } + let!(:photo) { FactoryBot.create :photo, title: 'hello photo' } + it { is_expected.to have_xpath("//img[contains(@src,'#{photo.thumbnail_url}')]") } + it { is_expected.to have_xpath("//a[contains(@href,'#{photo_path(photo)}')]") } + end + context 'has 50 photos' do + before { seed.photos = photos } + let!(:photos) { FactoryBot.create_list :photo, 50 } + it "shows newest photo" do + is_expected.to have_xpath("//img[contains(@src,'#{photos.last.thumbnail_url}')]") + end + it "links to newest photo" do + is_expected.to have_xpath("//a[contains(@href,'#{photo_path(photos.last)}')]") + end + it "does not show oldest photo" do + is_expected.not_to have_xpath("//img[contains(@src,'#{photos.first.thumbnail_url}')]") + end + it "does not link to oldest photo" do + is_expected.not_to have_xpath("//a[contains(@href,'#{photo_path(photos.first)}')]") + end + end +end diff --git a/spec/models/crop_spec.rb b/spec/models/crop_spec.rb index 73a460302..f89cc3377 100644 --- a/spec/models/crop_spec.rb +++ b/spec/models/crop_spec.rb @@ -269,78 +269,55 @@ describe Crop do end context 'interesting' do - it 'lists interesting crops' do - # first, a couple of candidate crops - @crop1 = FactoryBot.create(:crop) - @crop2 = FactoryBot.create(:crop) + let(:photo) { FactoryBot.create :photo } + # first, a couple of candidate crops + let(:crop1) { FactoryBot.create(:crop) } + let(:crop2) { FactoryBot.create(:crop) } - # they need 3+ plantings each to be interesting - 3.times do - FactoryBot.create(:planting, crop: @crop1) - end - 3.times do - FactoryBot.create(:planting, crop: @crop2) + let(:crop1_planting) { crop1.plantings.first } + let(:crop2_planting) { crop2.plantings.first } + + subject { Crop.interesting } + + describe 'lists interesting crops' do + before do + # they need 3+ plantings each to be interesting + FactoryBot.create_list(:planting, 3, crop: crop1) + FactoryBot.create_list(:planting, 3, crop: crop2) + # crops need 3+ photos to be interesting + crop1_planting.photos = FactoryBot.create_list :photo, 3 + crop2_planting.photos = FactoryBot.create_list :photo, 3 end - # crops need 3+ photos to be interesting - @photo = FactoryBot.create(:photo) - [@crop1, @crop2].each do |c| - 3.times do - c.plantings.first.photos << @photo - c.plantings.first.save - end - end - - Crop.interesting.should include @crop1 - Crop.interesting.should include @crop2 - Crop.interesting.size.should == 2 + it { is_expected.to include crop1 } + it { is_expected.to include crop2 } + it { expect(subject.size).to eq 2 } end - it 'ignores crops without plantings' do - # first, a couple of candidate crops - @crop1 = FactoryBot.create(:crop) - @crop2 = FactoryBot.create(:crop) - - # only crop1 has plantings - 3.times do - FactoryBot.create(:planting, crop: @crop1) + describe 'crops without plantings are not interesting' do + before do + # only crop1 has plantings + FactoryBot.create_list(:planting, 3, crop: crop1) + # ... and photos + crop1_planting.photos = FactoryBot.create_list(:photo, 3) end - - # ... and photos - @photo = FactoryBot.create(:photo) - 3.times do - @crop1.plantings.first.photos << @photo - @crop1.plantings.first.save - end - - Crop.interesting.should include @crop1 - Crop.interesting.should_not include @crop2 - Crop.interesting.size.should == 1 + it { is_expected.to include crop1 } + it { is_expected.not_to include crop2 } + it { expect(subject.size).to eq 1 } end - it 'ignores crops without photos' do - # first, a couple of candidate crops - @crop1 = FactoryBot.create(:crop) - @crop2 = FactoryBot.create(:crop) + describe 'crops without photos are not interesting' do + before do + # both crops have plantings + FactoryBot.create_list(:planting, 3, crop: crop1) + FactoryBot.create_list(:planting, 3, crop: crop2) - # both crops have plantings - 3.times do - FactoryBot.create(:planting, crop: @crop1) + # but only crop1 has photos + crop1_planting.photos = FactoryBot.create_list(:photo, 3) end - 3.times do - FactoryBot.create(:planting, crop: @crop2) - end - - # but only crop1 has photos - @photo = FactoryBot.create(:photo) - 3.times do - @crop1.plantings.first.photos << @photo - @crop1.plantings.first.save - end - - Crop.interesting.should include @crop1 - Crop.interesting.should_not include @crop2 - Crop.interesting.size.should == 1 + it { is_expected.to include crop1 } + it { is_expected.not_to include crop2 } + it { expect(subject.size).to eq 1 } end end @@ -349,34 +326,20 @@ describe Crop do let(:pp2) { FactoryBot.create(:plant_part) } context "harvests" do - let(:h1) do - FactoryBot.create(:harvest, - crop: maize, - plant_part: pp1) - end - - let(:h2) do - FactoryBot.create(:harvest, - crop: maize, - plant_part: pp2) - end - + let(:h1) { FactoryBot.create(:harvest, crop: maize, plant_part: pp1) } + let(:h2) { FactoryBot.create(:harvest, crop: maize, plant_part: pp2) } + let!(:crop) { FactoryBot.create(:crop) } + let!(:harvest) { FactoryBot.create(:harvest, crop: crop) } it "has harvests" do - crop = FactoryBot.create(:crop) - harvest = FactoryBot.create(:harvest, crop: crop) - crop.harvests.should eq [harvest] + expect(crop.harvests).to eq [harvest] end end it "doesn't duplicate plant_parts" do @maize = FactoryBot.create(:maize) @pp1 = FactoryBot.create(:plant_part) - @h1 = FactoryBot.create(:harvest, - crop: @maize, - plant_part: @pp1) - @h2 = FactoryBot.create(:harvest, - crop: @maize, - plant_part: @pp1) + @h1 = FactoryBot.create(:harvest, crop: @maize, plant_part: @pp1) + @h2 = FactoryBot.create(:harvest, crop: @maize, plant_part: @pp1) @maize.plant_parts.should eq [@pp1] end diff --git a/spec/views/seeds/show.html.haml_spec.rb b/spec/views/seeds/show.html.haml_spec.rb index 975f2baa1..4feb75d55 100644 --- a/spec/views/seeds/show.html.haml_spec.rb +++ b/spec/views/seeds/show.html.haml_spec.rb @@ -5,6 +5,7 @@ describe "seeds/show" do controller.stub(:current_user) { nil } @seed = FactoryBot.create(:seed) assign(:seed, @seed) + assign(:photos, @seed.photos) end it "renders attributes in
" do