Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed alert emails #7318

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ group :test do
gem "rack_session_access"
gem "selenium-webdriver"
gem "shoulda-matchers"
gem "uri-query_params"
gem "vcr"
gem "webmock"
end
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,7 @@ GEM
unicode (0.4.4.5)
unicode-display_width (2.6.0)
uri (1.0.2)
uri-query_params (0.8.2)
useragent (0.16.10)
valid_email2 (7.0.0)
activemodel (>= 6.0)
Expand Down Expand Up @@ -860,6 +861,7 @@ DEPENDENCIES
slim_lint
solargraph
tzinfo-data
uri-query_params
valid_email2
validate_url
vcr
Expand Down
19 changes: 10 additions & 9 deletions app/jobs/alert_email/base.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
class AlertEmail::Base < ApplicationJob
MAXIMUM_RESULTS_PER_RUN = 500
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed? I only see this removed call

Copy link
Contributor Author

@starswan starswan Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was - I've put the usage back.


def perform
return if DisableExpensiveJobs.enabled?

subscriptions.find_each do |subscription|
next if subscription.alert_run_today?
# The intent here is that if we don't have keyword or location searches, then this operation can all be done in memory
# really fast (1 week's worth of vacancies is around 2000, so not worth leaving on disk for each of 100k daily subscriptions
default_scope = Vacancy.includes(:organisations).live.order(publish_on: :desc).search_by_filter(from_date: from_date, to_date: Date.current)

already_run_ids = AlertRun.for_today.pluck(:subscription_id)

vacancies = vacancies_for_subscription(subscription)
next if vacancies.blank?
subscriptions.find_each.reject { |sub| already_run_ids.include?(sub.id) }.each do |subscription|
vacancies = subscription.vacancies_matching(default_scope).first(MAXIMUM_RESULTS_PER_RUN)

Jobseekers::AlertMailer.alert(subscription.id, vacancies.pluck(:id)).deliver_later
Jobseekers::AlertMailer.alert(subscription.id, vacancies.pluck(:id)).deliver_later if vacancies.any?
end
Sentry.capture_message("#{self.class.name} run successfully", level: :info)
end

def vacancies_for_subscription(subscription)
subscription.vacancies_for_range(from_date, Date.current)
end
end
2 changes: 2 additions & 0 deletions app/models/alert_run.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
class AlertRun < ApplicationRecord
enum :status, { pending: 0, sent: 1 }
belongs_to :subscription

scope :for_today, -> { where(run_on: Date.current) }
end
3 changes: 0 additions & 3 deletions app/models/location_polygon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ class LocationPolygon < ApplicationRecord

# Scope that expands any polygons returned by subsequent scopes by the provided radius
# by overriding the `area` attribute with a buffered version of itself
#
# TODO: This is a "clever" temporary solution to allow us to generate expanded polygons
# to send to Algolia, and should be removed once we search through ActiveRecord.
scope :buffered, ->(radius_in_miles) { select("*, ST_Buffer(area, #{convert_miles_to_metres(radius_in_miles || 0)}) AS area") }

def self.with_name(location)
Expand Down
61 changes: 54 additions & 7 deletions app/models/subscription.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
class Subscription < ApplicationRecord
MAXIMUM_RESULTS_PER_RUN = 500

enum :frequency, { daily: 0, weekly: 1 }

has_many :alert_runs, dependent: :destroy
Expand All @@ -10,6 +8,18 @@ class Subscription < ApplicationRecord

validates :email, email_address: true, if: -> { email_changed? } # Allows data created prior to validation to still be valid

