diff --git a/Gemfile b/Gemfile index b82ba44..748ba79 100755 --- a/Gemfile +++ b/Gemfile @@ -31,6 +31,9 @@ gem 'sdoc', '~> 0.4.0', group: :doc # Use Capistrano for deployment # gem 'capistrano-rails', group: :development +gem 'sidekiq' +gem 'gibbon' + group :development, :test do # Call 'byebug' anywhere in the code to stop execution and get a debugger console gem 'byebug' @@ -45,3 +48,6 @@ group :development do gem 'spring' end +group :test do + gem 'webmock' +end diff --git a/Gemfile.lock b/Gemfile.lock index ca28e0e..5c39b0f 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -35,6 +35,8 @@ GEM minitest (~> 5.1) thread_safe (~> 0.3, >= 0.3.4) tzinfo (~> 1.1) + addressable (2.5.2) + public_suffix (>= 2.0.2, < 4.0) angular_rails_csrf (1.0.4) rails (>= 3, < 5) angularjs-rails (1.4.8) @@ -51,14 +53,23 @@ GEM execjs coffee-script-source (1.10.0) concurrent-ruby (1.0.5) + connection_pool (2.2.2) + crack (0.4.3) + safe_yaml (~> 1.0.0) crass (1.0.4) debug_inspector (0.0.2) diff-lcs (1.3) erubis (2.7.0) execjs (2.6.0) + faraday (0.15.2) + multipart-post (>= 1.2, < 3) ffi (1.9.23) + gibbon (3.2.0) + faraday (>= 0.9.1) + multi_json (>= 1.11.0) globalid (0.4.1) activesupport (>= 4.2.0) + hashdiff (0.3.7) i18n (0.9.5) concurrent-ruby (~> 1.0) jbuilder (2.4.0) @@ -74,9 +85,13 @@ GEM mini_portile2 (2.3.0) minitest (5.11.3) multi_json (1.11.2) + multipart-post (2.0.0) nokogiri (1.8.2) mini_portile2 (~> 2.3.0) + public_suffix (3.0.2) rack (1.6.9) + rack-protection (2.0.3) + rack rack-test (0.6.3) rack (>= 1.0) rails (4.2.10) @@ -109,6 +124,7 @@ GEM ffi (>= 0.5.0, < 2) rdoc (4.2.1) json (~> 1.4) + redis (4.0.1) rspec-core (3.7.1) rspec-support (~> 3.7.0) rspec-expectations (3.7.0) @@ -126,6 +142,7 @@ GEM rspec-mocks (~> 3.7.0) rspec-support (~> 3.7.0) rspec-support (3.7.1) + safe_yaml (1.0.4) sass (3.5.5) sass-listen (~> 4.0.0) sass-listen (4.0.0) @@ -140,6 +157,11 @@ GEM sdoc (0.4.1) json (~> 1.7, >= 1.7.7) rdoc (~> 4.0) + sidekiq (5.1.3) + concurrent-ruby (~> 1.0) + connection_pool (~> 2.2, >= 2.2.0) + rack-protection (>= 1.5.0) + redis (>= 3.3.5, < 5) spring (1.6.2) sprockets (3.7.1) concurrent-ruby (~> 1.0) @@ -162,6 +184,10 @@ GEM binding_of_caller (>= 0.7.2) railties (>= 4.0) sprockets-rails (>= 2.0, < 4.0) + webmock (3.4.2) + addressable (>= 2.3.6) + crack (>= 0.3.2) + hashdiff PLATFORMS ruby @@ -171,15 +197,18 @@ DEPENDENCIES angularjs-rails byebug coffee-rails (~> 4.1.0) + gibbon jbuilder (~> 2.0) rails (= 4.2.10) rspec-rails sass-rails (~> 5.0) sdoc (~> 0.4.0) + sidekiq spring sqlite3 uglifier (>= 1.3.0) web-console (~> 2.0) + webmock BUNDLED WITH - 1.16.1 + 1.16.2 diff --git a/app/controllers/entries_controller.rb b/app/controllers/entries_controller.rb index 14d6cb1..62be2d5 100755 --- a/app/controllers/entries_controller.rb +++ b/app/controllers/entries_controller.rb @@ -5,6 +5,7 @@ def create @entry = Entry.new(entry_params) if @entry.save + MailingListSubscriberJob.perform_later @entry.id render json: {success: true} else render json: {success: false, errors: @entry.errors} @@ -14,6 +15,6 @@ def create private # Never trust parameters from the scary internet, only allow the white list through. def entry_params - params.require(:entry).permit(:competition_id, :name, :email) + params.require(:entry).permit(:competition_id, :given_name, :family_name, :email) end end diff --git a/app/jobs/mailing_list_subscriber_job.rb b/app/jobs/mailing_list_subscriber_job.rb new file mode 100644 index 0000000..a0cd205 --- /dev/null +++ b/app/jobs/mailing_list_subscriber_job.rb @@ -0,0 +1,12 @@ +class MailingListSubscriberJob < ActiveJob::Base + queue_as :default + + def perform(entry_id) + entry = Entry.find(entry_id) + + MailingList + .find(entry.competition.mailing_list_id) + .add_subscriber(given_name: entry.given_name, family_name: entry.family_name, + email: entry.email) + end +end diff --git a/app/models/entry.rb b/app/models/entry.rb index 9f690f4..3e91009 100755 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -8,7 +8,7 @@ class Entry < ActiveRecord::Base validates_presence_of :competition, inverse_of: :entries validates_presence_of :email validates_format_of :email, :with => EMAIL_REGEX, allow_blank: true, allow_nil: true - validates_presence_of :name, if: :requires_name + validates_presence_of :given_name, :family_name, if: :requires_name validates_uniqueness_of :email, scope: :competition, message: "has already entered this competition" private diff --git a/app/views/competitions/entrant_page.erb b/app/views/competitions/entrant_page.erb index 262c316..41691b1 100755 --- a/app/views/competitions/entrant_page.erb +++ b/app/views/competitions/entrant_page.erb @@ -6,7 +6,7 @@
- Sorry, ther was a problem saving your entry: + Sorry, there was a problem saving your entry:

