From e564f8e5d62b8900c58c5662ee47c2f8cea71514 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 3 Dec 2020 10:48:39 +0000 Subject: [PATCH 01/56] LongTextAnswer table has UUIDs We can't reverse this migration as I couldn't think of a way to generate sequential IDs. Give that we have no real production data or users, having a non reversible seems okay for the time being. --- CHANGELOG.md | 1 + ...203103421_change_long_text_answer_to_uuid.rb | 17 +++++++++++++++++ db/schema.rb | 5 +++-- 3 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20201203103421_change_long_text_answer_to_uuid.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c4aae4a9..f6b9fb08d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog 1.0.0]. - refactor name of "Question" to "Step" - refactor redis caching into reusable class - users can be shown a step in the journey that is only static content +- fix primary key type on long_text_answers table to UUID ## [release-002] - 2020-11-16 diff --git a/db/migrate/20201203103421_change_long_text_answer_to_uuid.rb b/db/migrate/20201203103421_change_long_text_answer_to_uuid.rb new file mode 100644 index 000000000..b4b14f905 --- /dev/null +++ b/db/migrate/20201203103421_change_long_text_answer_to_uuid.rb @@ -0,0 +1,17 @@ +class ChangeLongTextAnswerToUuid < ActiveRecord::Migration[6.0] + def up + enable_extension "uuid-ossp" unless extension_enabled?("uuid-ossp") + + add_column :long_text_answers, :uuid, :uuid, default: "gen_random_uuid()", null: false + + change_table :long_text_answers do |t| + t.remove :id + t.rename :uuid, :id + end + execute "ALTER TABLE long_text_answers ADD PRIMARY KEY (id);" + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/schema.rb b/db/schema.rb index f43e73088..02efda6e6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,11 +10,12 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_12_02_150359) do +ActiveRecord::Schema.define(version: 2020_12_03_103421) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" + enable_extension "uuid-ossp" create_table "journeys", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "category", null: false @@ -23,7 +24,7 @@ t.string "next_entry_id" end - create_table "long_text_answers", force: :cascade do |t| + create_table "long_text_answers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "step_id" t.text "response", null: false t.datetime "created_at", precision: 6, null: false From d49ba47db8a4f6c68bcc0c2f07f33da17548793b Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 3 Dec 2020 10:52:57 +0000 Subject: [PATCH 02/56] Increase the chances new tables have UUID as the primary key If you use the Rails generator with "create", Rails will prepopulate a migration with the right ID type: ``` rails g migration CreateFooTable foo bar => create_table :foo_tables, id: :uuid do |t| t.string :foo t.string :bar end ``` --- config/application.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/config/application.rb b/config/application.rb index 4ba252194..4951d581c 100644 --- a/config/application.rb +++ b/config/application.rb @@ -32,5 +32,11 @@ class Application < Rails::Application # Application configuration can go into files in config/initializers # -- all .rb files in that directory are automatically loaded after loading # the framework and any gems in your application. + + # Automatically add UUID as the type of primary key on new tables, if you + # use the Rails migration generator with 'create' + config.generators do |g| + g.orm :active_record, primary_key_type: :uuid + end end end From 5f40ff01341b7703d0b53e3d0c31de753c53abf9 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 3 Dec 2020 11:33:45 +0000 Subject: [PATCH 03/56] Install and add a Sidekiq process We'll use this to run asynchronous tasks, starting with warming the existing contentful cache. Keep Sidekiq Job data separate from Redis cache entries by using a different Redis URL. We could have used the same environment variable and hardcoded in different databases with /1, /2 etc. Using different variables for the whole value will allow their values to diverge as required without disrupting the other. --- .env.example | 1 + .env.test | 3 ++- Gemfile | 1 + Gemfile.lock | 6 ++++++ config/application.rb | 2 ++ config/initializers/sidekiq.rb | 7 +++++++ config/sidekiq.yml | 6 ++++++ docker-compose.yml | 13 +++++++++++++ heroku.yml | 4 ++++ 9 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 config/initializers/sidekiq.rb create mode 100644 config/sidekiq.yml diff --git a/.env.example b/.env.example index df498325f..178000046 100644 --- a/.env.example +++ b/.env.example @@ -11,6 +11,7 @@ ROLLBAR_ENV=development DATABASE_URL=postgres://postgres@localhost:5432/buy-for-your-school-development REDIS_CACHE_URL=redis://localhost:6379/1 +REDIS_SIDEKIQ_URL=redis://localhost:6379/2 # Contentful CONTENTFUL_URL=cdn.contentful.com diff --git a/.env.test b/.env.test index 36b12b2ff..1ed0b00d1 100644 --- a/.env.test +++ b/.env.test @@ -6,7 +6,8 @@ # Reference: https://github.com/bkeepers/dotenv#what-other-env-files-can-i-use DATABASE_URL=postgres://postgres@localhost:5432/buy-for-your-school-test -REDIS_CACHE_URL=redis://localhost:6379/2 +REDIS_CACHE_URL=redis://localhost:6379/11 +REDIS_SIDEKIQ_URL=redis://localhost:6379/12 # Contentful CONTENTFUL_URL=cdn.contentful.com diff --git a/Gemfile b/Gemfile index 40627cfff..c86cc5e99 100644 --- a/Gemfile +++ b/Gemfile @@ -20,6 +20,7 @@ gem "redis-namespace" gem "rollbar" gem "rails", "~> 6.0.0" gem "sass-rails", "~> 6.0" +gem "sidekiq", "~> 6.1" gem "turbolinks", "~> 5" gem "tzinfo-data", platforms: %i[mingw mswin x64_mingw jruby] gem "uglifier", ">= 1.3.0" diff --git a/Gemfile.lock b/Gemfile.lock index 41d862126..5f6ed2ec5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -91,6 +91,7 @@ GEM execjs coffee-script-source (1.12.2) concurrent-ruby (1.1.7) + connection_pool (2.2.3) contentful (2.15.4) http (> 0.8, < 5.0) multi_json (~> 1) @@ -267,6 +268,10 @@ GEM rubyzip (>= 1.2.2) shoulda-matchers (4.4.1) activesupport (>= 4.2.0) + sidekiq (6.1.2) + connection_pool (>= 2.2.2) + rack (~> 2.0) + redis (>= 4.2.0) simplecov (0.20.0) docile (~> 1.1) simplecov-html (~> 0.11) @@ -353,6 +358,7 @@ DEPENDENCIES sass-rails (~> 6.0) selenium-webdriver shoulda-matchers + sidekiq (~> 6.1) simplecov spring spring-commands-rspec diff --git a/config/application.rb b/config/application.rb index 4951d581c..0b9010946 100644 --- a/config/application.rb +++ b/config/application.rb @@ -38,5 +38,7 @@ class Application < Rails::Application config.generators do |g| g.orm :active_record, primary_key_type: :uuid end + + config.active_job.queue_adapter = :sidekiq end end diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb new file mode 100644 index 000000000..ad68a764d --- /dev/null +++ b/config/initializers/sidekiq.rb @@ -0,0 +1,7 @@ +Sidekiq.configure_server do |config| + config.redis = {url: ENV["REDIS_SIDEKIQ_URL"]} +end + +Sidekiq.configure_client do |config| + config.redis = {url: ENV["REDIS_SIDEKIQ_URL"]} +end diff --git a/config/sidekiq.yml b/config/sidekiq.yml new file mode 100644 index 000000000..e7bf86115 --- /dev/null +++ b/config/sidekiq.yml @@ -0,0 +1,6 @@ +--- +:concurrency: 5 +:queues: + - default + - [high_priority, 2] + - [caching, 1] diff --git a/docker-compose.yml b/docker-compose.yml index da51c0054..a68d0031a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -9,6 +9,7 @@ services: command: bundle exec rails s -p 3000 -b '0.0.0.0' ports: - "3000:3000" + image: "buy_for_your_school:dev" depends_on: - db - redis @@ -17,6 +18,7 @@ services: environment: DATABASE_URL: postgres://postgres:password@db:5432/buy-for-your-school-development REDIS_CACHE_URL: redis://redis:6379/1 + REDIS_SIDEKIQ_URL: redis://redis:6379/2 volumes: - .:/srv/app tty: true @@ -40,6 +42,17 @@ services: networks: - dev + sidekiq: + image: buy_for_your_school:dev + command: bundle exec sidekiq -C config/sidekiq.yml + depends_on: + - db + - redis + volumes: + - .:/srv/app + networks: + - dev + networks: dev: volumes: diff --git a/heroku.yml b/heroku.yml index a9aa82ec7..17b96f089 100644 --- a/heroku.yml +++ b/heroku.yml @@ -7,3 +7,7 @@ release: - rake db:migrate run: web: bundle exec puma -C config/puma.rb + worker: + command: + - bundle exec sidekiq -C config/sidekiq.yml + image: web From 5ca6924243eb097ad90c4abe9c5fc36187cb0728 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 3 Dec 2020 11:34:37 +0000 Subject: [PATCH 04/56] Sidekiq Cron installed to enable scheduled tasks Sidekiq can manage and process routine tasks in a similar way to Cron. The advantage to using Sidekiq for this now is that it makes it easier to extend the service with other async tasks. There's a good chance we'll need more of these and to then have to configure and look after a cron _and_ sidekiq process is more costly and introduces another box, which makes the overal architecture a little more complex. --- Gemfile | 1 + Gemfile.lock | 10 ++++++++++ config/initializers/sidekiq.rb | 6 ++++++ 3 files changed, 17 insertions(+) diff --git a/Gemfile b/Gemfile index c86cc5e99..627654ebc 100644 --- a/Gemfile +++ b/Gemfile @@ -21,6 +21,7 @@ gem "rollbar" gem "rails", "~> 6.0.0" gem "sass-rails", "~> 6.0" gem "sidekiq", "~> 6.1" +gem "sidekiq-cron", "~> 1.2" gem "turbolinks", "~> 5" gem "tzinfo-data", platforms: %i[mingw mswin x64_mingw jruby] gem "uglifier", ">= 1.3.0" diff --git a/Gemfile.lock b/Gemfile.lock index 5f6ed2ec5..7a7dcb2ae 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -106,6 +106,8 @@ GEM dotenv (= 2.7.6) railties (>= 3.2) erubi (1.10.0) + et-orbi (1.2.4) + tzinfo execjs (2.7.0) factory_bot (6.1.0) activesupport (>= 5.0.0) @@ -118,6 +120,9 @@ GEM ffi-compiler (1.0.1) ffi (>= 1.0.0) rake + fugit (1.4.1) + et-orbi (~> 1.1, >= 1.1.8) + raabro (~> 1.4) globalid (0.4.2) activesupport (>= 4.2.0) govuk_design_system_formbuilder (2.1.5) @@ -179,6 +184,7 @@ GEM public_suffix (4.0.6) puma (5.1.0) nio4r (~> 2.0) + raabro (1.4.0) rack (2.2.3) rack-test (1.1.0) rack (>= 1.0, < 3) @@ -272,6 +278,9 @@ GEM connection_pool (>= 2.2.2) rack (~> 2.0) redis (>= 4.2.0) + sidekiq-cron (1.2.0) + fugit (~> 1.1) + sidekiq (>= 4.2.1) simplecov (0.20.0) docile (~> 1.1) simplecov-html (~> 0.11) @@ -359,6 +368,7 @@ DEPENDENCIES selenium-webdriver shoulda-matchers sidekiq (~> 6.1) + sidekiq-cron (~> 1.2) simplecov spring spring-commands-rspec diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index ad68a764d..e80787b84 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -1,5 +1,11 @@ Sidekiq.configure_server do |config| config.redis = {url: ENV["REDIS_SIDEKIQ_URL"]} + + # Sidekiq Cron + schedule_file = "config/schedule.yml" + if File.exist?(schedule_file) + Sidekiq::Cron::Job.load_from_hash YAML.load_file(schedule_file) + end end Sidekiq.configure_client do |config| From faa66723e7933f02d36c7facab32be27c36b643a Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 3 Dec 2020 12:05:33 +0000 Subject: [PATCH 05/56] Refactor cache class so a key has to be passed to each method Setting a key on initialise was perhaps trying too hard to reduce the amount of code needed. Whilst the approach works when setting up a cache for a single entry, the same design doesn't work when we want to use the same cache and interact with many different entries. We shouldn't have to initialise a new cache object each time there's a new key. This change changes the approach so that the cache key needs to be passed as a parameter to each method. Less clean but is preferable to modifing a cache object's key data during an enumeration. --- app/services/cache.rb | 9 ++++----- app/services/concerns/cacheable_entry.rb | 12 ++++++------ app/services/get_contentful_entry.rb | 11 +++++++---- spec/services/cache_spec.rb | 25 ++++++++++++------------ 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/app/services/cache.rb b/app/services/cache.rb index d6b0d1d0a..d543b8875 100644 --- a/app/services/cache.rb +++ b/app/services/cache.rb @@ -1,8 +1,7 @@ class Cache attr_accessor :enabled, :key, :ttl - def initialize(enabled:, key:, ttl:) + def initialize(enabled:, ttl:) self.enabled = enabled - self.key = key self.ttl = ttl end @@ -10,7 +9,7 @@ def redis_cache @redis_cache ||= RedisCache.redis end - def hit? + def hit?(key:) if enabled == "true" redis_cache.exists?(key) else @@ -18,11 +17,11 @@ def hit? end end - def get + def get(key:) redis_cache.get(key) end - def set(value:) + def set(key:, value:) return unless enabled == "true" redis_cache.set(key, value) redis_cache.expire(key, ttl) diff --git a/app/services/concerns/cacheable_entry.rb b/app/services/concerns/cacheable_entry.rb index f265f6225..df8d7f859 100644 --- a/app/services/concerns/cacheable_entry.rb +++ b/app/services/concerns/cacheable_entry.rb @@ -1,23 +1,23 @@ module CacheableEntry extend ActiveSupport::Concern - def find_and_build_entry_from_cache(cache:) + def find_and_build_entry_from_cache(cache:, key:) Contentful::ResourceBuilder.new( - load_from_cache(cache: cache) + load_from_cache(cache: cache, key: key) ).run end - def store_in_cache(cache:, entry:) + def store_in_cache(cache:, key:, entry:) return unless entry.present? && entry.respond_to?(:raw) - cache.set(value: JSON.dump(entry.raw.to_json)) + cache.set(key: key, value: JSON.dump(entry.raw.to_json)) end private - def load_from_cache(cache:) + def load_from_cache(cache:, key:) # rubocop:disable Security/JSONLoad - serialised_json_string = cache.get + serialised_json_string = cache.get(key: key) unserialised_json_string = JSON.restore(serialised_json_string) JSON.parse(unserialised_json_string) # rubocop:enable Security/JSONLoad diff --git a/app/services/get_contentful_entry.rb b/app/services/get_contentful_entry.rb index 665e81a12..3afb626fb 100644 --- a/app/services/get_contentful_entry.rb +++ b/app/services/get_contentful_entry.rb @@ -9,17 +9,16 @@ def initialize(entry_id:, contentful_connector: ContentfulConnector.new) @contentful_connector = contentful_connector self.cache = Cache.new( enabled: ENV.fetch("CONTENTFUL_ENTRY_CACHING"), - key: "contentful:entry:#{entry_id}", ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 172_800) # 48 hours ) end def call - if cache.hit? - entry = find_and_build_entry_from_cache(cache: cache) + if cache.hit?(key: cache_key) + entry = find_and_build_entry_from_cache(cache: cache, key: cache_key) else entry = @contentful_connector.get_entry_by_id(entry_id) - store_in_cache(cache: cache, entry: entry) + store_in_cache(cache: cache, key: cache_key, entry: entry) end if entry.nil? @@ -32,6 +31,10 @@ def call private + def cache_key + "contentful:entry:#{entry_id}" + end + def send_rollbar_warning Rollbar.warning( "The following Contentful entry identifier could not be found.", diff --git a/spec/services/cache_spec.rb b/spec/services/cache_spec.rb index 52b3b4184..c8dbe8d0a 100644 --- a/spec/services/cache_spec.rb +++ b/spec/services/cache_spec.rb @@ -2,15 +2,14 @@ RSpec.describe Cache do it "requires a key, enabled flag and ttl" do - result = described_class.new(key: "redis-key", enabled: "true", ttl: 123) - expect(result.key).to eql("redis-key") + result = described_class.new(enabled: "true", ttl: 123) expect(result.enabled).to eql("true") expect(result.ttl).to eql(123) end describe "#redis_cache" do it "returns an object that quacks like a redis instance (since MockRedis is used in Test)" do - result = described_class.new(key: anything, enabled: anything, ttl: anything) + result = described_class.new(enabled: anything, ttl: anything) .redis_cache expect(result.respond_to?(:get)).to eq(true) @@ -23,23 +22,23 @@ describe "#hit?" do context "when enabled" do it "asks redis if that key exists" do - cache = described_class.new(key: "key-to-find", enabled: "true", ttl: anything) + cache = described_class.new(enabled: "true", ttl: anything) redis = cache.redis_cache expect(redis).to receive(:exists?).with("key-to-find") - cache.hit? + cache.hit?(key: "key-to-find") end end context "when disabled" do it "returns false" do - cache = described_class.new(key: "key-to-find", enabled: "false", ttl: anything) + cache = described_class.new(enabled: "false", ttl: anything) redis = cache.redis_cache expect(redis).not_to receive(:exists?).with("key-to-find") - result = cache.hit? + result = cache.hit?(key: "key-to-find") expect(result).to eq(false) end end @@ -47,37 +46,37 @@ describe "#get" do it "asks redis to get that key" do - cache = described_class.new(key: "key-to-find", enabled: "false", ttl: anything) + cache = described_class.new(enabled: "false", ttl: anything) redis = cache.redis_cache expect(redis).to receive(:get).with("key-to-find") - cache.get + cache.get(key: "key-to-find") end end describe "#set" do context "when enabled" do it "asks redis to set that key with the ttl" do - cache = described_class.new(key: "key-to-set", enabled: "true", ttl: 123) + cache = described_class.new(enabled: "true", ttl: 123) redis = cache.redis_cache expect(redis).to receive(:set).with("key-to-set", "value-to-set") expect(redis).to receive(:expire).with("key-to-set", 123) - cache.set(value: "value-to-set") + cache.set(key: "key-to-set", value: "value-to-set") end end context "when disabled" do it "does not ask redis to set that key" do - cache = described_class.new(key: "key-to-set", enabled: "false", ttl: 123) + cache = described_class.new(enabled: "false", ttl: 123) redis = cache.redis_cache expect(redis).not_to receive(:set).with("key-to-set", "value-to-set") expect(redis).not_to receive(:expire).with("key-to-set", 123) - cache.set(value: "value-to-set") + cache.set(key: "key-to-set", value: "value-to-set") end end end From ea6dd0f0cf49f796a5fa1dce03dee551eadeb397 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 12:08:48 +0000 Subject: [PATCH 06/56] Refactor default TTL to remove the need for a comment --- app/services/get_contentful_entry.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/get_contentful_entry.rb b/app/services/get_contentful_entry.rb index 3afb626fb..ccf43341f 100644 --- a/app/services/get_contentful_entry.rb +++ b/app/services/get_contentful_entry.rb @@ -9,7 +9,7 @@ def initialize(entry_id:, contentful_connector: ContentfulConnector.new) @contentful_connector = contentful_connector self.cache = Cache.new( enabled: ENV.fetch("CONTENTFUL_ENTRY_CACHING"), - ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 172_800) # 48 hours + ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 60 * 60 * 48) ) end From 9e88d4fe26075e27c84889a78e8fd81e8776da72 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 3 Dec 2020 11:34:52 +0000 Subject: [PATCH 07/56] Warm the Contentful Entry cache every night at 3am * Retry 5 times instead of the Sidekiq default which would continue to retry for 21 days. The same job will be requeued the next day so it doesn't make sense to have any job life for longer than 24 hours. If we cannot connect within 5 retries then it's unlikely that it will recover as Contentful is likely to be down. * It isn't straightforward to create a `Contenful::Array` as we would a normal object. It doesn't accept an array of entry objects. We have to use another Contentful method to help us create it in our test suite: `Contentful::ResourceBuilder.new(response_hash).run` * We are asserting that the serialised data is inserted into the Redis cache, rather than taking extra steps within the test to deserialise to make it more readable for us. --- CHANGELOG.md | 4 +- app/jobs/warm_entry_cache_job.rb | 22 ++++ app/services/concerns/cacheable_entry.rb | 1 - config/schedule.yml | 8 ++ ...0008-use-sidekiq-for-asynchronous-tasks.md | 28 +++++ .../contentful/multiple-entries-example.json | 102 ++++++++++++++++++ spec/jobs/warm_entry_cache_job_spec.rb | 46 ++++++++ 7 files changed, 209 insertions(+), 2 deletions(-) create mode 100644 app/jobs/warm_entry_cache_job.rb create mode 100644 config/schedule.yml create mode 100644 doc/architecture/decisions/0008-use-sidekiq-for-asynchronous-tasks.md create mode 100644 spec/fixtures/contentful/multiple-entries-example.json create mode 100644 spec/jobs/warm_entry_cache_job_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f6b9fb08d..848d45996 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog 1.0.0]. ## [Unreleased] +- fix primary key type on long_text_answers table to UUID +- nightly task to warm the Contentful cache for all entries + ## [release-003] - 2020-12-07 - add database foreign key constraints for better data integrity @@ -15,7 +18,6 @@ The format is based on [Keep a Changelog 1.0.0]. - refactor name of "Question" to "Step" - refactor redis caching into reusable class - users can be shown a step in the journey that is only static content -- fix primary key type on long_text_answers table to UUID ## [release-002] - 2020-11-16 diff --git a/app/jobs/warm_entry_cache_job.rb b/app/jobs/warm_entry_cache_job.rb new file mode 100644 index 000000000..19cfec270 --- /dev/null +++ b/app/jobs/warm_entry_cache_job.rb @@ -0,0 +1,22 @@ +class WarmEntryCacheJob < ApplicationJob + include CacheableEntry + + queue_as :caching + sidekiq_options retry: 5 + + def perform + entries = GetAllContentfulEntries.new.call + entries.each do |entry| + store_in_cache(cache: cache, key: "contentful:entry:#{entry.id}", entry: entry) + end + end + + private + + def cache + @cache ||= Cache.new( + enabled: ENV.fetch("CONTENTFUL_ENTRY_CACHING"), + ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 60 * 60 * 48) + ) + end +end diff --git a/app/services/concerns/cacheable_entry.rb b/app/services/concerns/cacheable_entry.rb index df8d7f859..3375184f4 100644 --- a/app/services/concerns/cacheable_entry.rb +++ b/app/services/concerns/cacheable_entry.rb @@ -9,7 +9,6 @@ def find_and_build_entry_from_cache(cache:, key:) def store_in_cache(cache:, key:, entry:) return unless entry.present? && entry.respond_to?(:raw) - cache.set(key: key, value: JSON.dump(entry.raw.to_json)) end diff --git a/config/schedule.yml b/config/schedule.yml new file mode 100644 index 000000000..544798e82 --- /dev/null +++ b/config/schedule.yml @@ -0,0 +1,8 @@ +warm_contentful_entry_cache: + cron: "0 3 * * *" + class: "WarmEntryCacheJob" + description: This is a content driven service and the content is sourced by + the Contentful API. As a single point of failure we have a Redis cache to + provide some resilience to downtime or minor connection issues. As this + service is used infrequently by real users, we automatically update the + cache. diff --git a/doc/architecture/decisions/0008-use-sidekiq-for-asynchronous-tasks.md b/doc/architecture/decisions/0008-use-sidekiq-for-asynchronous-tasks.md new file mode 100644 index 000000000..26d412b5b --- /dev/null +++ b/doc/architecture/decisions/0008-use-sidekiq-for-asynchronous-tasks.md @@ -0,0 +1,28 @@ +# 8. use sidekiq for asynchronous tasks + +Date: 2020-11-30 + +## Status + +Accepted + +## Context + +We need a way for the service to automatically and regularly run a task. + +We already have Redis available as our caching layer and Sidekiq works great with it. + +An alternative could be to use Cron for scheduled tasks and a postgres backed asynchronous jobs, perhaps even run inline. We know how to get Sidekiq running with Docker and reusing Redis (rather than Postgres) for job data that is ephemeral feels a better fit given we already have Redis. + +## Decision + +Use Sidekiq for processing asynchronous tasks. + +## Consequences + +- We add another process/box of complexity to the system architecture though we would need to do this with Cron too +- It should take less time to add another asynchronous task in future +- It should take less time to add another scheduled task +- A clear separation of concerns between persistent data being in Postgres and ephemeral data in Redis +- Sidekiq can manage the scheduled task queue on initialiser which automatically takes care of provisioning jobs on all environments +- Adding Sidekiq takes more time now and will require another widget to Terraform when moving to GPaaS. We have done this before. diff --git a/spec/fixtures/contentful/multiple-entries-example.json b/spec/fixtures/contentful/multiple-entries-example.json new file mode 100644 index 000000000..6570017e1 --- /dev/null +++ b/spec/fixtures/contentful/multiple-entries-example.json @@ -0,0 +1,102 @@ +{ + "sys": { + "type": "Array" + }, + "total": 5, + "skip": 0, + "limit": 100, + "items": [ + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "5kZ9hIFDvNCEhjWs72SFwj", + "type": "Entry", + "createdAt": "2020-12-02T10:48:35.748Z", + "updatedAt": "2020-12-02T10:48:35.748Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 1, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "staticContent" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/timelines", + "title": "When you should start", + "body": "Procuring a new catering contract can take up to 6 months to consult, create, review and award. \n\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.", + "type": "paragraphs", + "next": { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hfjJgWRg4xiiiImwVRDtZ" + } + } + } + }, + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "52Ni9UFvZj8BYXSbhs373C", + "type": "Entry", + "createdAt": "2020-11-04T12:28:30.442Z", + "updatedAt": "2020-11-26T16:39:54.188Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 6, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "question" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/dev-start-which-service", + "title": "Which service do you need?", + "helpText": "Tell us which service you need.", + "type": "radios", + "options": [ + "Catering", + "Cleaning" + ], + "next": { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hfjJgWRg4xiiiImwVRDtZ" + } + } + } + } + ] +} diff --git a/spec/jobs/warm_entry_cache_job_spec.rb b/spec/jobs/warm_entry_cache_job_spec.rb new file mode 100644 index 000000000..844e28912 --- /dev/null +++ b/spec/jobs/warm_entry_cache_job_spec.rb @@ -0,0 +1,46 @@ +require "rails_helper" + +RSpec.describe WarmEntryCacheJob, type: :job do + include ActiveJob::TestHelper + + before(:each) { ActiveJob::Base.queue_adapter = :test } + + around do |example| + ClimateControl.modify( + CONTENTFUL_ENTRY_CACHING: "true" + ) do + example.run + end + end + + describe ".perform_later" do + it "enqueues a job asynchronously on the caching queue" do + expect { + described_class.perform_later + }.to have_enqueued_job.on_queue("caching") + end + + it "asks GetAllContentfulEntries for the Contentful entries" do + raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/multiple-entries-example.json") + response_hash = JSON.parse(raw_response) + fake_contentful_entry_array = Contentful::ResourceBuilder.new(response_hash).run + + get_all_contentful_entries_double = instance_double(GetAllContentfulEntries) + allow(GetAllContentfulEntries).to receive(:new).and_return(get_all_contentful_entries_double) + allow(get_all_contentful_entries_double).to receive(:call).and_return(fake_contentful_entry_array) + + perform_enqueued_jobs do + described_class.perform_later + end + + expect(RedisCache.redis.get("contentful:entry:5kZ9hIFDvNCEhjWs72SFwj")) + .to eql( + "\"{\\\"sys\\\":{\\\"space\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Space\\\",\\\"id\\\":\\\"rwl7tyzv9sys\\\"}},\\\"id\\\":\\\"5kZ9hIFDvNCEhjWs72SFwj\\\",\\\"type\\\":\\\"Entry\\\",\\\"createdAt\\\":\\\"2020-12-02T10:48:35.748Z\\\",\\\"updatedAt\\\":\\\"2020-12-02T10:48:35.748Z\\\",\\\"environment\\\":{\\\"sys\\\":{\\\"id\\\":\\\"develop\\\",\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Environment\\\"}},\\\"revision\\\":1,\\\"contentType\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"ContentType\\\",\\\"id\\\":\\\"staticContent\\\"}},\\\"locale\\\":\\\"en-US\\\"},\\\"fields\\\":{\\\"slug\\\":\\\"/timelines\\\",\\\"title\\\":\\\"When you should start\\\",\\\"body\\\":\\\"Procuring a new catering contract can take up to 6 months to consult, create, review and award. \\\\n\\\\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.\\\",\\\"type\\\":\\\"paragraphs\\\",\\\"next\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Entry\\\",\\\"id\\\":\\\"hfjJgWRg4xiiiImwVRDtZ\\\"}}}}\"" + ) + expect(RedisCache.redis.get("contentful:entry:52Ni9UFvZj8BYXSbhs373C")) + .to eql( + "\"{\\\"sys\\\":{\\\"space\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Space\\\",\\\"id\\\":\\\"rwl7tyzv9sys\\\"}},\\\"id\\\":\\\"52Ni9UFvZj8BYXSbhs373C\\\",\\\"type\\\":\\\"Entry\\\",\\\"createdAt\\\":\\\"2020-11-04T12:28:30.442Z\\\",\\\"updatedAt\\\":\\\"2020-11-26T16:39:54.188Z\\\",\\\"environment\\\":{\\\"sys\\\":{\\\"id\\\":\\\"develop\\\",\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Environment\\\"}},\\\"revision\\\":6,\\\"contentType\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"ContentType\\\",\\\"id\\\":\\\"question\\\"}},\\\"locale\\\":\\\"en-US\\\"},\\\"fields\\\":{\\\"slug\\\":\\\"/dev-start-which-service\\\",\\\"title\\\":\\\"Which service do you need?\\\",\\\"helpText\\\":\\\"Tell us which service you need.\\\",\\\"type\\\":\\\"radios\\\",\\\"options\\\":[\\\"Catering\\\",\\\"Cleaning\\\"],\\\"next\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Entry\\\",\\\"id\\\":\\\"hfjJgWRg4xiiiImwVRDtZ\\\"}}}}\"" + ) + end + end +end From 77c9b79bb5ee6777c1c42e913472c30ac0646f37 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 15:12:13 +0000 Subject: [PATCH 08/56] Increase default cache TTL fro 48 to 72 hours This should cover a full weekend of downtime that we think will give Contentful enough time to fix a critical out of hours incident. --- app/jobs/warm_entry_cache_job.rb | 2 +- app/services/get_contentful_entry.rb | 2 +- spec/requests/contentful_caching_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/jobs/warm_entry_cache_job.rb b/app/jobs/warm_entry_cache_job.rb index 19cfec270..c8ec34877 100644 --- a/app/jobs/warm_entry_cache_job.rb +++ b/app/jobs/warm_entry_cache_job.rb @@ -16,7 +16,7 @@ def perform def cache @cache ||= Cache.new( enabled: ENV.fetch("CONTENTFUL_ENTRY_CACHING"), - ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 60 * 60 * 48) + ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 60 * 60 * 72) ) end end diff --git a/app/services/get_contentful_entry.rb b/app/services/get_contentful_entry.rb index ccf43341f..304d3d975 100644 --- a/app/services/get_contentful_entry.rb +++ b/app/services/get_contentful_entry.rb @@ -9,7 +9,7 @@ def initialize(entry_id:, contentful_connector: ContentfulConnector.new) @contentful_connector = contentful_connector self.cache = Cache.new( enabled: ENV.fetch("CONTENTFUL_ENTRY_CACHING"), - ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 60 * 60 * 48) + ttl: ENV.fetch("CONTENTFUL_ENTRY_CACHING_TTL", 60 * 60 * 72) ) end diff --git a/spec/requests/contentful_caching_spec.rb b/spec/requests/contentful_caching_spec.rb index c723b5684..ef2843584 100644 --- a/spec/requests/contentful_caching_spec.rb +++ b/spec/requests/contentful_caching_spec.rb @@ -40,7 +40,7 @@ RedisCache.redis.del("contentful:entry:1UjQurSOi5MWkcRuGxdXZS") end - it "sets a TTL to 48 hours by default" do + it "sets a TTL to 72 hours by default" do journey = create(:journey, next_entry_id: "1UjQurSOi5MWkcRuGxdXZS") stub_get_contentful_entry( entry_id: "1UjQurSOi5MWkcRuGxdXZS", @@ -51,7 +51,7 @@ get new_journey_step_path(journey) expect(RedisCache.redis.ttl("contentful:entry:1UjQurSOi5MWkcRuGxdXZS")) - .to eq(172_800) + .to eq(60 * 60 * 72) end RedisCache.redis.del("contentful:entry:1UjQurSOi5MWkcRuGxdXZS") From 4a999ab262d5917468aaa6a2b787c3d836c5bd3b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 7 Dec 2020 17:56:23 +0000 Subject: [PATCH 09/56] Bump standard from 0.10.1 to 0.10.2 Bumps [standard](https://github.com/testdouble/standard) from 0.10.1 to 0.10.2. - [Release notes](https://github.com/testdouble/standard/releases) - [Changelog](https://github.com/testdouble/standard/blob/master/CHANGELOG.md) - [Commits](https://github.com/testdouble/standard/compare/v0.10.1...v0.10.2) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 41d862126..5ce916b54 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -286,7 +286,7 @@ GEM actionpack (>= 4.0) activesupport (>= 4.0) sprockets (>= 3.0.0) - standard (0.10.1) + standard (0.10.2) rubocop (= 1.4.2) rubocop-performance (= 1.9.1) thor (1.0.1) From fcb662239c8b9edd1b2d2a0c3e2428e26dac3d37 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 16:28:34 +0000 Subject: [PATCH 10/56] Use service name that is sentence cased We named the service header with the same. This change doesn't start the conversation about what casing is right, it seeks to make the language we have chosen, consistent. --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index 6ade9bdff..a3ffb7ca9 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -52,4 +52,4 @@ Let everybody know that a new release has been shipped. The usual place to do this is #sct-buy-for-your-school, with a message like: -> 🚢 Release # for Buy for Your School is now live. Changes in this release: [link to changelog] 🚀 +> 🚢 Release # for Buy for your school is now live. Changes in this release: [link to changelog] 🚀 From 64c3109bb80a6429d955b14d3f9a05b969c3007b Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 16:30:02 +0000 Subject: [PATCH 11/56] Release message prompts for full release number Using "#" could be mistaken as please use a single number. In all of our release documentation we use 3 digits with zero padding eg. 001. Let's prompt us to use this in all messaging by using three placeholders. --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index a3ffb7ca9..dbcba4328 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -52,4 +52,4 @@ Let everybody know that a new release has been shipped. The usual place to do this is #sct-buy-for-your-school, with a message like: -> 🚢 Release # for Buy for your school is now live. Changes in this release: [link to changelog] 🚀 +> 🚢 Release ### for Buy for your school is now live. Changes in this release: [link to changelog] 🚀 From 9e46758a6d3088fe599cbc12554da97d3a1d4231 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 16:32:14 +0000 Subject: [PATCH 12/56] Include example of how to show changes in the release Linking to the commit diff isn't that useful for non-dev folk. Let's use the CHANGELOG.md document, anchor linked to the human readable list we've already prepared. --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index dbcba4328..2ca4f9f93 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -52,4 +52,4 @@ Let everybody know that a new release has been shipped. The usual place to do this is #sct-buy-for-your-school, with a message like: -> 🚢 Release ### for Buy for your school is now live. Changes in this release: [link to changelog] 🚀 +> 🚢 Release ### for Buy for your school is now live. (Changes in this release)[https://github.com/DFE-Digital/buy-for-your-school/blob/develop/CHANGELOG.md#release-###---YYYY-MM-DD] 🚀 From 4cfef8118784f4f261b164e810971c35915e0b1b Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 16:33:11 +0000 Subject: [PATCH 13/56] Release message includes link to production A helpful touch to help non-team folk find the service to have a look, after being told about it in the wider Slack channel. --- doc/release-process.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/release-process.md b/doc/release-process.md index 2ca4f9f93..adae30ae1 100644 --- a/doc/release-process.md +++ b/doc/release-process.md @@ -52,4 +52,4 @@ Let everybody know that a new release has been shipped. The usual place to do this is #sct-buy-for-your-school, with a message like: -> 🚢 Release ### for Buy for your school is now live. (Changes in this release)[https://github.com/DFE-Digital/buy-for-your-school/blob/develop/CHANGELOG.md#release-###---YYYY-MM-DD] 🚀 +> 🚢 Release ### for Buy for your school is now live. (Changes in this release)[https://github.com/DFE-Digital/buy-for-your-school/blob/develop/CHANGELOG.md#release-###---YYYY-MM-DD] - (Production environment)[https://dfe-buy-for-your-school-beta.herokuapp.com/] 🚀 From 38fa030b97206b6a827ec77751b55b0a26b727d5 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 17:08:44 +0000 Subject: [PATCH 14/56] Reword existing Contentful feature tests --- .../visitors/anyone_can_complete_a_journey_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb index 34b905b95..80df4573c 100644 --- a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb +++ b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb @@ -81,7 +81,7 @@ expect(page).to have_content(I18n.t("errors.contentful_entry_not_found.page_body")) end - context "when Contentful entry is of type short_text" do + context "when the Contentful Entry is of type short_text" do around do |example| ClimateControl.modify( CONTENTFUL_PLANNING_START_ENTRY_ID: "hfjJgWRg4xiiiImwVRDtZ" @@ -106,7 +106,7 @@ end end - context "when Contentful entry is of type long_text" do + context "when the Contentful Entry is of type long_text" do around do |example| ClimateControl.modify( CONTENTFUL_PLANNING_START_ENTRY_ID: "2jWIO1MrVIya9NZrFWT4e" @@ -135,7 +135,7 @@ end end - context "when Contentful entry is of type static_content" do + context "when the Contentful Entry is of type static_content" do around do |example| ClimateControl.modify( CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" @@ -167,7 +167,7 @@ end end - context "when Contentful entry model wasn't a type of question" do + context "when the Contentful Entry model wasn't of type 'question'" do around do |example| ClimateControl.modify( CONTENTFUL_PLANNING_START_ENTRY_ID: "6EKsv389ETYcQql3htK3Z2" @@ -191,7 +191,7 @@ end end - context "when Contentful step entry wasn't an expected type" do + context "when the Contentful Entry wasn't an expected step type" do around do |example| ClimateControl.modify( CONTENTFUL_PLANNING_START_ENTRY_ID: "8as7df68uhasdnuasdf" From 42604f9cce7a804f82c8a317249ee4cac8e933fd Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 17:47:41 +0000 Subject: [PATCH 15/56] Allow Contentful to set a primary button text on each step An optional field that allows the content team to set different content. We should take this data out of `raw` and store it on a field as we intend to use it. --- ...20201207171647_add_primary_call_to_action_text_to_step.rb | 5 +++++ db/schema.rb | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20201207171647_add_primary_call_to_action_text_to_step.rb diff --git a/db/migrate/20201207171647_add_primary_call_to_action_text_to_step.rb b/db/migrate/20201207171647_add_primary_call_to_action_text_to_step.rb new file mode 100644 index 000000000..19a710411 --- /dev/null +++ b/db/migrate/20201207171647_add_primary_call_to_action_text_to_step.rb @@ -0,0 +1,5 @@ +class AddPrimaryCallToActionTextToStep < ActiveRecord::Migration[6.0] + def change + add_column :steps, :primary_call_to_action_text, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 02efda6e6..57c3cbca9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_12_03_103421) do +ActiveRecord::Schema.define(version: 2020_12_07_171647) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -59,6 +59,7 @@ t.datetime "updated_at", precision: 6, null: false t.text "body" t.string "contentful_model" + t.string "primary_call_to_action_text" t.index ["journey_id"], name: "index_steps_on_journey_id" end From 93543ae9c56b9fca9de7ef403eddce6bd88f9e2d Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 7 Dec 2020 17:50:05 +0000 Subject: [PATCH 16/56] Primary form button text is configurable This is an optional field. If it's not provided or forgotten there's a fallback of "Continue". Next steps would be to create a StepPresenter for the view logic. We may need secondary buttons "skip" or tertiary buttons like "cancel" or "back". We use the name "primary" in anticipating for that. --- CHANGELOG.md | 1 + app/models/step.rb | 5 ++ app/services/create_journey_step.rb | 6 +++ app/views/steps/new.long_text.html.erb | 2 +- app/views/steps/new.paragraphs.html.erb | 2 +- app/views/steps/new.radios.html.erb | 2 +- app/views/steps/new.short_text.html.erb | 2 +- config/locales/en.yml | 1 - .../anyone_can_complete_a_journey_spec.rb | 46 ++++++++++++++++--- .../contentful/no-primary-button-example.json | 37 +++++++++++++++ .../contentful/primary-button-example.json | 38 +++++++++++++++ spec/services/create_journey_step_spec.rb | 30 ++++++++++++ spec/support/contentful_helpers.rb | 1 + 13 files changed, 162 insertions(+), 11 deletions(-) create mode 100644 spec/fixtures/contentful/no-primary-button-example.json create mode 100644 spec/fixtures/contentful/primary-button-example.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 848d45996..a4f57b8f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog 1.0.0]. - fix primary key type on long_text_answers table to UUID - nightly task to warm the Contentful cache for all entries +- form button content is configurable through Contentful ## [release-003] - 2020-12-07 diff --git a/app/models/step.rb b/app/models/step.rb index 6ce516683..50d1be7ac 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -17,4 +17,9 @@ def answer def question? contentful_model == "question" end + + def primary_call_to_action_text + return I18n.t("generic.button.next") unless super.present? + super + end end diff --git a/app/services/create_journey_step.rb b/app/services/create_journey_step.rb index d8af03a5b..b2aa6a09a 100644 --- a/app/services/create_journey_step.rb +++ b/app/services/create_journey_step.rb @@ -30,6 +30,7 @@ def call contentful_model: content_model, contentful_type: step_type, options: options, + primary_call_to_action_text: primary_call_to_action_text, raw: raw, journey: journey ) @@ -88,6 +89,11 @@ def step_type contentful_entry.type.tr(" ", "_") end + def primary_call_to_action_text + return nil unless contentful_entry.respond_to?(:primary_call_to_action) + contentful_entry.primary_call_to_action + end + def raw contentful_entry.raw end diff --git a/app/views/steps/new.long_text.html.erb b/app/views/steps/new.long_text.html.erb index dfa991e31..3a1f65b24 100644 --- a/app/views/steps/new.long_text.html.erb +++ b/app/views/steps/new.long_text.html.erb @@ -7,5 +7,5 @@ width: "one-third", rows: 9 %> - <%= f.submit I18n.t("generic.button.soft_finish"), class: "govuk-button" %> + <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> diff --git a/app/views/steps/new.paragraphs.html.erb b/app/views/steps/new.paragraphs.html.erb index 7306d4b5e..67a00db56 100644 --- a/app/views/steps/new.paragraphs.html.erb +++ b/app/views/steps/new.paragraphs.html.erb @@ -4,4 +4,4 @@
<%= simple_format(@step.body, wrapper_tag: "p", class: "govuk-body") %>
-<%= link_to I18n.t("generic.button.next"), new_journey_step_path, class: "govuk-button" %> +<%= link_to @step.primary_call_to_action_text, new_journey_step_path, class: "govuk-button" %> diff --git a/app/views/steps/new.radios.html.erb b/app/views/steps/new.radios.html.erb index cb5558178..d853fa087 100644 --- a/app/views/steps/new.radios.html.erb +++ b/app/views/steps/new.radios.html.erb @@ -11,5 +11,5 @@ hint: { text: @step.help_text }, inline: false %> - <%= f.submit I18n.t("generic.button.soft_finish"), class: "govuk-button" %> + <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> diff --git a/app/views/steps/new.short_text.html.erb b/app/views/steps/new.short_text.html.erb index 4ee893f35..2b9e9902e 100644 --- a/app/views/steps/new.short_text.html.erb +++ b/app/views/steps/new.short_text.html.erb @@ -6,5 +6,5 @@ hint: { text: @step.help_text }, width: "one-third" %> - <%= f.submit I18n.t("generic.button.soft_finish"), class: "govuk-button" %> + <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index f607c4ca4..f6fa9bfcf 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -5,7 +5,6 @@ en: button: start: "Continue" next: "Continue" - soft_finish: "Save and continue later" banner: beta: tag: "beta" diff --git a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb index 80df4573c..bcf483819 100644 --- a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb +++ b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb @@ -15,7 +15,7 @@ choose("Catering") - click_on(I18n.t("generic.button.soft_finish")) + click_on(I18n.t("generic.button.next")) end scenario "an answer must be provided" do @@ -27,7 +27,7 @@ # Omit a choice - click_on(I18n.t("generic.button.soft_finish")) + click_on(I18n.t("generic.button.next")) expect(page).to have_content("can't be blank") end @@ -56,10 +56,11 @@ entry_id: "5lYcZs1ootDrOnk09LDLZg", fixture_filename: "no-next-question-example.json" ) - click_on(I18n.t("generic.button.soft_finish")) + + click_on(I18n.t("generic.button.next")) choose("Stationary") - click_on(I18n.t("generic.button.soft_finish")) + click_on(I18n.t("generic.button.next")) expect(page).to have_content("Catering") expect(page).to have_content("Stationary") @@ -100,7 +101,7 @@ click_on(I18n.t("generic.button.start")) fill_in "answer[response]", with: "email@example.com" - click_on(I18n.t("generic.button.soft_finish")) + click_on(I18n.t("generic.button.next")) expect(page).to have_content("email@example") end @@ -125,7 +126,7 @@ click_on(I18n.t("generic.button.start")) fill_in "answer[response]", with: "We would like a supplier to provide catering from September 2020.\r\nThey must be able to supply us for 3 years minumum." - click_on(I18n.t("generic.button.soft_finish")) + click_on(I18n.t("generic.button.next")) within(".govuk-summary-list") do paragraphs_elements = find_all("p") @@ -167,6 +168,39 @@ end end + context "when the Contentful Entry has a 'primaryCallToAction' field" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + scenario "user can read static content and proceed without answering" do + stub_get_contentful_entry( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "primary-button-example.json" + ) + + visit root_path + click_on(I18n.t("generic.button.start")) + + expect(page).to have_content("When you should start") + + within(".static-content") do + paragraphs_elements = find_all("p") + expect(paragraphs_elements.first.text).to have_content("Procuring a new catering contract can take up to 6 months to consult, create, review and award.") + expect(paragraphs_elements.last.text).to have_content("Usually existing contracts start and end in the month of September. We recommend starting this process around March.") + end + + expect(page).not_to have_content(I18n.t("generic.button.next")) + click_on("Go onwards!") # Language from fixture + + expect(page).to have_content("Catering") + end + end + context "when the Contentful Entry model wasn't of type 'question'" do around do |example| ClimateControl.modify( diff --git a/spec/fixtures/contentful/no-primary-button-example.json b/spec/fixtures/contentful/no-primary-button-example.json new file mode 100644 index 000000000..07f995490 --- /dev/null +++ b/spec/fixtures/contentful/no-primary-button-example.json @@ -0,0 +1,37 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "5kZ9hIFDvNCEhjWs72SFwj", + "type": "Entry", + "createdAt": "2020-12-02T10:48:35.748Z", + "updatedAt": "2020-12-07T17:04:21.854Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "staticContent" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/timelines", + "title": "When you should start", + "body": "Procuring a new catering contract can take up to 6 months to consult, create, review and award. \n\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.", + "type": "paragraphs" + } +} diff --git a/spec/fixtures/contentful/primary-button-example.json b/spec/fixtures/contentful/primary-button-example.json new file mode 100644 index 000000000..5e80952ab --- /dev/null +++ b/spec/fixtures/contentful/primary-button-example.json @@ -0,0 +1,38 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "5kZ9hIFDvNCEhjWs72SFwj", + "type": "Entry", + "createdAt": "2020-12-02T10:48:35.748Z", + "updatedAt": "2020-12-07T17:04:21.854Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "staticContent" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/timelines", + "title": "When you should start", + "body": "Procuring a new catering contract can take up to 6 months to consult, create, review and award. \n\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.", + "type": "paragraphs", + "primaryCallToAction": "Go onwards!" + } +} diff --git a/spec/services/create_journey_step_spec.rb b/spec/services/create_journey_step_spec.rb index 86edab1cf..c6b450e3f 100644 --- a/spec/services/create_journey_step_spec.rb +++ b/spec/services/create_journey_step_spec.rb @@ -86,6 +86,36 @@ end end + context "when the new entry has a 'primaryCallToAction' field" do + it "updates the step with the body" do + journey = create(:journey, :catering) + fake_entry = fake_contentful_step_entry( + contentful_fixture_filename: "primary-button-example.json" + ) + + step, _answer = described_class.new( + journey: journey, contentful_entry: fake_entry + ).call + + expect(step.primary_call_to_action_text).to eq("Go onwards!") + end + end + + context "when no 'primaryCallToAction' is provided" do + it "default copy is used for the button" do + journey = create(:journey, :catering) + fake_entry = fake_contentful_step_entry( + contentful_fixture_filename: "no-primary-button-example.json" + ) + + step, _answer = described_class.new( + journey: journey, contentful_entry: fake_entry + ).call + + expect(step.primary_call_to_action_text).to eq(I18n.t("generic.button.next")) + end + end + context "when the new entry has an unexpected content model" do it "raises an error" do journey = create(:journey, :catering) diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index 819898e5b..531078b96 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -49,6 +49,7 @@ def fake_contentful_step_entry(contentful_fixture_filename:) options: hash_response.dig("fields", "options"), type: hash_response.dig("fields", "type"), next: double(id: hash_response.dig("fields", "next", "sys", "id")), + primary_call_to_action: hash_response.dig("fields", "primaryCallToAction"), raw: raw_response, content_type: double(id: hash_response.dig("sys", "contentType", "sys", "id")) ) From 536a99a95d1c213eda885d157bbcaac138367871 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 8 Dec 2020 10:55:51 +0000 Subject: [PATCH 17/56] Group and order feature tests to separate StaticContent * Also move the unhappy test of "a Contentful entry_id does not exist" to the bottom of the vertical hierarchy of importance --- .../anyone_can_complete_a_journey_spec.rb | 187 ++++++++---------- 1 file changed, 79 insertions(+), 108 deletions(-) diff --git a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb index bcf483819..5765e6463 100644 --- a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb +++ b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb @@ -67,141 +67,97 @@ end end - scenario "a Contentful entry_id does not exist" do - contentful_client = stub_contentful_client - - allow(contentful_client).to receive(:entry) - .with(anything) - .and_raise(GetContentfulEntry::EntryNotFound.new("The following Contentful error could not be found: sss ")) - - visit root_path - - click_on(I18n.t("generic.button.start")) - - expect(page).to have_content(I18n.t("errors.contentful_entry_not_found.page_title")) - expect(page).to have_content(I18n.t("errors.contentful_entry_not_found.page_body")) - end - - context "when the Contentful Entry is of type short_text" do - around do |example| - ClimateControl.modify( - CONTENTFUL_PLANNING_START_ENTRY_ID: "hfjJgWRg4xiiiImwVRDtZ" - ) do - example.run - end - end - - scenario "user can answer using free text" do - stub_get_contentful_entry( - entry_id: "hfjJgWRg4xiiiImwVRDtZ", - fixture_filename: "short-text-question-example.json" - ) - - visit root_path - click_on(I18n.t("generic.button.start")) - - fill_in "answer[response]", with: "email@example.com" - click_on(I18n.t("generic.button.next")) - - expect(page).to have_content("email@example") - end - end - - context "when the Contentful Entry is of type long_text" do - around do |example| - ClimateControl.modify( - CONTENTFUL_PLANNING_START_ENTRY_ID: "2jWIO1MrVIya9NZrFWT4e" - ) do - example.run + context "when the Contentful model is of type question" do + context "when Contentful entry is of type short_text" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "hfjJgWRg4xiiiImwVRDtZ" + ) do + example.run + end end - end - scenario "user can answer using free text with multiple lines" do - stub_get_contentful_entry( - entry_id: "2jWIO1MrVIya9NZrFWT4e", - fixture_filename: "long-text-question-example.json" - ) + scenario "user can answer using free text" do + stub_get_contentful_entry( + entry_id: "hfjJgWRg4xiiiImwVRDtZ", + fixture_filename: "short-text-question-example.json" + ) - visit root_path - click_on(I18n.t("generic.button.start")) + visit root_path + click_on(I18n.t("generic.button.start")) - fill_in "answer[response]", with: "We would like a supplier to provide catering from September 2020.\r\nThey must be able to supply us for 3 years minumum." - click_on(I18n.t("generic.button.next")) + fill_in "answer[response]", with: "email@example.com" + click_on(I18n.t("generic.button.next")) - within(".govuk-summary-list") do - paragraphs_elements = find_all("p") - expect(paragraphs_elements.first.text).to have_content("We would like a supplier to provide catering from September 2020.") - expect(paragraphs_elements.last.text).to have_content("They must be able to supply us for 3 years minumum.") + expect(page).to have_content("email@example") end end - end - context "when the Contentful Entry is of type static_content" do - around do |example| - ClimateControl.modify( - CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" - ) do - example.run + context "when Contentful entry is of type long_text" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "2jWIO1MrVIya9NZrFWT4e" + ) do + example.run + end end - end - scenario "user can read static content and proceed without answering" do - stub_get_contentful_entry( - entry_id: "5kZ9hIFDvNCEhjWs72SFwj", - fixture_filename: "static-content-example.json" - ) + scenario "user can answer using free text with multiple lines" do + stub_get_contentful_entry( + entry_id: "2jWIO1MrVIya9NZrFWT4e", + fixture_filename: "long-text-question-example.json" + ) - visit root_path - click_on(I18n.t("generic.button.start")) + visit root_path + click_on(I18n.t("generic.button.start")) - expect(page).to have_content("When you should start") + fill_in "answer[response]", with: "We would like a supplier to provide catering from September 2020.\r\nThey must be able to supply us for 3 years minumum." + click_on(I18n.t("generic.button.next")) - within(".static-content") do - paragraphs_elements = find_all("p") - expect(paragraphs_elements.first.text).to have_content("Procuring a new catering contract can take up to 6 months to consult, create, review and award.") - expect(paragraphs_elements.last.text).to have_content("Usually existing contracts start and end in the month of September. We recommend starting this process around March.") + within(".govuk-summary-list") do + paragraphs_elements = find_all("p") + expect(paragraphs_elements.first.text).to have_content("We would like a supplier to provide catering from September 2020.") + expect(paragraphs_elements.last.text).to have_content("They must be able to supply us for 3 years minumum.") + end end - - click_on(I18n.t("generic.button.next")) - - expect(page).to have_content("Catering") end end - context "when the Contentful Entry has a 'primaryCallToAction' field" do - around do |example| - ClimateControl.modify( - CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" - ) do - example.run + context "when the Contentful model is of type staticContent" do + context "when Contentful entry is of type paragraphs" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end end - end - scenario "user can read static content and proceed without answering" do - stub_get_contentful_entry( - entry_id: "5kZ9hIFDvNCEhjWs72SFwj", - fixture_filename: "primary-button-example.json" - ) + scenario "user can read static content and proceed without answering" do + stub_get_contentful_entry( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "static-content-example.json" + ) - visit root_path - click_on(I18n.t("generic.button.start")) + visit root_path + click_on(I18n.t("generic.button.start")) - expect(page).to have_content("When you should start") + expect(page).to have_content("When you should start") - within(".static-content") do - paragraphs_elements = find_all("p") - expect(paragraphs_elements.first.text).to have_content("Procuring a new catering contract can take up to 6 months to consult, create, review and award.") - expect(paragraphs_elements.last.text).to have_content("Usually existing contracts start and end in the month of September. We recommend starting this process around March.") - end + within(".static-content") do + paragraphs_elements = find_all("p") + expect(paragraphs_elements.first.text).to have_content("Procuring a new catering contract can take up to 6 months to consult, create, review and award.") + expect(paragraphs_elements.last.text).to have_content("Usually existing contracts start and end in the month of September. We recommend starting this process around March.") + end - expect(page).not_to have_content(I18n.t("generic.button.next")) - click_on("Go onwards!") # Language from fixture + click_on(I18n.t("generic.button.next")) - expect(page).to have_content("Catering") + expect(page).to have_content("Catering") + end end end - context "when the Contentful Entry model wasn't of type 'question'" do + context "when Contentful entry model wasn't an expected type" do around do |example| ClimateControl.modify( CONTENTFUL_PLANNING_START_ENTRY_ID: "6EKsv389ETYcQql3htK3Z2" @@ -248,4 +204,19 @@ expect(page).to have_content(I18n.t("errors.unexpected_contentful_step_type.page_body")) end end + + scenario "a Contentful entry_id does not exist" do + contentful_client = stub_contentful_client + + allow(contentful_client).to receive(:entry) + .with(anything) + .and_raise(GetContentfulEntry::EntryNotFound.new("The following Contentful error could not be found: sss ")) + + visit root_path + + click_on(I18n.t("generic.button.start")) + + expect(page).to have_content(I18n.t("errors.contentful_entry_not_found.page_title")) + expect(page).to have_content(I18n.t("errors.contentful_entry_not_found.page_body")) + end end From 5c5da158ba916bf954b2ded6a82b7ecbc028a507 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 8 Dec 2020 11:07:21 +0000 Subject: [PATCH 18/56] Retrofit LongTextAnswer tests --- app/controllers/journeys_controller.rb | 2 +- config/environments/test.rb | 1 + spec/models/long_text_answer_spec.rb | 14 ++++++++++++++ spec/models/step_spec.rb | 9 +++++++++ 4 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 spec/models/long_text_answer_spec.rb diff --git a/app/controllers/journeys_controller.rb b/app/controllers/journeys_controller.rb index 39774f62d..5e5f845a6 100644 --- a/app/controllers/journeys_controller.rb +++ b/app/controllers/journeys_controller.rb @@ -8,7 +8,7 @@ def new def show @journey = Journey.includes( - steps: [:radio_answer, :short_text_answer] + steps: [:radio_answer, :short_text_answer, :long_text_answer] ).find(journey_id) end diff --git a/config/environments/test.rb b/config/environments/test.rb index cc7c118b5..7979f199c 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -61,5 +61,6 @@ Bullet.raise = true # raise an error if n+1 query occurs Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :radio_answer Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :short_text_answer + Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :long_text_answer end end diff --git a/spec/models/long_text_answer_spec.rb b/spec/models/long_text_answer_spec.rb new file mode 100644 index 000000000..f71ec374c --- /dev/null +++ b/spec/models/long_text_answer_spec.rb @@ -0,0 +1,14 @@ +require "rails_helper" + +RSpec.describe LongTextAnswer, type: :model do + it { should belong_to(:step) } + + it "captures the users response as a string" do + answer = build(:long_text_answer, response: "Yellow") + expect(answer.response).to eql("Yellow") + end + + describe "validations" do + it { is_expected.to validate_presence_of(:response) } + end +end diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index 11bfb2abe..e9ea01999 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -4,6 +4,7 @@ it { should belong_to(:journey) } it { should have_one(:radio_answer) } it { should have_one(:short_text_answer) } + it { should have_one(:long_text_answer) } it "store the basic fields of a contentful response" do step = build(:step, @@ -41,6 +42,14 @@ expect(step.answer).to eq(short_text_answer) end end + + context "when a LongTextAnswer is associated to the step" do + it "returns the LongTextAnswer object" do + long_text_answer = create(:long_text_answer) + step = create(:step, :long_text, long_text_answer: long_text_answer) + expect(step.answer).to eq(long_text_answer) + end + end end describe "#question?" do From 6d1bd5201609a27124bfc1a258a3f597b68e6d95 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 8 Dec 2020 11:43:53 +0000 Subject: [PATCH 19/56] Introduce SingleDateAnswer as a new resource * This will be used to support a new Contentful Entry of type single_date next. * I imagine that we may need to handle a separate type for a date range where we ask for 2 dates as part of the same question. We could then have SingleDateAnswer and DateRangeAnswer. --- app/models/single_date_answer.rb | 6 ++++++ app/models/step.rb | 7 ++++++- .../20201208110342_create_single_date_answer.rb | 9 +++++++++ db/schema.rb | 10 +++++++++- spec/factories/answer.rb | 6 ++++++ spec/models/single_date_answer_spec.rb | 16 ++++++++++++++++ 6 files changed, 52 insertions(+), 2 deletions(-) create mode 100644 app/models/single_date_answer.rb create mode 100644 db/migrate/20201208110342_create_single_date_answer.rb create mode 100644 spec/models/single_date_answer_spec.rb diff --git a/app/models/single_date_answer.rb b/app/models/single_date_answer.rb new file mode 100644 index 000000000..b85a192c3 --- /dev/null +++ b/app/models/single_date_answer.rb @@ -0,0 +1,6 @@ +class SingleDateAnswer < ActiveRecord::Base + self.implicit_order_column = "created_at" + belongs_to :step + + validates :response, presence: true +end diff --git a/app/models/step.rb b/app/models/step.rb index 50d1be7ac..ce62729be 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -5,13 +5,18 @@ class Step < ApplicationRecord has_one :radio_answer has_one :short_text_answer has_one :long_text_answer + has_one :single_date_answer def radio_options options.map { |option| OpenStruct.new(id: option.downcase, name: option.titleize) } end def answer - @answer ||= radio_answer || short_text_answer || long_text_answer + @answer ||= + radio_answer || + short_text_answer || + long_text_answer || + single_date_answer end def question? diff --git a/db/migrate/20201208110342_create_single_date_answer.rb b/db/migrate/20201208110342_create_single_date_answer.rb new file mode 100644 index 000000000..c84bbebd0 --- /dev/null +++ b/db/migrate/20201208110342_create_single_date_answer.rb @@ -0,0 +1,9 @@ +class CreateSingleDateAnswer < ActiveRecord::Migration[6.0] + def change + create_table :single_date_answers, id: :uuid do |t| + t.references :step, type: :uuid + t.date :response, null: false + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 57c3cbca9..6b648588a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_12_07_171647) do +ActiveRecord::Schema.define(version: 2020_12_08_110342) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -48,6 +48,14 @@ t.index ["step_id"], name: "index_short_text_answers_on_step_id" end + create_table "single_date_answers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "step_id" + t.date "response", null: false + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["step_id"], name: "index_single_date_answers_on_step_id" + end + create_table "steps", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.uuid "journey_id" t.string "title", null: false diff --git a/spec/factories/answer.rb b/spec/factories/answer.rb index fd1a42adf..0c4902ddd 100644 --- a/spec/factories/answer.rb +++ b/spec/factories/answer.rb @@ -16,4 +16,10 @@ response { "Lots of green" } end + + factory :single_date_answer do + association :step, factory: :step, contentful_type: "single_date" + + response { 1.year.from_now } + end end diff --git a/spec/models/single_date_answer_spec.rb b/spec/models/single_date_answer_spec.rb new file mode 100644 index 000000000..58d8ac896 --- /dev/null +++ b/spec/models/single_date_answer_spec.rb @@ -0,0 +1,16 @@ +require "rails_helper" + +RSpec.describe SingleDateAnswer, type: :model do + it { should belong_to(:step) } + + describe "#response" do + it "returns a date" do + answer = build(:single_date_answer, response: Date.new(2020, 10, 1)) + expect(answer.response).to eq(Date.new(2020, 10, 1)) + end + end + + describe "validations" do + it { is_expected.to validate_presence_of(:response) } + end +end From fa11b7b554934a7ab438b8c8f808cff252962b44 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 8 Dec 2020 11:45:02 +0000 Subject: [PATCH 20/56] Users can be asked Single date questions * Add a default formatter to make them human readable * Use the DfE Form builder gem to handle the design pattern --- CHANGELOG.md | 1 + app/services/answer_factory.rb | 1 + app/services/create_journey_step.rb | 2 +- .../journeys/_single_date_answer.html.erb | 4 +++ app/views/steps/new.single_date.html.erb | 9 +++++ config/environments/test.rb | 1 + config/locales/en.yml | 3 ++ spec/factories/step.rb | 7 ++++ .../anyone_can_complete_a_journey_spec.rb | 28 +++++++++++++++ .../contentful/single-date-example.json | 36 +++++++++++++++++++ spec/models/step_spec.rb | 9 +++++ 11 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 app/views/journeys/_single_date_answer.html.erb create mode 100644 app/views/steps/new.single_date.html.erb create mode 100644 spec/fixtures/contentful/single-date-example.json diff --git a/CHANGELOG.md b/CHANGELOG.md index a4f57b8f8..a8c1ac3ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog 1.0.0]. - fix primary key type on long_text_answers table to UUID - nightly task to warm the Contentful cache for all entries - form button content is configurable through Contentful +- users can be asked to provide a single date answer ## [release-003] - 2020-12-07 diff --git a/app/services/answer_factory.rb b/app/services/answer_factory.rb index eb3cfc5d6..a35599cd1 100644 --- a/app/services/answer_factory.rb +++ b/app/services/answer_factory.rb @@ -10,6 +10,7 @@ def call when "radios" then RadioAnswer.new when "short_text" then ShortTextAnswer.new when "long_text" then LongTextAnswer.new + when "single_date" then SingleDateAnswer.new end end end diff --git a/app/services/create_journey_step.rb b/app/services/create_journey_step.rb index b2aa6a09a..bbf8331af 100644 --- a/app/services/create_journey_step.rb +++ b/app/services/create_journey_step.rb @@ -4,7 +4,7 @@ class UnexpectedContentfulModel < StandardError; end class UnexpectedContentfulStepType < StandardError; end ALLOWED_CONTENTFUL_MODELS = %w[question staticContent].freeze - ALLOWED_CONTENTFUL_ENTRY_TYPES = %w[radios short_text long_text paragraphs].freeze + ALLOWED_CONTENTFUL_ENTRY_TYPES = %w[radios short_text long_text paragraphs single_date].freeze attr_accessor :journey, :contentful_entry def initialize(journey:, contentful_entry:) diff --git a/app/views/journeys/_single_date_answer.html.erb b/app/views/journeys/_single_date_answer.html.erb new file mode 100644 index 000000000..f8ccefc7d --- /dev/null +++ b/app/views/journeys/_single_date_answer.html.erb @@ -0,0 +1,4 @@ +
+
<%= step.title %>
+
<%= I18n.l(answer.response) %>
+
diff --git a/app/views/steps/new.single_date.html.erb b/app/views/steps/new.single_date.html.erb new file mode 100644 index 000000000..bcee8f975 --- /dev/null +++ b/app/views/steps/new.single_date.html.erb @@ -0,0 +1,9 @@ +<%= content_for :title, @step.title %> + +<%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> + <%= f.govuk_date_field :response, + legend: { text: @step.title, size: "xl" }, + hint: { text: @step.help_text } + %> + <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> +<% end %> diff --git a/config/environments/test.rb b/config/environments/test.rb index 7979f199c..e5e40667f 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -62,5 +62,6 @@ Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :radio_answer Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :short_text_answer Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :long_text_answer + Bullet.add_whitelist type: :unused_eager_loading, class_name: "Step", association: :single_date_answer end end diff --git a/config/locales/en.yml b/config/locales/en.yml index f6fa9bfcf..3980afa29 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1,6 +1,9 @@ en: app: name: Buy for your school + date: + formats: + default: "%-d %b %Y" generic: button: start: "Continue" diff --git a/spec/factories/step.rb b/spec/factories/step.rb index 40b9cfd3c..2b912e197 100644 --- a/spec/factories/step.rb +++ b/spec/factories/step.rb @@ -27,6 +27,13 @@ association :long_text_answer end + trait :single_date do + options { nil } + contentful_model { "question" } + contentful_type { "single_date" } + association :single_date_answer + end + trait :static_content do options { nil } contentful_model { "staticContent" } diff --git a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb index 5765e6463..bac92331e 100644 --- a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb +++ b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb @@ -121,6 +121,34 @@ end end end + + context "when Contentful entry is of type single_date" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "55G5kpCLLL3h5yBQLiVlYy" + ) do + example.run + end + end + + scenario "user can answer using a date input" do + stub_get_contentful_entry( + entry_id: "55G5kpCLLL3h5yBQLiVlYy", + fixture_filename: "single-date-example.json" + ) + + visit root_path + click_on(I18n.t("generic.button.start")) + + fill_in "answer[response(3i)]", with: "12" + fill_in "answer[response(2i)]", with: "8" + fill_in "answer[response(1i)]", with: "2020" + + click_on(I18n.t("generic.button.next")) + + expect(page).to have_content("12 Aug 2020") + end + end end context "when the Contentful model is of type staticContent" do diff --git a/spec/fixtures/contentful/single-date-example.json b/spec/fixtures/contentful/single-date-example.json new file mode 100644 index 000000000..752133c8f --- /dev/null +++ b/spec/fixtures/contentful/single-date-example.json @@ -0,0 +1,36 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "55G5kpCLLL3h5yBQLiVlYy", + "type": "Entry", + "createdAt": "2020-12-08T10:43:19.102Z", + "updatedAt": "2020-12-08T10:43:19.102Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 1, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "question" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/starting-date", + "type": "single date", + "title": "When will this start?" + } +} diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index e9ea01999..1ff4b0c3b 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -5,6 +5,7 @@ it { should have_one(:radio_answer) } it { should have_one(:short_text_answer) } it { should have_one(:long_text_answer) } + it { should have_one(:single_date_answer) } it "store the basic fields of a contentful response" do step = build(:step, @@ -50,6 +51,14 @@ expect(step.answer).to eq(long_text_answer) end end + + context "when a SingleDateAnswer is associated to the step" do + it "returns the SingleDateAnswer object" do + single_date_answer = create(:single_date_answer) + step = create(:step, :single_date, single_date_answer: single_date_answer) + expect(step.answer).to eq(single_date_answer) + end + end end describe "#question?" do From 5c98ef61d81c54da7a12bf4e25168a14fc4e411e Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 8 Dec 2020 14:19:36 +0000 Subject: [PATCH 21/56] Use a Step Presenter for extra view logic Instead of grouping methods for different ways to view a Step resource inside the model, break this out into a presenter object. Using this pattern will enable the model to remain concerned with the data management layer and also multiple presenters for HTML, XML etc to be introduced. --- app/controllers/journeys_controller.rb | 1 + app/models/step.rb | 4 ---- app/presenters/step_presenter.rb | 5 +++++ app/views/journeys/show.html.erb | 2 +- spec/models/step_spec.rb | 16 ---------------- spec/presenters/step_presenter_spec.rb | 21 +++++++++++++++++++++ 6 files changed, 28 insertions(+), 21 deletions(-) create mode 100644 app/presenters/step_presenter.rb create mode 100644 spec/presenters/step_presenter_spec.rb diff --git a/app/controllers/journeys_controller.rb b/app/controllers/journeys_controller.rb index 5e5f845a6..3b8815076 100644 --- a/app/controllers/journeys_controller.rb +++ b/app/controllers/journeys_controller.rb @@ -10,6 +10,7 @@ def show @journey = Journey.includes( steps: [:radio_answer, :short_text_answer, :long_text_answer] ).find(journey_id) + @steps = @journey.steps.map { |step| StepPresenter.new(step) } end private diff --git a/app/models/step.rb b/app/models/step.rb index ce62729be..e9bff9b0d 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -19,10 +19,6 @@ def answer single_date_answer end - def question? - contentful_model == "question" - end - def primary_call_to_action_text return I18n.t("generic.button.next") unless super.present? super diff --git a/app/presenters/step_presenter.rb b/app/presenters/step_presenter.rb new file mode 100644 index 000000000..e018c14d8 --- /dev/null +++ b/app/presenters/step_presenter.rb @@ -0,0 +1,5 @@ +class StepPresenter < SimpleDelegator + def question? + contentful_model == "question" + end +end diff --git a/app/views/journeys/show.html.erb b/app/views/journeys/show.html.erb index d98c848ca..ceb4f5d5f 100644 --- a/app/views/journeys/show.html.erb +++ b/app/views/journeys/show.html.erb @@ -2,7 +2,7 @@

<%= @journey.category.capitalize %>

- <% @journey.steps.each do |step| %> + <% @steps.each do |step| %> <% if step.question? %> <%= render "#{step.contentful_type}_answer", step: step, answer: step.answer %> <% end %> diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index 1ff4b0c3b..f22e0280b 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -60,20 +60,4 @@ end end end - - describe "#question?" do - context "when the contentful model is 'question'" do - it "returns true" do - step = build(:step, :radio, contentful_model: "question") - expect(step.question?).to eq(true) - end - end - - context "when the contentful model is NOT 'question'" do - it "returns false" do - step = build(:step, contentful_model: "staticContent") - expect(step.question?).to eq(false) - end - end - end end diff --git a/spec/presenters/step_presenter_spec.rb b/spec/presenters/step_presenter_spec.rb new file mode 100644 index 000000000..1fc44e9e2 --- /dev/null +++ b/spec/presenters/step_presenter_spec.rb @@ -0,0 +1,21 @@ +require "rails_helper" + +RSpec.describe StepPresenter do + describe "#question?" do + context "when the contentful model is 'question'" do + it "returns true" do + step = build(:step, :radio, contentful_model: "question") + presenter = described_class.new(step) + expect(presenter.question?).to eq(true) + end + end + + context "when the contentful model is NOT 'question'" do + it "returns false" do + step = build(:step, contentful_model: "staticContent") + presenter = described_class.new(step) + expect(presenter.question?).to eq(false) + end + end + end +end From 8f0567b027b9ae0e84c65b400a69e19fa1f53612 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 14 Dec 2020 11:03:42 +0000 Subject: [PATCH 22/56] Bump bullet from 6.1.0 to 6.1.2 Bumps [bullet](https://github.com/flyerhzm/bullet) from 6.1.0 to 6.1.2. - [Release notes](https://github.com/flyerhzm/bullet/releases) - [Changelog](https://github.com/flyerhzm/bullet/blob/master/CHANGELOG.md) - [Commits](https://github.com/flyerhzm/bullet/compare/6.1.0...6.1.2) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 03b50a5e8..848ffd783 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -68,7 +68,7 @@ GEM msgpack (~> 1.0) brakeman (4.10.0) builder (3.2.4) - bullet (6.1.0) + bullet (6.1.2) activesupport (>= 3.0.0) uniform_notifier (~> 1.11) byebug (11.1.3) From 0c7f963cf21b29c1fb3ae5434d6d19199ac0ab72 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 14 Dec 2020 14:19:10 +0000 Subject: [PATCH 23/56] Bump puma from 5.1.0 to 5.1.1 Bumps [puma](https://github.com/puma/puma) from 5.1.0 to 5.1.1. - [Release notes](https://github.com/puma/puma/releases) - [Changelog](https://github.com/puma/puma/blob/master/History.md) - [Commits](https://github.com/puma/puma/compare/v5.1.0...v5.1.1) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 848ffd783..46bf50588 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -182,7 +182,7 @@ GEM coderay (~> 1.1) method_source (~> 1.0) public_suffix (4.0.6) - puma (5.1.0) + puma (5.1.1) nio4r (~> 2.0) raabro (1.4.0) rack (2.2.3) From 6eb7c4f7579ada473fa5741a4190dda1709db4a0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 14 Dec 2020 14:29:23 +0000 Subject: [PATCH 24/56] Bump rails from 6.0.3.4 to 6.1.0 Bumps [rails](https://github.com/rails/rails) from 6.0.3.4 to 6.1.0. - [Release notes](https://github.com/rails/rails/releases) - [Commits](https://github.com/rails/rails/compare/v6.0.3.4...v6.1.0) Signed-off-by: dependabot[bot] --- Gemfile | 2 +- Gemfile.lock | 125 ++++++++++++++++++++++++++------------------------- 2 files changed, 65 insertions(+), 62 deletions(-) diff --git a/Gemfile b/Gemfile index 627654ebc..1a90f9669 100644 --- a/Gemfile +++ b/Gemfile @@ -18,7 +18,7 @@ gem "puma", "~> 5.1" gem "redis", "~> 4.2" gem "redis-namespace" gem "rollbar" -gem "rails", "~> 6.0.0" +gem "rails", "~> 6.1.0" gem "sass-rails", "~> 6.0" gem "sidekiq", "~> 6.1" gem "sidekiq-cron", "~> 1.2" diff --git a/Gemfile.lock b/Gemfile.lock index 46bf50588..171498f3a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,61 +1,65 @@ GEM remote: https://rubygems.org/ specs: - actioncable (6.0.3.4) - actionpack (= 6.0.3.4) + actioncable (6.1.0) + actionpack (= 6.1.0) + activesupport (= 6.1.0) nio4r (~> 2.0) websocket-driver (>= 0.6.1) - actionmailbox (6.0.3.4) - actionpack (= 6.0.3.4) - activejob (= 6.0.3.4) - activerecord (= 6.0.3.4) - activestorage (= 6.0.3.4) - activesupport (= 6.0.3.4) + actionmailbox (6.1.0) + actionpack (= 6.1.0) + activejob (= 6.1.0) + activerecord (= 6.1.0) + activestorage (= 6.1.0) + activesupport (= 6.1.0) mail (>= 2.7.1) - actionmailer (6.0.3.4) - actionpack (= 6.0.3.4) - actionview (= 6.0.3.4) - activejob (= 6.0.3.4) + actionmailer (6.1.0) + actionpack (= 6.1.0) + actionview (= 6.1.0) + activejob (= 6.1.0) + activesupport (= 6.1.0) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 2.0) - actionpack (6.0.3.4) - actionview (= 6.0.3.4) - activesupport (= 6.0.3.4) - rack (~> 2.0, >= 2.0.8) + actionpack (6.1.0) + actionview (= 6.1.0) + activesupport (= 6.1.0) + rack (~> 2.0, >= 2.0.9) rack-test (>= 0.6.3) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.2.0) - actiontext (6.0.3.4) - actionpack (= 6.0.3.4) - activerecord (= 6.0.3.4) - activestorage (= 6.0.3.4) - activesupport (= 6.0.3.4) + actiontext (6.1.0) + actionpack (= 6.1.0) + activerecord (= 6.1.0) + activestorage (= 6.1.0) + activesupport (= 6.1.0) nokogiri (>= 1.8.5) - actionview (6.0.3.4) - activesupport (= 6.0.3.4) + actionview (6.1.0) + activesupport (= 6.1.0) builder (~> 3.1) erubi (~> 1.4) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.1, >= 1.2.0) - activejob (6.0.3.4) - activesupport (= 6.0.3.4) + activejob (6.1.0) + activesupport (= 6.1.0) globalid (>= 0.3.6) - activemodel (6.0.3.4) - activesupport (= 6.0.3.4) - activerecord (6.0.3.4) - activemodel (= 6.0.3.4) - activesupport (= 6.0.3.4) - activestorage (6.0.3.4) - actionpack (= 6.0.3.4) - activejob (= 6.0.3.4) - activerecord (= 6.0.3.4) + activemodel (6.1.0) + activesupport (= 6.1.0) + activerecord (6.1.0) + activemodel (= 6.1.0) + activesupport (= 6.1.0) + activestorage (6.1.0) + actionpack (= 6.1.0) + activejob (= 6.1.0) + activerecord (= 6.1.0) + activesupport (= 6.1.0) marcel (~> 0.3.1) - activesupport (6.0.3.4) + mimemagic (~> 0.3.2) + activesupport (6.1.0) concurrent-ruby (~> 1.0, >= 1.0.2) - i18n (>= 0.7, < 2) - minitest (~> 5.1) - tzinfo (~> 1.1) - zeitwerk (~> 2.2, >= 2.2.2) + i18n (>= 1.6, < 2) + minitest (>= 5.1) + tzinfo (~> 2.0) + zeitwerk (~> 2.3) addressable (2.7.0) public_suffix (>= 2.0.2, < 5.0) ast (2.4.1) @@ -188,20 +192,20 @@ GEM rack (2.2.3) rack-test (1.1.0) rack (>= 1.0, < 3) - rails (6.0.3.4) - actioncable (= 6.0.3.4) - actionmailbox (= 6.0.3.4) - actionmailer (= 6.0.3.4) - actionpack (= 6.0.3.4) - actiontext (= 6.0.3.4) - actionview (= 6.0.3.4) - activejob (= 6.0.3.4) - activemodel (= 6.0.3.4) - activerecord (= 6.0.3.4) - activestorage (= 6.0.3.4) - activesupport (= 6.0.3.4) - bundler (>= 1.3.0) - railties (= 6.0.3.4) + rails (6.1.0) + actioncable (= 6.1.0) + actionmailbox (= 6.1.0) + actionmailer (= 6.1.0) + actionpack (= 6.1.0) + actiontext (= 6.1.0) + actionview (= 6.1.0) + activejob (= 6.1.0) + activemodel (= 6.1.0) + activerecord (= 6.1.0) + activestorage (= 6.1.0) + activesupport (= 6.1.0) + bundler (>= 1.15.0) + railties (= 6.1.0) sprockets-rails (>= 2.0.0) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) @@ -209,12 +213,12 @@ GEM rails-html-sanitizer (1.3.0) loofah (~> 2.3) rails_layout (1.0.42) - railties (6.0.3.4) - actionpack (= 6.0.3.4) - activesupport (= 6.0.3.4) + railties (6.1.0) + actionpack (= 6.1.0) + activesupport (= 6.1.0) method_source rake (>= 0.8.7) - thor (>= 0.20.3, < 2.0) + thor (~> 1.0) rainbow (3.0.0) rake (13.0.1) rb-fsevent (0.10.4) @@ -304,13 +308,12 @@ GEM rubocop (= 1.4.2) rubocop-performance (= 1.9.1) thor (1.0.1) - thread_safe (0.3.6) tilt (2.0.10) turbolinks (5.2.1) turbolinks-source (~> 5.2) turbolinks-source (5.2.0) - tzinfo (1.2.8) - thread_safe (~> 0.1) + tzinfo (2.0.3) + concurrent-ruby (~> 1.0) uglifier (4.2.0) execjs (>= 0.3.0, < 3) unf (0.1.4) @@ -358,7 +361,7 @@ DEPENDENCIES pg pry puma (~> 5.1) - rails (~> 6.0.0) + rails (~> 6.1.0) rails_layout redis (~> 4.2) redis-namespace From 365ed67f0c8b893f35556badc8a6d7d49b52b2a4 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 14 Dec 2020 14:45:42 +0000 Subject: [PATCH 25/56] Fix test to work with Rails 6.1 The syntax for this has changed as is documented here: https://github.com/rails/rails/pull/38749 --- spec/jobs/warm_entry_cache_job_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/jobs/warm_entry_cache_job_spec.rb b/spec/jobs/warm_entry_cache_job_spec.rb index 844e28912..d88cbdaa2 100644 --- a/spec/jobs/warm_entry_cache_job_spec.rb +++ b/spec/jobs/warm_entry_cache_job_spec.rb @@ -29,9 +29,8 @@ allow(GetAllContentfulEntries).to receive(:new).and_return(get_all_contentful_entries_double) allow(get_all_contentful_entries_double).to receive(:call).and_return(fake_contentful_entry_array) - perform_enqueued_jobs do - described_class.perform_later - end + described_class.perform_later + perform_enqueued_jobs expect(RedisCache.redis.get("contentful:entry:5kZ9hIFDvNCEhjWs72SFwj")) .to eql( From 4043f02ab5cfa442040b3b66bd3a50232a4c621d Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 14 Dec 2020 12:11:21 +0000 Subject: [PATCH 26/56] Test suite detects external HTTP requests Using webmock will give the team more explicit feedback on any failures due to a real request being made. Instead of seeing a real Contentful 404 we should see a Webmock error that explains that a real HTTP request was detected and that we should look to stub it. --- CHANGELOG.md | 1 + Gemfile | 1 + Gemfile.lock | 7 +++++++ spec/spec_helper.rb | 3 +++ 4 files changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8c1ac3ec..a8bb02bad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog 1.0.0]. - nightly task to warm the Contentful cache for all entries - form button content is configurable through Contentful - users can be asked to provide a single date answer +- add Webmock to prevent real http requests in the test suite ## [release-003] - 2020-12-07 diff --git a/Gemfile b/Gemfile index 1a90f9669..f8ffc488b 100644 --- a/Gemfile +++ b/Gemfile @@ -39,6 +39,7 @@ group :test do gem "selenium-webdriver" gem "shoulda-matchers" gem "simplecov" + gem "webmock" end group :development do diff --git a/Gemfile.lock b/Gemfile.lock index 171498f3a..3811a422a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -99,6 +99,7 @@ GEM contentful (2.15.4) http (> 0.8, < 5.0) multi_json (~> 1) + crack (0.4.4) crass (1.0.6) database_cleaner (1.8.5) diff-lcs (1.4.4) @@ -133,6 +134,7 @@ GEM actionview (>= 5.2) activemodel (>= 5.2) activesupport (>= 5.2) + hashdiff (1.0.1) high_voltage (3.1.2) http (4.4.1) addressable (~> 2.3) @@ -326,6 +328,10 @@ GEM activemodel (>= 6.0.0) bindex (>= 0.4.0) railties (>= 6.0.0) + webmock (3.10.0) + addressable (>= 2.3.6) + crack (>= 0.3.2) + hashdiff (>= 0.4.0, < 2.0.0) websocket-driver (0.7.3) websocket-extensions (>= 0.1.0) websocket-extensions (0.1.5) @@ -381,6 +387,7 @@ DEPENDENCIES tzinfo-data uglifier (>= 1.3.0) web-console (>= 3.3.0) + webmock RUBY VERSION ruby 2.6.6p146 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 2fe7150d9..b5c1e182a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -20,6 +20,9 @@ SimpleCov.minimum_coverage 99 SimpleCov.start "rails" +require "webmock/rspec" +WebMock.disable_net_connect!(allow_localhost: true) + RSpec.configure do |config| # rspec-expectations config goes here. You can use an alternate # assertion/expectation library such as wrong or the stdlib/minitest From edb8434734e437dbcc6a7f4d7f8c60e36111eb8d Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 11:19:43 +0000 Subject: [PATCH 27/56] Refactor step form views to be more DRY Now we have a few different steps the duplication between them has become clear. This change will make it easier to add new partials and enable easier editing journeys with a reusable form. --- app/views/steps/_form_wrapper.html.erb | 5 +++++ app/views/steps/new.long_text.html.erb | 5 +---- app/views/steps/new.radios.html.erb | 5 +---- app/views/steps/new.short_text.html.erb | 5 +---- app/views/steps/new.single_date.html.erb | 5 +---- 5 files changed, 9 insertions(+), 16 deletions(-) create mode 100644 app/views/steps/_form_wrapper.html.erb diff --git a/app/views/steps/_form_wrapper.html.erb b/app/views/steps/_form_wrapper.html.erb new file mode 100644 index 000000000..f72359486 --- /dev/null +++ b/app/views/steps/_form_wrapper.html.erb @@ -0,0 +1,5 @@ +<%= content_for :title, @step.title %> +<%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> + <%= yield f %> + <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> +<% end %> diff --git a/app/views/steps/new.long_text.html.erb b/app/views/steps/new.long_text.html.erb index 3a1f65b24..9158af50a 100644 --- a/app/views/steps/new.long_text.html.erb +++ b/app/views/steps/new.long_text.html.erb @@ -1,11 +1,8 @@ -<%= content_for :title, @step.title %> - -<%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> +<%= render layout: "steps/form_wrapper" do |f| %> <%= f.govuk_text_area :response, label: { text: @step.title, size: 'xl' }, hint: { text: @step.help_text }, width: "one-third", rows: 9 %> - <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> diff --git a/app/views/steps/new.radios.html.erb b/app/views/steps/new.radios.html.erb index d853fa087..55545f874 100644 --- a/app/views/steps/new.radios.html.erb +++ b/app/views/steps/new.radios.html.erb @@ -1,6 +1,4 @@ -<%= content_for :title, @step.title %> - -<%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> +<%= render layout: "steps/form_wrapper" do |f| %> <%= f.hidden_field :response, value: nil %> <%= f.govuk_collection_radio_buttons :response, @step.radio_options, @@ -11,5 +9,4 @@ hint: { text: @step.help_text }, inline: false %> - <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> diff --git a/app/views/steps/new.short_text.html.erb b/app/views/steps/new.short_text.html.erb index 2b9e9902e..930033520 100644 --- a/app/views/steps/new.short_text.html.erb +++ b/app/views/steps/new.short_text.html.erb @@ -1,10 +1,7 @@ -<%= content_for :title, @step.title %> - -<%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> +<%= render layout: "steps/form_wrapper" do |f| %> <%= f.govuk_text_field :response, label: { text: @step.title, size: 'xl' }, hint: { text: @step.help_text }, width: "one-third" %> - <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> diff --git a/app/views/steps/new.single_date.html.erb b/app/views/steps/new.single_date.html.erb index bcee8f975..6f24a7b0a 100644 --- a/app/views/steps/new.single_date.html.erb +++ b/app/views/steps/new.single_date.html.erb @@ -1,9 +1,6 @@ -<%= content_for :title, @step.title %> - -<%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> +<%= render layout: "steps/form_wrapper" do |f| %> <%= f.govuk_date_field :response, legend: { text: @step.title, size: "xl" }, hint: { text: @step.help_text } %> - <%= f.submit @step.primary_call_to_action_text, class: "govuk-button" %> <% end %> From d38bc14ad1ec6880c4929d943e4fe1c8c60aef1d Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 11:27:56 +0000 Subject: [PATCH 28/56] Refactor form steps so they can be used in a new and edit context This should prevent the need for duplicate `new.step` and `edit.step` views. We can pass a layout local in the edit action to modify the form. --- app/controllers/answers_controller.rb | 2 +- app/controllers/steps_controller.rb | 2 +- .../{_form_wrapper.html.erb => _new_form_wrapper.html.erb} | 0 app/views/steps/{new.long_text.html.erb => long_text.html.erb} | 2 +- .../steps/{new.paragraphs.html.erb => paragraphs.html.erb} | 0 app/views/steps/{new.radios.html.erb => radios.html.erb} | 2 +- .../steps/{new.short_text.html.erb => short_text.html.erb} | 2 +- .../steps/{new.single_date.html.erb => single_date.html.erb} | 2 +- 8 files changed, 6 insertions(+), 6 deletions(-) rename app/views/steps/{_form_wrapper.html.erb => _new_form_wrapper.html.erb} (100%) rename app/views/steps/{new.long_text.html.erb => long_text.html.erb} (77%) rename app/views/steps/{new.paragraphs.html.erb => paragraphs.html.erb} (100%) rename app/views/steps/{new.radios.html.erb => radios.html.erb} (85%) rename app/views/steps/{new.short_text.html.erb => short_text.html.erb} (75%) rename app/views/steps/{new.single_date.html.erb => single_date.html.erb} (72%) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index c4019c885..51337aa0c 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -17,7 +17,7 @@ def create redirect_to journey_path(@journey) end else - render "steps/new.#{@step.contentful_type}" + render "steps/#{@step.contentful_type}", locals: {layout: "steps/new_form_wrapper"} end end diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index 20cee9e3e..eec2b6538 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -31,7 +31,7 @@ def show @step = Step.find(params[:id]) @answer = AnswerFactory.new(step: @step).call - render "new.#{@step.contentful_type}" + render @step.contentful_type, locals: {layout: "steps/new_form_wrapper"} end private diff --git a/app/views/steps/_form_wrapper.html.erb b/app/views/steps/_new_form_wrapper.html.erb similarity index 100% rename from app/views/steps/_form_wrapper.html.erb rename to app/views/steps/_new_form_wrapper.html.erb diff --git a/app/views/steps/new.long_text.html.erb b/app/views/steps/long_text.html.erb similarity index 77% rename from app/views/steps/new.long_text.html.erb rename to app/views/steps/long_text.html.erb index 9158af50a..5933f8d45 100644 --- a/app/views/steps/new.long_text.html.erb +++ b/app/views/steps/long_text.html.erb @@ -1,4 +1,4 @@ -<%= render layout: "steps/form_wrapper" do |f| %> +<%= render layout: layout do |f| %> <%= f.govuk_text_area :response, label: { text: @step.title, size: 'xl' }, hint: { text: @step.help_text }, diff --git a/app/views/steps/new.paragraphs.html.erb b/app/views/steps/paragraphs.html.erb similarity index 100% rename from app/views/steps/new.paragraphs.html.erb rename to app/views/steps/paragraphs.html.erb diff --git a/app/views/steps/new.radios.html.erb b/app/views/steps/radios.html.erb similarity index 85% rename from app/views/steps/new.radios.html.erb rename to app/views/steps/radios.html.erb index 55545f874..bd535cbb4 100644 --- a/app/views/steps/new.radios.html.erb +++ b/app/views/steps/radios.html.erb @@ -1,4 +1,4 @@ -<%= render layout: "steps/form_wrapper" do |f| %> +<%= render layout: layout do |f| %> <%= f.hidden_field :response, value: nil %> <%= f.govuk_collection_radio_buttons :response, @step.radio_options, diff --git a/app/views/steps/new.short_text.html.erb b/app/views/steps/short_text.html.erb similarity index 75% rename from app/views/steps/new.short_text.html.erb rename to app/views/steps/short_text.html.erb index 930033520..3d414cc28 100644 --- a/app/views/steps/new.short_text.html.erb +++ b/app/views/steps/short_text.html.erb @@ -1,4 +1,4 @@ -<%= render layout: "steps/form_wrapper" do |f| %> +<%= render layout: layout do |f| %> <%= f.govuk_text_field :response, label: { text: @step.title, size: 'xl' }, hint: { text: @step.help_text }, diff --git a/app/views/steps/new.single_date.html.erb b/app/views/steps/single_date.html.erb similarity index 72% rename from app/views/steps/new.single_date.html.erb rename to app/views/steps/single_date.html.erb index 6f24a7b0a..0cc149ca1 100644 --- a/app/views/steps/new.single_date.html.erb +++ b/app/views/steps/single_date.html.erb @@ -1,4 +1,4 @@ -<%= render layout: "steps/form_wrapper" do |f| %> +<%= render layout: layout do |f| %> <%= f.govuk_date_field :response, legend: { text: @step.title, size: "xl" }, hint: { text: @step.help_text } From 92260f483271c9b5b124819083416de0585debd3 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 15:17:49 +0000 Subject: [PATCH 29/56] Correct test fixture total count When creating this fixture we originally reduced the number of items from 5 to 2 (since 2 is enough to prove the scenario) but forgot the count. This probably has no direct affect at the moment but serves to make the fixture more accurate. --- spec/fixtures/contentful/multiple-entries-example.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/fixtures/contentful/multiple-entries-example.json b/spec/fixtures/contentful/multiple-entries-example.json index 6570017e1..066ca3279 100644 --- a/spec/fixtures/contentful/multiple-entries-example.json +++ b/spec/fixtures/contentful/multiple-entries-example.json @@ -2,7 +2,7 @@ "sys": { "type": "Array" }, - "total": 5, + "total": 2, "skip": 0, "limit": 100, "items": [ From c6064eca6f214f56f5bf00d43ff4240c9496ed10 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 16:51:10 +0000 Subject: [PATCH 30/56] Remove unused testing method --- spec/support/contentful_helpers.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index 531078b96..57c110493 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -30,12 +30,6 @@ def stub_contentful_client contentful_client end - def stub_contentful_step(fake_entry: fake_contentful_step_entry) - get_contentful_step_double = instance_double(GetContentfulEntry) - allow(GetContentfulEntry).to receive(:new).and_return(get_contentful_step_double) - allow(get_contentful_step_double).to receive(:call).and_return(fake_entry) - end - def fake_contentful_step_entry(contentful_fixture_filename:) raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{contentful_fixture_filename}") hash_response = JSON.parse(raw_response) From 7f6339dfa71b2018046b987880a36748fec9da08 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 16:53:55 +0000 Subject: [PATCH 31/56] Rename test helper for clarity Step and Entry are names for a very similar thing. Step is the internal represenation of what a Contentful Entry is. I'd like to create a new method called `fake_contentful_entries` which is the reason for this change. I think it would make sense with this existing method renamed to be the singular version. --- spec/services/create_journey_step_spec.rb | 24 +++++++++++------------ spec/support/contentful_helpers.rb | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/services/create_journey_step_spec.rb b/spec/services/create_journey_step_spec.rb index c6b450e3f..be607fae6 100644 --- a/spec/services/create_journey_step_spec.rb +++ b/spec/services/create_journey_step_spec.rb @@ -5,7 +5,7 @@ context "when the new step is of type step" do it "creates a local copy of the new step" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "radio-question-example.json" ) @@ -21,7 +21,7 @@ it "updates the journey with a new next_entry_id" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "has-next-question-example.json" ) @@ -34,7 +34,7 @@ context "when the question is of type 'short_text'" do it "sets help_text and options to nil" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "short-text-question-example.json" ) @@ -45,7 +45,7 @@ it "replaces spaces with underscores" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "short-text-question-example.json" ) @@ -58,7 +58,7 @@ context "when the new step does not have a following step" do it "updates the journey by setting the next_entry_id to nil" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "radio-question-example.json" ) @@ -71,7 +71,7 @@ context "when the new entry has a body field" do it "updates the step with the body" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "static-content-example.json" ) @@ -89,7 +89,7 @@ context "when the new entry has a 'primaryCallToAction' field" do it "updates the step with the body" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "primary-button-example.json" ) @@ -104,7 +104,7 @@ context "when no 'primaryCallToAction' is provided" do it "default copy is used for the button" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "no-primary-button-example.json" ) @@ -119,7 +119,7 @@ context "when the new entry has an unexpected content model" do it "raises an error" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "an-unexpected-model-example.json" ) @@ -130,7 +130,7 @@ it "raises a rollbar event" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "an-unexpected-model-example.json" ) @@ -153,7 +153,7 @@ context "when the new step has an unexpected step type" do it "raises an error" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "an-unexpected-question-type-example.json" ) @@ -164,7 +164,7 @@ it "raises a rollbar event" do journey = create(:journey, :catering) - fake_entry = fake_contentful_step_entry( + fake_entry = fake_contentful_entry( contentful_fixture_filename: "an-unexpected-question-type-example.json" ) diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index 57c110493..29af5b6c0 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -6,7 +6,7 @@ def stub_get_contentful_entry( raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{fixture_filename}") contentful_connector = stub_contentful_connector - contentful_response = fake_contentful_step_entry(contentful_fixture_filename: fixture_filename) + contentful_response = fake_contentful_entry(contentful_fixture_filename: fixture_filename) allow(contentful_connector).to receive(:get_entry_by_id) .with(entry_id) .and_return(contentful_response) @@ -30,7 +30,7 @@ def stub_contentful_client contentful_client end - def fake_contentful_step_entry(contentful_fixture_filename:) + def fake_contentful_entry(contentful_fixture_filename:) raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{contentful_fixture_filename}") hash_response = JSON.parse(raw_response) From ef5d9343d40e7348ae648b4e12071e454417d1da Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 17:06:23 +0000 Subject: [PATCH 32/56] Refactor existing mocking of the call for all Entries We'd like to shortly reuse this test set up in another test. Before we do, let's move the existing version into the Contentful Helpers. --- spec/jobs/warm_entry_cache_job_spec.rb | 11 ++++------- spec/support/contentful_helpers.rb | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/spec/jobs/warm_entry_cache_job_spec.rb b/spec/jobs/warm_entry_cache_job_spec.rb index d88cbdaa2..66093513e 100644 --- a/spec/jobs/warm_entry_cache_job_spec.rb +++ b/spec/jobs/warm_entry_cache_job_spec.rb @@ -21,13 +21,10 @@ end it "asks GetAllContentfulEntries for the Contentful entries" do - raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/multiple-entries-example.json") - response_hash = JSON.parse(raw_response) - fake_contentful_entry_array = Contentful::ResourceBuilder.new(response_hash).run - - get_all_contentful_entries_double = instance_double(GetAllContentfulEntries) - allow(GetAllContentfulEntries).to receive(:new).and_return(get_all_contentful_entries_double) - allow(get_all_contentful_entries_double).to receive(:call).and_return(fake_contentful_entry_array) + stub_get_contentful_entries( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "multiple-entries-example.json" + ) described_class.perform_later perform_enqueued_jobs diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index 29af5b6c0..e9dfc378c 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -15,6 +15,21 @@ def stub_get_contentful_entry( .and_return(raw_response) end + def stub_get_contentful_entries( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "multiple-entries-example.json" + ) + raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{fixture_filename}") + + contentful_connector = stub_contentful_connector + contentful_response = fake_contentful_entry_array(contentful_fixture_filename: fixture_filename) + allow(contentful_connector).to receive(:get_all_entries) + .and_return(contentful_response) + + allow(contentful_response).to receive(:raw) + .and_return(raw_response) + end + def stub_contentful_connector contentful_connector = instance_double(ContentfulConnector) expect(ContentfulConnector).to receive(:new) @@ -48,4 +63,11 @@ def fake_contentful_entry(contentful_fixture_filename:) content_type: double(id: hash_response.dig("sys", "contentType", "sys", "id")) ) end + + def fake_contentful_entry_array(contentful_fixture_filename:) + raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{contentful_fixture_filename}") + response_hash = JSON.parse(raw_response) + + Contentful::ResourceBuilder.new(response_hash).run + end end From 891dfe886af79631f64fa0dadf523187ee8b62d7 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 16:28:32 +0000 Subject: [PATCH 33/56] Hidden endpoint for generating a basic journey map This currently produces a list of the starting `entry_id` followed by each subsequent `entry_id` it references using the `next` element. The plan will be to iterate this list with more features like using human readable language and linking to that entry wthin contentful. --- CHANGELOG.md | 1 + app/controllers/journey_maps_controller.rb | 27 ++++++ app/views/journey_maps/new.html.erb | 7 ++ config/routes.rb | 1 + ..._see_the_map_of_journey_steps_page_spec.rb | 24 +++++ .../closed-path-with-multiple-example.json | 95 +++++++++++++++++++ 6 files changed, 155 insertions(+) create mode 100644 app/controllers/journey_maps_controller.rb create mode 100644 app/views/journey_maps/new.html.erb create mode 100644 spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb create mode 100644 spec/fixtures/contentful/closed-path-with-multiple-example.json diff --git a/CHANGELOG.md b/CHANGELOG.md index a8bb02bad..f3d563294 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog 1.0.0]. - form button content is configurable through Contentful - users can be asked to provide a single date answer - add Webmock to prevent real http requests in the test suite +- content users can see a journey map of all the Contentful steps ## [release-003] - 2020-12-07 diff --git a/app/controllers/journey_maps_controller.rb b/app/controllers/journey_maps_controller.rb new file mode 100644 index 000000000..1c1ea337f --- /dev/null +++ b/app/controllers/journey_maps_controller.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class JourneyMapsController < ApplicationController + def new + hash_of_entries = GetAllContentfulEntries.new.call.to_h { |entry| [entry.id, entry] } + @journey_map = recursive_path( + all_entries: hash_of_entries, + next_entry_id: ENV["CONTENTFUL_PLANNING_START_ENTRY_ID"], + entry_id_sequence: [] + ) + end + + def recursive_path(all_entries:, next_entry_id:, entry_id_sequence:) + entry = all_entries.fetch(next_entry_id, nil) + entry_id_sequence << entry.id if entry.present? + + if entry.respond_to?(:next) + recursive_path( + all_entries: all_entries, + next_entry_id: entry.next.id, + entry_id_sequence: entry_id_sequence + ) + else + entry_id_sequence + end + end +end diff --git a/app/views/journey_maps/new.html.erb b/app/views/journey_maps/new.html.erb new file mode 100644 index 000000000..8f8f341f0 --- /dev/null +++ b/app/views/journey_maps/new.html.erb @@ -0,0 +1,7 @@ +
    + <% @journey_map.each do |entry_id| %> +
  1. + <%= entry_id %> +
    2. + <% end %> +
diff --git a/config/routes.rb b/config/routes.rb index 6e3bf2aa6..d64cb836a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -4,6 +4,7 @@ get "health_check" => "application#health_check" root to: "high_voltage/pages#show", id: "planning_start_page" + resource :journey_map, only: [:new] resources :journeys, only: [:new, :show] do resources :steps, only: [:new, :show] do resources :answers, only: [:create] diff --git a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb new file mode 100644 index 000000000..1a0a6ad44 --- /dev/null +++ b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb @@ -0,0 +1,24 @@ +feature "Users can see all the steps of a journey" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + scenario "Multiple journey steps" do + stub_get_contentful_entries( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "closed-path-with-multiple-example.json" + ) + + visit new_journey_map_path + + within(".govuk-list") do + list_items = find_all("li") + expect(list_items.first.text).to have_content("5kZ9hIFDvNCEhjWs72SFwj") + expect(list_items.last.text).to have_content("hfjJgWRg4xiiiImwVRDtZ") + end + end +end diff --git a/spec/fixtures/contentful/closed-path-with-multiple-example.json b/spec/fixtures/contentful/closed-path-with-multiple-example.json new file mode 100644 index 000000000..23b0b4e99 --- /dev/null +++ b/spec/fixtures/contentful/closed-path-with-multiple-example.json @@ -0,0 +1,95 @@ +{ + "sys": { + "type": "Array" + }, + "total": 2, + "skip": 0, + "limit": 100, + "items": [ + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "5kZ9hIFDvNCEhjWs72SFwj", + "type": "Entry", + "createdAt": "2020-12-02T10:48:35.748Z", + "updatedAt": "2020-12-02T10:48:35.748Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 1, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "staticContent" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/timelines", + "title": "When you should start", + "body": "Procuring a new catering contract can take up to 6 months to consult, create, review and award. \n\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.", + "type": "paragraphs", + "next": { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hfjJgWRg4xiiiImwVRDtZ" + } + } + } + }, + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "hfjJgWRg4xiiiImwVRDtZ", + "type": "Entry", + "createdAt": "2020-11-04T12:28:30.442Z", + "updatedAt": "2020-11-26T16:39:54.188Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 6, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "question" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/dev-start-which-service", + "title": "Which service do you need?", + "helpText": "Tell us which service you need.", + "type": "radios", + "options": [ + "Catering", + "Cleaning" + ] + } + } + ] +} From 13e2eb14f7f3f7bf03bdf156832607f580edd474 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 17:47:59 +0000 Subject: [PATCH 34/56] Refactor Entry path building into a service This leads to a smaller controller and a place where we can further extend with more unit tests for more scenarios. --- app/controllers/journey_maps_controller.rb | 26 +++------------ app/services/build_journey_order.rb | 37 ++++++++++++++++++++++ app/views/journey_maps/new.html.erb | 4 +-- spec/services/build_journey_order_spec.rb | 28 ++++++++++++++++ 4 files changed, 72 insertions(+), 23 deletions(-) create mode 100644 app/services/build_journey_order.rb create mode 100644 spec/services/build_journey_order_spec.rb diff --git a/app/controllers/journey_maps_controller.rb b/app/controllers/journey_maps_controller.rb index 1c1ea337f..ef0decb22 100644 --- a/app/controllers/journey_maps_controller.rb +++ b/app/controllers/journey_maps_controller.rb @@ -2,26 +2,10 @@ class JourneyMapsController < ApplicationController def new - hash_of_entries = GetAllContentfulEntries.new.call.to_h { |entry| [entry.id, entry] } - @journey_map = recursive_path( - all_entries: hash_of_entries, - next_entry_id: ENV["CONTENTFUL_PLANNING_START_ENTRY_ID"], - entry_id_sequence: [] - ) - end - - def recursive_path(all_entries:, next_entry_id:, entry_id_sequence:) - entry = all_entries.fetch(next_entry_id, nil) - entry_id_sequence << entry.id if entry.present? - - if entry.respond_to?(:next) - recursive_path( - all_entries: all_entries, - next_entry_id: entry.next.id, - entry_id_sequence: entry_id_sequence - ) - else - entry_id_sequence - end + entries = GetAllContentfulEntries.new.call + @journey_map = BuildJourneyOrder.new( + entries: entries.to_a, + starting_entry_id: ENV["CONTENTFUL_PLANNING_START_ENTRY_ID"] + ).call end end diff --git a/app/services/build_journey_order.rb b/app/services/build_journey_order.rb new file mode 100644 index 000000000..0b7d675f1 --- /dev/null +++ b/app/services/build_journey_order.rb @@ -0,0 +1,37 @@ +class BuildJourneyOrder + attr_accessor :entries, :starting_entry_id + + def initialize(entries:, starting_entry_id:) + self.entries = entries + self.starting_entry_id = starting_entry_id + end + + def call + recursive_path( + entry_lookup: entry_lookup_hash, + next_entry_id: starting_entry_id, + entries: [] + ) + end + + private + + def entry_lookup_hash + @entry_lookup_hash ||= entries.to_h { |entry| [entry.id, entry] } + end + + def recursive_path(entry_lookup:, next_entry_id:, entries:) + entry = entry_lookup.fetch(next_entry_id, nil) + entries << entry if entry.present? + + if entry.respond_to?(:next) + recursive_path( + entry_lookup: entry_lookup, + next_entry_id: entry.next.id, + entries: entries + ) + else + entries + end + end +end diff --git a/app/views/journey_maps/new.html.erb b/app/views/journey_maps/new.html.erb index 8f8f341f0..e2e979aa7 100644 --- a/app/views/journey_maps/new.html.erb +++ b/app/views/journey_maps/new.html.erb @@ -1,7 +1,7 @@
    - <% @journey_map.each do |entry_id| %> + <% @journey_map.each do |entry| %>
  1. - <%= entry_id %> + <%= entry.id %>
    2. <% end %>
diff --git a/spec/services/build_journey_order_spec.rb b/spec/services/build_journey_order_spec.rb new file mode 100644 index 000000000..d1d888ba6 --- /dev/null +++ b/spec/services/build_journey_order_spec.rb @@ -0,0 +1,28 @@ +require "rails_helper" + +RSpec.describe BuildJourneyOrder do + describe "#call" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + context "when there are multiple entries" do + it "returns each ID in the order they appear" do + fake_entries = fake_contentful_entry_array( + contentful_fixture_filename: "closed-path-with-multiple-example.json" + ) + + result = described_class.new( + entries: fake_entries, + starting_entry_id: "5kZ9hIFDvNCEhjWs72SFwj" + ).call + + expect(result).to match([fake_entries.first, fake_entries.last]) + end + end + end +end From 7964605231c39a3f008ba26359083b64fa916207 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 10 Dec 2020 17:48:03 +0000 Subject: [PATCH 35/56] Journey map links each step to Contentful for editing Building upon the listing of entry ids, we now use the title to make them human readable and link them to contentful. The user will be able to use this to reference the exact entry coming from Contentful and make changes. The environment variables "CONTENTFUL_SPACE" and "CONTENTFUL_ENVIRONMENT" are not secret values that a member of the public could use to access anything. There is perhaps a small bit of security through obscurity to be gained by hiding these but ultimately we are trusting Contentful's authentication mechanism. In future, this page will also be behind an authentication mechanism of our own. We use target blank for the link as I suspect the user will be jumping between our service and Contentful to compare and review a number of entries. --- app/views/journey_maps/new.html.erb | 5 ++++- config/locales/en.yml | 2 ++ ...nyone_can_see_the_map_of_journey_steps_page_spec.rb | 10 ++++++++-- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/views/journey_maps/new.html.erb b/app/views/journey_maps/new.html.erb index e2e979aa7..02268a6b5 100644 --- a/app/views/journey_maps/new.html.erb +++ b/app/views/journey_maps/new.html.erb @@ -1,7 +1,10 @@ +<%= content_for :title, I18n.t("journey_map.page_title") %> +

<%= I18n.t("journey_map.page_title") %>

+
    <% @journey_map.each do |entry| %>
  1. - <%= entry.id %> + <%= link_to entry.title, "https://app.contentful.com/spaces/#{ENV["CONTENTFUL_SPACE"]}/environments/#{ENV["CONTENTFUL_ENVIRONMENT"]}/entries/#{entry.id}", target: :blank, class: "govuk-link" %>
    2. <% end %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 3980afa29..26f0641b4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -34,6 +34,8 @@ en: before_you_start_body: "It will take around 3 minutes to complete the process" errors: contentful_entry_not_found: "An unexpected error occurred. The starting step has been revoked by the content team." + journey_map: + page_title: "Contentful entry map" errors: contentful_entry_not_found: page_title: "An unexpected error occurred" diff --git a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb index 1a0a6ad44..29dbe4a3d 100644 --- a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb +++ b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb @@ -15,10 +15,16 @@ visit new_journey_map_path + expect(page).to have_content(I18n.t("journey_map.page_title")) + within(".govuk-list") do list_items = find_all("li") - expect(list_items.first.text).to have_content("5kZ9hIFDvNCEhjWs72SFwj") - expect(list_items.last.text).to have_content("hfjJgWRg4xiiiImwVRDtZ") + within(list_items.first) do + expect(page).to have_link("When you should start", href: "https://app.contentful.com/spaces/#{ENV["CONTENTFUL_SPACE"]}/environments/#{ENV["CONTENTFUL_ENVIRONMENT"]}/entries/5kZ9hIFDvNCEhjWs72SFwj") + end + within(list_items.last) do + expect(page).to have_link("Which service do you need?", href: "https://app.contentful.com/spaces/#{ENV["CONTENTFUL_SPACE"]}/environments/#{ENV["CONTENTFUL_ENVIRONMENT"]}/entries/hfjJgWRg4xiiiImwVRDtZ") + end end end end From 96dcc13d33cfd490e004db1a964eac9345f5d842 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 16 Dec 2020 15:02:35 +0000 Subject: [PATCH 36/56] Rails 6.1 migrations updated the schema comment This was being changed automatically and will continue to do so unless one of us commits it. --- db/schema.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 6b648588a..567971db2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2,8 +2,8 @@ # of editing this file, please use the migrations feature of Active Record to # incrementally modify your database, and then regenerate this schema definition. # -# This file is the source Rails uses to define your schema when running `rails -# db:schema:load`. When creating a new database, `rails db:schema:load` tends to +# This file is the source Rails uses to define your schema when running `bin/rails +# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to # be faster and is potentially less error prone than running all of your # migrations from scratch. Old migrations may fail to apply correctly if those # migrations use external dependencies or application code. From a074dcb1e7404b59a472c1ae2fe5b07dc2849bf1 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 9 Dec 2020 15:56:17 +0000 Subject: [PATCH 37/56] Refactor radio_options method into helper I am about to add a new method for checkbox options and noticed that this view only logic should probably live in a view helper rather than the data layer. --- app/helpers/step_helper.rb | 9 +++++++++ app/models/step.rb | 4 ---- app/views/steps/radios.html.erb | 2 +- spec/helpers/step_helper_spec.rb | 14 ++++++++++++++ 4 files changed, 24 insertions(+), 5 deletions(-) create mode 100644 app/helpers/step_helper.rb create mode 100644 spec/helpers/step_helper_spec.rb diff --git a/app/helpers/step_helper.rb b/app/helpers/step_helper.rb new file mode 100644 index 000000000..c2c2796da --- /dev/null +++ b/app/helpers/step_helper.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module StepHelper + def radio_options(array_of_options:) + array_of_options.map { |option| + OpenStruct.new(id: option.downcase, name: option.titleize) + } + end +end diff --git a/app/models/step.rb b/app/models/step.rb index e9bff9b0d..8b6a2b53c 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -7,10 +7,6 @@ class Step < ApplicationRecord has_one :long_text_answer has_one :single_date_answer - def radio_options - options.map { |option| OpenStruct.new(id: option.downcase, name: option.titleize) } - end - def answer @answer ||= radio_answer || diff --git a/app/views/steps/radios.html.erb b/app/views/steps/radios.html.erb index bd535cbb4..d0dcfb0ce 100644 --- a/app/views/steps/radios.html.erb +++ b/app/views/steps/radios.html.erb @@ -1,7 +1,7 @@ <%= render layout: layout do |f| %> <%= f.hidden_field :response, value: nil %> <%= f.govuk_collection_radio_buttons :response, - @step.radio_options, + radio_options(array_of_options: @step.options), :id, ->(option) { option.name }, :description, diff --git a/spec/helpers/step_helper_spec.rb b/spec/helpers/step_helper_spec.rb new file mode 100644 index 000000000..46a08cc25 --- /dev/null +++ b/spec/helpers/step_helper_spec.rb @@ -0,0 +1,14 @@ +require "rails_helper" + +RSpec.describe StepHelper, type: :helper do + describe "#radio_options" do + it "returns an array of OpenStruct objects" do + options = ["green", "yellow"] + result = helper.radio_options(array_of_options: options) + expect(result).to match([ + OpenStruct.new(id: "green", name: "Green"), + OpenStruct.new(id: "yellow", name: "Yellow") + ]) + end + end +end From 9d86ee37cdf585f52227bade3904018584cebe98 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 9 Dec 2020 16:06:15 +0000 Subject: [PATCH 38/56] Introduce CheckboxAnswers This model will collect a response that's an array of strings. One for each option selected and an empty string for one not selected. If a question Step has options of ["Foo", "Bar"] then the associated `CheckboxAnswers` response could be ["Foo", ""] where "" was unchecked. This array is what's passed along from the form so it seemed the most compatible route to then store this data (rather than converting it to a hash for an hstore) so that when it's used for an edit form, the field is going to be able to pre check the already selected fields. That's the hope but without an edit form it's not simple to test. I'm using "Answers" in the plural because by the very nature of the checkbox type, multiple options are presented and multiple options can be chosen. --- app/models/checkbox_answers.rb | 6 ++++++ app/models/step.rb | 1 + .../20201209122555_create_checkbox_answers.rb | 9 +++++++++ db/schema.rb | 10 +++++++++- spec/factories/answer.rb | 6 ++++++ spec/models/checkbox_answers_spec.rb | 16 ++++++++++++++++ spec/models/step_spec.rb | 1 + 7 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 app/models/checkbox_answers.rb create mode 100644 db/migrate/20201209122555_create_checkbox_answers.rb create mode 100644 spec/models/checkbox_answers_spec.rb diff --git a/app/models/checkbox_answers.rb b/app/models/checkbox_answers.rb new file mode 100644 index 000000000..aa5a55dfa --- /dev/null +++ b/app/models/checkbox_answers.rb @@ -0,0 +1,6 @@ +class CheckboxAnswers < ActiveRecord::Base + self.implicit_order_column = "created_at" + belongs_to :step + + validates :response, presence: true +end diff --git a/app/models/step.rb b/app/models/step.rb index 8b6a2b53c..5f18d57aa 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -6,6 +6,7 @@ class Step < ApplicationRecord has_one :short_text_answer has_one :long_text_answer has_one :single_date_answer + has_one :checkbox_answers def answer @answer ||= diff --git a/db/migrate/20201209122555_create_checkbox_answers.rb b/db/migrate/20201209122555_create_checkbox_answers.rb new file mode 100644 index 000000000..5424846e6 --- /dev/null +++ b/db/migrate/20201209122555_create_checkbox_answers.rb @@ -0,0 +1,9 @@ +class CreateCheckboxAnswers < ActiveRecord::Migration[6.0] + def change + create_table :checkbox_answers, id: :uuid do |t| + t.references :step, type: :uuid + t.string :response, array: true, default: [] + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 567971db2..6a3483107 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,13 +10,21 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_12_08_110342) do +ActiveRecord::Schema.define(version: 2020_12_09_122555) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" enable_extension "uuid-ossp" + create_table "checkbox_answers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "step_id" + t.string "response", default: [], array: true + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.index ["step_id"], name: "index_checkbox_answers_on_step_id" + end + create_table "journeys", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "category", null: false t.datetime "created_at", precision: 6, null: false diff --git a/spec/factories/answer.rb b/spec/factories/answer.rb index 0c4902ddd..532a44969 100644 --- a/spec/factories/answer.rb +++ b/spec/factories/answer.rb @@ -22,4 +22,10 @@ response { 1.year.from_now } end + + factory :checkbox_answers do + association :step, factory: :step, contentful_type: "checkboxes" + + response { ["breakfast", "lunch", ""] } + end end diff --git a/spec/models/checkbox_answers_spec.rb b/spec/models/checkbox_answers_spec.rb new file mode 100644 index 000000000..7994354fc --- /dev/null +++ b/spec/models/checkbox_answers_spec.rb @@ -0,0 +1,16 @@ +require "rails_helper" + +RSpec.describe CheckboxAnswers, type: :model do + it { should belong_to(:step) } + + describe "#response" do + it "returns an array of strings" do + answer = build(:checkbox_answers, response: ["Blue", "Green"]) + expect(answer.response).to eq(["Blue", "Green"]) + end + end + + describe "validations" do + it { is_expected.to validate_presence_of(:response) } + end +end diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index f22e0280b..79da40413 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -6,6 +6,7 @@ it { should have_one(:short_text_answer) } it { should have_one(:long_text_answer) } it { should have_one(:single_date_answer) } + it { should have_one(:checkbox_answers) } it "store the basic fields of a contentful response" do step = build(:step, From fc81f3d067a78a07940c9a01321c88ad1d019313 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 9 Dec 2020 16:13:27 +0000 Subject: [PATCH 39/56] Helper method to construct objects for a checkbox form element This array is currently identical to the previous `radio_options` method. I could have renamed that method into `options` to make it reusable for 2 different types of form element. I suspect we may wish to keep the flexiblity to change how we construct these so we can change their behaviour independently. --- app/helpers/step_helper.rb | 6 ++++++ spec/helpers/step_helper_spec.rb | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/app/helpers/step_helper.rb b/app/helpers/step_helper.rb index c2c2796da..5b7ced134 100644 --- a/app/helpers/step_helper.rb +++ b/app/helpers/step_helper.rb @@ -6,4 +6,10 @@ def radio_options(array_of_options:) OpenStruct.new(id: option.downcase, name: option.titleize) } end + + def checkbox_options(array_of_options:) + array_of_options.map { |option| + OpenStruct.new(id: option.downcase, name: option.titleize) + } + end end diff --git a/spec/helpers/step_helper_spec.rb b/spec/helpers/step_helper_spec.rb index 46a08cc25..aa5172f53 100644 --- a/spec/helpers/step_helper_spec.rb +++ b/spec/helpers/step_helper_spec.rb @@ -11,4 +11,15 @@ ]) end end + + describe "#checkbox_options" do + it "returns an array of OpenStruct objects" do + options = ["red", "blue"] + result = helper.checkbox_options(array_of_options: options) + expect(result).to match([ + OpenStruct.new(id: "red", name: "Red"), + OpenStruct.new(id: "blue", name: "Blue") + ]) + end + end end From 5f494347a818afbc72ec906b8e93c84fdc8e2e1b Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 9 Dec 2020 16:19:58 +0000 Subject: [PATCH 40/56] Users can be asked a checkbox question * Multiple answers can be stored on a CheckboxAnswers resource. * We construct a comma separated string when rendering this value back in the show page - this should probably be a presenter but adding a presenter for every Answer seemed overkill. * Assigning an array to an @answer object in the controller required a little extra work since `answer_params` would not return a value not of type string. --- CHANGELOG.md | 1 + app/controllers/answers_controller.rb | 12 +++++- app/models/step.rb | 3 +- app/services/answer_factory.rb | 1 + app/services/create_journey_step.rb | 9 +++- .../journeys/_checkboxes_answer.html.erb | 4 ++ app/views/steps/checkboxes.html.erb | 8 ++++ spec/factories/step.rb | 7 ++++ .../anyone_can_complete_a_journey_spec.rb | 27 ++++++++++++ .../fixtures/contentful/checkbox-example.json | 41 +++++++++++++++++++ spec/models/step_spec.rb | 8 ++++ 11 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 app/views/journeys/_checkboxes_answer.html.erb create mode 100644 app/views/steps/checkboxes.html.erb create mode 100644 spec/fixtures/contentful/checkbox-example.json diff --git a/CHANGELOG.md b/CHANGELOG.md index f3d563294..78e225d19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog 1.0.0]. - users can be asked to provide a single date answer - add Webmock to prevent real http requests in the test suite - content users can see a journey map of all the Contentful steps +- users can be asked to provide multiple answers via a checkbox question ## [release-003] - 2020-12-07 diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 51337aa0c..65b54700e 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -6,9 +6,15 @@ def create @step = Step.find(step_id) @answer = AnswerFactory.new(step: @step).call - @answer.assign_attributes(answer_params) @answer.step = @step + case @step.contentful_type + when "checkboxes" + @answer.assign_attributes(checkbox_params) + else + @answer.assign_attributes(answer_params) + end + if @answer.valid? @answer.save if @journey.next_entry_id.present? @@ -34,4 +40,8 @@ def step_id def answer_params params.require(:answer).permit(:response) end + + def checkbox_params + params.require(:answer).permit(response: []) + end end diff --git a/app/models/step.rb b/app/models/step.rb index 5f18d57aa..dc464aeb3 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -13,7 +13,8 @@ def answer radio_answer || short_text_answer || long_text_answer || - single_date_answer + single_date_answer || + checkbox_answers end def primary_call_to_action_text diff --git a/app/services/answer_factory.rb b/app/services/answer_factory.rb index a35599cd1..b42b9392e 100644 --- a/app/services/answer_factory.rb +++ b/app/services/answer_factory.rb @@ -11,6 +11,7 @@ def call when "short_text" then ShortTextAnswer.new when "long_text" then LongTextAnswer.new when "single_date" then SingleDateAnswer.new + when "checkboxes" then CheckboxAnswers.new end end end diff --git a/app/services/create_journey_step.rb b/app/services/create_journey_step.rb index bbf8331af..d544bbf88 100644 --- a/app/services/create_journey_step.rb +++ b/app/services/create_journey_step.rb @@ -4,7 +4,14 @@ class UnexpectedContentfulModel < StandardError; end class UnexpectedContentfulStepType < StandardError; end ALLOWED_CONTENTFUL_MODELS = %w[question staticContent].freeze - ALLOWED_CONTENTFUL_ENTRY_TYPES = %w[radios short_text long_text paragraphs single_date].freeze + ALLOWED_CONTENTFUL_ENTRY_TYPES = %w[ + radios + short_text + long_text + paragraphs + single_date + checkboxes + ].freeze attr_accessor :journey, :contentful_entry def initialize(journey:, contentful_entry:) diff --git a/app/views/journeys/_checkboxes_answer.html.erb b/app/views/journeys/_checkboxes_answer.html.erb new file mode 100644 index 000000000..18cd08e5a --- /dev/null +++ b/app/views/journeys/_checkboxes_answer.html.erb @@ -0,0 +1,4 @@ +
+
<%= step.title %>
+
<%= answer.response.reject(&:blank?).map(&:capitalize).join(", ") %>
+
diff --git a/app/views/steps/checkboxes.html.erb b/app/views/steps/checkboxes.html.erb new file mode 100644 index 000000000..63d43274f --- /dev/null +++ b/app/views/steps/checkboxes.html.erb @@ -0,0 +1,8 @@ +<%= render layout: layout do |f| %> + <%= f.govuk_collection_check_boxes :response, + checkbox_options(array_of_options: @step.options), + :id, + :name, + legend: { text: @step.title, size: "xl" } + %> +<% end %> diff --git a/spec/factories/step.rb b/spec/factories/step.rb index 2b912e197..c3597da27 100644 --- a/spec/factories/step.rb +++ b/spec/factories/step.rb @@ -34,6 +34,13 @@ association :single_date_answer end + trait :checkbox_answers do + options { ["Brown", "Gold"] } + contentful_model { "question" } + contentful_type { "checkboxes" } + association :checkbox_answers + end + trait :static_content do options { nil } contentful_model { "staticContent" } diff --git a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb index bac92331e..dea093f24 100644 --- a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb +++ b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb @@ -149,6 +149,33 @@ expect(page).to have_content("12 Aug 2020") end end + + context "when Contentful entry is of type checkboxes" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "1DqhwF2XkJJ0Um6NSweWlZ" + ) do + example.run + end + end + + scenario "user can select multiple answers" do + stub_get_contentful_entry( + entry_id: "1DqhwF2XkJJ0Um6NSweWlZ", + fixture_filename: "checkbox-example.json" + ) + + visit root_path + click_on(I18n.t("generic.button.start")) + + check "Breakfast" + check "Lunch" + + click_on(I18n.t("generic.button.next")) + + expect(page).to have_content("Breakfast, Lunch") + end + end end context "when the Contentful model is of type staticContent" do diff --git a/spec/fixtures/contentful/checkbox-example.json b/spec/fixtures/contentful/checkbox-example.json new file mode 100644 index 000000000..d606d24f7 --- /dev/null +++ b/spec/fixtures/contentful/checkbox-example.json @@ -0,0 +1,41 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "1DqhwF2XkJJ0Um6NSweWlZ", + "type": "Entry", + "createdAt": "2020-12-09T11:53:52.744Z", + "updatedAt": "2020-12-09T11:53:52.744Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 1, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "question" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/everyday-services", + "type": "checkboxes", + "title": "Everyday services that are required and need to be considered", + "options": [ + "breakfast", + "lunch", + "dinner" + ] + } +} diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index 79da40413..ae6acfc35 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -60,5 +60,13 @@ expect(step.answer).to eq(single_date_answer) end end + + context "when a CheckboxAnswers is associated to the step" do + it "returns the CheckboxAnswers object" do + checkbox_answers = create(:checkbox_answers) + step = create(:step, :checkbox_answers, checkbox_answers: checkbox_answers) + expect(step.answer).to eq(checkbox_answers) + end + end end end From 2035c4cef425069e9388de3b61bbbd0fe5cf2bfe Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 9 Dec 2020 16:59:31 +0000 Subject: [PATCH 41/56] Validate that at least one Checkbox item is checked If no checkbox is selected then `[""]` is passed to the controller. This is then stored on the model as such and passes the presence validation. In order to fire the validation when no item is chosen, we only store the chosen options so that if an empty array is validated against it behaves as expected. --- app/models/checkbox_answers.rb | 7 +++++++ spec/models/checkbox_answers_spec.rb | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/app/models/checkbox_answers.rb b/app/models/checkbox_answers.rb index aa5a55dfa..c93e5b78d 100644 --- a/app/models/checkbox_answers.rb +++ b/app/models/checkbox_answers.rb @@ -3,4 +3,11 @@ class CheckboxAnswers < ActiveRecord::Base belongs_to :step validates :response, presence: true + + def response=(args) + return if args.blank? + args.reject!(&:blank?) if args.is_a?(Array) + + super(args) + end end diff --git a/spec/models/checkbox_answers_spec.rb b/spec/models/checkbox_answers_spec.rb index 7994354fc..e2bf38f8e 100644 --- a/spec/models/checkbox_answers_spec.rb +++ b/spec/models/checkbox_answers_spec.rb @@ -13,4 +13,20 @@ describe "validations" do it { is_expected.to validate_presence_of(:response) } end + + describe "#response=" do + context "when the response only includes blank items" do + it "should be invalid" do + answer = build(:checkbox_answers, response: ["", ""]) + expect(answer.valid?).to eq(false) + end + end + + context "when the response a mix of present and blank items" do + it "should only store the present values" do + answer = build(:checkbox_answers, response: ["Foo", ""]) + expect(answer.response).to eq(["Foo"]) + end + end + end end From 545f0b0616c47a8e3dbcb3854087e3049e0757a7 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 15 Dec 2020 14:53:56 +0000 Subject: [PATCH 42/56] Some feature tests no longer depend on correct ENV If CONTENTFUL_PLANNING_START_ENTRY_ID is set locally as a different value, this test starts to break. We should refactor the way we are setting up all these feature tests by mocking the ENV and handover the majority by stubbing a journey with the desired `journey.next_entry_id`. --- .../visitors/anyone_can_complete_a_journey_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb index dea093f24..c27a39bd1 100644 --- a/spec/features/visitors/anyone_can_complete_a_journey_spec.rb +++ b/spec/features/visitors/anyone_can_complete_a_journey_spec.rb @@ -1,6 +1,14 @@ require "rails_helper" feature "Anyone can start a journey" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "1UjQurSOi5MWkcRuGxdXZS" + ) do + example.run + end + end + scenario "Start page includes a call to action" do stub_get_contentful_entry From a84f25bc5a061ed5d48bd109a9f6601ff80320d7 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 15 Dec 2020 15:58:14 +0000 Subject: [PATCH 43/56] Raise an error if we try to map the same entry twice When we try to generate a map, abort and return an error if we detect that we've gone over the same entry twice. We don't imagine when a user will ever need to answer the same question twice. This is our basic check for infinite loops. --- CHANGELOG.md | 1 + app/controllers/journey_maps_controller.rb | 4 + app/services/build_journey_order.rb | 17 ++ ...at_step_in_the_contentful_journey.html.erb | 6 + config/locales/en.yml | 3 + ..._see_the_map_of_journey_steps_page_spec.rb | 26 ++++ .../contentful/repeat-entry-example.json | 146 ++++++++++++++++++ spec/services/build_journey_order_spec.rb | 31 ++++ 8 files changed, 234 insertions(+) create mode 100644 app/views/errors/repeat_step_in_the_contentful_journey.html.erb create mode 100644 spec/fixtures/contentful/repeat-entry-example.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 78e225d19..8f05486b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format is based on [Keep a Changelog 1.0.0]. - add Webmock to prevent real http requests in the test suite - content users can see a journey map of all the Contentful steps - users can be asked to provide multiple answers via a checkbox question +- journey map shows an error to the content team if a duplicate entry is detected ## [release-003] - 2020-12-07 diff --git a/app/controllers/journey_maps_controller.rb b/app/controllers/journey_maps_controller.rb index ef0decb22..1a285a62b 100644 --- a/app/controllers/journey_maps_controller.rb +++ b/app/controllers/journey_maps_controller.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true class JourneyMapsController < ApplicationController + rescue_from BuildJourneyOrder::RepeatEntryDetected do |exception| + render "errors/repeat_step_in_the_contentful_journey", status: 500, locals: {error: exception} + end + def new entries = GetAllContentfulEntries.new.call @journey_map = BuildJourneyOrder.new( diff --git a/app/services/build_journey_order.rb b/app/services/build_journey_order.rb index 0b7d675f1..63f5cc3a4 100644 --- a/app/services/build_journey_order.rb +++ b/app/services/build_journey_order.rb @@ -1,4 +1,5 @@ class BuildJourneyOrder + class RepeatEntryDetected < StandardError; end attr_accessor :entries, :starting_entry_id def initialize(entries:, starting_entry_id:) @@ -22,6 +23,12 @@ def entry_lookup_hash def recursive_path(entry_lookup:, next_entry_id:, entries:) entry = entry_lookup.fetch(next_entry_id, nil) + + if entries.include?(entry) + send_rollbar_error(message: "A repeated Contentful entry was found in the same journey", entry_id: entry.id) + raise RepeatEntryDetected.new(entry.id) + end + entries << entry if entry.present? if entry.respond_to?(:next) @@ -34,4 +41,14 @@ def recursive_path(entry_lookup:, next_entry_id:, entries:) entries end end + + def send_rollbar_error(message:, entry_id:) + Rollbar.error( + message, + contentful_url: ENV["CONTENTFUL_URL"], + contentful_space_id: ENV["CONTENTFUL_SPACE"], + contentful_environment: ENV["CONTENTFUL_ENVIRONMENT"], + contentful_entry_id: entry_id + ) + end end diff --git a/app/views/errors/repeat_step_in_the_contentful_journey.html.erb b/app/views/errors/repeat_step_in_the_contentful_journey.html.erb new file mode 100644 index 000000000..aba030975 --- /dev/null +++ b/app/views/errors/repeat_step_in_the_contentful_journey.html.erb @@ -0,0 +1,6 @@ +<%= content_for :title, I18n.t("errors.repeat_step_in_the_contentful_journey.page_title") %> + +

<%= I18n.t("errors.repeat_step_in_the_contentful_journey.page_title") %>

+

+ <%= I18n.t("errors.repeat_step_in_the_contentful_journey.page_body", entry_id: error.message) %> +

diff --git a/config/locales/en.yml b/config/locales/en.yml index 26f0641b4..beb428fe3 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -46,3 +46,6 @@ en: unexpected_contentful_step_type: page_title: "An unexpected error occurred" page_body: "The service has had a problem trying to retrieve the required step. The team have been notified of this problem and you should be able to retry shortly." + repeat_step_in_the_contentful_journey: + page_title: "An unexpected error occurred" + page_body: "One or more steps in the Contentful journey would leave the user in an infinite loop. This entry ID was presented more than once to the user: %{entry_id}" diff --git a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb index 29dbe4a3d..755cde863 100644 --- a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb +++ b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb @@ -27,4 +27,30 @@ end end end + + context "when the map isn't valid" do + context "when the same entry is found twice" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + it "returns an error message" do + stub_get_contentful_entries( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "repeat-entry-example.json" + ) + + visit new_journey_map_path + + expect(page).to have_content(I18n.t("errors.repeat_step_in_the_contentful_journey.page_title")) + expect(page).to have_content( + I18n.t("errors.repeat_step_in_the_contentful_journey.page_body", entry_id: "5kZ9hIFDvNCEhjWs72SFwj") + ) + end + end + end end diff --git a/spec/fixtures/contentful/repeat-entry-example.json b/spec/fixtures/contentful/repeat-entry-example.json new file mode 100644 index 000000000..72abdf553 --- /dev/null +++ b/spec/fixtures/contentful/repeat-entry-example.json @@ -0,0 +1,146 @@ +{ + "sys": { + "type": "Array" + }, + "total": 4, + "skip": 0, + "limit": 100, + "items": [ + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "5kZ9hIFDvNCEhjWs72SFwj", + "type": "Entry", + "createdAt": "2020-12-02T10:48:35.748Z", + "updatedAt": "2020-12-02T10:48:35.748Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 1, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "staticContent" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/timelines", + "title": "When you should start", + "body": "Procuring a new catering contract can take up to 6 months to consult, create, review and award. \n\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.", + "type": "paragraphs", + "next": { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hfjJgWRg4xiiiImwVRDtZ" + } + } + } + }, + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "hfjJgWRg4xiiiImwVRDtZ", + "type": "Entry", + "createdAt": "2020-11-04T12:28:30.442Z", + "updatedAt": "2020-11-26T16:39:54.188Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 6, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "question" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/dev-start-which-service", + "title": "Which service do you need?", + "helpText": "Tell us which service you need.", + "type": "radios", + "options": [ + "Catering", + "Cleaning" + ], + "next": { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "5kZ9hIFDvNCEhjWs72SFwj" + } + } + } + }, + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "5kZ9hIFDvNCEhjWs72SFwj", + "type": "Entry", + "createdAt": "2020-12-02T10:48:35.748Z", + "updatedAt": "2020-12-02T10:48:35.748Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 1, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "staticContent" + } + }, + "locale": "en-US" + }, + "fields": { + "slug": "/timelines", + "title": "When you should start", + "body": "Procuring a new catering contract can take up to 6 months to consult, create, review and award. \n\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.", + "type": "paragraphs", + "next": { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hfjJgWRg4xiiiImwVRDtZ" + } + } + } + } + ] +} diff --git a/spec/services/build_journey_order_spec.rb b/spec/services/build_journey_order_spec.rb index d1d888ba6..a6d9f1a3a 100644 --- a/spec/services/build_journey_order_spec.rb +++ b/spec/services/build_journey_order_spec.rb @@ -24,5 +24,36 @@ expect(result).to match([fake_entries.first, fake_entries.last]) end end + + context "when the journey visits the same node twice" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + it "raises a rollbar event" do + fake_entries = fake_contentful_entry_array( + contentful_fixture_filename: "repeat-entry-example.json" + ) + + expect(Rollbar).to receive(:error) + .with("A repeated Contentful entry was found in the same journey", + contentful_url: ENV["CONTENTFUL_URL"], + contentful_space_id: ENV["CONTENTFUL_SPACE"], + contentful_environment: ENV["CONTENTFUL_ENVIRONMENT"], + contentful_entry_id: "5kZ9hIFDvNCEhjWs72SFwj") + .and_call_original + + expect { + described_class.new( + entries: fake_entries, + starting_entry_id: "5kZ9hIFDvNCEhjWs72SFwj" + ).call + }.to raise_error(BuildJourneyOrder::RepeatEntryDetected) + end + end end end From f00043851733e77b8b936d0e450717db37b0e687 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 17 Dec 2020 10:38:55 +0000 Subject: [PATCH 44/56] Raise an error if the journey is longer than 50 steps The number 50 is arbitrary. We could have picked 100. This is used as a blunt instrument in order to detect where a journey does not have an end. A single journey where a user has to click through more than 30-40 steps seems enormous and should perhaps be broken down. The number 50 tries to round up to a number we should never realistically hit. Though if we do, it's easy to change. If there is no end within a map of 50 entries, presume it's incorrectly configured and therefore invalid. This is not the total count of entries coming back from Contentful, it's the total within a journey. Given the journey map is not put into the cache, there's no way we can render the last working journey. This may also confuse the user when they're told there was a problem whilst being able to see te last healthy map. We fail loudly for now. Creating a real fixture with 50 entries brought the test suite to a crawl. It took 2 minutes. For this reason I use a smaller fixture and stub the limit to lower the threshold for when the error is thrown. --- CHANGELOG.md | 1 + app/controllers/journey_maps_controller.rb | 4 +++ app/services/build_journey_order.rb | 10 ++++++ ...y_steps_in_the_contentful_journey.html.erb | 6 ++++ config/locales/en.yml | 3 ++ ..._see_the_map_of_journey_steps_page_spec.rb | 25 +++++++++++++ spec/services/build_journey_order_spec.rb | 35 +++++++++++++++++++ 7 files changed, 84 insertions(+) create mode 100644 app/views/errors/too_many_steps_in_the_contentful_journey.html.erb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f05486b4..09f8c7c58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog 1.0.0]. - content users can see a journey map of all the Contentful steps - users can be asked to provide multiple answers via a checkbox question - journey map shows an error to the content team if a duplicate entry is detected +- journey map shows an error to the content team if the journey doesn't end within 50 steps ## [release-003] - 2020-12-07 diff --git a/app/controllers/journey_maps_controller.rb b/app/controllers/journey_maps_controller.rb index 1a285a62b..eab7d3d64 100644 --- a/app/controllers/journey_maps_controller.rb +++ b/app/controllers/journey_maps_controller.rb @@ -5,6 +5,10 @@ class JourneyMapsController < ApplicationController render "errors/repeat_step_in_the_contentful_journey", status: 500, locals: {error: exception} end + rescue_from BuildJourneyOrder::TooManyChainedEntriesDetected do |exception| + render "errors/too_many_steps_in_the_contentful_journey", status: 500, locals: {error: exception} + end + def new entries = GetAllContentfulEntries.new.call @journey_map = BuildJourneyOrder.new( diff --git a/app/services/build_journey_order.rb b/app/services/build_journey_order.rb index 63f5cc3a4..b2de6141d 100644 --- a/app/services/build_journey_order.rb +++ b/app/services/build_journey_order.rb @@ -1,5 +1,10 @@ class BuildJourneyOrder class RepeatEntryDetected < StandardError; end + + class TooManyChainedEntriesDetected < StandardError; end + + ENTRY_JOURNEY_MAX_LENGTH = 50 + attr_accessor :entries, :starting_entry_id def initialize(entries:, starting_entry_id:) @@ -29,6 +34,11 @@ def recursive_path(entry_lookup:, next_entry_id:, entries:) raise RepeatEntryDetected.new(entry.id) end + if entries.count >= ENTRY_JOURNEY_MAX_LENGTH + send_rollbar_error(message: "More than #{ENTRY_JOURNEY_MAX_LENGTH} steps were found in a journey map", entry_id: entry.id) + raise TooManyChainedEntriesDetected.new(entry.id) + end + entries << entry if entry.present? if entry.respond_to?(:next) diff --git a/app/views/errors/too_many_steps_in_the_contentful_journey.html.erb b/app/views/errors/too_many_steps_in_the_contentful_journey.html.erb new file mode 100644 index 000000000..f63b298cf --- /dev/null +++ b/app/views/errors/too_many_steps_in_the_contentful_journey.html.erb @@ -0,0 +1,6 @@ +<%= content_for :title, I18n.t("errors.too_many_steps_in_the_contentful_journey.page_title") %> + +

<%= I18n.t("errors.too_many_steps_in_the_contentful_journey.page_title") %>

+

+ <%= I18n.t("errors.too_many_steps_in_the_contentful_journey.page_body", entry_id: error.message, step_count: BuildJourneyOrder::ENTRY_JOURNEY_MAX_LENGTH) %> +

diff --git a/config/locales/en.yml b/config/locales/en.yml index beb428fe3..71571dfd3 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -49,3 +49,6 @@ en: repeat_step_in_the_contentful_journey: page_title: "An unexpected error occurred" page_body: "One or more steps in the Contentful journey would leave the user in an infinite loop. This entry ID was presented more than once to the user: %{entry_id}" + too_many_steps_in_the_contentful_journey: + page_title: "An unexpected error occurred" + page_body: "More than %{step_count} steps were found in the Contentful journey. Is the journey missing an end? The last Entry ID was: %{entry_id}" diff --git a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb index 755cde863..44ea047b6 100644 --- a/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb +++ b/spec/features/visitors/anyone_can_see_the_map_of_journey_steps_page_spec.rb @@ -52,5 +52,30 @@ ) end end + + context "when the chain becomes obviously too long" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + it "returns an error message" do + stub_const("BuildJourneyOrder::ENTRY_JOURNEY_MAX_LENGTH", 1) + stub_get_contentful_entries( + entry_id: "hfjJgWRg4xiiiImwVRDtZ", + fixture_filename: "closed-path-with-multiple-example.json" + ) + + visit new_journey_map_path + + expect(page).to have_content(I18n.t("errors.too_many_steps_in_the_contentful_journey.page_title")) + expect(page).to have_content( + I18n.t("errors.too_many_steps_in_the_contentful_journey.page_body", entry_id: "hfjJgWRg4xiiiImwVRDtZ", step_count: 1) + ) + end + end end end diff --git a/spec/services/build_journey_order_spec.rb b/spec/services/build_journey_order_spec.rb index a6d9f1a3a..f4ce4ea2a 100644 --- a/spec/services/build_journey_order_spec.rb +++ b/spec/services/build_journey_order_spec.rb @@ -55,5 +55,40 @@ }.to raise_error(BuildJourneyOrder::RepeatEntryDetected) end end + + context "when the journey visits more than the maximum permitted number of entries" do + around do |example| + ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj" + ) do + example.run + end + end + + it "raises a rollbar event with the last entry" do + # Creating 50 linked entries in a fixture slows the test suite down + # We assume these 50 entries are chained together in a linear sequence. + fake_entries = fake_contentful_entry_array( + contentful_fixture_filename: "closed-path-with-multiple-example.json" + ) + expect(BuildJourneyOrder::ENTRY_JOURNEY_MAX_LENGTH).to eq(50) + stub_const("BuildJourneyOrder::ENTRY_JOURNEY_MAX_LENGTH", 1) + + expect(Rollbar).to receive(:error) + .with("More than #{BuildJourneyOrder::ENTRY_JOURNEY_MAX_LENGTH} steps were found in a journey map", + contentful_url: ENV["CONTENTFUL_URL"], + contentful_space_id: ENV["CONTENTFUL_SPACE"], + contentful_environment: ENV["CONTENTFUL_ENVIRONMENT"], + contentful_entry_id: "hfjJgWRg4xiiiImwVRDtZ") + .and_call_original + + expect { + described_class.new( + entries: fake_entries, + starting_entry_id: "5kZ9hIFDvNCEhjWs72SFwj" + ).call + }.to raise_error(BuildJourneyOrder::TooManyChainedEntriesDetected) + end + end end end From d73e37039a01d0bbac07d1a80592efa2c44db973 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 16 Dec 2020 15:57:17 +0000 Subject: [PATCH 45/56] Access a Step's raw response returns a hash Rather than a serialised string. We as a contentful object for its `raw` data. This returns a hash. When we save this against a `binary` field in the database it is serliased into a string. This isn't very useful when we want to use it. Convert the serialised data into a json serialisable string. Then ask JSON to parse it until we are back where we started with a hash. A better fix might be to look at changing the field type from `binary` to `jsonb` to see if we this serialising can be done for us. --- app/models/step.rb | 5 +++ spec/models/step_spec.rb | 2 +- spec/services/create_journey_step_spec.rb | 39 ++++++++++++++++++++++- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/app/models/step.rb b/app/models/step.rb index dc464aeb3..8e881be86 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -21,4 +21,9 @@ def primary_call_to_action_text return I18n.t("generic.button.next") unless super.present? super end + + # TODO: Change raw field type to jsonb + def raw + JSON.parse super.gsub("=>", ":") + end end diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index ae6acfc35..3d6977bcc 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -25,7 +25,7 @@ step = build(:step, raw: contentful_json_response) - expect(step.raw).to eql("{\"foo\":{}}") + expect(step.raw).to eql({"foo" => {}}) end describe "#answer" do diff --git a/spec/services/create_journey_step_spec.rb b/spec/services/create_journey_step_spec.rb index be607fae6..086b880c5 100644 --- a/spec/services/create_journey_step_spec.rb +++ b/spec/services/create_journey_step_spec.rb @@ -16,7 +16,44 @@ expect(step.contentful_model).to eq("question") expect(step.contentful_type).to eq("radios") expect(step.options).to eq(["Catering", "Cleaning"]) - expect(step.raw).to eq(fake_entry.raw) + expect(step.raw).to eq( + "fields" => { + "helpText" => "Tell us which service you need.", + "options" => ["Catering", "Cleaning"], + "slug" => "/which-service", + "title" => "Which service do you need?", + "type" => "radios" + }, + "sys" => { + "contentType" => { + "sys" => { + "id" => "question", + "linkType" => "ContentType", + "type" => "Link" + } + }, + "createdAt" => "2020-09-07T10:56:40.585Z", + "environment" => { + "sys" => { + "id" => "master", + "linkType" => "Environment", + "type" => "Link" + } + }, + "id" => "1UjQurSOi5MWkcRuGxdXZS", + "locale" => "en-US", + "revision" => 7, + "space" => { + "sys" => { + "id" => "jspwts36h1os", + "linkType" => "Space", + "type" => "Link" + } + }, + "type" => "Entry", + "updatedAt" => "2020-09-14T22:16:54.633Z" + } + ) end it "updates the journey with a new next_entry_id" do From 54462df526864885bfdab5ee95c5f3396112cb0a Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 16 Dec 2020 15:58:57 +0000 Subject: [PATCH 46/56] Every Step has its associated Contentful ID For existing Steps we tap into each Step's raw attribute to grab the original entry ID, and set it. As a foreign key that we are going to start depending on when generating a specification this link is important. We set database presence validation to ensure it exists for all records. --- CHANGELOG.md | 1 + app/services/create_journey_step.rb | 1 + ...20201216150806_add_contentful_id_to_step.rb | 18 ++++++++++++++++++ db/schema.rb | 3 ++- spec/factories/step.rb | 3 ++- spec/services/create_journey_step_spec.rb | 1 + 6 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20201216150806_add_contentful_id_to_step.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 09f8c7c58..f6248d76b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog 1.0.0]. - users can be asked to provide multiple answers via a checkbox question - journey map shows an error to the content team if a duplicate entry is detected - journey map shows an error to the content team if the journey doesn't end within 50 steps +- refactor how we store and access a Step's associated Contentful Entry ID ## [release-003] - 2020-12-07 diff --git a/app/services/create_journey_step.rb b/app/services/create_journey_step.rb index d544bbf88..e4da8b37a 100644 --- a/app/services/create_journey_step.rb +++ b/app/services/create_journey_step.rb @@ -34,6 +34,7 @@ def call title: title, help_text: help_text, body: body, + contentful_id: content_entry_id, contentful_model: content_model, contentful_type: step_type, options: options, diff --git a/db/migrate/20201216150806_add_contentful_id_to_step.rb b/db/migrate/20201216150806_add_contentful_id_to_step.rb new file mode 100644 index 000000000..964bff8e7 --- /dev/null +++ b/db/migrate/20201216150806_add_contentful_id_to_step.rb @@ -0,0 +1,18 @@ +class AddContentfulIdToStep < ActiveRecord::Migration[6.1] + def up + add_column :steps, :contentful_id, :string + + ActiveRecord::Base.transaction do + Step.all.map do |step| + contentful_id = step.raw.dig("sys", "id") + step.update(contentful_id: contentful_id) + end + end + + change_column :steps, :contentful_id, :string, null: false + end + + def down + remove_column :steps, :contentful_id, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 6a3483107..aee7f64e8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_12_09_122555) do +ActiveRecord::Schema.define(version: 2020_12_16_150806) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -76,6 +76,7 @@ t.text "body" t.string "contentful_model" t.string "primary_call_to_action_text" + t.string "contentful_id", null: false t.index ["journey_id"], name: "index_steps_on_journey_id" end diff --git a/spec/factories/step.rb b/spec/factories/step.rb index c3597da27..e1ed7a3b0 100644 --- a/spec/factories/step.rb +++ b/spec/factories/step.rb @@ -2,7 +2,8 @@ factory :step do title { "What is your favourite colour?" } help_text { "Choose the primary colour closest to your choice" } - raw { "{\"sys\":{}}" } + raw { "{\"sys\":{\"id\"=>\"123\"}}" } + contentful_id { "123" } association :journey, factory: :journey diff --git a/spec/services/create_journey_step_spec.rb b/spec/services/create_journey_step_spec.rb index 086b880c5..4d08d6028 100644 --- a/spec/services/create_journey_step_spec.rb +++ b/spec/services/create_journey_step_spec.rb @@ -13,6 +13,7 @@ expect(step.title).to eq("Which service do you need?") expect(step.help_text).to eq("Tell us which service you need.") + expect(step.contentful_id).to eq("1UjQurSOi5MWkcRuGxdXZS") expect(step.contentful_model).to eq("question") expect(step.contentful_type).to eq("radios") expect(step.options).to eq(["Catering", "Cleaning"]) From c3c5096bb14cf0c58bd5bc8323be5a69c8fa45a7 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 16 Dec 2020 17:06:48 +0000 Subject: [PATCH 47/56] Refactor how we set and access step.raw In reality sending the message `Contentful::Entry.raw` returns a `hash`. In the test suite we were trying to set a JSON string instead, which wasn't accurate. This change does 2 things: 1. Change the raw column type from `binary` to `jsonb` so when we ask `step.raw` we receive a hash rather than a serialised string. This allows us to easily work with and access information within it 2. Update the test suite to be more realistic, setting a hash rather than a json blob --- app/models/step.rb | 5 ---- ...6160028_change_step_raw_field_to_json_b.rb | 25 +++++++++++++++++++ db/schema.rb | 4 +-- spec/models/step_spec.rb | 7 ++---- spec/support/contentful_helpers.rb | 2 +- 5 files changed, 30 insertions(+), 13 deletions(-) create mode 100644 db/migrate/20201216160028_change_step_raw_field_to_json_b.rb diff --git a/app/models/step.rb b/app/models/step.rb index 8e881be86..dc464aeb3 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -21,9 +21,4 @@ def primary_call_to_action_text return I18n.t("generic.button.next") unless super.present? super end - - # TODO: Change raw field type to jsonb - def raw - JSON.parse super.gsub("=>", ":") - end end diff --git a/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb b/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb new file mode 100644 index 000000000..c5f3b9014 --- /dev/null +++ b/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb @@ -0,0 +1,25 @@ +class ChangeStepRawFieldToJsonB < ActiveRecord::Migration[6.1] + def up + add_column :steps, :raw_jsonb, :jsonb, null: false + ActiveRecord::Base.transaction do + Step.all.map do |step| + hash = JSON.parse(step.raw.gsub("=>", ":")) + step.update(raw_jsonb: hash) + end + end + remove_column :steps, :raw + rename_column :steps, :raw_jsonb, :raw + end + + def down + add_column :steps, :raw_binary, :binary, null: false + ActiveRecord::Base.transaction do + Step.all.map do |step| + string = step.raw.to_s + step.update(raw_binary: string) + end + end + remove_column :steps, :raw + rename_column :steps, :raw_binary, :raw + end +end diff --git a/db/schema.rb b/db/schema.rb index aee7f64e8..e24feb3ef 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_12_16_150806) do +ActiveRecord::Schema.define(version: 2020_12_16_160028) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -70,13 +70,13 @@ t.string "help_text" t.string "contentful_type", null: false t.string "options", array: true - t.binary "raw", null: false t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false t.text "body" t.string "contentful_model" t.string "primary_call_to_action_text" t.string "contentful_id", null: false + t.jsonb "raw", null: false t.index ["journey_id"], name: "index_steps_on_journey_id" end diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index 3d6977bcc..54c8a1956 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -21,11 +21,8 @@ end it "captures the raw contentful response" do - contentful_json_response = JSON("foo": {}) - step = build(:step, - raw: contentful_json_response) - - expect(step.raw).to eql({"foo" => {}}) + step = build(:step, raw: {foo: "bar"}) + expect(step.raw).to eql({"foo" => "bar"}) end describe "#answer" do diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index e9dfc378c..7d0389c6b 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -59,7 +59,7 @@ def fake_contentful_entry(contentful_fixture_filename:) type: hash_response.dig("fields", "type"), next: double(id: hash_response.dig("fields", "next", "sys", "id")), primary_call_to_action: hash_response.dig("fields", "primaryCallToAction"), - raw: raw_response, + raw: hash_response, content_type: double(id: hash_response.dig("sys", "contentType", "sys", "id")) ) end From 48b3b804b4b5207f9a043f237187988a475b59f5 Mon Sep 17 00:00:00 2001 From: Lorna Date: Wed, 2 Dec 2020 14:13:53 +0000 Subject: [PATCH 48/56] User can edit an answer Users will need to be able to change their answers after they have given them. --- CHANGELOG.md | 1 + app/controllers/answers_controller.rb | 15 ++++++++ app/controllers/steps_controller.rb | 8 ++++ app/views/journeys/_long_text_answer.html.erb | 1 + app/views/journeys/_radios_answer.html.erb | 1 + .../journeys/_short_text_answer.html.erb | 1 + app/views/steps/_edit_form_wrapper.html.erb | 5 +++ config/locales/en.yml | 2 + config/routes.rb | 4 +- .../anyone_can_edit_their_answers_spec.rb | 37 +++++++++++++++++++ 10 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 app/views/steps/_edit_form_wrapper.html.erb create mode 100644 spec/features/visitors/anyone_can_edit_their_answers_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f6248d76b..24234be09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog 1.0.0]. - journey map shows an error to the content team if a duplicate entry is detected - journey map shows an error to the content team if the journey doesn't end within 50 steps - refactor how we store and access a Step's associated Contentful Entry ID +- users can edit their answers ## [release-003] - 2020-12-07 diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 65b54700e..1d1bd3c52 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -27,6 +27,21 @@ def create end end + def update + @journey = Journey.find(journey_id) + @step = Step.find(step_id) + @answer = @step.answer + + @answer.assign_attributes(answer_params) + + if @answer.valid? + @answer.save + redirect_to journey_path(@journey) + else + render "steps/#{@step.contentful_type}", locals: {layout: "steps/edit_form_wrapper"} + end + end + private def journey_id diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index eec2b6538..f25f4da6b 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -34,6 +34,14 @@ def show render @step.contentful_type, locals: {layout: "steps/new_form_wrapper"} end + def edit + @journey = Journey.find(journey_id) + @step = Step.find(params[:id]) + @answer = @step.answer + + render "steps/#{@step.contentful_type}", locals: {layout: "steps/edit_form_wrapper"} + end + private def journey_id diff --git a/app/views/journeys/_long_text_answer.html.erb b/app/views/journeys/_long_text_answer.html.erb index e3fdd9bd6..6f13b8a51 100644 --- a/app/views/journeys/_long_text_answer.html.erb +++ b/app/views/journeys/_long_text_answer.html.erb @@ -1,4 +1,5 @@
<%= step.title %>
<%= simple_format(step.answer.response) %>
+
<%= link_to I18n.t("generic.button.change_answer"), edit_journey_step_path(@journey, step), class: "govuk-link" %>
diff --git a/app/views/journeys/_radios_answer.html.erb b/app/views/journeys/_radios_answer.html.erb index 7774e2258..feb47caec 100644 --- a/app/views/journeys/_radios_answer.html.erb +++ b/app/views/journeys/_radios_answer.html.erb @@ -1,4 +1,5 @@
<%= step.title %>
<%= answer.response.capitalize %>
+
<%= link_to I18n.t("generic.button.change_answer"), edit_journey_step_path(@journey, step), class: "govuk-link" %>
diff --git a/app/views/journeys/_short_text_answer.html.erb b/app/views/journeys/_short_text_answer.html.erb index 35e6b130e..2f009b423 100644 --- a/app/views/journeys/_short_text_answer.html.erb +++ b/app/views/journeys/_short_text_answer.html.erb @@ -1,4 +1,5 @@
<%= step.title %>
<%= answer.response %>
+
<%= link_to I18n.t("generic.button.change_answer"), edit_journey_step_path(@journey, step), class: "govuk-link" %>
diff --git a/app/views/steps/_edit_form_wrapper.html.erb b/app/views/steps/_edit_form_wrapper.html.erb new file mode 100644 index 000000000..72300574c --- /dev/null +++ b/app/views/steps/_edit_form_wrapper.html.erb @@ -0,0 +1,5 @@ +<%= content_for :title, @step.title %> +<%= form_for @answer, as: :answer, method: "put", url: journey_step_answer_path(@journey, @step, @answer) do |f| %> + <%= yield f %> + <%= f.submit I18n.t("generic.button.update"), class: "govuk-button" %> +<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 71571dfd3..64910dae2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -8,6 +8,8 @@ en: button: start: "Continue" next: "Continue" + change_answer: "Change" + update: "Update" banner: beta: tag: "beta" diff --git a/config/routes.rb b/config/routes.rb index d64cb836a..af256d000 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -6,8 +6,8 @@ resource :journey_map, only: [:new] resources :journeys, only: [:new, :show] do - resources :steps, only: [:new, :show] do - resources :answers, only: [:create] + resources :steps, only: [:new, :show, :edit] do + resources :answers, only: [:create, :update] end end diff --git a/spec/features/visitors/anyone_can_edit_their_answers_spec.rb b/spec/features/visitors/anyone_can_edit_their_answers_spec.rb new file mode 100644 index 000000000..ee64ff27d --- /dev/null +++ b/spec/features/visitors/anyone_can_edit_their_answers_spec.rb @@ -0,0 +1,37 @@ +require "rails_helper" + +feature "Users can edit their answers" do + let(:answer) { create(:short_text_answer, response: "answer") } + scenario "The journey summary page displays a change link" do + visit journey_path(answer.step.journey) + + expect(page).to have_content("answer") + expect(page).to have_content("Change") + end + + scenario "The edited answer is saved" do + visit journey_path(answer.step.journey) + + click_on(I18n.t("generic.button.change_answer")) + + fill_in "answer[response]", with: "email@example.com" + + click_on(I18n.t("generic.button.update")) + + expect(page).to have_content("email@example.com") + end + + context "An error is thrown" do + scenario "When an answer is invalid" do + visit journey_path(answer.step.journey) + + click_on(I18n.t("generic.button.change_answer")) + + fill_in "answer[response]", with: "" + + click_on(I18n.t("generic.button.update")) + + expect(page).to have_content("can't be blank") + end + end +end From 07786991db045b3edbb3d251d4c748048e1251cf Mon Sep 17 00:00:00 2001 From: Lorna Date: Tue, 8 Dec 2020 16:53:27 +0000 Subject: [PATCH 49/56] Add contenful_model override for answer FactoryBot This override has been set to ensure that contentful_model has a value of "question". This will need looking into and I will require help to ensure this override isn't needed for future tests. --- spec/factories/answer.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/factories/answer.rb b/spec/factories/answer.rb index 532a44969..6b89a1498 100644 --- a/spec/factories/answer.rb +++ b/spec/factories/answer.rb @@ -1,18 +1,18 @@ FactoryBot.define do factory :radio_answer do - association :step, factory: :step, contentful_type: "radios" + association :step, factory: :step, contentful_type: "radios", contentful_model: "question" response { "Green" } end factory :short_text_answer do - association :step, factory: :step, contentful_type: "short_text" + association :step, factory: :step, contentful_type: "short_text", contentful_model: "question" response { "Green" } end factory :long_text_answer do - association :step, factory: :step, contentful_type: "long_text" + association :step, factory: :step, contentful_type: "long_text", contentful_model: "question" response { "Lots of green" } end From 756d62720bd0dbbf4dc605bf04f75a323d7d54da Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 17 Dec 2020 12:01:48 +0000 Subject: [PATCH 50/56] Contentful ID migration can run This worked locally until I removed the method the temporary method that parsed the sting into a hash https://github.com/DFE-Digital/buy-for-your-school/pull/84/commits/c3c5096bb14cf0c58bd5bc8323be5a69c8fa45a7#diff-d0a4710bfb2223d4b6f70da35e5d63f599611786b7b688571d372362c2f57df4L27 This change moves that code into the migration so it can run regardless of the state of the step methods. --- db/migrate/20201216150806_add_contentful_id_to_step.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/db/migrate/20201216150806_add_contentful_id_to_step.rb b/db/migrate/20201216150806_add_contentful_id_to_step.rb index 964bff8e7..0f3e8eba0 100644 --- a/db/migrate/20201216150806_add_contentful_id_to_step.rb +++ b/db/migrate/20201216150806_add_contentful_id_to_step.rb @@ -4,7 +4,9 @@ def up ActiveRecord::Base.transaction do Step.all.map do |step| - contentful_id = step.raw.dig("sys", "id") + contentful_id = JSON.parse( + step.raw.gsub("=>", ":") + ).dig("sys", "id") step.update(contentful_id: contentful_id) end end From 890d9f9adc5549fc3e59088295c2fc921ae0132a Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 16 Dec 2020 10:19:23 +0000 Subject: [PATCH 51/56] Fix previous test fixture so the two entries link to eachother --- spec/fixtures/contentful/multiple-entries-example.json | 4 ++-- spec/jobs/warm_entry_cache_job_spec.rb | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/spec/fixtures/contentful/multiple-entries-example.json b/spec/fixtures/contentful/multiple-entries-example.json index 066ca3279..ec1d49501 100644 --- a/spec/fixtures/contentful/multiple-entries-example.json +++ b/spec/fixtures/contentful/multiple-entries-example.json @@ -59,7 +59,7 @@ "id": "rwl7tyzv9sys" } }, - "id": "52Ni9UFvZj8BYXSbhs373C", + "id": "hfjJgWRg4xiiiImwVRDtZ", "type": "Entry", "createdAt": "2020-11-04T12:28:30.442Z", "updatedAt": "2020-11-26T16:39:54.188Z", @@ -93,7 +93,7 @@ "sys": { "type": "Link", "linkType": "Entry", - "id": "hfjJgWRg4xiiiImwVRDtZ" + "id": "52Ni9UFvZj8BYXSbhs373C" } } } diff --git a/spec/jobs/warm_entry_cache_job_spec.rb b/spec/jobs/warm_entry_cache_job_spec.rb index 66093513e..da0b2f24d 100644 --- a/spec/jobs/warm_entry_cache_job_spec.rb +++ b/spec/jobs/warm_entry_cache_job_spec.rb @@ -7,6 +7,7 @@ around do |example| ClimateControl.modify( + CONTENTFUL_PLANNING_START_ENTRY_ID: "5kZ9hIFDvNCEhjWs72SFwj", CONTENTFUL_ENTRY_CACHING: "true" ) do example.run @@ -33,9 +34,9 @@ .to eql( "\"{\\\"sys\\\":{\\\"space\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Space\\\",\\\"id\\\":\\\"rwl7tyzv9sys\\\"}},\\\"id\\\":\\\"5kZ9hIFDvNCEhjWs72SFwj\\\",\\\"type\\\":\\\"Entry\\\",\\\"createdAt\\\":\\\"2020-12-02T10:48:35.748Z\\\",\\\"updatedAt\\\":\\\"2020-12-02T10:48:35.748Z\\\",\\\"environment\\\":{\\\"sys\\\":{\\\"id\\\":\\\"develop\\\",\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Environment\\\"}},\\\"revision\\\":1,\\\"contentType\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"ContentType\\\",\\\"id\\\":\\\"staticContent\\\"}},\\\"locale\\\":\\\"en-US\\\"},\\\"fields\\\":{\\\"slug\\\":\\\"/timelines\\\",\\\"title\\\":\\\"When you should start\\\",\\\"body\\\":\\\"Procuring a new catering contract can take up to 6 months to consult, create, review and award. \\\\n\\\\nUsually existing contracts start and end in the month of September. We recommend starting this process around March.\\\",\\\"type\\\":\\\"paragraphs\\\",\\\"next\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Entry\\\",\\\"id\\\":\\\"hfjJgWRg4xiiiImwVRDtZ\\\"}}}}\"" ) - expect(RedisCache.redis.get("contentful:entry:52Ni9UFvZj8BYXSbhs373C")) + expect(RedisCache.redis.get("contentful:entry:hfjJgWRg4xiiiImwVRDtZ")) .to eql( - "\"{\\\"sys\\\":{\\\"space\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Space\\\",\\\"id\\\":\\\"rwl7tyzv9sys\\\"}},\\\"id\\\":\\\"52Ni9UFvZj8BYXSbhs373C\\\",\\\"type\\\":\\\"Entry\\\",\\\"createdAt\\\":\\\"2020-11-04T12:28:30.442Z\\\",\\\"updatedAt\\\":\\\"2020-11-26T16:39:54.188Z\\\",\\\"environment\\\":{\\\"sys\\\":{\\\"id\\\":\\\"develop\\\",\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Environment\\\"}},\\\"revision\\\":6,\\\"contentType\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"ContentType\\\",\\\"id\\\":\\\"question\\\"}},\\\"locale\\\":\\\"en-US\\\"},\\\"fields\\\":{\\\"slug\\\":\\\"/dev-start-which-service\\\",\\\"title\\\":\\\"Which service do you need?\\\",\\\"helpText\\\":\\\"Tell us which service you need.\\\",\\\"type\\\":\\\"radios\\\",\\\"options\\\":[\\\"Catering\\\",\\\"Cleaning\\\"],\\\"next\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Entry\\\",\\\"id\\\":\\\"hfjJgWRg4xiiiImwVRDtZ\\\"}}}}\"" + "\"{\\\"sys\\\":{\\\"space\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Space\\\",\\\"id\\\":\\\"rwl7tyzv9sys\\\"}},\\\"id\\\":\\\"hfjJgWRg4xiiiImwVRDtZ\\\",\\\"type\\\":\\\"Entry\\\",\\\"createdAt\\\":\\\"2020-11-04T12:28:30.442Z\\\",\\\"updatedAt\\\":\\\"2020-11-26T16:39:54.188Z\\\",\\\"environment\\\":{\\\"sys\\\":{\\\"id\\\":\\\"develop\\\",\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Environment\\\"}},\\\"revision\\\":6,\\\"contentType\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"ContentType\\\",\\\"id\\\":\\\"question\\\"}},\\\"locale\\\":\\\"en-US\\\"},\\\"fields\\\":{\\\"slug\\\":\\\"/dev-start-which-service\\\",\\\"title\\\":\\\"Which service do you need?\\\",\\\"helpText\\\":\\\"Tell us which service you need.\\\",\\\"type\\\":\\\"radios\\\",\\\"options\\\":[\\\"Catering\\\",\\\"Cleaning\\\"],\\\"next\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Entry\\\",\\\"id\\\":\\\"52Ni9UFvZj8BYXSbhs373C\\\"}}}}\"" ) end end From d26f29861d8033be99aa75169b4bf573f78b598c Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 16 Dec 2020 11:14:25 +0000 Subject: [PATCH 52/56] Warming the cache only adds new content if valid If a valid contentful journey is found, we will continue to add those new items to the cache for 24 hours. If the `BuildJourneyOrder` service raises an error to the effect that the journey was invalid, we instead take the approach of extending all prior entries. This should ensure that a single valid journey remains in the cache for users to use. Rollbar alerts are managed by `BuildJourneyOrder` so the team should be alerted when this happens. We do some extra work to make sure we combine TTL times to avoid the case where we actually lower the TTL overall. Though we only have redis keys/items that are entries at the moment, we make sure we don't accidentally scoop up other types of item by checking the key prefixes. --- CHANGELOG.md | 1 + app/jobs/warm_entry_cache_job.rb | 9 +++++- app/services/cache.rb | 9 ++++++ spec/jobs/warm_entry_cache_job_spec.rb | 44 ++++++++++++++++++++++++++ spec/services/cache_spec.rb | 41 ++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24234be09..79b37b4ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog 1.0.0]. - journey map shows an error to the content team if the journey doesn't end within 50 steps - refactor how we store and access a Step's associated Contentful Entry ID - users can edit their answers +- only add Contentful entries that form part of a valid journey to the cache ## [release-003] - 2020-12-07 diff --git a/app/jobs/warm_entry_cache_job.rb b/app/jobs/warm_entry_cache_job.rb index c8ec34877..3b04dfcbe 100644 --- a/app/jobs/warm_entry_cache_job.rb +++ b/app/jobs/warm_entry_cache_job.rb @@ -5,10 +5,17 @@ class WarmEntryCacheJob < ApplicationJob sidekiq_options retry: 5 def perform - entries = GetAllContentfulEntries.new.call + entries = BuildJourneyOrder.new( + entries: GetAllContentfulEntries.new.call, + starting_entry_id: ENV["CONTENTFUL_PLANNING_START_ENTRY_ID"] + ).call + entries.each do |entry| store_in_cache(cache: cache, key: "contentful:entry:#{entry.id}", entry: entry) end + rescue BuildJourneyOrder::RepeatEntryDetected, + BuildJourneyOrder::TooManyChainedEntriesDetected + cache.extend_ttl_on_all_entries end private diff --git a/app/services/cache.rb b/app/services/cache.rb index d543b8875..685a35757 100644 --- a/app/services/cache.rb +++ b/app/services/cache.rb @@ -26,4 +26,13 @@ def set(key:, value:) redis_cache.set(key, value) redis_cache.expire(key, ttl) end + + def extend_ttl_on_all_entries(extension: (60 * 60 * 24)) + redis_cache.keys("contentful:entry:*").map do |key| + previous_ttl = redis_cache.ttl(key) + extended_ttl = extension + previous_ttl + + redis_cache.expire(key, extended_ttl) + end + end end diff --git a/spec/jobs/warm_entry_cache_job_spec.rb b/spec/jobs/warm_entry_cache_job_spec.rb index da0b2f24d..7de1f74b3 100644 --- a/spec/jobs/warm_entry_cache_job_spec.rb +++ b/spec/jobs/warm_entry_cache_job_spec.rb @@ -4,6 +4,7 @@ include ActiveJob::TestHelper before(:each) { ActiveJob::Base.queue_adapter = :test } + after(:each) { RedisCache.redis.flushdb } around do |example| ClimateControl.modify( @@ -39,5 +40,48 @@ "\"{\\\"sys\\\":{\\\"space\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Space\\\",\\\"id\\\":\\\"rwl7tyzv9sys\\\"}},\\\"id\\\":\\\"hfjJgWRg4xiiiImwVRDtZ\\\",\\\"type\\\":\\\"Entry\\\",\\\"createdAt\\\":\\\"2020-11-04T12:28:30.442Z\\\",\\\"updatedAt\\\":\\\"2020-11-26T16:39:54.188Z\\\",\\\"environment\\\":{\\\"sys\\\":{\\\"id\\\":\\\"develop\\\",\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Environment\\\"}},\\\"revision\\\":6,\\\"contentType\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"ContentType\\\",\\\"id\\\":\\\"question\\\"}},\\\"locale\\\":\\\"en-US\\\"},\\\"fields\\\":{\\\"slug\\\":\\\"/dev-start-which-service\\\",\\\"title\\\":\\\"Which service do you need?\\\",\\\"helpText\\\":\\\"Tell us which service you need.\\\",\\\"type\\\":\\\"radios\\\",\\\"options\\\":[\\\"Catering\\\",\\\"Cleaning\\\"],\\\"next\\\":{\\\"sys\\\":{\\\"type\\\":\\\"Link\\\",\\\"linkType\\\":\\\"Entry\\\",\\\"id\\\":\\\"52Ni9UFvZj8BYXSbhs373C\\\"}}}}\"" ) end + + context "when the journey order cannot be built" do + it "does not add new items to the cache" do + allow_any_instance_of(BuildJourneyOrder) + .to receive(:call) + .and_raise(BuildJourneyOrder::RepeatEntryDetected) + + stub_get_contentful_entries( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "repeat-entry-example.json" + ) + + described_class.perform_later + perform_enqueued_jobs + + expect(RedisCache.redis).not_to receive(:set).with("contentful:entry:5kZ9hIFDvNCEhjWs72SFwj", anything) + end + + it "extends the TTL on all existing items by 24 hours" do + RedisCache.redis.set("contentful:entry:5kZ9hIFDvNCEhjWs72SFwj", "\"{\\}\"") + + allow_any_instance_of(BuildJourneyOrder) + .to receive(:call) + .and_raise(BuildJourneyOrder::RepeatEntryDetected) + + stub_get_contentful_entries( + entry_id: "5kZ9hIFDvNCEhjWs72SFwj", + fixture_filename: "repeat-entry-example.json" + ) + + freeze_time do + ttl_extension = 60 * 60 * 24 + existing_ttl = -1 + + expect(RedisCache.redis) + .to receive(:expire) + .with("contentful:entry:5kZ9hIFDvNCEhjWs72SFwj", ttl_extension + existing_ttl) + + described_class.perform_later + perform_enqueued_jobs + end + end + end end end diff --git a/spec/services/cache_spec.rb b/spec/services/cache_spec.rb index c8dbe8d0a..e8c473adc 100644 --- a/spec/services/cache_spec.rb +++ b/spec/services/cache_spec.rb @@ -1,6 +1,10 @@ require "rails_helper" RSpec.describe Cache do + after(:each) do + RedisCache.redis.flushdb + end + it "requires a key, enabled flag and ttl" do result = described_class.new(enabled: "true", ttl: 123) expect(result.enabled).to eql("true") @@ -80,4 +84,41 @@ end end end + + describe "#extend_ttl_on_all_entries" do + it "only updates redis keys associated to contentful entries" do + cache = described_class.new(enabled: anything, ttl: anything) + redis = cache.redis_cache + + redis.set("contentful:entry:123", "value") + expect(redis).to receive(:expire).with("contentful:entry:123", (60 * 60 * 24) + -1) + + cache.extend_ttl_on_all_entries + end + + context "when other redis keys exist" do + it "only updates redis keys associated to contentful entries" do + cache = described_class.new(enabled: anything, ttl: anything) + redis = cache.redis_cache + + redis.set("another_key", "another_value") + expect(redis).not_to receive(:expire).with("another_key") + + cache.extend_ttl_on_all_entries + end + end + + context "when the key already has a TTL" do + it "sets a combined TTL with the existing and the extension" do + cache = described_class.new(enabled: anything, ttl: anything) + redis = cache.redis_cache + + redis.set("contentful:entry:123", "value") + redis.expire("contentful:entry:123", 10) + expect(redis).to receive(:expire).with("contentful:entry:123", (60 * 60 * 24) + 10) + + cache.extend_ttl_on_all_entries + end + end + end end From 3b8298e55060dc4eec63c973de0e17b32d68e87b Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 17 Dec 2020 12:38:51 +0000 Subject: [PATCH 53/56] Fix migration so presence validation is applied at the end Applying this straight away fails because of course the new field has no value at all. This shouldn't have got through local testing but alas it did and now we have to fix it. Create the new temp field without validation, and add it at the end. By adding it at the end, after populating the columns, we should the be told if we still have missing values. That failure is the one we want. --- .../20201216160028_change_step_raw_field_to_json_b.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb b/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb index c5f3b9014..3b0fb4708 100644 --- a/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb +++ b/db/migrate/20201216160028_change_step_raw_field_to_json_b.rb @@ -1,6 +1,6 @@ class ChangeStepRawFieldToJsonB < ActiveRecord::Migration[6.1] def up - add_column :steps, :raw_jsonb, :jsonb, null: false + add_column :steps, :raw_jsonb, :jsonb ActiveRecord::Base.transaction do Step.all.map do |step| hash = JSON.parse(step.raw.gsub("=>", ":")) @@ -9,10 +9,11 @@ def up end remove_column :steps, :raw rename_column :steps, :raw_jsonb, :raw + change_column :steps, :raw, :jsonb, null: false end def down - add_column :steps, :raw_binary, :binary, null: false + add_column :steps, :raw_binary, :binary ActiveRecord::Base.transaction do Step.all.map do |step| string = step.raw.to_s @@ -21,5 +22,6 @@ def down end remove_column :steps, :raw rename_column :steps, :raw_binary, :raw + change_column :steps, :raw, :binary, null: false end end From 60f846d9719b17f2fb2691ca0b947a1348dfad25 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 17 Dec 2020 14:43:29 +0000 Subject: [PATCH 54/56] Add change links to the single_date step partial --- app/views/journeys/_single_date_answer.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/journeys/_single_date_answer.html.erb b/app/views/journeys/_single_date_answer.html.erb index f8ccefc7d..2e008f4cc 100644 --- a/app/views/journeys/_single_date_answer.html.erb +++ b/app/views/journeys/_single_date_answer.html.erb @@ -1,4 +1,5 @@
<%= step.title %>
<%= I18n.l(answer.response) %>
+
<%= link_to I18n.t("generic.button.change_answer"), edit_journey_step_path(@journey, step), class: "govuk-link" %>
From 8fa2dacd826c3dc0ae1a54180a41a06254aabe12 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 17 Dec 2020 14:43:38 +0000 Subject: [PATCH 55/56] Add change links to the checkbox step partial --- app/views/journeys/_checkboxes_answer.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/journeys/_checkboxes_answer.html.erb b/app/views/journeys/_checkboxes_answer.html.erb index 18cd08e5a..5bb8d711b 100644 --- a/app/views/journeys/_checkboxes_answer.html.erb +++ b/app/views/journeys/_checkboxes_answer.html.erb @@ -1,4 +1,5 @@
<%= step.title %>
<%= answer.response.reject(&:blank?).map(&:capitalize).join(", ") %>
+
<%= link_to I18n.t("generic.button.change_answer"), edit_journey_step_path(@journey, step), class: "govuk-link" %>
From b1f71ecfa258e67d41c00c29f44c6aec94cacfd6 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 17 Dec 2020 15:14:22 +0000 Subject: [PATCH 56/56] release-004 --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 79b37b4ba..260c36446 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog 1.0.0]. ## [Unreleased] +## [release-004] - 2020-12-17 + - fix primary key type on long_text_answers table to UUID - nightly task to warm the Contentful cache for all entries - form button content is configurable through Contentful @@ -49,7 +51,8 @@ Contentful fixture - Contentful can redirect users to preview endpoints - users can be asked to answer a long text question -[unreleased]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-003...HEAD +[unreleased]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-004...HEAD +[release-004]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-003...release-004 [release-003]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-002...release-003 [release-002]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-001...release-002 [release-001]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-000...release-001