FILTERS = {
teaching_job_roles: ->(vacancy, value) { vacancy.job_roles.intersect?(value) },
support_job_roles: ->(vacancy, value) { vacancy.job_roles.intersect?(value) },
visa_sponsorship_availability: ->(vacancy, value) { value.include? vacancy.visa_sponsorship_available.to_s },
ect_statuses: ->(vacancy, value) { value.include?(vacancy.ect_status) },
subjects: ->(vacancy, value) { vacancy.subjects.intersect?(value) },
phases: ->(vacancy, value) { vacancy.phases.intersect?(value) },
working_patterns: ->(vacancy, value) { vacancy.working_patterns.intersect?(value) },
organisation_slug: ->(vacancy, value) { vacancy.organisations.map(&:slug).include?(value) },
keyword: ->(vacancy, value) { vacancy.searchable_content.include? value.downcase.strip },
}.freeze

def self.encryptor(serializer: :json_allow_marshal)
key_generator_secret = SUBSCRIPTION_KEY_GENERATOR_SECRET
key_generator_salt = SUBSCRIPTION_KEY_GENERATOR_SALT
Expand Down Expand Up @@ -41,11 +51,6 @@ def unsubscribe
update(email: nil, active: false, unsubscribed_at: Time.current)
end

def vacancies_for_range(date_from, date_to)
criteria = search_criteria.symbolize_keys.merge(from_date: date_from, to_date: date_to)
Search::VacancySearch.new(criteria).vacancies.limit(MAXIMUM_RESULTS_PER_RUN)
end

def alert_run_today
alert_runs.find_by(run_on: Date.current)
end
Expand All @@ -61,4 +66,46 @@ def create_alert_run
def organisation
Organisation.find_by(slug: search_criteria["organisation_slug"]) if search_criteria["organisation_slug"]
end

def vacancies_matching(default_scope)
scope = default_scope
criteria = search_criteria.symbolize_keys
scope, criteria = handle_location(scope, criteria)

scope.select do |vacancy|
criteria.all? { |criterion, value| FILTERS.fetch(criterion).call(vacancy, value) }
end
end

private

extend DistanceHelper

class << self
def limit_by_location(vacancies, location, radius_in_miles)
query = location.strip.downcase
if query.blank? || LocationQuery::NATIONWIDE_LOCATIONS.include?(query)
vacancies
else
polygon = LocationPolygon.buffered(radius_in_miles).with_name(query)
if polygon.present?
vacancies.select { |v| v.organisations.map(&:geopoint).any? { |point| polygon.area.contains?(point) } }
else
radius_in_metres = convert_miles_to_metres radius_in_miles
coordinates = Geocoding.new(query).coordinates
search_point = RGeo::Geographic.spherical_factory.point(coordinates.second, coordinates.first)
vacancies.select { |v| v.organisations.map(&:geopoint).any? { |point| search_point.distance(point) < radius_in_metres } }
end
end
end
end

def handle_location(scope, criteria)
if criteria.key?(:location)
[self.class.limit_by_location(scope, criteria[:location], criteria[:radius]), criteria.except(:location, :radius)]

else
[scope, criteria]
end
end
end
5 changes: 3 additions & 2 deletions app/services/search/vacancy_search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@ class Search::VacancySearch

attr_reader :search_criteria, :keyword, :location, :radius, :organisation_slug, :sort

def initialize(search_criteria, sort: nil)
def initialize(search_criteria, sort: nil, scope: Vacancy.live)
@search_criteria = search_criteria
@keyword = search_criteria[:keyword]
@location = search_criteria[:location]
@radius = search_criteria[:radius]
@organisation_slug = search_criteria[:organisation_slug]
@sort = sort || Search::VacancySort.new(keyword: keyword, location: location)
@scope = scope
end

def active_criteria
Expand Down Expand Up @@ -56,7 +57,7 @@ def total_count

def scope
sort_by_distance = sort.by == "distance"
scope = Vacancy.live.includes(:organisations)
scope = @scope.includes(:organisations)
scope = scope.where(id: organisation.all_vacancies.pluck(:id)) if organisation
scope = scope.search_by_location(location, radius, polygon:, sort_by_distance:) if location
scope = scope.search_by_filter(search_criteria) if search_criteria.any?
Expand Down
2 changes: 1 addition & 1 deletion config/initializers/geocoder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
http_headers: { "User-Agent" => "Teaching Vacancies Service [email protected]" },

