From 19b1240bf85b8d1b95dc8b76deb1685a94b0aaa4 Mon Sep 17 00:00:00 2001
From: Martha Thompson <437455+MothOnMars@users.noreply.github.com>
Date: Tue, 30 Mar 2021 15:25:23 -0400
Subject: [PATCH] SRCH-1951 fix SaytSuggestion.expire (#675)
- fix various Rubocop offenses
---
app/models/sayt_suggestion.rb | 48 +++--
...ayt_suggestions_updated_at_is_protected.rb | 5 +
db/structure.sql | 6 +-
spec/models/sayt_suggestion_spec.rb | 201 ++++++++++++------
4 files changed, 167 insertions(+), 93 deletions(-)
create mode 100644 db/migrate/20210323175952_add_index_on_sayt_suggestions_updated_at_is_protected.rb
diff --git a/app/models/sayt_suggestion.rb b/app/models/sayt_suggestion.rb
index 9aba93e526..9039c8d4aa 100644
--- a/app/models/sayt_suggestion.rb
+++ b/app/models/sayt_suggestion.rb
@@ -1,16 +1,17 @@
-# coding: utf-8
+# frozen_string_literal: true
+
class SaytSuggestion < ApplicationRecord
include Dupable
- LETTERS_WITH_DIACRITIC = "áéíóúÁÉÍÓÚüÜñÑ¿¡"
+ LETTERS_WITH_DIACRITIC = 'áéíóúÁÉÍÓÚüÜñÑ¿¡'
before_validation :squish_whitespace_and_downcase
before_save :set_whitelisted_status
- validates :affiliate, :presence => true
- validates_presence_of :phrase
- validates_uniqueness_of :phrase, :scope => :affiliate_id, :case_sensitive => false
- validates_length_of :phrase, :within => (3..80)
- validates_format_of :phrase, :with => /\A[a-z0-9#{LETTERS_WITH_DIACRITIC}]+([\s_\.'\-]+[a-z0-9#{LETTERS_WITH_DIACRITIC}]+)*\z/iu
+ validates :affiliate, presence: true
+ validates :phrase, presence: true
+ validates :phrase, uniqueness: { scope: :affiliate_id, case_sensitive: false }
+ validates :phrase, length: { within: (3..80) }
+ validates :phrase, format: { with: /\A[a-z0-9#{LETTERS_WITH_DIACRITIC}]+([\s_\.'\-]+[a-z0-9#{LETTERS_WITH_DIACRITIC}]+)*\z/iu }
belongs_to :affiliate
MAX_POPULARITY = 2**30
@@ -18,12 +19,13 @@ class SaytSuggestion < ApplicationRecord
class << self
def related_search(query, affiliate, options = {})
return [] unless affiliate.is_related_searches_enabled?
+
search_options = { affiliate_id: affiliate.id,
language: affiliate.indexing_locale,
size: 5,
q: query }.reverse_merge(options)
- elastic_results = ElasticSaytSuggestion.search_for search_options
- elastic_results.results.collect { |result| result.phrase }
+ elastic_results = ElasticSaytSuggestion.search_for(search_options)
+ elastic_results.results.collect(&:phrase)
end
def fetch_by_affiliate_id(affiliate_id, query, num_of_suggestions)
@@ -36,7 +38,7 @@ def fetch_by_affiliate_id(affiliate_id, query, num_of_suggestions)
end
def populate_for(day, limit)
- name_id_list = Affiliate.select([:id, :name]).collect { |aff| { :name => aff.name, :id => aff.id } }
+ name_id_list = Affiliate.select([:id, :name]).collect { |aff| { name: aff.name, id: aff.id } }
name_id_list.each { |element| populate_for_affiliate_on(element[:name], element[:id], day, limit) }
end
@@ -45,31 +47,35 @@ def populate_for_affiliate_on(affiliate_name, affiliate_id, day, limit)
end
def process_sayt_suggestion_txt_upload(txtfile, affiliate)
- valid_content_types = %w(application/octet-stream text/plain txt)
- if valid_content_types.include? txtfile.content_type
- created, ignored = 0, 0
+ valid_content_types = %w[application/octet-stream text/plain txt]
+ if valid_content_types.include?(txtfile.content_type)
+ created = 0
+ ignored = 0
txtfile.tempfile.readlines.each do |phrase|
entry = phrase.chomp.strip
- unless entry.blank?
- create(:phrase => entry, :affiliate => affiliate, :is_protected => true, :popularity => MAX_POPULARITY).id.nil? ? (ignored += 1) : (created += 1)
+ if entry.present?
+ create(phrase: entry, affiliate: affiliate, is_protected: true, popularity: MAX_POPULARITY).id.nil? ? (ignored += 1) : (created += 1)
end
end
- { :created => created, :ignored => ignored }
+ { created: created, ignored: ignored }
end
end
def expire(days_back)
- destroy_all(["updated_at < ? AND is_protected = ?", days_back.days.ago.beginning_of_day.to_s(:db), false])
+ where(
+ 'updated_at < ? AND is_protected = ?',
+ days_back.days.ago.beginning_of_day.to_s(:db),
+ false
+ ).in_batches.destroy_all
end
-
end
def squish_whitespace_and_downcase
- self.phrase = self.phrase.squish.downcase unless self.phrase.nil?
+ self.phrase = phrase.squish.downcase unless phrase.nil?
end
def spellcheck
- self.phrase = Misspelling.correct(self.phrase) unless self.phrase.nil?
+ self.phrase = Misspelling.correct(phrase) unless phrase.nil?
end
def squish_whitespace_and_downcase_and_spellcheck
@@ -82,6 +88,6 @@ def to_label
end
def set_whitelisted_status
- self.is_whitelisted = true if SaytFilter.filters_match?(SaytFilter.accept, self.phrase)
+ self.is_whitelisted = true if SaytFilter.filters_match?(SaytFilter.accept, phrase)
end
end
diff --git a/db/migrate/20210323175952_add_index_on_sayt_suggestions_updated_at_is_protected.rb b/db/migrate/20210323175952_add_index_on_sayt_suggestions_updated_at_is_protected.rb
new file mode 100644
index 0000000000..b0ee9baae3
--- /dev/null
+++ b/db/migrate/20210323175952_add_index_on_sayt_suggestions_updated_at_is_protected.rb
@@ -0,0 +1,5 @@
+class AddIndexOnSaytSuggestionsUpdatedAtIsProtected < ActiveRecord::Migration[5.2]
+ def change
+ add_index :sayt_suggestions, [:updated_at, :is_protected]
+ end
+end
diff --git a/db/structure.sql b/db/structure.sql
index a60e8d2017..a1bb644056 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -835,7 +835,8 @@ CREATE TABLE `sayt_suggestions` (
`deleted_at` datetime DEFAULT NULL,
`is_whitelisted` tinyint(1) NOT NULL DEFAULT '0',
PRIMARY KEY (`id`),
- UNIQUE KEY `index_sayt_suggestions_on_affiliate_id_and_phrase` (`affiliate_id`,`phrase`)
+ UNIQUE KEY `index_sayt_suggestions_on_affiliate_id_and_phrase` (`affiliate_id`,`phrase`),
+ KEY `index_sayt_suggestions_on_updated_at_and_is_protected` (`updated_at`,`is_protected`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci ROW_FORMAT=DYNAMIC;
/*!40101 SET character_set_client = @saved_cs_client */;
DROP TABLE IF EXISTS `schema_migrations`;
@@ -1931,6 +1932,7 @@ INSERT INTO `schema_migrations` (version) VALUES
('20200121220041'),
('20200212183209'),
('20200728194854'),
-('20210317234859');
+('20210317234859'),
+('20210323175952');
diff --git a/spec/models/sayt_suggestion_spec.rb b/spec/models/sayt_suggestion_spec.rb
index 00e2bd812a..9cac56783b 100644
--- a/spec/models/sayt_suggestion_spec.rb
+++ b/spec/models/sayt_suggestion_spec.rb
@@ -1,102 +1,153 @@
-require 'spec_helper'
+# frozen_string_literal: true
describe SaytSuggestion do
- fixtures :sayt_suggestions, :misspellings, :affiliates
- before do
- @affiliate = affiliates(:power_affiliate)
- @valid_attributes = {
- affiliate_id: @affiliate.id,
+ let(:affiliate) { affiliates(:power_affiliate) }
+ let(:valid_attributes) do
+ {
+ affiliate_id: affiliate.id,
phrase: 'some valid suggestion',
popularity: 100
}
end
+ describe 'schema' do
+ it { is_expected.to have_db_index([:updated_at, :is_protected]) }
+ end
+
describe 'Creating new instance' do
it { is_expected.to belong_to :affiliate }
it { is_expected.to validate_presence_of :affiliate }
it { is_expected.to validate_presence_of :phrase }
it { is_expected.to validate_length_of(:phrase).is_at_least(3).is_at_most(80) }
- ['citizenship[', 'email@address.com', '"over quoted"', 'colon: here', 'http:something', 'site:something', 'intitle:something', "passports'", '.mp3', "' pictures"].each do |phrase|
+
+ ['citizenship[',
+ 'email@address.com',
+ '"over quoted"',
+ 'colon: here',
+ 'http:something',
+ 'site:something',
+ 'intitle:something',
+ "passports'",
+ '.mp3',
+ "' pictures"].each do |phrase|
it { is_expected.not_to allow_value(phrase).for(:phrase) }
end
- ['basic phrase', 'my-name', '1099 form', 'Senator Frank S. Farley State Marina', "Oswald West State Park's Smuggler Cove", 'en español', 'último pronóstico', '¿Qué'].each do |phrase|
+
+ ['basic phrase',
+ 'my-name',
+ '1099 form',
+ 'Senator Frank S. Farley State Marina',
+ "Oswald West State Park's Smuggler Cove",
+ 'en español',
+ 'último pronóstico',
+ '¿Qué'].each do |phrase|
it { is_expected.to allow_value(phrase).for(:phrase) }
end
it 'validates the uniqueness of the phrase scoped to the affiliate id' do
- described_class.create!(@valid_attributes)
- expect(described_class.new(@valid_attributes)).to_not be_valid
+ described_class.create!(valid_attributes)
+ expect(described_class.new(valid_attributes)).to_not be_valid
end
- it 'should create a new instance given valid attributes' do
- described_class.create!(@valid_attributes)
+ it 'creates a new instance given valid attributes' do
+ described_class.create!(valid_attributes)
end
- it 'should downcase the phrase before entering into DB' do
- described_class.create!(phrase: 'ALL CAPS', affiliate: @affiliate)
- expect(described_class.find_by_phrase('all caps').phrase).to eq('all caps')
+ it 'downcases the phrase before entering into DB' do
+ described_class.create!(phrase: 'ALL CAPS', affiliate: affiliate)
+ expect(described_class.find_by(phrase: 'all caps').phrase).to eq('all caps')
end
- it 'should strip whitespace from phrase before inserting in DB' do
+ it 'strips whitespace from phrase before inserting in DB' do
phrase = ' leading and trailing whitespaces '
- sf = described_class.create!(phrase: phrase, affiliate: @affiliate)
+ sf = described_class.create!(phrase: phrase, affiliate: affiliate)
expect(sf.phrase).to eq(phrase.strip)
end
- it 'should squish multiple whitespaces between words in the phrase before entering into DB' do
- described_class.create!(phrase: 'two spaces', affiliate: @affiliate)
- expect(described_class.find_by_phrase('two spaces').phrase).to eq('two spaces')
+ it 'squishes multiple whitespaces between words in the phrase before entering into DB' do
+ described_class.create!(phrase: 'two spaces', affiliate: affiliate)
+ expect(described_class.find_by(phrase: 'two spaces').phrase).to eq('two spaces')
end
- it 'should not correct misspellings before entering in DB if the suggestion belongs to an affiliate' do
+ it 'does not correct misspellings before entering in DB if the suggestion belongs to an affiliate' do
described_class.create!(phrase: 'barack ubama', affiliate: affiliates(:basic_affiliate))
- expect(described_class.find_by_phrase('barack ubama')).not_to be_nil
+ expect(described_class.find_by(phrase: 'barack ubama')).not_to be_nil
end
- it 'should default popularity to 1 if not specified' do
- described_class.create!(phrase: 'popular', affiliate: @affiliate)
- expect(described_class.find_by_phrase('popular').popularity).to eq(1)
+ it 'defaults popularity to 1 if not specified' do
+ described_class.create!(phrase: 'popular', affiliate: affiliate)
+ expect(described_class.find_by(phrase: 'popular').popularity).to eq(1)
end
- it 'should default protected status to false' do
- suggestion = described_class.create!(phrase: 'unprotected', affiliate: @affiliate)
+ it 'defaults protected status to false' do
+ suggestion = described_class.create!(phrase: 'unprotected', affiliate: affiliate)
expect(suggestion.is_protected).to be false
end
- it 'should not create a new suggestion if one exists, but is marked as deleted' do
- described_class.create!(phrase: 'deleted', affiliate: @affiliate, deleted_at: Time.now)
- expect(described_class.create(phrase: 'deleted', affiliate: @affiliate).id).to be_nil
+ it 'does not create a new suggestion if one exists, but is marked as deleted' do
+ described_class.create!(
+ phrase: 'deleted',
+ affiliate: affiliate,
+ deleted_at: Time.current
+ )
+ expect(described_class.create(phrase: 'deleted', affiliate: affiliate).id).to be_nil
end
end
describe 'saving an instance' do
before do
- SaytFilter.create!(phrase: 'accept me', is_regex: false, filter_only_exact_phrase: false, accept: true)
+ SaytFilter.create!(
+ phrase: 'accept me',
+ is_regex: false,
+ filter_only_exact_phrase: false,
+ accept: true
+ )
end
- it 'should set the is_whitelisted flag accordingly' do
- ss = described_class.create!(phrase: 'accept me please', affiliate: @affiliate, deleted_at: Time.now)
+ it 'sets the is_whitelisted flag accordingly' do
+ ss = described_class.create!(
+ phrase: 'accept me please', affiliate: affiliate, deleted_at: Time.current
+ )
expect(ss.is_whitelisted).to be true
- ss = described_class.create!(phrase: 'not me please', affiliate: @affiliate, deleted_at: Time.now)
+ ss = described_class.create!(
+ phrase: 'not me please', affiliate: affiliate, deleted_at: Time.current
+ )
expect(ss.is_whitelisted).to be false
end
end
- describe '#expire(days_back)' do
- it 'should destroy suggestions that have not been updated in X days, and that are unprotected' do
- expect(described_class).to receive(:destroy_all).with(['updated_at < ? AND is_protected = ?', 30.days.ago.beginning_of_day.to_s(:db), false])
- described_class.expire(30)
+ describe '.expire(days_back)' do
+ subject(:expire) { described_class.expire(30) }
+
+ context 'when suggestions exist' do
+ before do
+ described_class.create!(
+ valid_attributes.merge(phrase: 'outdated', updated_at: 31.days.ago)
+ )
+ described_class.create!(
+ valid_attributes.merge(phrase: 'outdated but protected',
+ is_protected: true,
+ updated_at: 31.days.ago)
+ )
+ described_class.create!(valid_attributes.merge(phrase: 'new'))
+ end
+
+ it 'destroys unprotected suggestions that have not been updated in X days' do
+ expire
+ expect(affiliate.sayt_suggestions.pluck(:phrase)).
+ to eq ['new', 'outdated but protected']
+ end
end
end
describe '#populate_for(day, limit = nil)' do
- it 'should populate SAYT suggestions for all affiliates in affiliate table' do
+ it 'populates SAYT suggestions for all affiliates in affiliate table' do
Affiliate.all.each do |aff|
- expect(described_class).to receive(:populate_for_affiliate_on).with(aff.name, aff.id, Date.current, 100)
+ expect(described_class).to receive(:populate_for_affiliate_on).
+ with(aff.name, aff.id, Date.current, 100)
end
described_class.populate_for(Date.current, 100)
end
-
end
describe '#populate_for_affiliate_on(affiliate_name, affiliate_id, day, limit)' do
@@ -106,22 +157,21 @@
let(:aff) { affiliates(:basic_affiliate) }
- it 'should enqueue the affiliate for processing' do
+ it 'enqueues the affiliate for processing' do
described_class.populate_for_affiliate_on(aff.name, aff.id, Date.current, 100)
expect(SaytSuggestionDiscovery).to have_queued(aff.name, aff.id, Date.current, 100)
end
-
end
describe '#fetch_by_affiliate_id(affiliate_id, query, num_suggestions)' do
let(:affiliate) { affiliates(:power_affiliate) }
- it 'should return empty array if there is no matching suggestion' do
+ it 'returns empty array if there is no matching suggestion' do
described_class.create!(phrase: 'child', popularity: 10, affiliate_id: affiliate.id)
expect(described_class.fetch_by_affiliate_id(affiliate.id, 'kids', 10)).to be_empty
end
- it 'should return records for that affiliate_id' do
+ it 'returns records for that affiliate_id' do
described_class.create!(phrase: 'child', popularity: 10, affiliate_id: affiliate.id)
described_class.create!(phrase: 'child care', popularity: 1, affiliate_id: affiliate.id)
described_class.create!(phrase: 'children', popularity: 100, affiliate_id: affiliate.id)
@@ -138,7 +188,7 @@
described_class.create!(phrase: 'children', popularity: 100, affiliate_id: affiliate.id)
end
- it 'should return at most num_suggestions results' do
+ it 'returns at most num_suggestions results' do
expect(described_class.fetch_by_affiliate_id(affiliate.id, 'child', 2).count).to eq(2)
end
end
@@ -150,7 +200,7 @@
described_class.create!(phrase: 'children', popularity: 100, affiliate_id: affiliate.id)
end
- it 'should return results in order of popularity' do
+ it 'returns results in order of popularity' do
suggestions = described_class.fetch_by_affiliate_id(affiliate.id, 'child', 10)
expect(suggestions.first.phrase).to eq('children')
expect(suggestions.last.phrase).to eq('child care')
@@ -164,7 +214,7 @@
described_class.create!(phrase: 'eliz ggg', popularity: 100, affiliate_id: affiliate.id)
end
- it 'should return results in alphabetical order' do
+ it 'returns results in alphabetical order' do
suggestions = described_class.fetch_by_affiliate_id(affiliate.id, 'eliz', 3)
expect(suggestions.first.phrase).to eq('eliz aaa')
expect(suggestions.last.phrase).to eq('eliz hhh')
@@ -173,52 +223,63 @@
end
describe '#process_sayt_suggestion_txt_upload' do
- fixtures :affiliates
+ let(:affiliate) { affiliates(:basic_affiliate) }
let(:content_type) { 'text/plain' }
-
- before do
- @affiliate = affiliates(:basic_affiliate)
- @phrases = %w{ one two three }
- tempfile = File.open('spec/fixtures/txt/sayt_suggestions.txt')
- @file = Rack::Test::UploadedFile.new(tempfile, content_type)
- @dummy_suggestion = described_class.create(phrase: 'dummy suggestions')
+ let(:phrases) { %w[one two three] }
+ let(:dummy_suggestion) { described_class.create(phrase: 'dummy suggestions') }
+ let(:file) do
+ Rack::Test::UploadedFile.new(
+ open_fixture_file('/txt/sayt_suggestions.txt'),
+ content_type
+ )
end
- it 'should create SAYT suggestions using the affiliate provided, if provided' do
- @phrases.each do |phrase|
- expect(described_class).to receive(:create).with({phrase: phrase, affiliate: @affiliate, is_protected: true, popularity: SaytSuggestion::MAX_POPULARITY}).and_return @dummy_suggestion
+ it 'creates SAYT suggestions using the affiliate provided, if provided' do
+ phrases.each do |phrase|
+ expect(described_class).to receive(:create).with(
+ phrase: phrase,
+ affiliate: affiliate,
+ is_protected: true,
+ popularity: SaytSuggestion::MAX_POPULARITY
+ ).and_return dummy_suggestion
end
- described_class.process_sayt_suggestion_txt_upload(@file, @affiliate)
+ described_class.process_sayt_suggestion_txt_upload(file, affiliate)
end
end
describe '#to_label' do
- it 'should return the phrase' do
- expect(described_class.new(phrase: 'dummy suggestion', affiliate: @affiliate).to_label).to eq('dummy suggestion')
+ subject(:to_label) { suggestion.to_label }
+
+ let(:suggestion) do
+ described_class.new(phrase: 'dummy suggestion', affiliate: affiliate)
+ end
+
+ it 'returns the phrase' do
+ expect(to_label).to eq('dummy suggestion')
end
end
describe '#related_search' do
+ let(:affiliate) { affiliates(:basic_affiliate) }
+
before do
- @affiliate = affiliates(:basic_affiliate)
described_class.destroy_all
- described_class.create!(affiliate_id: @affiliate.id, phrase: 'suggest me', popularity: 30)
+ described_class.create!(affiliate_id: affiliate.id, phrase: 'suggest me', popularity: 30)
ElasticSaytSuggestion.commit
end
- it 'should return an array of highlighted strings' do
- expect(described_class.related_search('suggest', @affiliate)).to eq(['suggest me'])
+ it 'returns an array of highlighted strings' do
+ expect(described_class.related_search('suggest', affiliate)).to eq(['suggest me'])
end
context 'when affiliate has related searches disabled' do
before do
- @affiliate.is_related_searches_enabled = false
+ affiliate.is_related_searches_enabled = false
end
- it 'should return an empty array' do
- expect(described_class.related_search('suggest', @affiliate)).to eq([])
+ it 'returns an empty array' do
+ expect(described_class.related_search('suggest', affiliate)).to eq([])
end
end
-
end
end