From 7c38f7eca6355366eb29def9ab1f640d718047de Mon Sep 17 00:00:00 2001 From: Brenda Wallace Date: Mon, 9 Dec 2019 10:03:48 +1300 Subject: [PATCH] Pull homepage data directly from elasticsearch (#2316) * Pull homepage data directly from elasticsearch * Removing non-elasticsearch options * [CodeFactor] Apply fixes to commit c46b2e7 --- .travis.yml | 19 ++-- app/models/alternate_name.rb | 2 +- app/models/concerns/crop_search.rb | 34 +++++++ app/models/crop.rb | 27 +----- app/models/scientific_name.rb | 2 +- app/services/crop_search_service.rb | 49 +++++------ app/views/home/_crops.html.haml | 15 +++- app/views/home/index.html.haml | 3 +- config/application.yml.example | 9 -- spec/controllers/crops_controller_spec.rb | 2 +- .../harvests/harvesting_a_crop_spec.rb | 2 +- spec/features/home/home_spec.rb | 2 + spec/features/percy/percy_spec.rb | 1 + .../plantings/planting_a_crop_spec.rb | 2 +- spec/features/plantings/prediction_spec.rb | 2 +- spec/features/plantings/show_spec.rb | 2 +- spec/features/seeds/adding_seeds_spec.rb | 2 +- spec/features/signin_spec.rb | 5 ++ spec/services/crop_search_service_spec.rb | 88 +++++++++---------- spec/spec_helper.rb | 10 +-- spec/support/feature_helpers.rb | 2 +- spec/views/home/index_spec.rb | 2 + 22 files changed, 141 insertions(+), 141 deletions(-) create mode 100644 app/models/concerns/crop_search.rb diff --git a/.travis.yml b/.travis.yml index acb5b3e3d..d073df4eb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,9 +16,8 @@ addons: secure: "PfhLGBKRgNqhKuYCJsK+VPhdAzcgWFGeeOyxC/eS8gtlvIISVdgyZE+r30uIei0DFI6zEiN62eW4d+xtT4j7/e2ZcAcx7U52mza/SnQNuu3nCGQDJB8VOvV5NbnwXfi8vfr4e889Mt7k3ocd2c4gqB4UtRqrzhygj7HN+B/GfEk=" env: matrix: - - GROWSTUFF_ELASTICSEARCH=true RSPEC_TAG=elasticsearch COVERAGE=true ELASTIC_SEARCH_VERSION="6.2.3" - - GROWSTUFF_ELASTICSEARCH=false RSPEC_TAG=~elasticsearch COVERAGE=false - - STATIC_CHECKS=true PERCY=true + - COVERAGE=true + - STATIC_CHECKS=true global: - secure: "Z5TpM2jEX4UCvNePnk/LwltQX48U2u9BRc+Iypr1x9QW2o228QJhPIOH39a8RMUrepGnkQIq9q3ZRUn98RfrJz1yThtlNFL3NmzdQ57gKgjGwfpa0e4Dwj/ZJqV2D84tDGjvdVYLP7zzaYZxQcwk/cgNpzKf/jq97HLNP7CYuf4=" - GROWSTUFF_EMAIL="noreply@test.growstuff.org" @@ -30,29 +29,21 @@ before_install: - sudo apt install dpkg - sudo apt install google-chrome-stable - ./script/install_linters.sh - - ./script/install_elasticsearch.sh + - ELASTIC_SEARCH_VERSION="6.2.3" ./script/install_elasticsearch.sh script: - - set -e - > if [ "${STATIC_CHECKS}" = "true" ]; then ./script/check_static.rb else - set +e; - RAILS_ENV=test bundle exec rake db:create db:migrate search:reindex; - bundle exec rake assets:precompile; - bundle exec rspec -fd --tag $RSPEC_TAG spec/; + RAILS_ENV=test bundle exec rake db:create db:migrate search:reindex assets:precompile; + npx percy exec -- bundle exec rspec spec fi; - - set +e after_script: - > if [ "${COVERAGE}" = "true" ]; then gem install codeclimate-test-reporter codeclimate-test-reporter fi - - > - if [ "${PERCY}" = "true" ]; then - ./scripts/percy.sh - fi before_deploy: - bundle exec script/heroku_maintenance.rb on deploy: diff --git a/app/models/alternate_name.rb b/app/models/alternate_name.rb index 43d0b9c39..f40665b1b 100644 --- a/app/models/alternate_name.rb +++ b/app/models/alternate_name.rb @@ -4,7 +4,7 @@ class AlternateName < ApplicationRecord validates :name, presence: true validates :crop, presence: true - after_commit :reindex if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" + after_commit :reindex delegate :reindex, to: :crop end diff --git a/app/models/concerns/crop_search.rb b/app/models/concerns/crop_search.rb new file mode 100644 index 000000000..d8de5d5cc --- /dev/null +++ b/app/models/concerns/crop_search.rb @@ -0,0 +1,34 @@ +module CropSearch + extend ActiveSupport::Concern + + included do + #################################### + # Elastic search configuration + searchkick word_start: %i(name alternate_names scientific_names), case_sensitive: false + + # Special scope to control if it's in the search index + scope :search_import, -> { includes(:scientific_names, :photos) } + + def should_index? + approved? + end + + def search_data + { + name: name, + slug: slug, + alternate_names: alternate_names.pluck(:name), + scientific_names: scientific_names.pluck(:name), + # boost the crops that are planted the most + plantings_count: plantings_count, + # boost this crop for these members + planters_ids: plantings.pluck(:owner_id), + has_photos: photos.size.positive?, + photo: default_photo&.thumbnail_url, + scientific_name: default_scientific_name&.name, + description: description, + created_at: created_at.to_i + } + end + end +end diff --git a/app/models/crop.rb b/app/models/crop.rb index f60313448..43cbeb974 100644 --- a/app/models/crop.rb +++ b/app/models/crop.rb @@ -2,6 +2,8 @@ class Crop < ApplicationRecord extend FriendlyId include PhotoCapable include OpenFarmData + include CropSearch + friendly_id :name, use: %i(slugged finders) ## @@ -37,9 +39,6 @@ class Crop < ApplicationRecord scope :has_photos, -> { includes(:photos).where.not(photos: { id: nil }) } scope :joins_members, -> { joins("INNER JOIN members ON members.id = harvests.owner_id") } - # Special scope to control if it's in the search index - scope :search_import, -> { approved } - ## ## Validations # Reasons are only necessary when rejecting @@ -54,10 +53,6 @@ class Crop < ApplicationRecord }, if: :approved? - #################################### - # Elastic search configuration - searchkick word_start: %i(name alternate_names scientific_names), case_sensitive: false if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" - def to_s name end @@ -154,24 +149,6 @@ class Crop < ApplicationRecord where(["lower(crops.name) = :value", { value: name.downcase }]) end - def should_index? - approved? - end - - def search_data - { - name: name, - alternate_names: alternate_names.pluck(:name), - scientific_names: scientific_names.pluck(:name), - plantings_count: plantings_count, # boost the crops that are planted the most - planters_ids: plantings.pluck(:owner_id) # boost this product for these members - } - end - - def fetch_from_openfarm! - OpenfarmService.new.update_crop(self) - end - private def count_uses_of_property(col_name) diff --git a/app/models/scientific_name.rb b/app/models/scientific_name.rb index e3b7b911b..b2da3320f 100644 --- a/app/models/scientific_name.rb +++ b/app/models/scientific_name.rb @@ -3,7 +3,7 @@ class ScientificName < ApplicationRecord belongs_to :creator, class_name: 'Member', inverse_of: :created_scientific_names validates :name, presence: true validates :crop, presence: true - after_commit :reindex if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" + after_commit :reindex delegate :reindex, to: :crop def to_s diff --git a/app/services/crop_search_service.rb b/app/services/crop_search_service.rb index c2df12ef5..57c042e48 100644 --- a/app/services/crop_search_service.rb +++ b/app/services/crop_search_service.rb @@ -1,14 +1,6 @@ class CropSearchService # Crop.search(string) def self.search(query, page: 1, per_page: 12, current_member: nil) - if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" - elasticsearch(query, page: page, per_page: per_page, current_member: current_member) - else - dbsearch(query, page: page, per_page: per_page) - end - end - - def self.elasticsearch(query, page: 1, per_page: 12, current_member: nil) search_params = { page: page, per_page: per_page, @@ -24,26 +16,27 @@ class CropSearchService Crop.search(query, search_params) end - def self.dbsearch(query, page: 1, per_page: 12) - # if we don't have elasticsearch, just do a basic SQL query. - # also, make sure it's an actual array not an activerecord - # collection, so it matches what we get from elasticsearch and we can - # manipulate it in the same ways (eg. deleting elements without deleting - # the whole record from the db) - matcher = "%#{query}%" - matches = Crop.approved - .left_outer_joins(:alternate_names, :scientific_names) - .where("crops.name ILIKE ? OR alternate_names.name ILIKE ? OR scientific_names.name ILIKE ?", - matcher, matcher, matcher) + def self.random_with_photos(limit) + body = { + "query": { + "function_score": { + "query": { "query_string": { "query": 'has_photos:true' } }, + "random_score": { "seed": DateTime.now.to_i } + } + } + } + Crop.search( + limit: limit, + load: false, + body: body + ).response['hits']['hits'].map { |c| c['_source'] } + end - matches = matches.to_a - # we want to make sure that exact matches come first, even if not - # using elasticsearch (eg. in development) - exact_match = Crop.approved.find_by(name: query) - if exact_match - matches.delete(exact_match) - matches.unshift(exact_match) - end - matches.paginate(page: page, per_page: per_page) + def self.recent(limit) + Crop.search( + limit: limit, + load: false, + boost_by: { created_at: { factor: 100 } } # default factor is 1 + ).response['hits']['hits'].map { |c| c['_source'] } end end diff --git a/app/views/home/_crops.html.haml b/app/views/home/_crops.html.haml index dd22b01e4..fe4b1492a 100644 --- a/app/views/home/_crops.html.haml +++ b/app/views/home/_crops.html.haml @@ -1,5 +1,16 @@ - cache cache_key_for(Crop, 'homepage'), expires_in: 1.day do .index-cards - - Crop.interesting.includes(:scientific_names, :photos, :parent).shuffle.first(18).each do |crop| - = render 'crops/thumbnail', crop: crop + - CropSearchService.random_with_photos(16).each do |crop| + .card.crop-thumbnail + = link_to crop_path(slug: crop['slug']) do + = image_tag(crop['photo'], + alt: crop['name'], + class: 'img img-card') + + .text + %h3.crop-name + = link_to crop['name'], crop_path(slug: crop['slug']) + %h5.crop-sci-name +   + = crop['scientific_name'] diff --git a/app/views/home/index.html.haml b/app/views/home/index.html.haml index 3a673870d..338cc117e 100644 --- a/app/views/home/index.html.haml +++ b/app/views/home/index.html.haml @@ -26,8 +26,7 @@ - cache cache_key_for(Crop, 'recent') do %h2= t('.recently_added') %p - != Crop.recent.limit(30).map { |c| link_to(c, c) }.join(", ") - + != CropSearchService.recent(30).map { |c| link_to(c['name'], crop_path(slug: c['slug'])) }.join(", ") .col-xl-2.col %section.plantings = cute_icon diff --git a/config/application.yml.example b/config/application.yml.example index 2a38dad4f..fdd60d407 100644 --- a/config/application.yml.example +++ b/config/application.yml.example @@ -57,13 +57,6 @@ GROWSTUFF_FACEBOOK_SECRET: "" GROWSTUFF_MAPBOX_MAP_ID: "" GROWSTUFF_MAPBOX_ACCESS_TOKEN: "" -# Elasticsearch is used for flexible search and it requires another component -# to be installed. To make it easy for people who don't need to test this feature -# it's been turned off for test and development environment as a default. -# If you want to test this functionality, install elasticsearch and -# set this flag to "true". -GROWSTUFF_ELASTICSEARCH: "false" - ############################################################################## # Other environments # You can override the above for staging, production, etc. @@ -83,8 +76,6 @@ test: staging: GROWSTUFF_SITE_NAME: Growstuff (staging) - GROWSTUFF_ELASTICSEARCH: "true" production: GROWSTUFF_SITE_NAME: Growstuff - GROWSTUFF_ELASTICSEARCH: "true" diff --git a/spec/controllers/crops_controller_spec.rb b/spec/controllers/crops_controller_spec.rb index 7efc1a9ba..1bb737c99 100644 --- a/spec/controllers/crops_controller_spec.rb +++ b/spec/controllers/crops_controller_spec.rb @@ -42,7 +42,7 @@ describe CropsController do describe 'fetches the crop search page' do let!(:tomato) { FactoryBot.create :tomato } let!(:maize) { FactoryBot.create :maize } - before { Crop.reindex if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" } + before { Crop.reindex } describe 'search form page' do before { get :search } diff --git a/spec/features/harvests/harvesting_a_crop_spec.rb b/spec/features/harvests/harvesting_a_crop_spec.rb index ca96c2f67..a551d4dfb 100644 --- a/spec/features/harvests/harvesting_a_crop_spec.rb +++ b/spec/features/harvests/harvesting_a_crop_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' require 'custom_matchers' -describe "Harvesting a crop", :js, :elasticsearch do +describe "Harvesting a crop", :js do context 'signed in' do include_context 'signed in member' let!(:maize) { create :maize } diff --git a/spec/features/home/home_spec.rb b/spec/features/home/home_spec.rb index c3ad7d8c9..cbd14b4d6 100644 --- a/spec/features/home/home_spec.rb +++ b/spec/features/home/home_spec.rb @@ -21,6 +21,7 @@ describe "home page" do planting.photos << photo seed.photos << photo harvest.photos << photo + Crop.reindex end before { visit root_path } @@ -54,6 +55,7 @@ describe "home page" do shared_examples "show crops" do describe 'shows crops section' do + before { crop.reindex } it { is_expected.to have_text 'Some of our crops' } it { is_expected.to have_link href: crop_path(crop) } end diff --git a/spec/features/percy/percy_spec.rb b/spec/features/percy/percy_spec.rb index d729d4d76..5cf9973a4 100644 --- a/spec/features/percy/percy_spec.rb +++ b/spec/features/percy/percy_spec.rb @@ -77,6 +77,7 @@ rest of the garden. maize: 'https://farm66.staticflickr.com/65535/46739264475_7cb55b2cbb_q.jpg' }.each do |crop_type, photo_url| crop = FactoryBot.create crop_type, creator: someone_else + crop.reindex owner = FactoryBot.create :interesting_member, login_name: crop_type.to_s.reverse, email: "#{crop.name}@example.com" planting = FactoryBot.create :planting, crop: crop, owner: owner, garden: owner.gardens.first photo = FactoryBot.create(:photo, owner: owner, diff --git a/spec/features/plantings/planting_a_crop_spec.rb b/spec/features/plantings/planting_a_crop_spec.rb index ca8c3dd6d..2ca3439d9 100644 --- a/spec/features/plantings/planting_a_crop_spec.rb +++ b/spec/features/plantings/planting_a_crop_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" require 'custom_matchers' -describe "Planting a crop", :js, :elasticsearch do +describe "Planting a crop", :js do let!(:maize) { FactoryBot.create :maize } let(:garden) { FactoryBot.create :garden, owner: member, name: 'Orchard' } let!(:planting) do diff --git a/spec/features/plantings/prediction_spec.rb b/spec/features/plantings/prediction_spec.rb index b775fd8fd..5f3020a08 100644 --- a/spec/features/plantings/prediction_spec.rb +++ b/spec/features/plantings/prediction_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" require 'custom_matchers' -describe "Display a planting", :js, :elasticsearch do +describe "Display a planting", :js do describe 'planting perennial' do let(:garden) { FactoryBot.create :garden, location: 'Edinburgh' } let(:crop) { FactoryBot.create(:crop, name: 'feijoa', perennial: true) } diff --git a/spec/features/plantings/show_spec.rb b/spec/features/plantings/show_spec.rb index f67abc6e2..864aff5d4 100644 --- a/spec/features/plantings/show_spec.rb +++ b/spec/features/plantings/show_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" require 'custom_matchers' -describe "Display a planting", :js, :elasticsearch do +describe "Display a planting", :js do context 'anonymous' do before { visit planting_path(planting) } diff --git a/spec/features/seeds/adding_seeds_spec.rb b/spec/features/seeds/adding_seeds_spec.rb index 6ec0b1bd3..9ab32788f 100644 --- a/spec/features/seeds/adding_seeds_spec.rb +++ b/spec/features/seeds/adding_seeds_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' require 'custom_matchers' -describe "Seeds", :js, :elasticsearch do +describe "Seeds", :js do context 'signed in' do include_context 'signed in member' let!(:maize) { create :maize } diff --git a/spec/features/signin_spec.rb b/spec/features/signin_spec.rb index 9ff9304fc..5d20b461f 100644 --- a/spec/features/signin_spec.rb +++ b/spec/features/signin_spec.rb @@ -5,6 +5,11 @@ describe "signin", js: true do let(:recipient) { FactoryBot.create :member } let(:wrangler) { FactoryBot.create :crop_wrangling_member } + before do + crop = FactoryBot.create :tomato + crop.reindex + end + def login fill_in 'Login', with: member.login_name fill_in 'Password', with: member.password diff --git a/spec/services/crop_search_service_spec.rb b/spec/services/crop_search_service_spec.rb index 288c9c6bf..f87a1423d 100644 --- a/spec/services/crop_search_service_spec.rb +++ b/spec/services/crop_search_service_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe CropSearchService, type: :service do - describe 'search', :elasticsearch do + describe 'search' do def search(term) CropSearchService.search(term).map(&:name) end @@ -26,7 +26,7 @@ RSpec.describe CropSearchService, type: :service do # Child record FactoryBot.create(:crop, name: 'portobello', parent: mushroom) - Crop.reindex if ENV['GROWSTUFF_ELASTICSEARCH'] == 'true' + Crop.reindex end describe 'finds exact match' do @@ -37,47 +37,45 @@ RSpec.describe CropSearchService, type: :service do it { expect(search('mush')).to eq ['mushroom'] } end - if ENV['GROWSTUFF_ELASTICSEARCH'] == 'true' - describe 'finds mispellings matches' do - it { expect(search('muhsroom')).to eq ['mushroom'] } - it { expect(search('mushrom')).to eq ['mushroom'] } - it { expect(search('zuchini')).to eq ['zucchini'] } - it { expect(search('brocoli')).to eq ['broccoli'] } + describe 'finds mispellings matches' do + it { expect(search('muhsroom')).to eq ['mushroom'] } + it { expect(search('mushrom')).to eq ['mushroom'] } + it { expect(search('zuchini')).to eq ['zucchini'] } + it { expect(search('brocoli')).to eq ['broccoli'] } + end + + describe 'biased' do + # Make some crops with planting counts + let!(:mushroom_parent) { FactoryBot.create :crop, name: 'mushroom' } + let!(:oyster) { FactoryBot.create :crop, name: 'oyster mushroom', parent: mushroom_parent } + let!(:shitake) { FactoryBot.create :crop, name: 'shitake mushroom', parent: mushroom_parent } + let!(:common) { FactoryBot.create :crop, name: 'common mushroom', parent: mushroom_parent } + let!(:brown) { FactoryBot.create :crop, name: 'brown mushroom', parent: mushroom_parent } + let!(:white) { FactoryBot.create :crop, name: 'white mushroom', parent: mushroom_parent } + + describe 'biased to higher planting counts' do + subject { search('mushroom') } + before do + # Having plantings should bring these crops to the top of the search results + FactoryBot.create_list :planting, 10, crop: white + FactoryBot.create_list :planting, 4, crop: shitake + Crop.reindex + end + it { expect(subject.first).to eq 'white mushroom' } + it { expect(subject.second).to eq 'shitake mushroom' } end - - describe 'biased' do - # Make some crops with planting counts - let!(:mushroom_parent) { FactoryBot.create :crop, name: 'mushroom' } - let!(:oyster) { FactoryBot.create :crop, name: 'oyster mushroom', parent: mushroom_parent } - let!(:shitake) { FactoryBot.create :crop, name: 'shitake mushroom', parent: mushroom_parent } - let!(:common) { FactoryBot.create :crop, name: 'common mushroom', parent: mushroom_parent } - let!(:brown) { FactoryBot.create :crop, name: 'brown mushroom', parent: mushroom_parent } - let!(:white) { FactoryBot.create :crop, name: 'white mushroom', parent: mushroom_parent } - - describe 'biased to higher planting counts' do - subject { search('mushroom') } - before do - # Having plantings should bring these crops to the top of the search results - FactoryBot.create_list :planting, 10, crop: white - FactoryBot.create_list :planting, 4, crop: shitake - Crop.reindex - end - it { expect(subject.first).to eq 'white mushroom' } - it { expect(subject.second).to eq 'shitake mushroom' } - end - describe "biased to crops you've planted" do - subject { CropSearchService.search('mushroom', current_member: owner).map(&:name) } - let(:owner) { FactoryBot.create :member } - before do - FactoryBot.create_list :planting, 10, crop: brown - FactoryBot.create :planting, crop: oyster, owner: owner - FactoryBot.create :planting, crop: oyster, owner: owner - FactoryBot.create :planting, crop: shitake, owner: owner - Crop.reindex - end - it { expect(subject.first).to eq oyster.name } - it { expect(subject.second).to eq shitake.name } + describe "biased to crops you've planted" do + subject { CropSearchService.search('mushroom', current_member: owner).map(&:name) } + let(:owner) { FactoryBot.create :member } + before do + FactoryBot.create_list :planting, 10, crop: brown + FactoryBot.create :planting, crop: oyster, owner: owner + FactoryBot.create :planting, crop: oyster, owner: owner + FactoryBot.create :planting, crop: shitake, owner: owner + Crop.reindex end + it { expect(subject.first).to eq oyster.name } + it { expect(subject.second).to eq shitake.name } end end @@ -85,11 +83,9 @@ RSpec.describe CropSearchService, type: :service do it { expect(search('coffee')).to eq [] } end - if ENV['GROWSTUFF_ELASTICSEARCH'] == 'true' - describe 'finds plurals' do - it { expect(search('mushrooms')).to eq ['mushroom'] } - it { expect(search('tomatoes')).to eq ['tomato'] } - end + describe 'finds plurals' do + it { expect(search('mushrooms')).to eq ['mushroom'] } + it { expect(search('tomatoes')).to eq ['tomato'] } end describe 'searches case insensitively' do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6677c8099..df3b6f92b 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -35,13 +35,11 @@ RSpec.configure do |config| end config.before(:suite) do - if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" - # reindex models - Crop.reindex + # reindex models + Crop.reindex - # and disable callbacks - Searchkick.disable_callbacks - end + # and disable callbacks + Searchkick.disable_callbacks end config.around(:each, search: true) do |example| diff --git a/spec/support/feature_helpers.rb b/spec/support/feature_helpers.rb index 0b85fe72a..973718629 100644 --- a/spec/support/feature_helpers.rb +++ b/spec/support/feature_helpers.rb @@ -1,6 +1,6 @@ module FeatureHelpers def fill_autocomplete(field, options = {}) - Crop.reindex if ENV["GROWSTUFF_ELASTICSEARCH"] == "true" + Crop.reindex fill_in field, with: options[:with] end diff --git a/spec/views/home/index_spec.rb b/spec/views/home/index_spec.rb index 61f40d3f1..1289df4eb 100644 --- a/spec/views/home/index_spec.rb +++ b/spec/views/home/index_spec.rb @@ -16,6 +16,8 @@ describe 'home/index.html.haml', type: "view" do assign(:crops, [@crop]) assign(:recent_crops, [@crop]) assign(:seeds, [FactoryBot.create(:tradable_seed)]) + + Crop.reindex end context 'logged out' do