google: {
api_key: Rails.env.test? ? "placeholder_key" : ENV.fetch("GOOGLE_LOCATION_SEARCH_API_KEY", ""),
api_key: ENV.fetch("GOOGLE_LOCATION_SEARCH_API_KEY", "placeholder_key"),
always_raise: [Geocoder::OverQueryLimitError],
},

Expand Down
10 changes: 2 additions & 8 deletions spec/controllers/publishers/vacancies/base_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,13 @@
let!(:vacancy) { create(:vacancy) }

context "when DisableExpensiveJobs is not enabled" do
before { allow(DisableExpensiveJobs).to receive(:enabled?).and_return(false) }

it "does perform the task" do
expect(UpdateGoogleIndexQueueJob).to receive(:perform_later)
controller.send(:update_google_index, vacancy)
end
end

context "when DisableExpensiveJobs is enabled" do
before { allow(DisableExpensiveJobs).to receive(:enabled?).and_return(true) }

context "when DisableExpensiveJobs is enabled", :disable_expensive_jobs do
it "does NOT perform the task" do
expect(UpdateGoogleIndexQueueJob).not_to receive(:perform_later)
controller.send(:update_google_index, vacancy)
Expand All @@ -35,9 +31,7 @@
end
end

context "when DisableExpensiveJobs is enabled" do
before { allow(DisableExpensiveJobs).to receive(:enabled?).and_return(true) }

context "when DisableExpensiveJobs is enabled", :disable_expensive_jobs do
it "does NOT perform the task" do
expect(RemoveGoogleIndexQueueJob).not_to receive(:perform_later)
controller.send(:remove_google_index, vacancy)
Expand Down
54 changes: 46 additions & 8 deletions spec/factories/subscriptions.rb
Original file line number Diff line number Diff line change
@@ -1,19 +1,57 @@
FactoryBot.define do
factory :subscription do
transient do
keyword { nil }
location { nil }
radius { nil }
working_patterns { nil }
teaching_job_roles { nil }
support_job_roles { nil }
ect_statuses { nil }
phases { nil }
visa_sponsorship_availability { nil }
subjects { nil }
organisation_slug { nil }
end

email { Faker::Internet.email(domain: "contoso.com") }
frequency { factory_sample(%i[daily weekly]) }
search_criteria do
{ keyword: Faker::Lorem.word,
location: Faker::Address.postcode,
radius: "10",
working_patterns: %w[full_time part_time],
teaching_job_roles: %w[teacher],
support_job_roles: %w[teaching_assistant it_support],
ect_statuses: %w[ect_suitable],
phases: %w[primary] }
{
keyword: keyword,
location: location,
radius: radius,
working_patterns: working_patterns,
teaching_job_roles: teaching_job_roles,
support_job_roles: support_job_roles,
ect_statuses: ect_statuses,
phases: phases,
visa_sponsorship_availability: visa_sponsorship_availability,
subjects: subjects,
organisation_slug: organisation_slug,
}.delete_if { |_k, v| v.nil? }
end
active { true }

trait :with_some_criteria do
keyword { Faker::Lorem.word }
location { Faker::Address.postcode }
radius { "10" }
working_patterns { %w[full_time part_time] }
teaching_job_roles { %w[teacher] }
support_job_roles { %w[teaching_assistant it_support] }
ect_statuses { %w[ect_suitable] }
phases { %w[primary] }
end

trait :visa_sponsorship_required do
visa_sponsorship_availability { %w[true] }
end

trait :ect_suitable do
ect_statuses { %w[ect_suitable] }
end

factory :daily_subscription do
frequency { :daily }
end
Expand Down
Loading
Loading