- - + + +

+ +

+ +

diff --git a/config/application.rb b/config/application.rb index 5607410..7227223 100755 --- a/config/application.rb +++ b/config/application.rb @@ -11,6 +11,7 @@ class Application < Rails::Application # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers # -- all .rb files in that directory are automatically loaded. + config.autoload_paths += %W(#{config.root}/lib) # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. @@ -22,5 +23,7 @@ class Application < Rails::Application # Do not swallow errors in after_commit/after_rollback callbacks. config.active_record.raise_in_transactional_callbacks = true + + config.active_job.queue_adapter = :sidekiq end end diff --git a/config/initializers/gibbon.rb b/config/initializers/gibbon.rb new file mode 100644 index 0000000..93f6cf8 --- /dev/null +++ b/config/initializers/gibbon.rb @@ -0,0 +1 @@ +Gibbon::Request.api_key = Rails.application.secrets.mailchimp_api_key diff --git a/config/routes.rb b/config/routes.rb index 924d8a8..f880b9e 100755 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,5 +1,4 @@ Rails.application.routes.draw do - resources :entries # The priority is based upon order of creation: first created -> highest priority. # See how all your routes lay out with "rake routes". diff --git a/config/secrets.yml b/config/secrets.yml index 2004f1b..2ae1672 100755 --- a/config/secrets.yml +++ b/config/secrets.yml @@ -12,11 +12,14 @@ development: secret_key_base: 6d8541683d523f8b9ad80440983b1a0e3845533bf03e9caa0df5e477295b95d279695c86f0338a1fbb29ec9e7470dab7b7a1bc4c8b5167dbd449efa904787212 + mailchimp_api_key: <%= ENV["MAILCHIMP_API_KEY"] %> test: secret_key_base: c77d08b24c34c9f962d0cf8dfe4a61843d2fa543acfc7c9d1bb478a9048d7c3b09491151f0a5b7ace298864344191c5be1351d7eefdbdf3a4c77918a64e8e2b4 + mailchimp_api_key: secretapikeyhere-us18 # Do not keep production secrets in the repository, # instead read values from the environment. production: secret_key_base: <%= ENV["SECRET_KEY_BASE"] %> + mailchimp_api_key: <%= ENV["MAILCHIMP_API_KEY"] %> diff --git a/db/migrate/20180615064619_add_mailing_list_id_to_competitions.rb b/db/migrate/20180615064619_add_mailing_list_id_to_competitions.rb new file mode 100644 index 0000000..047f351 --- /dev/null +++ b/db/migrate/20180615064619_add_mailing_list_id_to_competitions.rb @@ -0,0 +1,10 @@ +class AddMailingListIdToCompetitions < ActiveRecord::Migration + def change + add_column :competitions, :mailing_list_id, :string + + Competition.reset_column_information + + Competition.find(1).update!(mailing_list_id: "a94641097a") + Competition.find(2).update!(mailing_list_id: "4827de065a") + end +end diff --git a/db/migrate/20180617055027_split_entry_name_into_given_family_names.rb b/db/migrate/20180617055027_split_entry_name_into_given_family_names.rb new file mode 100644 index 0000000..fec5897 --- /dev/null +++ b/db/migrate/20180617055027_split_entry_name_into_given_family_names.rb @@ -0,0 +1,32 @@ +class SplitEntryNameIntoGivenFamilyNames < ActiveRecord::Migration + def up + add_column :entries, :given_name, :string + add_column :entries, :family_name, :string + + Entry.reset_column_information + + Entry.where.not(name: nil).find_each do |e| + puts e.name + name_parts = e.name.split(" ", 2) + e.given_name = name_parts[0] + e.family_name = name_parts[1] + e.save! + end + + remove_column :entries, :name + end + + def down + add_column :entries, :name, :string + + Entry.reset_column_information + + Entry.where.not(given_name: nil).find_each do |e| + e.name = "#{e.given_name} #{e.family_name}" + e.save! + end + + remove_column :entries, :given_name + remove_column :entries, :family_name + end +end diff --git a/db/schema.rb b/db/schema.rb index 34ae951..4430672 100755 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,21 +11,23 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160125020403) do +ActiveRecord::Schema.define(version: 20180617055027) do create_table "competitions", force: :cascade do |t| t.string "name" t.boolean "requires_entry_name", default: true t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "mailing_list_id" end create_table "entries", force: :cascade do |t| t.integer "competition_id" - t.text "name" t.text "email" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "given_name" + t.string "family_name" end add_index "entries", ["competition_id", "email"], name: "index_entries_on_competition_id_and_email", unique: true diff --git a/doc/considerations.md b/doc/considerations.md new file mode 100644 index 0000000..16512ca --- /dev/null +++ b/doc/considerations.md @@ -0,0 +1,17 @@ +# Thought Processes and Other Considerations + +* To minimize performance impact for users, accessing the external API should be done outside the main controller process, ie. in a background job. I've added a basic Sidekiq setup for this purpose. + +* I've decided that Mailchimp integration with competition management is out of the scope of this task. In the real app, when managing a competition, there could be several possibilities: + * The admin could be given the option of selecting an existing Mailchimp mailing list to subscribe users to, or + * A new mailing list is auto-created for each new competition. +Either way, the ID of that mailing list will be stored with the Competition record. I've created some test mailing lists in my personal Mailchimp account and hardcoded those via a data migration. + +* Secrets support was minimal in Rails 4.2, so I've relied on loading my Mailchimp API key from an ENV var in both dev and production. If using Rails 5 I'd use encrypted secrets, or a library like Figaro. + +* For i18n purposes, I decided against splitting the data entered into the name field, into first and last name components - there's no way to do this totally correctly even in English. Instead, I elected to change the one "Name" field into two - for given name and family name. + * I've used naive string splitting for existing data only because I know there isn't any existing data that would fail by doing this :) + * Alternatively, Mailchimp could have been configured to store a single "Name" field, but then this would prevent things like addressing emails with just a user's given name. + +* Possible optimizations include: + * Batching subscriptions into groups of a certain size or time range, to save hitting the API too many times diff --git a/lib/mailing_list.rb b/lib/mailing_list.rb new file mode 100644 index 0000000..3d072c8 --- /dev/null +++ b/lib/mailing_list.rb @@ -0,0 +1,36 @@ +require "digest" + +class MailingList + class APIError < ::StandardError; end + + def self.find(id) + new(id) + end + + def add_subscriber(details) + email = details[:email] + + raw_request.members(email_hash(email)).upsert(body: {email_address: email, status: "subscribed", + merge_fields: { FNAME: details[:given_name], LNAME: details[:family_name]}}) + true + rescue Gibbon::MailChimpError => e + raise APIError.new e.body + end + + private + + def initialize(id) + @id = id + end + + # The request object should be reinstantiated for each API call. + # Each request keeps a list of "path parts" and that list resets after each call - but we always + # want to keep the list details in that path. + def raw_request + Gibbon::Request.lists(@id) + end + + def email_hash(email) + Digest::MD5.hexdigest email + end +end diff --git a/spec/controllers/entries_controller_spec.rb b/spec/controllers/entries_controller_spec.rb new file mode 100644 index 0000000..622f5b6 --- /dev/null +++ b/spec/controllers/entries_controller_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +RSpec.describe EntriesController do + let(:competition) { Competition.create!(name: "Stargate Competition") } + + describe "#create" do + context "when the entry is saved successfully" do + + it "adds the entry to the competition mailing list" do + allow(MailingListSubscriberJob).to receive(:perform_later) + + post :create, entry: { competition_id: competition, given_name: "Daniel", + family_name: "Jackson", email: "djackson@example.com" } + + entry = Entry.find_by!(email: "djackson@example.com") + expect(MailingListSubscriberJob).to have_received(:perform_later).with(entry.id) + end + end + + context "when the entry could not be saved" do + it "does not attempt to add the entry to the competition mailing list" do + expect(MailingListSubscriberJob).not_to receive(:perform_later) + + post :create, entry: { competition_id: competition } + end + end + end +end diff --git a/spec/jobs/mailing_list_subscriber_job_spec.rb b/spec/jobs/mailing_list_subscriber_job_spec.rb new file mode 100644 index 0000000..13de386 --- /dev/null +++ b/spec/jobs/mailing_list_subscriber_job_spec.rb @@ -0,0 +1,18 @@ +require 'rails_helper' + +RSpec.describe MailingListSubscriberJob, type: :job do + describe "#perform" do + it "subscribes the user to the correct mailing list for the competition" do + competition = Competition.create!(name: "Stargate competition", mailing_list_id: "12345") + entry = competition.entries.create!(given_name: "Samantha", family_name: "Carter", + email: "scarter@example.com") + + mailing_list_double = instance_double(MailingList) + expect(MailingList).to receive(:find).with("12345") { mailing_list_double } + expect(mailing_list_double).to receive(:add_subscriber).with(given_name: "Samantha", + family_name: "Carter", email: "scarter@example.com") + + MailingListSubscriberJob.perform_now(entry.id) + end + end +end diff --git a/spec/lib/mailing_list_spec.rb b/spec/lib/mailing_list_spec.rb new file mode 100644 index 0000000..917772c --- /dev/null +++ b/spec/lib/mailing_list_spec.rb @@ -0,0 +1,36 @@ +require "rails_helper" + +RSpec.describe MailingList do + describe "#add_subscriber" do + context "when the Mailchimp API responds successfully" do + before :each do + # The hash is the lowercase md5 of the email address. + stub_request(:put, "https://us18.api.mailchimp.com/3.0/lists/123456/members/79005d16381eec2c1195b1f2371f2631") + .with(body: hash_including(email_address: "tealc@example.com", merge_fields: { + FNAME: "Teal'c", LNAME: "of Chulak"})) + .to_return(status: 200) + end + + it "returns true" do + expect(MailingList.find("123456").add_subscriber(given_name: "Teal'c", + family_name: "of Chulak", email: "tealc@example.com")).to eq true + end + + it "does not error when the same address is subscribed multiple times" do + 2.times { MailingList.find("123456").add_subscriber(given_name: "Teal'c", + family_name: "of Chulak", email: "tealc@example.com") } + end + end + + context "when the Mailchimp API responds with an error" do + it "rewraps the error and throws it" do + stub_request(:put, "https://us18.api.mailchimp.com/3.0/lists/123456/members/d41d8cd98f00b204e9800998ecf8427e") + .with(body: hash_including(email_address: "")) + .to_return(status: 400) + + expect { MailingList.find("123456").add_subscriber(email: "") } + .to raise_error(MailingList::APIError) + end + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 88ff2d0..42b1283 100755 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -6,6 +6,9 @@ require 'spec_helper' require 'rspec/rails' # Add additional requires below this line. Rails is not loaded until this point! +require 'webmock/rspec' + +WebMock.disable_net_connect!(allow_localhost: true) # Requires supporting ruby files with custom matchers and macros, etc, in # spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are