From 676345815c64a5f6497c482b44e7dfd92a4caadc Mon Sep 17 00:00:00 2001 From: Brenda Wallace Date: Tue, 31 Dec 2019 21:01:57 +1300 Subject: [PATCH] tests for photos searching --- app/models/concerns/search_photos.rb | 26 ++--- app/models/crop.rb | 6 +- app/models/photo.rb | 24 +++-- app/models/photo_association.rb | 17 +++- spec/models/photo_spec.rb | 137 +++++++++++++++++++-------- 5 files changed, 143 insertions(+), 67 deletions(-) diff --git a/app/models/concerns/search_photos.rb b/app/models/concerns/search_photos.rb index 04f4b0d2b..5bf48ec67 100644 --- a/app/models/concerns/search_photos.rb +++ b/app/models/concerns/search_photos.rb @@ -11,19 +11,23 @@ module SearchPhotos } } - scope :search_import, -> { includes(:owner, :crops, :plantings, :harvests, :seeds, :posts) } - def search_data { - id: id, - title: title, - crops: photo_associations.map(&:crop_id), - owner_id: owner_id, - owner_login_name: owner.login_name, - likes_count: likes_count, - thumbnail_url: thumbnail_url, - fullsize_url: fullsize_url, - created_at: created_at.to_i + id: id, + title: title, + thumbnail_url: thumbnail_url, + fullsize_url: fullsize_url, + # crops + crops: crops.pluck(:id), + # likes + liked_by_members_names: liked_by_members_names, + # owner + owner_id: owner_id, + owner_login_name: owner.login_name, + # counts + likes_count: likes_count, + + created_at: created_at.to_i } end end diff --git a/app/models/crop.rb b/app/models/crop.rb index d983dff58..c5de8aa2c 100644 --- a/app/models/crop.rb +++ b/app/models/crop.rb @@ -18,7 +18,7 @@ class Crop < ApplicationRecord has_many :plantings, dependent: :destroy has_many :seeds, dependent: :destroy has_many :harvests, dependent: :destroy - has_many :photo_associations, dependent: :delete_all + has_many :photo_associations, dependent: :delete_all, inverse_of: :crop has_many :photos, through: :photo_associations has_many :plant_parts, -> { joins_members.distinct.order("plant_parts.name") }, through: :harvests has_many :varieties, class_name: 'Crop', foreign_key: 'parent_id', dependent: :nullify, inverse_of: :parent @@ -50,10 +50,10 @@ class Crop < ApplicationRecord ## Wikipedia urls are only necessary when approving a crop validates :en_wikipedia_url, format: { - with: %r{\Ahttps?:\/\/en\.wikipedia\.org\/wiki\/[[:alnum:]%_\.()-]+\z}, + with: %r{\Ahttps?:\/\/en\.wikipedia\.org\/wiki\/[[:alnum:]%_\.()-]+\z}, message: 'is not a valid English Wikipedia URL' }, - if: :approved? + if: :approved? def to_s name diff --git a/app/models/photo.rb b/app/models/photo.rb index 7518cc80d..0c407bc1e 100644 --- a/app/models/photo.rb +++ b/app/models/photo.rb @@ -8,7 +8,13 @@ class Photo < ApplicationRecord PHOTO_CAPABLE = %w(Garden Planting Harvest Seed Post Crop).freeze has_many :photo_associations, foreign_key: :photo_id, dependent: :delete_all, inverse_of: :photo - has_many :crops, through: :photo_associations, counter_cache: true + + # This doesn't work, ActiveRecord tries to use the polymoriphinc photographable + # relationship instead. + # has_many :crops, through: :photo_associations + def crops + Crop.distinct.joins(:photo_associations).where(photo_associations: { photo: self }) + end validates :fullsize_url, url: true validates :thumbnail_url, url: true @@ -16,8 +22,8 @@ class Photo < ApplicationRecord # creates a relationship for each assignee type PHOTO_CAPABLE.each do |type| has_many type.downcase.pluralize.to_s.to_sym, - through: :photo_associations, - source: :photographable, + through: :photo_associations, + source: :photographable, source_type: type end @@ -36,13 +42,13 @@ class Photo < ApplicationRecord licenses = flickr.photos.licenses.getInfo license = licenses.find { |l| l.id == info.license } { - title: calculate_title(info), - license_name: license.name, - license_url: license.url, + title: calculate_title(info), + license_name: license.name, + license_url: license.url, thumbnail_url: FlickRaw.url_q(info), - fullsize_url: FlickRaw.url_z(info), - link_url: FlickRaw.url_photopage(info), - date_taken: info.dates.taken + fullsize_url: FlickRaw.url_z(info), + link_url: FlickRaw.url_photopage(info), + date_taken: info.dates.taken } end diff --git a/app/models/photo_association.rb b/app/models/photo_association.rb index 647c1bb78..9f92160fb 100644 --- a/app/models/photo_association.rb +++ b/app/models/photo_association.rb @@ -1,20 +1,23 @@ # frozen_string_literal: true class PhotoAssociation < ApplicationRecord - belongs_to :photo, inverse_of: :photo_associations + belongs_to :photo, touch: true + belongs_to :crop, optional: true, touch: true #, counter_cache: true belongs_to :photographable, polymorphic: true, touch: true - belongs_to :crop, optional: true, counter_cache: true validate :photo_and_item_have_same_owner + validate :crop_present ## ## Triggers - before_save :set_crop + before_validation :set_crop def self.item(item_id, item_type) find_by!(photographable_id: item_id, photographable_type: item_type).photographable end + private + def set_crop if %w(Planting Seed Harvest).include?(photographable_type) self.crop_id = photographable.crop_id @@ -23,11 +26,15 @@ class PhotoAssociation < ApplicationRecord end end - private - def photo_and_item_have_same_owner return if photographable_type == 'Crop' errors.add(:photo, "must have same owner as item it links to") unless photographable.owner_id == photo.owner_id end + + def crop_present + if %w(Planting Seed Harvest).include?(photographable_type) + errors.add(:crop_id, "failed to calculate crop") unless crop_id.present? + end + end end diff --git a/spec/models/photo_spec.rb b/spec/models/photo_spec.rb index 85986c01c..7eb917f35 100644 --- a/spec/models/photo_spec.rb +++ b/spec/models/photo_spec.rb @@ -3,8 +3,7 @@ require 'rails_helper' describe Photo do - let(:photo) { FactoryBot.create(:photo, owner: member) } - let(:old_photo) { FactoryBot.create(:photo, owner: member, created_at: 1.year.ago, date_taken: 2.years.ago) } + let(:photo) { FactoryBot.create(:photo, :reindex, owner: member) } let(:member) { FactoryBot.create(:member) } it_behaves_like "it is likeable" @@ -20,13 +19,14 @@ describe Photo do describe 'to a planting' do before { planting.photos << photo } - it { expect(planting.photos.size).to eq 1 } + it { expect(planting.photos.count).to eq 1 } it { expect(planting.photos.first).to eq photo } # there's only one photo, so that's the default it { expect(planting.default_photo).to eq photo } it { expect(planting.crop.default_photo).to eq photo } describe 'with a second older photo' do + let(:old_photo) { FactoryBot.create(:photo, owner: member, created_at: 1.year.ago, date_taken: 2.years.ago) } # Add an old photo before { planting.photos << old_photo } it { expect(planting.default_photo).to eq photo } @@ -40,28 +40,45 @@ describe Photo do end end - it 'to a harvest' do - harvest.photos << photo - expect(harvest.photos.size).to eq 1 - expect(harvest.photos.first).to eq photo + describe 'to a harvest' do + let(:crop){ harvest.crop } + before { harvest.photos << photo } + + it { expect(harvest.photos).to eq [photo] } + it { expect(harvest.photo_associations.count).to eq 1 } + + # Check the relationship from crop + it { expect(crop.photo_associations.count).to eq 1 } + it { expect(crop.photos.count).to eq 1 } + it { expect(crop.photos).to eq [photo] } + + # Check the relationship from the photo + it { expect(photo.photo_associations.count).to eq 1 } + it { expect(photo.photo_associations.map(&:crop)).to eq [ crop ]} + it { expect(photo.crops.count).to eq 1 } + it { expect(photo.crops).to eq [crop] } end it 'to a seed' do seed.photos << photo - expect(seed.photos.size).to eq 1 - expect(seed.photos.first).to eq photo + expect(seed.photos).to eq [photo] + expect(photo.crops).to eq [seed.crop] + end + + it 'to a planting' do + planting.photos << photo + expect(planting.photos).to eq [photo] + expect(photo.crops).to eq [planting.crop] end it 'to a garden' do garden.photos << photo - expect(garden.photos.size).to eq 1 - expect(garden.photos.first).to eq photo + expect(garden.photos).to eq [photo] end it 'to a post' do post.photos << photo - expect(post.photos.size).to eq 1 - expect(post.photos.first).to eq photo + expect(post.photos).to eq [photo] end end @@ -69,19 +86,19 @@ describe Photo do it 'from a planting' do planting.photos << photo photo.destroy - expect(planting.photos.size).to eq 0 + expect(planting.photos.count).to eq 0 end it 'from a harvest' do harvest.photos << photo photo.destroy - expect(harvest.photos.size).to eq 0 + expect(harvest.photos.count).to eq 0 end it 'from a garden' do garden.photos << photo photo.destroy - expect(garden.photos.size).to eq 0 + expect(garden.photos.count).to eq 0 end it "automatically if unused" do @@ -117,23 +134,23 @@ describe Photo do planting.photos << photo harvest.photos << photo garden.photos << photo - expect(photo.plantings.size).to eq 1 - expect(photo.harvests.size).to eq 1 - expect(photo.gardens.size).to eq 1 + expect(photo.plantings.count).to eq 1 + expect(photo.harvests.count).to eq 1 + expect(photo.gardens.count).to eq 1 planting.destroy # photo is still used by harvest and garden photo.reload - expect(photo.plantings.size).to eq 0 - expect(photo.harvests.size).to eq 1 + expect(photo.plantings.count).to eq 0 + expect(photo.harvests.count).to eq 1 harvest.destroy garden.destroy # photo is now no longer used by anything photo.reload - expect(photo.plantings.size).to eq 0 - expect(photo.harvests.size).to eq 0 - expect(photo.gardens.size).to eq 0 + expect(photo.plantings.count).to eq 0 + expect(photo.harvests.count).to eq 0 + expect(photo.gardens.count).to eq 0 photo.destroy_if_unused expect(-> { photo.reload }).to raise_error ActiveRecord::RecordNotFound end @@ -163,16 +180,16 @@ describe Photo do expect(Photo.joins(:owner).all).not_to include(photo) end - describe 'scopes' do - let(:harvest_crop) { FactoryBot.create :crop } + describe 'assocations' do + let(:harvest_crop) { FactoryBot.create :crop, name: 'harvest_crop' } let!(:harvest) { FactoryBot.create :harvest, owner: member, crop: harvest_crop } let!(:harvest_photo) { FactoryBot.create :photo, owner: member } - let(:planting_crop) { FactoryBot.create :crop } + let(:planting_crop) { FactoryBot.create :crop, name: 'planting_crop' } let!(:planting) { FactoryBot.create :planting, owner: member, crop: planting_crop } let!(:planting_photo) { FactoryBot.create :photo, owner: member } - let(:seed_crop) { FactoryBot.create :crop } + let(:seed_crop) { FactoryBot.create :crop, name: 'seed_crop' } let!(:seed) { FactoryBot.create :seed, owner: member, crop: seed_crop } let!(:seed_photo) { FactoryBot.create :photo, owner: member } @@ -180,28 +197,70 @@ describe Photo do harvest.photos << harvest_photo planting.photos << planting_photo seed.photos << seed_photo + + # harvest_photo.reload + # harvest.reload + # # harvest.reindex + + # planting_photo.reload + # planting.reload + # # planting.reindex + + # seed_photo.reload + # seed.reload + # seed.reindex end - it { expect(Photo.by_model(Harvest)).to eq([harvest_photo]) } - it { expect(Photo.by_model(Planting)).to eq([planting_photo]) } - it { expect(Photo.by_model(Seed)).to eq([seed_photo]) } + describe 'relationships' do + it { expect(seed_photo.crops).to eq [seed_crop] } + it { expect(seed_crop.photos).to eq [seed_photo] } - it { expect(Photo.by_crop(harvest_crop)).to eq([harvest_photo]) } - it { expect(Photo.by_crop(planting_crop)).to eq([planting_photo]) } - it { expect(Photo.by_crop(seed_crop)).to eq([seed_photo]) } + it { expect(harvest_photo.crops).to eq [harvest_crop] } + it { expect(harvest_crop.photos).to eq [harvest_photo] } + + it { expect(planting_photo.crops).to eq [planting_crop] } + it { expect(planting_crop.photos).to eq [planting_photo] } + end + + describe 'scopes' do + it { expect(Photo.by_model(Harvest)).to eq([harvest_photo]) } + it { expect(Photo.by_model(Planting)).to eq([planting_photo]) } + it { expect(Photo.by_model(Seed)).to eq([seed_photo]) } + + it { expect(Photo.by_crop(harvest_crop)).to eq([harvest_photo]) } + it { expect(Photo.by_crop(planting_crop)).to eq([planting_photo]) } + it { expect(Photo.by_crop(seed_crop)).to eq([seed_photo]) } + end end describe 'Elastic search indexing', search: true do - describe "finds new photos in search index" do - let!(:photo) { FactoryBot.create :photo, :reindex } - before { Photo.searchkick_index.refresh } + let!(:planting) { FactoryBot.create(:planting, :reindex, owner: photo.owner) } + let!(:crop) { FactoryBot.create :crop, :reindex } + before do + planting.photos << photo + Photo.reindex + Photo.searchkick_index.refresh + end + + describe "finds all photos in search index" do it "finds just one" do - expect(Photo.search.size).to eq 1 + expect(Photo.search.count).to eq 1 end it "finds the matching photo" do - expect(Photo.search.first.id).to eq photo.id + expect(Photo.search).to include photo end + + it "retrieves crops from ES" do + expect(Photo.search(load: false).first.crops).to eq [planting.crop.id] + end + end + + it "finds photos by owner in search index" do + expect(Photo.search(where: { owner_id: planting.owner_id })).to include photo + end + it "finds photos by crop in search index" do + expect(Photo.search(where: { crops: planting.crop.id })).to include photo end end end