From 229749df5fac7d05a37387e59f6445279fcaa794 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Fri, 15 Jun 2018 13:06:25 +0800 Subject: [PATCH 1/9] Implement the controller interface for subscribing users to a mailing list --- Gemfile | 3 ++- Gemfile.lock | 12 ++++++++- app/controllers/entries_controller.rb | 1 + app/jobs/mailing_list_subscriber_job.rb | 7 +++++ config/application.rb | 2 ++ doc/considerations.md | 3 +++ spec/controllers/entries_controller_spec.rb | 27 +++++++++++++++++++ spec/jobs/mailing_list_subscriber_job_spec.rb | 5 ++++ 8 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 app/jobs/mailing_list_subscriber_job.rb create mode 100644 doc/considerations.md create mode 100644 spec/controllers/entries_controller_spec.rb create mode 100644 spec/jobs/mailing_list_subscriber_job_spec.rb diff --git a/Gemfile b/Gemfile index b82ba44..9218b4b 100755 --- a/Gemfile +++ b/Gemfile @@ -31,6 +31,8 @@ gem 'sdoc', '~> 0.4.0', group: :doc # Use Capistrano for deployment # gem 'capistrano-rails', group: :development +gem 'sidekiq' + group :development, :test do # Call 'byebug' anywhere in the code to stop execution and get a debugger console gem 'byebug' @@ -44,4 +46,3 @@ group :development do # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' end - diff --git a/Gemfile.lock b/Gemfile.lock index ca28e0e..a4f4b2a 100755 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -51,6 +51,7 @@ GEM execjs coffee-script-source (1.10.0) concurrent-ruby (1.0.5) + connection_pool (2.2.2) crass (1.0.4) debug_inspector (0.0.2) diff-lcs (1.3) @@ -77,6 +78,8 @@ GEM nokogiri (1.8.2) mini_portile2 (~> 2.3.0) rack (1.6.9) + rack-protection (2.0.3) + rack rack-test (0.6.3) rack (>= 1.0) rails (4.2.10) @@ -109,6 +112,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) @@ -140,6 +144,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) @@ -176,10 +185,11 @@ DEPENDENCIES rspec-rails sass-rails (~> 5.0) sdoc (~> 0.4.0) + sidekiq spring sqlite3 uglifier (>= 1.3.0) web-console (~> 2.0) BUNDLED WITH - 1.16.1 + 1.16.2 diff --git a/app/controllers/entries_controller.rb b/app/controllers/entries_controller.rb index 14d6cb1..45b3155 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} diff --git a/app/jobs/mailing_list_subscriber_job.rb b/app/jobs/mailing_list_subscriber_job.rb new file mode 100644 index 0000000..ebe27dc --- /dev/null +++ b/app/jobs/mailing_list_subscriber_job.rb @@ -0,0 +1,7 @@ +class MailingListSubscriberJob < ActiveJob::Base + queue_as :default + + def perform(*args) + # Do something later + end +end diff --git a/config/application.rb b/config/application.rb index 5607410..ac98284 100755 --- a/config/application.rb +++ b/config/application.rb @@ -22,5 +22,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/doc/considerations.md b/doc/considerations.md new file mode 100644 index 0000000..db93bd8 --- /dev/null +++ b/doc/considerations.md @@ -0,0 +1,3 @@ +# 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. diff --git a/spec/controllers/entries_controller_spec.rb b/spec/controllers/entries_controller_spec.rb new file mode 100644 index 0000000..8b94868 --- /dev/null +++ b/spec/controllers/entries_controller_spec.rb @@ -0,0 +1,27 @@ +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, name: "Daniel 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..7f784c1 --- /dev/null +++ b/spec/jobs/mailing_list_subscriber_job_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe MailingListSubscriberJob, type: :job do + pending "add some examples to (or delete) #{__FILE__}" +end From 6c8946dff01e13465fe7954dcf48572df098c8d4 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Fri, 15 Jun 2018 13:12:14 +0800 Subject: [PATCH 2/9] Remove unused routes that do not have any implementation --- config/routes.rb | 1 - 1 file changed, 1 deletion(-) 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". From 79fa202c12ceebe2aa195d2785a25e0311e67d9c Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Fri, 15 Jun 2018 14:55:22 +0800 Subject: [PATCH 3/9] Add the ability to link competition records to Mailchimp mailing lists, with some hardcoded data --- ...180615064619_add_mailing_list_id_to_competitions.rb | 10 ++++++++++ db/schema.rb | 3 ++- doc/considerations.md | 5 +++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20180615064619_add_mailing_list_id_to_competitions.rb 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/schema.rb b/db/schema.rb index 34ae951..c977f3b 100755 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,13 +11,14 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160125020403) do +ActiveRecord::Schema.define(version: 20180615064619) 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| diff --git a/doc/considerations.md b/doc/considerations.md index db93bd8..ed0a947 100644 --- a/doc/considerations.md +++ b/doc/considerations.md @@ -1,3 +1,8 @@ # 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. From 27f424491d0046c8a6ddd9b1b8a03511d63b784c Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Fri, 15 Jun 2018 15:18:23 +0800 Subject: [PATCH 4/9] Allow the mailing list subscriber to talk to a API wrapper class to perform the actual subscription --- app/jobs/mailing_list_subscriber_job.rb | 8 ++++++-- config/application.rb | 1 + lib/mailing_list.rb | 7 +++++++ spec/jobs/mailing_list_subscriber_job_spec.rb | 13 ++++++++++++- 4 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 lib/mailing_list.rb diff --git a/app/jobs/mailing_list_subscriber_job.rb b/app/jobs/mailing_list_subscriber_job.rb index ebe27dc..801916b 100644 --- a/app/jobs/mailing_list_subscriber_job.rb +++ b/app/jobs/mailing_list_subscriber_job.rb @@ -1,7 +1,11 @@ class MailingListSubscriberJob < ActiveJob::Base queue_as :default - def perform(*args) - # Do something later + def perform(entry_id) + entry = Entry.find(entry_id) + + MailingList + .find(entry.competition.mailing_list_id) + .add_subscriber(name: entry.name, email: entry.email) end end diff --git a/config/application.rb b/config/application.rb index ac98284..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. diff --git a/lib/mailing_list.rb b/lib/mailing_list.rb new file mode 100644 index 0000000..0c260ae --- /dev/null +++ b/lib/mailing_list.rb @@ -0,0 +1,7 @@ +class MailingList + def self.find(id) + end + + def add_subscriber(details) + end +end diff --git a/spec/jobs/mailing_list_subscriber_job_spec.rb b/spec/jobs/mailing_list_subscriber_job_spec.rb index 7f784c1..d7ddebc 100644 --- a/spec/jobs/mailing_list_subscriber_job_spec.rb +++ b/spec/jobs/mailing_list_subscriber_job_spec.rb @@ -1,5 +1,16 @@ require 'rails_helper' RSpec.describe MailingListSubscriberJob, type: :job do - pending "add some examples to (or delete) #{__FILE__}" + 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!(name: "Samantha 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(name: "Samantha Carter", email: "scarter@example.com") + + MailingListSubscriberJob.perform_now(entry.id) + end + end end From 60319a24352861c88573df2fa050b56d3f77fb31 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Fri, 15 Jun 2018 17:53:26 +0800 Subject: [PATCH 5/9] Subscribe entries to Mailchimp using email only --- Gemfile | 5 +++++ Gemfile.lock | 19 +++++++++++++++++ config/initializers/gibbon.rb | 1 + config/secrets.yml | 3 +++ doc/considerations.md | 9 +++++++- lib/mailing_list.rb | 20 ++++++++++++++++++ spec/lib/mailing_list_spec.rb | 39 +++++++++++++++++++++++++++++++++++ spec/rails_helper.rb | 3 +++ 8 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 config/initializers/gibbon.rb create mode 100644 spec/lib/mailing_list_spec.rb diff --git a/Gemfile b/Gemfile index 9218b4b..748ba79 100755 --- a/Gemfile +++ b/Gemfile @@ -32,6 +32,7 @@ gem 'sdoc', '~> 0.4.0', group: :doc # 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 @@ -46,3 +47,7 @@ group :development do # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' end + +group :test do + gem 'webmock' +end diff --git a/Gemfile.lock b/Gemfile.lock index a4f4b2a..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) @@ -52,14 +54,22 @@ GEM 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) @@ -75,8 +85,10 @@ 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 @@ -130,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) @@ -171,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 @@ -180,6 +197,7 @@ DEPENDENCIES angularjs-rails byebug coffee-rails (~> 4.1.0) + gibbon jbuilder (~> 2.0) rails (= 4.2.10) rspec-rails @@ -190,6 +208,7 @@ DEPENDENCIES sqlite3 uglifier (>= 1.3.0) web-console (~> 2.0) + webmock BUNDLED WITH 1.16.2 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/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/doc/considerations.md b/doc/considerations.md index ed0a947..0334061 100644 --- a/doc/considerations.md +++ b/doc/considerations.md @@ -1,4 +1,4 @@ -# Considerations +# 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. @@ -6,3 +6,10 @@ * 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. + +* At the moment saving the entry name to Mailchimp is not supported. + +* 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 index 0c260ae..3615e59 100644 --- a/lib/mailing_list.rb +++ b/lib/mailing_list.rb @@ -1,7 +1,27 @@ class MailingList + class APIError < ::StandardError; end + def self.find(id) + new(id) end def add_subscriber(details) + raw_request.members.create(body: {email_address: details[:email], status: "subscribed"}) + 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 end diff --git a/spec/lib/mailing_list_spec.rb b/spec/lib/mailing_list_spec.rb new file mode 100644 index 0000000..bfa7481 --- /dev/null +++ b/spec/lib/mailing_list_spec.rb @@ -0,0 +1,39 @@ +require "rails_helper" + +RSpec.describe MailingList do + describe "#add_subscriber" do + context "when the Mailchimp API responds successfully" do + it "returns true" do + # The actual response is a lot bigger but this is enough to ensure the integration is working. + stub_request(:post, "https://us18.api.mailchimp.com/3.0/lists/123456/members") + .with(body: hash_including(email_address: "tealc@example.com")) + .to_return(status: 200, body: '{ + "id":"bb365e8477053edcc55b40c3d27781d8", + "email_address":"tealc@example.com", + "unique_email_id":"38d27fcb79", + "email_type":"html", + "status":"subscribed" + }') + + expect(MailingList.find("123456").add_subscriber(email: "tealc@example.com")).to eq true + end + end + + context "when the Mailchimp API responds with an error" do + it "rewraps the error and throws it" do + stub_request(:post, "https://us18.api.mailchimp.com/3.0/lists/123456/members") + .with(body: hash_including(email_address: "")) + .to_return(status: 400, body: '{ + "type":"http://developer.mailchimp.com/documentation/mailchimp/guides/error-glossary/", + "title":"Invalid Resource", + "status":400, + "detail":"Please provide a valid email address.", + "instance":"8b85d2a7-5ad6-4d84-87f1-7dc9c2ddc540" + }') + + 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 From 264abfb728b50ff6aa13160a9cf00516e222d295 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Fri, 15 Jun 2018 18:15:44 +0800 Subject: [PATCH 6/9] Fix errors raised when adding duplicate email addresses to Mailchimp --- lib/mailing_list.rb | 10 +++++++++- spec/lib/mailing_list_spec.rb | 30 ++++++++++++------------------ 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/lib/mailing_list.rb b/lib/mailing_list.rb index 3615e59..f372a84 100644 --- a/lib/mailing_list.rb +++ b/lib/mailing_list.rb @@ -1,3 +1,5 @@ +require "digest" + class MailingList class APIError < ::StandardError; end @@ -6,7 +8,9 @@ def self.find(id) end def add_subscriber(details) - raw_request.members.create(body: {email_address: details[:email], status: "subscribed"}) + email = details[:email] + + raw_request.members(email_hash(email)).upsert(body: {email_address: email, status: "subscribed"}) true rescue Gibbon::MailChimpError => e raise APIError.new e.body @@ -24,4 +28,8 @@ def initialize(id) def raw_request Gibbon::Request.lists(@id) end + + def email_hash(email) + Digest::MD5.hexdigest email + end end diff --git a/spec/lib/mailing_list_spec.rb b/spec/lib/mailing_list_spec.rb index bfa7481..5311550 100644 --- a/spec/lib/mailing_list_spec.rb +++ b/spec/lib/mailing_list_spec.rb @@ -3,33 +3,27 @@ RSpec.describe MailingList do describe "#add_subscriber" do context "when the Mailchimp API responds successfully" do - it "returns true" do - # The actual response is a lot bigger but this is enough to ensure the integration is working. - stub_request(:post, "https://us18.api.mailchimp.com/3.0/lists/123456/members") + 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")) - .to_return(status: 200, body: '{ - "id":"bb365e8477053edcc55b40c3d27781d8", - "email_address":"tealc@example.com", - "unique_email_id":"38d27fcb79", - "email_type":"html", - "status":"subscribed" - }') + .to_return(status: 200) + end + it "returns true" do expect(MailingList.find("123456").add_subscriber(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(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(:post, "https://us18.api.mailchimp.com/3.0/lists/123456/members") + stub_request(:put, "https://us18.api.mailchimp.com/3.0/lists/123456/members/d41d8cd98f00b204e9800998ecf8427e") .with(body: hash_including(email_address: "")) - .to_return(status: 400, body: '{ - "type":"http://developer.mailchimp.com/documentation/mailchimp/guides/error-glossary/", - "title":"Invalid Resource", - "status":400, - "detail":"Please provide a valid email address.", - "instance":"8b85d2a7-5ad6-4d84-87f1-7dc9c2ddc540" - }') + .to_return(status: 400) expect { MailingList.find("123456").add_subscriber(email: "") } .to raise_error(MailingList::APIError) From 9bf6436ffea29b058734c99fddbbba687060ed42 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Sun, 17 Jun 2018 14:35:22 +0800 Subject: [PATCH 7/9] Save names on entries as given name/family name, instead of a single name field This will allow for more accurate use of the name parts, ie. addressing an email by the user's first name. --- app/controllers/entries_controller.rb | 2 +- app/jobs/mailing_list_subscriber_job.rb | 3 +- app/models/entry.rb | 2 +- ...plit_entry_name_into_given_family_names.rb | 32 +++++++++++++++++++ db/schema.rb | 5 +-- doc/considerations.md | 4 ++- spec/controllers/entries_controller_spec.rb | 3 +- spec/jobs/mailing_list_subscriber_job_spec.rb | 6 ++-- 8 files changed, 48 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20180617055027_split_entry_name_into_given_family_names.rb diff --git a/app/controllers/entries_controller.rb b/app/controllers/entries_controller.rb index 45b3155..62be2d5 100755 --- a/app/controllers/entries_controller.rb +++ b/app/controllers/entries_controller.rb @@ -15,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 index 801916b..a0cd205 100644 --- a/app/jobs/mailing_list_subscriber_job.rb +++ b/app/jobs/mailing_list_subscriber_job.rb @@ -6,6 +6,7 @@ def perform(entry_id) MailingList .find(entry.competition.mailing_list_id) - .add_subscriber(name: entry.name, email: entry.email) + .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/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 c977f3b..4430672 100755 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180615064619) do +ActiveRecord::Schema.define(version: 20180617055027) do create_table "competitions", force: :cascade do |t| t.string "name" @@ -23,10 +23,11 @@ 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 index 0334061..16512ca 100644 --- a/doc/considerations.md +++ b/doc/considerations.md @@ -9,7 +9,9 @@ Either way, the ID of that mailing list will be stored with the Competition reco * 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. -* At the moment saving the entry name to Mailchimp is not supported. +* 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/spec/controllers/entries_controller_spec.rb b/spec/controllers/entries_controller_spec.rb index 8b94868..622f5b6 100644 --- a/spec/controllers/entries_controller_spec.rb +++ b/spec/controllers/entries_controller_spec.rb @@ -9,7 +9,8 @@ it "adds the entry to the competition mailing list" do allow(MailingListSubscriberJob).to receive(:perform_later) - post :create, entry: { competition_id: competition, name: "Daniel Jackson", email: "djackson@example.com" } + 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) diff --git a/spec/jobs/mailing_list_subscriber_job_spec.rb b/spec/jobs/mailing_list_subscriber_job_spec.rb index d7ddebc..13de386 100644 --- a/spec/jobs/mailing_list_subscriber_job_spec.rb +++ b/spec/jobs/mailing_list_subscriber_job_spec.rb @@ -4,11 +4,13 @@ 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!(name: "Samantha Carter", email: "scarter@example.com") + 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(name: "Samantha Carter", email: "scarter@example.com") + 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 From bc16c9acaf265961d369d4a62e29a5f2da826272 Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Sun, 17 Jun 2018 14:43:13 +0800 Subject: [PATCH 8/9] Save the entry given name and family name to the Mailchimp mailing list --- lib/mailing_list.rb | 3 ++- spec/lib/mailing_list_spec.rb | 9 ++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/mailing_list.rb b/lib/mailing_list.rb index f372a84..3d072c8 100644 --- a/lib/mailing_list.rb +++ b/lib/mailing_list.rb @@ -10,7 +10,8 @@ def self.find(id) def add_subscriber(details) email = details[:email] - raw_request.members(email_hash(email)).upsert(body: {email_address: email, status: "subscribed"}) + 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 diff --git a/spec/lib/mailing_list_spec.rb b/spec/lib/mailing_list_spec.rb index 5311550..917772c 100644 --- a/spec/lib/mailing_list_spec.rb +++ b/spec/lib/mailing_list_spec.rb @@ -6,16 +6,19 @@ 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")) + .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(email: "tealc@example.com")).to eq true + 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(email: "tealc@example.com") } + 2.times { MailingList.find("123456").add_subscriber(given_name: "Teal'c", + family_name: "of Chulak", email: "tealc@example.com") } end end From 9bbdb67f07969a92214f327947f6c727f4f39d6c Mon Sep 17 00:00:00 2001 From: Rebecca Le Date: Sun, 17 Jun 2018 15:18:54 +0800 Subject: [PATCH 9/9] Update the HTML entry page to support given and family name fields --- app/views/competitions/entrant_page.erb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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:
  • {{field}}: @@ -16,8 +16,13 @@

- - + + +

+ +

+ +