Skip to content

Commit

Permalink
Add Requests::Submission#to_h method that is sidekiq-compatible
Browse files Browse the repository at this point in the history
Sidekiq is very restrictive about which data structures we can pass in
with jobs, including to email jobs.

See https://github.com/sidekiq/sidekiq/wiki/Best-Practices#1-make-your-job-parameters-small-and-simple

Our Requests::Submission object was not built with sidekiq's restrictions
in mind, so this commit adds a method that can convert the object into
one that is compatible with Sidekiq.

Note that we do not yet use this method anywhere, this is just a preparatory
step.  The next steps will be to start incrementally teaching our mailer
views to use the to_h output rather than Requests::Submission directly.
  • Loading branch information
sandbergja committed Jan 16, 2025
1 parent 4a11226 commit 7f097ab
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 5 deletions.
21 changes: 20 additions & 1 deletion app/models/requests/submission.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def service_errors
end

def pick_up_location
Requests::BibdataService.delivery_locations[items.first["pick_up"]]["library"]
Requests::BibdataService.delivery_locations.dig(items.first&.dig('pick_up'), "library") || {}
end

def marquand?
Expand All @@ -96,6 +96,25 @@ def edd?(item)
delivery_mode.present? && delivery_mode == "edd"
end

# Convert the submission to a hash that is compatible that
# Sidekiq can safely serialize to JSON for redis. This means:
# * No application-specific objects (unless they are activerecord db objects)
# * No ActiveSupport::HashWithIndifferentAccess objects, we can only have Hash objects
# * The keys of the hash must all be strings, not symbols
# See https://github.com/sidekiq/sidekiq/wiki/Best-Practices#1-make-your-job-parameters-small-and-simple
def to_h
{
bib: bib.to_hash.stringify_keys,
email: email,
errors: errors.messages.stringify_keys,
items: items.map { |hash| hash.to_hash.stringify_keys },
patron: patron.to_h.to_hash.stringify_keys,
pick_up_location: pick_up_location.to_hash.stringify_keys,
user_barcode:,
user_name:
}.stringify_keys
end

private

def service_by_type(type)
Expand Down
57 changes: 53 additions & 4 deletions spec/models/requests/submission_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@
"edd_author" => "",
"edd_art_title" => "",
"edd_note" => "",
"pick_up" => "Firestone Library"
},
"pick_up" => "PF"
}.with_indifferent_access,
{
"selected" => "false"
}
}.with_indifferent_access
]
end
let(:bib) do
Expand All @@ -50,7 +50,7 @@
"title" => "County and city data book.",
"author" => "United States",
"date" => "1949"
}
}.with_indifferent_access
end
let(:params) do
{
Expand Down Expand Up @@ -105,6 +105,39 @@
expect(submission.partner_item?(submission.items.first)).to be false
end
end

describe '#to_h' do
before { stub_delivery_locations }
it 'is compatible with sidekiq' do
expect({ 'args' => submission.to_h }).to be_compatible_with_sidekiq
end
it 'contains a bib' do
expect(submission.to_h['bib']['id']).to eq '994916543506421'
expect(submission.to_h['bib']['title']).to eq 'County and city data book.'
end
it 'contains items' do
expect(submission.to_h['items'].first['call_number']).to eq 'HA202 .U581'
end
it 'contains an email' do
expect(submission.to_h['email']).to eq 'foo@princeton.edu'
end
it 'contains errors' do
expect(submission.to_h['errors']).to be_empty
end
it 'contains a patron' do
expect(submission.to_h['patron']['first_name']).to eq 'Foo'
expect(submission.to_h['patron']['last_name']).to eq 'Request'
end
it 'contains a pick_up_location' do
expect(submission.to_h['pick_up_location']['label']).to eq 'Firestone Library'
end
it 'contains a user_barcode' do
expect(submission.to_h['user_barcode']).to eq '22101007797777'
end
it 'contains a user_name' do
expect(submission.to_h['user_name']).to eq 'foo'
end
end
end

context 'An invalid Submission' do
Expand Down Expand Up @@ -132,6 +165,12 @@
expect(invalid_submission.errors.full_messages.size).to be > 0
end
end
describe '#to_h' do
before { stub_delivery_locations }
it 'is compatible with sidekiq' do
expect({ 'args' => invalid_submission.to_h }).to be_compatible_with_sidekiq
end
end
end

context 'Recap' do
Expand Down Expand Up @@ -247,6 +286,16 @@
it 'contains an error message' do
expect(submission.errors.messages).to be_truthy
end

describe '#to_h' do
before { stub_delivery_locations }
it 'is compatible with sidekiq' do
expect(submission.to_h).to be_compatible_with_sidekiq
end
it 'contains errors' do
expect(submission.to_h['errors']).to eq({ "items" => [{ "empty_set" => { "text" => "Please Select an Item to Request!", "type" => "options" } }] })
end
end
end

describe 'A submission without a pick-up location' do
Expand Down
39 changes: 39 additions & 0 deletions spec/support/sidekiq_data_check.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true
# This class is responsible for checking whether a provided data structure
# is compatible with sidekiq.
#
# See https://github.com/sidekiq/sidekiq/wiki/Best-Practices#1-make-your-job-parameters-small-and-simple
class SidekiqDataCheck
include Sidekiq::JobUtil
def initialize(data_to_check)
@data_to_check = { "args" => data_to_check }
end

def valid?
check
true
rescue ArgumentError
false
end

def error_message
check
rescue ArgumentError => e
e.message
end

private

attr_reader :data_to_check

# Returns nil if valid, raises ArgumentError if not
def check
Sidekiq::Config::DEFAULTS[:on_complex_arguments] = :raise
verify_json data_to_check
end
end

RSpec::Matchers.define :be_compatible_with_sidekiq do
match { |actual| SidekiqDataCheck.new(actual).valid? }
failure_message { |actual| SidekiqDataCheck.new(actual).error_message }
end

0 comments on commit 7f097ab

Please sign in to comment.