From 2703cad7dfa49965711a6683234eb37cc3e20da0 Mon Sep 17 00:00:00 2001 From: Lorna Date: Thu, 15 Apr 2021 14:35:36 +0100 Subject: [PATCH 01/32] Add email address within header banner for feedback purposes --- .env.example | 2 ++ .env.test | 2 ++ app/helpers/application_helper.rb | 2 +- app/views/layouts/application.html.erb | 2 +- config/locales/en.yml | 3 ++- .../visitors/anyone_can_see_the_service_banners_spec.rb | 3 ++- spec/helpers/application_helper_spec.rb | 2 +- 7 files changed, 11 insertions(+), 5 deletions(-) diff --git a/.env.example b/.env.example index 58c48edbf..627a01b3f 100644 --- a/.env.example +++ b/.env.example @@ -17,6 +17,8 @@ REDIS_URL=redis://localhost:6379 # Miscellaneous DAYS_A_JOURNEY_CAN_BE_INACTIVE_FOR=30 +SUPPORT_EMAIL=email@example.gov.uk + # Contentful CONTENTFUL_URL=cdn.contentful.com CONTENTFUL_SPACE= diff --git a/.env.test b/.env.test index 763f048ec..2c83605b4 100644 --- a/.env.test +++ b/.env.test @@ -21,3 +21,5 @@ CONTENTFUL_DEFAULT_CATEGORY_ENTRY_ID=contentful-category-entry # API CONTENTFUL_WEBHOOK_API_KEY= + +SUPPORT_EMAIL=email@example.gov.uk diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index df1e0c629..03cf36e15 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -12,6 +12,6 @@ def banner_tag def banner_message return I18n.t("banner.preview.message") if ENV["CONTENTFUL_PREVIEW_APP"].eql?("true") - I18n.t("banner.beta.message") + I18n.t("banner.beta.message", support_email: ENV.fetch("SUPPORT_EMAIL")) end end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index c52241019..a7e35a217 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -64,7 +64,7 @@ <%= banner_tag %> - <%= banner_message %> + <%= banner_message.html_safe %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 3fbd34747..4e1577a2b 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -15,7 +15,8 @@ en: banner: beta: tag: "beta" - message: "This is a new service - your feedback will help us to improve it." + message: + This is a new service – your feedback will help us to improve it. preview: tag: "preview" message: "This environment is only for previewing Contentful changes before publishing." diff --git a/spec/features/visitors/anyone_can_see_the_service_banners_spec.rb b/spec/features/visitors/anyone_can_see_the_service_banners_spec.rb index ccd85d3c3..e83c1b1a5 100644 --- a/spec/features/visitors/anyone_can_see_the_service_banners_spec.rb +++ b/spec/features/visitors/anyone_can_see_the_service_banners_spec.rb @@ -3,7 +3,8 @@ visit root_path expect(page).to have_content(I18n.t("banner.beta.tag")) - expect(page).to have_content(I18n.t("banner.beta.message")) + expect(page.html).to include(I18n.t("banner.beta.message", support_email: ENV.fetch("SUPPORT_EMAIL"))) + expect(page).to have_content(ENV["SUPPORT_EMAIL"]) end context "when the app is configured as a Contenetful preview app" do diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index f920635db..4d81590df 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -39,7 +39,7 @@ describe "#banner_message" do it "returns the beta message by default" do - expect(helper.banner_message).to eq(I18n.t("banner.beta.message")) + expect(helper.banner_message).to eq(I18n.t("banner.beta.message", support_email: ENV.fetch("SUPPORT_EMAIL"))) end context "when PREVIEW_APP is configured to true" do From b27095d533fbb201cae668cb08a91f00640896be Mon Sep 17 00:00:00 2001 From: Lorna Date: Thu, 15 Apr 2021 17:04:39 +0100 Subject: [PATCH 02/32] Add data policy details within footer --- CHANGELOG.md | 1 + app/helpers/application_helper.rb | 4 ++++ app/views/layouts/application.html.erb | 4 ++++ config/locales/en.yml | 3 +++ ....rb => anyone_can_see_the_service_components_spec.rb} | 9 ++++++++- spec/helpers/application_helper_spec.rb | 6 ++++++ 6 files changed, 26 insertions(+), 1 deletion(-) rename spec/features/visitors/{anyone_can_see_the_service_banners_spec.rb => anyone_can_see_the_service_components_spec.rb} (70%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 725f21f21..c0f1ac389 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog 1.0.0]. ## [Unreleased] +- add header and footer information for feedback and data requests ## [release-009] - 2021-05-21 diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 03cf36e15..bc9625f77 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -14,4 +14,8 @@ def banner_message return I18n.t("banner.preview.message") if ENV["CONTENTFUL_PREVIEW_APP"].eql?("true") I18n.t("banner.beta.message", support_email: ENV.fetch("SUPPORT_EMAIL")) end + + def footer_message + I18n.t("banner.footer.message", support_email: ENV.fetch("SUPPORT_EMAIL")) + end end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index a7e35a217..c27cff712 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -79,6 +79,10 @@
+
+ <%= footer_message.html_safe %> +
+ diff --git a/config/locales/en.yml b/config/locales/en.yml index 4e1577a2b..bba9160db 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -17,6 +17,9 @@ en: tag: "beta" message: This is a new service – your feedback will help us to improve it. + footer: + message: + For privacy information for this service, or to request the deletion of any personal data, email %{support_email} preview: tag: "preview" message: "This environment is only for previewing Contentful changes before publishing." diff --git a/spec/features/visitors/anyone_can_see_the_service_banners_spec.rb b/spec/features/visitors/anyone_can_see_the_service_components_spec.rb similarity index 70% rename from spec/features/visitors/anyone_can_see_the_service_banners_spec.rb rename to spec/features/visitors/anyone_can_see_the_service_components_spec.rb index e83c1b1a5..814223325 100644 --- a/spec/features/visitors/anyone_can_see_the_service_banners_spec.rb +++ b/spec/features/visitors/anyone_can_see_the_service_components_spec.rb @@ -1,4 +1,4 @@ -feature "Users can see the service banners" do +feature "Users can see the service components" do scenario "A beta phase banner helps set expectations of the service" do visit root_path @@ -23,4 +23,11 @@ expect(page).to have_content(I18n.t("banner.preview.message")) end end + + scenario "A footer provides other information for the service" do + visit root_path + + expect(page.html).to include(I18n.t("banner.footer.message", support_email: ENV.fetch("SUPPORT_EMAIL"))) + expect(page).to have_content(ENV["SUPPORT_EMAIL"]) + end end diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 4d81590df..fc000fb38 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -54,4 +54,10 @@ end end end + + describe "#footer_message" do + it "returns the footer message by default" do + expect(helper.footer_message).to eq(I18n.t("banner.footer.message", support_email: ENV.fetch("SUPPORT_EMAIL"))) + end + end end From 360d2c01178a257077f58927cb045d5be0a3279b Mon Sep 17 00:00:00 2001 From: Nick Jackson Date: Thu, 22 Apr 2021 10:22:10 +0100 Subject: [PATCH 03/32] Add SUPPORT_EMAIL env var to examples and initialiser --- config/initializers/_dotenv.rb | 3 ++- terraform/workspace-variables/preview_app_env.yml.example | 3 +++ terraform/workspace-variables/prod_app_env.yml.example | 3 +++ terraform/workspace-variables/research_app_env.yml.example | 3 +++ terraform/workspace-variables/staging_app_env.yml.example | 3 +++ 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/config/initializers/_dotenv.rb b/config/initializers/_dotenv.rb index aca34a326..e81407eef 100644 --- a/config/initializers/_dotenv.rb +++ b/config/initializers/_dotenv.rb @@ -9,6 +9,7 @@ "CONTENTFUL_ACCESS_TOKEN", "CONTENTFUL_DEFAULT_CATEGORY_ENTRY_ID", "CONTENTFUL_PREVIEW_APP", - "CONTENTFUL_ENTRY_CACHING" + "CONTENTFUL_ENTRY_CACHING", + "SUPPORT_EMAIL" ) end diff --git a/terraform/workspace-variables/preview_app_env.yml.example b/terraform/workspace-variables/preview_app_env.yml.example index 6f9d71bf1..96889270f 100644 --- a/terraform/workspace-variables/preview_app_env.yml.example +++ b/terraform/workspace-variables/preview_app_env.yml.example @@ -26,3 +26,6 @@ DFE_SIGN_IN_IDENTIFIER: DFE_SIGN_IN_ISSUER: DFE_SIGN_IN_REDIRECT_URL: DFE_SIGN_IN_SECRET: + +# Support Email +SUPPORT_EMAIL: diff --git a/terraform/workspace-variables/prod_app_env.yml.example b/terraform/workspace-variables/prod_app_env.yml.example index 01fe792e1..5524b6544 100644 --- a/terraform/workspace-variables/prod_app_env.yml.example +++ b/terraform/workspace-variables/prod_app_env.yml.example @@ -26,3 +26,6 @@ DFE_SIGN_IN_IDENTIFIER: DFE_SIGN_IN_ISSUER: DFE_SIGN_IN_REDIRECT_URL: DFE_SIGN_IN_SECRET: + +# Support Email +SUPPORT_EMAIL: diff --git a/terraform/workspace-variables/research_app_env.yml.example b/terraform/workspace-variables/research_app_env.yml.example index e5b9e4cfa..54269f99b 100644 --- a/terraform/workspace-variables/research_app_env.yml.example +++ b/terraform/workspace-variables/research_app_env.yml.example @@ -26,3 +26,6 @@ DFE_SIGN_IN_IDENTIFIER: DFE_SIGN_IN_ISSUER: DFE_SIGN_IN_REDIRECT_URL: DFE_SIGN_IN_SECRET: + +# Support Email +SUPPORT_EMAIL: diff --git a/terraform/workspace-variables/staging_app_env.yml.example b/terraform/workspace-variables/staging_app_env.yml.example index ffe09360c..987a28f69 100644 --- a/terraform/workspace-variables/staging_app_env.yml.example +++ b/terraform/workspace-variables/staging_app_env.yml.example @@ -26,3 +26,6 @@ DFE_SIGN_IN_IDENTIFIER: DFE_SIGN_IN_ISSUER: DFE_SIGN_IN_REDIRECT_URL: DFE_SIGN_IN_SECRET: + +# Support Email +SUPPORT_EMAIL: From 673574645ad5dcbe14d304548f1785c18b410cfe Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 12:00:24 +0100 Subject: [PATCH 04/32] Force SSL in production This does several things that make our app more secure: 1. Redirects any HTTP traffic to HTTPS 1. Enables HSTS 1. Marks cookies as secure https://github.com/rails/rails/blob/794252d70990162d14da17ee854ad353e74297ee/actionpack/lib/action_dispatch/middleware/ssl.rb --- CHANGELOG.md | 2 ++ config/environments/production.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0f1ac389..d7d115da2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ The format is based on [Keep a Changelog 1.0.0]. ## [Unreleased] - add header and footer information for feedback and data requests +- force SSL in production to only accept HTTPS traffic, enable HSTS and secure tower cookies + ## [release-009] - 2021-05-21 - fix multiple specification fields diff --git a/config/environments/production.rb b/config/environments/production.rb index 4713d7a99..fb6d94cbb 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -52,7 +52,7 @@ # config.action_cable.allowed_request_origins = [ 'http://example.com', /http:\/\/example.*/ ] # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. - # config.force_ssl = true + config.force_ssl = true # Use the lowest log level to ensure availability of diagnostic information # when problems arise. From 1f5afd8ff7a4ac673f0e7baf149bdf256bd248b7 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 14:46:36 +0100 Subject: [PATCH 05/32] Use Redis-rails to manage session data instead of the default Rails cookie based approach https://github.com/redis-store/redis-rails#session-storage This will keep session data server side. This will give us more control over session data so we can expire those for simultaneous logins. --- Gemfile | 1 + Gemfile.lock | 17 +++++++++++++++++ config/initializers/session_store.rb | 8 ++++++++ 3 files changed, 26 insertions(+) create mode 100644 config/initializers/session_store.rb diff --git a/Gemfile b/Gemfile index 70ab9fc8b..3a424322f 100644 --- a/Gemfile +++ b/Gemfile @@ -23,6 +23,7 @@ gem "puma", "~> 5.2" gem "redcarpet", "~> 3.5" gem "redis", "~> 4.2" gem "redis-namespace" +gem "redis-rails" gem "rollbar" gem "rails", "~> 6.1.3" gem "sass-rails", "~> 6.0" diff --git a/Gemfile.lock b/Gemfile.lock index 978acf4fd..978b21fbd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -277,8 +277,24 @@ GEM ffi (~> 1.0) redcarpet (3.5.1) redis (4.2.5) + redis-actionpack (5.2.0) + actionpack (>= 5, < 7) + redis-rack (>= 2.1.0, < 3) + redis-store (>= 1.1.0, < 2) + redis-activesupport (5.2.0) + activesupport (>= 3, < 7) + redis-store (>= 1.3, < 2) redis-namespace (1.8.1) redis (>= 3.0.4) + redis-rack (2.1.3) + rack (>= 2.0.8, < 3) + redis-store (>= 1.2, < 2) + redis-rails (5.0.2) + redis-actionpack (>= 5.0, < 6) + redis-activesupport (>= 5.0, < 6) + redis-store (>= 1.2, < 2) + redis-store (1.9.0) + redis (>= 4, < 5) regexp_parser (2.1.1) rexml (3.2.5) rollbar (3.1.2) @@ -441,6 +457,7 @@ DEPENDENCIES redcarpet (~> 3.5) redis (~> 4.2) redis-namespace + redis-rails rollbar rspec-rails sass-rails (~> 6.0) diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb new file mode 100644 index 000000000..6c55fe999 --- /dev/null +++ b/config/initializers/session_store.rb @@ -0,0 +1,8 @@ +redis_session_uri = URI(ENV.fetch("REDIS_URL")) + +BuyForYourSchool::Application.config.session_store :redis_store, + servers: ["#{redis_session_uri}/0/session"], + expire_after: 1.day, + key: "_buy_for_your_school_session", + threadsafe: true, + secure: Rails.env.production? From dda9b90a3b29202f079cfecaa5608c0477339bf6 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 14:47:03 +0100 Subject: [PATCH 06/32] Rename Redis initialiser to be RedisCache To make room for RedisSessions if we need. --- config/initializers/{redis.rb => redis_cache.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename config/initializers/{redis.rb => redis_cache.rb} (100%) diff --git a/config/initializers/redis.rb b/config/initializers/redis_cache.rb similarity index 100% rename from config/initializers/redis.rb rename to config/initializers/redis_cache.rb From 0436595d6ed04e697926a49f5ac52c38d14e316e Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 17:16:41 +0100 Subject: [PATCH 07/32] RedisCache uses a namespace that describes it This would be more useful if we ever want to ask `Redis::Namespace.caching.flushdb` --- config/initializers/redis_cache.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/redis_cache.rb b/config/initializers/redis_cache.rb index c97ceae99..42a8c46f0 100644 --- a/config/initializers/redis_cache.rb +++ b/config/initializers/redis_cache.rb @@ -5,7 +5,7 @@ def redis MockRedis.new else Redis::Namespace.new( - "buy_for_your_school", + "caching", redis: Redis.new(url: ENV["REDIS_URL"], db: 1) ) end From 9fcb46b0aac1225121e9ca8de8b2c7100dd6d156 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 17:21:24 +0100 Subject: [PATCH 08/32] Prevent users from having more than 1 active session If a second authentication for a unique DfE Sign in UID happens, we remove the session data for the previous session, effectively signing them out. On the first users next request they will be taken to the root_path with a flash message that says they've been signed out. The message is generic because we have no current way to connect the new session to the old session. We can't say "Someone else logged you out" without more work that might see us storing more state in redis. We can't configure the Redis session store keys. They are in the format `2::28734342427213406823417860`. This long value corresponds to the private_id of the Session. As we can't manipulate this key to include the DfE Sign-in ID: `2::28734342427213406823417860:` we have to keep track of who is logged in and what where their session data lives. We d this using `RedisSessionLookup`. --- CHANGELOG.md | 2 + app/controllers/application_controller.rb | 2 +- app/controllers/sessions_controller.rb | 5 +- app/models/user_session.rb | 22 ++++++++ config/initializers/redis_session_lookup.rb | 14 +++++ config/initializers/redis_sessions.rb | 11 ++++ spec/models/user_session_spec.rb | 59 +++++++++++++++++++++ spec/requests/authentication_spec.rb | 32 +++++++++++ 8 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 config/initializers/redis_session_lookup.rb create mode 100644 config/initializers/redis_sessions.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index d7d115da2..0fa36e68f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ The format is based on [Keep a Changelog 1.0.0]. - force SSL in production to only accept HTTPS traffic, enable HSTS and secure tower cookies +- prevent concurrent sign ins + ## [release-009] - 2021-05-21 - fix multiple specification fields diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 89e441516..ab911d644 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -27,7 +27,7 @@ def authenticate_user! return if current_user session.delete(:dfe_sign_in_uid) - redirect_to root_path + redirect_to root_path, notice: "You've been signed out." end def current_journey diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 036fa8ae7..5d149acc2 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -5,8 +5,9 @@ class SessionsController < ApplicationController protect_from_forgery except: :bypass_callback def create - UserSession.new(session: session) - .persist_successful_dfe_sign_in_claim!(omniauth_hash: auth_hash) + user_session = UserSession.new(session: session) + user_session.persist_successful_dfe_sign_in_claim!(omniauth_hash: auth_hash) + user_session.invalidate_other_user_sessions(omniauth_hash: auth_hash) redirect_to dashboard_path end diff --git a/app/models/user_session.rb b/app/models/user_session.rb index a9d204d20..3af6ddd42 100644 --- a/app/models/user_session.rb +++ b/app/models/user_session.rb @@ -31,4 +31,26 @@ def sign_out_url "#{ENV.fetch("DFE_SIGN_IN_ISSUER")}/session/end?#{query.to_query}" end + + # Sign out concurrent logins + # + # When the same DfE Sign-in user is signed into our service, invalidate the + # first one, effectively signing it out. + # + # We can't manipulate the key the redis_store gem sets as the session key as: + # redis:6379:2:2488646bef3bd44839efdb874fd133d578d729fb87fb004dd24a7e91cb3a3d62 + # + def invalidate_other_user_sessions(omniauth_hash:) + redis_sessions = RedisSessions.redis + redis_session_lookup = RedisSessionLookup.redis + dfe_sign_in_uid = omniauth_hash.fetch("uid") + session_lookup_key = "user_dsi_id:#{dfe_sign_in_uid}" + + existing_redis_session_key = redis_session_lookup.get(session_lookup_key) + if existing_redis_session_key + redis_sessions.del("session:#{existing_redis_session_key}") + end + + redis_session_lookup.set(session_lookup_key, session.id.private_id) + end end diff --git a/config/initializers/redis_session_lookup.rb b/config/initializers/redis_session_lookup.rb new file mode 100644 index 000000000..c5a08714a --- /dev/null +++ b/config/initializers/redis_session_lookup.rb @@ -0,0 +1,14 @@ +module RedisSessionLookup + class << self + def redis + @redis ||= if Rails.env.test? + MockRedis.new + else + Redis::Namespace.new( + "session_lookup", + redis: Redis.new(url: ENV["REDIS_URL"], db: 0) + ) + end + end + end +end diff --git a/config/initializers/redis_sessions.rb b/config/initializers/redis_sessions.rb new file mode 100644 index 000000000..759de5896 --- /dev/null +++ b/config/initializers/redis_sessions.rb @@ -0,0 +1,11 @@ +module RedisSessions + class << self + def redis + @redis ||= if Rails.env.test? + MockRedis.new + else + Redis.new(url: ENV["REDIS_URL"], db: 0) + end + end + end +end diff --git a/spec/models/user_session_spec.rb b/spec/models/user_session_spec.rb index 950d3acdf..868a2b78f 100644 --- a/spec/models/user_session_spec.rb +++ b/spec/models/user_session_spec.rb @@ -129,4 +129,63 @@ end end end + + describe "#invalidate_other_user_sessions" do + after(:each) do + RedisSessions.redis.flushdb + RedisSessionLookup.redis.flushdb + end + + let(:session) do + double(ActionDispatch::Request::Session, id: + double(Rack::Session::SessionId, private_id: "2::5347845262539")) + end + + let(:omniauth_hash) do + { + "uid" => "123", + "credentials" => { + "id_token" => "456" + } + } + end + + context "when no other session exists for that DfE Sign in user" do + it "does not delete any pre-existing session data from the Redis session store" do + user_session = described_class.new(session: session) + + fake_session_redis = MockRedis.new + allow(RedisSessions).to receive(:redis).and_return(fake_session_redis) + expect(fake_session_redis).not_to receive(:del) + + user_session.invalidate_other_user_sessions(omniauth_hash: omniauth_hash) + end + + it "adds a new session lookup" do + user_session = described_class.new(session: session) + + fake_session_lookup_redis = MockRedis.new + allow(RedisSessionLookup).to receive(:redis).and_return(fake_session_lookup_redis) + expect(fake_session_lookup_redis).to receive(:set).with("user_dsi_id:123", "2::5347845262539") + + user_session.invalidate_other_user_sessions(omniauth_hash: omniauth_hash) + end + end + + context "when a session already exists for that DfE Sign in user" do + it "deletes the pre-existing session data" do + RedisSessions.redis.set("session:2::5347845262539", + Marshal.dump({"_csrf_token" => "1", "dfe_sign_in_uid" => "123"})) + RedisSessionLookup.redis.set("user_dsi_id:123", "2::5347845262539") + + user_session = described_class.new(session: session) + + fake_session_redis = MockRedis.new + allow(RedisSessions).to receive(:redis).and_return(fake_session_redis) + expect(fake_session_redis).to receive(:del).with("session:2::5347845262539") + + user_session.invalidate_other_user_sessions(omniauth_hash: omniauth_hash) + end + end + end end diff --git a/spec/requests/authentication_spec.rb b/spec/requests/authentication_spec.rb index f58d705d5..500ecc255 100644 --- a/spec/requests/authentication_spec.rb +++ b/spec/requests/authentication_spec.rb @@ -104,4 +104,36 @@ end end end + + describe "Concurrent sign ins" do + context "when a DSI user is already signed in" do + after(:each) do + RedisSessions.redis.flushdb + RedisSessionLookup.redis.flushdb + end + + before(:each) do + # Simulate what session_store does with a new session + RedisSessions.redis.set("redis:6379::2:1098345703928457320948572304", + Marshal.dump({"_csrf_token" => "1", "dfe_sign_in_uid" => "123"})) + + # Simulate how we create a session lookup store + RedisSessionLookup.redis.set("user_dsi_id:123", "2::1098345703928457320948572304") + end + + it "destroys the previous users session so they have to authenticate again" do + user_exists_in_dfe_sign_in(dsi_uid: "123") + + mock_redis = MockRedis.new + allow(RedisSessions).to receive(:redis).and_return(mock_redis) + expect(mock_redis).to receive(:del) + .with("session:2::1098345703928457320948572304") + .and_return(0) + + get auth_dfe_callback_path + + expect(response).to have_http_status(:found) + end + end + end end From c262d682c47adadb2a21f778b298e846f5bbec94 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 22 Apr 2021 10:40:11 +0000 Subject: [PATCH 09/32] Bump bootsnap from 1.7.3 to 1.7.4 Bumps [bootsnap](https://github.com/Shopify/bootsnap) from 1.7.3 to 1.7.4. - [Release notes](https://github.com/Shopify/bootsnap/releases) - [Changelog](https://github.com/Shopify/bootsnap/blob/master/CHANGELOG.md) - [Commits](https://github.com/Shopify/bootsnap/compare/v1.7.3...v1.7.4) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 978b21fbd..b9f09ba0a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -71,7 +71,7 @@ GEM rack (>= 0.9.0) bindata (2.4.8) bindex (0.8.1) - bootsnap (1.7.3) + bootsnap (1.7.4) msgpack (~> 1.0) brakeman (5.0.0) builder (3.2.4) From ee8286310f72263e786772118ede0aa442796458 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 10:48:51 +0100 Subject: [PATCH 10/32] Update documentation on DSI environment usage --- doc/dfe-sign-in.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/dfe-sign-in.md b/doc/dfe-sign-in.md index 020e2dfae..431f99af7 100644 --- a/doc/dfe-sign-in.md +++ b/doc/dfe-sign-in.md @@ -19,7 +19,7 @@ This service has numerous environments and each needs to be paired and configure | | Enabled | DfE Sign-in Env | DfE manage service | | :------------- | :----------: | :-------------------------------------------------------------------: | :----------------: | | Development | false | | | -| Staging | true | [test-integration](https://test-interactions.signin.education.gov.uk) | [manage](https://test-manage.signin.education.gov.uk/services/FD39DCFC-9B60-46C4-ACDC-699A2468B46F/service-configuration) -| Research | false | | | -| Preview | false | | | -| Production | false | | | +| Staging | true | [pre-prod](https://pp-services.signin.education.gov.uk/) | [manage](https://pp-manage.signin.education.gov.uk/services/00487750-C9B8-414C-8746-1076885456E0/service-configuration) +| Research | true | [pre-prod](https://pp-services.signin.education.gov.uk/) | [manage](https://pp-manage.signin.education.gov.uk/services/00487750-C9B8-414C-8746-1076885456E0/service-configuration) +| Preview | true | [pre-prod](https://pp-services.signin.education.gov.uk/) | [manage](https://pp-manage.signin.education.gov.uk/services/00487750-C9B8-414C-8746-1076885456E0/service-configuration) +| Production | true | [prod](https://manage.signin.education.gov.uk) | [manage](https://manage.signin.education.gov.uk/services/9D1B3879-3495-4D3F-AB7A-ED9B8E968EFF/service-configuration) | From 4db8289b9c558a19a1c30ca60ce0e4570bc246c8 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Wed, 21 Apr 2021 10:35:29 +0100 Subject: [PATCH 11/32] Document how to find application logs --- README.md | 1 + doc/logging.md | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 doc/logging.md diff --git a/README.md b/README.md index 86c7c83b4..d13e3475b 100644 --- a/README.md +++ b/README.md @@ -7,6 +7,7 @@ A service to help school buying professionals create tender documents that compl 1. [Managing environment variables](/doc/managing-environment-variables.md) 1. [DfE Sign-in](/doc/dfe-sign-in.md) 1. [Console access](/doc/console-access.md) +1. [Logging](/doc/logging.md) ## Making changes diff --git a/doc/logging.md b/doc/logging.md new file mode 100644 index 000000000..b4e1ae8e2 --- /dev/null +++ b/doc/logging.md @@ -0,0 +1,43 @@ +# Logging + +## Logit + +Logs for all live environments are aggregated in the [Logit serivce](https://dashboard.logit.io). + +An existing developer should be able to invite you. + +Environment variables are used to tell the infrastructure which log stream URL to send logs to. These can be changed in GitHub Secrets within the repository: + +``` +PROD_TF_VAR_SYSLOG_DRAIN_URL= +STAGING_TF_VAR_SYSLOG_DRAIN_URL= +PREVIEW_TF_VAR_SYSLOG_DRAIN_URL= +RESEARCH_TF_VAR_SYSLOG_DRAIN_URL= +``` + +Remember to deploy the application again to propagate environment variable changes. + +## GPaaS + +If Logit isn't working you can ask GPaaS to view the logs for individual apps using the Cloud Foundry CLI. + +Log in: +``` +$ cf login -a api.london.cloud.service.gov.uk -u REDACTED@digital.education.gov.uk +``` + +Select the intended space: +``` +$ cf spaces +$ cf target -s sct-staging +``` + +Find the app name for the process you want to see logs for: +``` +$ cf apps +``` + +Ask for the logs: +``` +$ cf logs buy-for-your-school-research +``` From 40764dea616282b5faadfbebcafbf2efd2cdede0 Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Tue, 6 Apr 2021 12:28:06 +0100 Subject: [PATCH 12/32] Fetch Tasks from Contentful & create them in the database When creating a new Journey and fetching the Sections & Steps from Contentful, also fetch any Tasks associated with a Section and create them in the database. Included in this commit is the new Task model, and a CreateTask service. --- CHANGELOG.md | 3 +++ app/services/create_journey.rb | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fa36e68f..a4534fb77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ The format is based on [Keep a Changelog 1.0.0]. - prevent concurrent sign ins +- Create Task model; fetch tasks from Contentful and create them in the + database. + ## [release-009] - 2021-05-21 - fix multiple specification fields diff --git a/app/services/create_journey.rb b/app/services/create_journey.rb index 85eb04043..1bd3702fb 100644 --- a/app/services/create_journey.rb +++ b/app/services/create_journey.rb @@ -36,6 +36,13 @@ def call end end + contentful_sections.each do |section| + tasks = GetTasksFromSection.new(section: section).call + tasks.each do |task| + CreateTask.new(section: section, contentful_task: task) + end + end + journey end From b90a32359a4b741ddf869133fda650b00a3a010e Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Mon, 19 Apr 2021 11:01:10 +0100 Subject: [PATCH 13/32] Create assn. between Steps & Tasks, remove assn. between Journey & Steps A step can belong to a task, and a task can have many steps. Steps are no longer directly associated with Journeys. This is a substantial refactor that could not be broken down into smaller commits unfortunately! Removing the direct association between Journey and Steps has changed the signature of many of the modeal creation services. We have managed to preserve the Journey#visible_steps association through multiple `through` associations. Note that CreateJourneyService contains an `if` block (denoted with a comment) which exists solely to support the fact that many (most) of our current fixtures do not have their steps nested within tasks. Once we update the fixtures to be more reflective of Contentful, we can remove this `if` block. --- CHANGELOG.md | 2 + app/controllers/journeys_controller.rb | 2 +- app/controllers/preview/entries_controller.rb | 4 +- app/controllers/specifications_controller.rb | 10 +-- app/models/journey.rb | 10 ++- app/models/section.rb | 3 +- app/models/step.rb | 6 +- app/models/task.rb | 1 + app/services/create_journey.rb | 42 ++++++++----- app/services/create_journey_step.rb | 8 +-- app/services/create_task.rb | 10 ++- ...419082411_add_task_association_to_steps.rb | 7 +++ db/schema.rb | 11 ++-- spec/factories/step.rb | 2 +- spec/factories/task.rb | 7 +++ ...nyone_can_sign_in_with_dfe_sign_in_spec.rb | 5 +- spec/helpers/journey_helper_spec.rb | 22 ++++--- spec/models/journey_spec.rb | 23 ++++--- spec/models/step_spec.rb | 2 +- spec/models/task_spec.rb | 1 + spec/services/create_journey_step_spec.rb | 63 +++++++++++++++---- spec/services/delete_stale_journeys_spec.rb | 5 +- spec/services/get_answers_for_steps_spec.rb | 6 +- spec/services/save_answer_spec.rb | 5 +- spec/services/toggle_additional_steps_spec.rb | 62 +++++++++--------- 25 files changed, 204 insertions(+), 115 deletions(-) create mode 100644 db/migrate/20210419082411_add_task_association_to_steps.rb create mode 100644 spec/factories/task.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index a4534fb77..8bcba5a74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ The format is based on [Keep a Changelog 1.0.0]. - Create Task model; fetch tasks from Contentful and create them in the database. +- Break direct association between Journey and Steps. Create new association + between Tasks and Steps. ## [release-009] - 2021-05-21 diff --git a/app/controllers/journeys_controller.rb b/app/controllers/journeys_controller.rb index da6276b6e..46a5581d8 100644 --- a/app/controllers/journeys_controller.rb +++ b/app/controllers/journeys_controller.rb @@ -7,7 +7,7 @@ class JourneysController < ApplicationController render "errors/specification_template_invalid", status: 500, locals: {error: exception} end - rescue_from CreateJourneyStep::UnexpectedContentfulModel do |exception| + rescue_from CreateJourneyStep::UnexpectedContentfulModel, CreateTask::UnexpectedContentfulModel do |exception| render "errors/unexpected_contentful_model", status: 500 end diff --git a/app/controllers/preview/entries_controller.rb b/app/controllers/preview/entries_controller.rb index aebefe3f9..faaad3f99 100644 --- a/app/controllers/preview/entries_controller.rb +++ b/app/controllers/preview/entries_controller.rb @@ -7,10 +7,12 @@ def show user: current_user, liquid_template: "

N/A

" ) + section = Section.create(title: "Preview Section", journey: @journey) + task = Task.create(title: "Preview Task", section: section) contentful_entry = GetEntry.new(entry_id: entry_id).call @step = CreateJourneyStep.new( - journey: @journey, contentful_entry: contentful_entry + task: task, contentful_entry: contentful_entry ).call redirect_to journey_step_path(@journey, @step) diff --git a/app/controllers/specifications_controller.rb b/app/controllers/specifications_controller.rb index 3464f9d08..99b066a16 100644 --- a/app/controllers/specifications_controller.rb +++ b/app/controllers/specifications_controller.rb @@ -5,15 +5,7 @@ class SpecificationsController < ApplicationController def show @journey = current_journey - @visible_steps = @journey.visible_steps.includes([ - :radio_answer, - :short_text_answer, - :long_text_answer, - :single_date_answer, - :checkbox_answers, - :number_answer, - :currency_answer - ]) + @visible_steps = @journey.visible_steps @step_presenters = @visible_steps.map { |step| StepPresenter.new(step) } specification_renderer = SpecificationRenderer.new( diff --git a/app/models/journey.rb b/app/models/journey.rb index 1dea375b1..8f17cb42e 100644 --- a/app/models/journey.rb +++ b/app/models/journey.rb @@ -1,12 +1,16 @@ class Journey < ApplicationRecord self.implicit_order_column = "created_at" - has_many :steps - has_many :sections - has_many :visible_steps, -> { where(steps: {hidden: false}) }, class_name: "Step" + has_many :sections, dependent: :destroy + has_many :tasks, through: :sections, class_name: "Task" + has_many :steps, through: :tasks, class_name: "Step" belongs_to :user validates :category, :liquid_template, presence: true + def visible_steps + steps.where(hidden: false) + end + def all_steps_completed? visible_steps.all? { |step| step.answer.present? } end diff --git a/app/models/section.rb b/app/models/section.rb index a4f71545d..0ab4a8d53 100644 --- a/app/models/section.rb +++ b/app/models/section.rb @@ -1,7 +1,8 @@ class Section < ApplicationRecord self.implicit_order_column = "created_at" belongs_to :journey - has_many :tasks + has_many :tasks, dependent: :destroy + has_many :steps, through: :tasks, class_name: "Step" validates :title, :contentful_id, presence: true end diff --git a/app/models/step.rb b/app/models/step.rb index 2ee714bcc..d27f8e0d1 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -1,6 +1,6 @@ class Step < ApplicationRecord self.implicit_order_column = "created_at" - belongs_to :journey + belongs_to :task has_one :radio_answer has_one :short_text_answer @@ -33,4 +33,8 @@ def primary_call_to_action_text def skippable? skip_call_to_action_text.present? end + + def journey + task.section.journey + end end diff --git a/app/models/task.rb b/app/models/task.rb index 4708e9ac3..143f1cf48 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -1,6 +1,7 @@ class Task < ApplicationRecord self.implicit_order_column = "created_at" belongs_to :section + has_many :steps, dependent: :destroy validates :title, :contentful_id, presence: true end diff --git a/app/services/create_journey.rb b/app/services/create_journey.rb index 1bd3702fb..147b47d61 100644 --- a/app/services/create_journey.rb +++ b/app/services/create_journey.rb @@ -21,28 +21,36 @@ def call journey.save! contentful_sections.each do |contentful_section| - CreateSection.new(journey: journey, contentful_section: contentful_section).call + section = CreateSection.new(journey: journey, contentful_section: contentful_section).call - question_entries = GetStepsFromSection.new(section: contentful_section).call - question_entries.each do |entry| - CreateJourneyStep.new( - journey: journey, contentful_entry: entry - ).call - end + # The entire `if` block here exists to support our fixtures which do not nest steps + # within tasks. Once we update our fixtures to the "new way" of doing things, this + # entire `if` block can be discarded. + if contentful_section.respond_to?(:tasks) && !contentful_section.tasks.any? + question_entries = GetStepsFromSection.new(section: contentful_section).call + question_entries.each do |entry| + fake_contentful_task = OpenStruct.new(title: entry.title, id: entry.id) + task = CreateTask.new(section: section, contentful_task: fake_contentful_task).call - tasks = GetTasksFromSection.new(section: contentful_section).call - tasks.each do |task| - CreateTask.new(section: contentful_section, contentful_task: task) - end - end + CreateJourneyStep.new( + task: task, contentful_entry: entry + ).call + end + else + contentful_tasks = GetTasksFromSection.new(section: contentful_section).call + contentful_tasks.each do |contentful_task| + task = CreateTask.new(section: section, contentful_task: contentful_task).call - contentful_sections.each do |section| - tasks = GetTasksFromSection.new(section: section).call - tasks.each do |task| - CreateTask.new(section: section, contentful_task: task) + question_entries = GetStepsFromTask(task: contentful_task).call + question_entries.each do |entry| + CreateJourneyStep.new( + contentful_entry: entry, task: task + ).call + end + end end end - + journey end diff --git a/app/services/create_journey_step.rb b/app/services/create_journey_step.rb index b2354254d..e59308180 100644 --- a/app/services/create_journey_step.rb +++ b/app/services/create_journey_step.rb @@ -15,9 +15,9 @@ class UnexpectedContentfulStepType < StandardError; end currency ].freeze - attr_accessor :journey, :contentful_entry - def initialize(journey:, contentful_entry:) - self.journey = journey + attr_accessor :task, :contentful_entry + def initialize(task:, contentful_entry:) + self.task = task self.contentful_entry = contentful_entry end @@ -45,7 +45,7 @@ def call hidden: hidden, additional_step_rules: additional_step_rules, raw: raw, - journey: journey + task: task ) end diff --git a/app/services/create_task.rb b/app/services/create_task.rb index 5f7d543ed..44e89a74d 100644 --- a/app/services/create_task.rb +++ b/app/services/create_task.rb @@ -1,4 +1,6 @@ class CreateTask + class UnexpectedContentfulModel < StandardError; end + attr_accessor :section, :contentful_task def initialize(section:, contentful_task:) @@ -8,7 +10,11 @@ def initialize(section:, contentful_task:) def call task = Task.new(section: section, title: contentful_task.title, contentful_id: contentful_task.id) - task.save! - task + begin + task.save! + task + rescue ActiveRecord::RecordInvalid + raise UnexpectedContentfulModel + end end end diff --git a/db/migrate/20210419082411_add_task_association_to_steps.rb b/db/migrate/20210419082411_add_task_association_to_steps.rb new file mode 100644 index 000000000..4b3104675 --- /dev/null +++ b/db/migrate/20210419082411_add_task_association_to_steps.rb @@ -0,0 +1,7 @@ +class AddTaskAssociationToSteps < ActiveRecord::Migration[6.1] + def change + add_column :steps, :task_id, :uuid + add_index :steps, :task_id + remove_column :steps, :journey_id, :uuid + end +end diff --git a/db/schema.rb b/db/schema.rb index 95f44eb2b..42d48e356 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: 2021_04_07_111600) do +ActiveRecord::Schema.define(version: 2021_04_19_082411) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -42,11 +42,11 @@ t.jsonb "liquid_template", null: false t.jsonb "section_groups" t.uuid "user_id" - t.index ["user_id"], name: "index_journeys_on_user_id" t.boolean "started", default: true - t.index ["started"], name: "index_journeys_on_started" t.datetime "last_worked_on" t.index ["last_worked_on"], name: "index_journeys_on_last_worked_on" + t.index ["started"], name: "index_journeys_on_started" + t.index ["user_id"], name: "index_journeys_on_user_id" end create_table "long_text_answers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| @@ -100,7 +100,6 @@ end create_table "steps", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| - t.uuid "journey_id" t.string "title", null: false t.string "help_text" t.string "contentful_type", null: false @@ -115,7 +114,8 @@ t.boolean "hidden", default: false t.jsonb "additional_step_rules" t.string "skip_call_to_action_text" - t.index ["journey_id"], name: "index_steps_on_journey_id" + t.uuid "task_id" + t.index ["task_id"], name: "index_steps_on_task_id" end create_table "tasks", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| @@ -136,5 +136,4 @@ add_foreign_key "long_text_answers", "steps", on_delete: :cascade add_foreign_key "radio_answers", "steps", on_delete: :cascade add_foreign_key "short_text_answers", "steps", on_delete: :cascade - add_foreign_key "steps", "journeys", on_delete: :cascade end diff --git a/spec/factories/step.rb b/spec/factories/step.rb index c2d75527f..c184f47a2 100644 --- a/spec/factories/step.rb +++ b/spec/factories/step.rb @@ -9,7 +9,7 @@ primary_call_to_action_text { nil } skip_call_to_action_text { nil } - association :journey, factory: :journey + association :task, factory: :task trait :radio do options { [{"value" => "Red"}, {"value" => "Green"}, {"value" => "Blue"}] } diff --git a/spec/factories/task.rb b/spec/factories/task.rb new file mode 100644 index 000000000..fe11ca70b --- /dev/null +++ b/spec/factories/task.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :task do + title { "Task title" } + contentful_id { "5m26U35YLau4cOaJq6FXZA" } + association :section + end +end diff --git a/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb b/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb index 92f7f8f6f..ea904a744 100644 --- a/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb +++ b/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb @@ -27,8 +27,9 @@ # Undo the OmniAuth stub to check we don't require it again OmniAuth.config.mock_auth[:dfe] = OmniAuth::AuthHash.new(foo: :bar) - journey = create(:journey, user: user) - step = create(:step, :radio, journey: journey) + step = create(:step, :radio) + journey = step.journey + journey.update(user: user) visit journey_step_path(journey, step) expect(page).to have_content(step.title) end diff --git a/spec/helpers/journey_helper_spec.rb b/spec/helpers/journey_helper_spec.rb index 1d4d7df67..27ab24c60 100644 --- a/spec/helpers/journey_helper_spec.rb +++ b/spec/helpers/journey_helper_spec.rb @@ -3,10 +3,12 @@ RSpec.describe JourneyHelper, type: :helper do describe "#section_group_with_steps" do it "returns an ordered array of steps" do - journey = create(:journey, section_groups: "", steps: []) - step_1 = StepPresenter.new(create(:step, :radio, journey: journey)) - step_2 = StepPresenter.new(create(:step, :long_text, journey: journey)) - step_3 = StepPresenter.new(create(:step, :short_text, journey: journey)) + journey = create(:journey, section_groups: "") + section = create(:section, journey: journey) + task = create(:task, section: section) + step_1 = StepPresenter.new(create(:step, :radio, task: task)) + step_2 = StepPresenter.new(create(:step, :long_text, task: task)) + step_3 = StepPresenter.new(create(:step, :short_text, task: task)) section_groups = [ { @@ -47,11 +49,13 @@ context "when the ordering we want does not match the order saved in the database" do it "the ordering defined by 'order' is preserved" do - journey = create(:journey, section_groups: "", steps: []) - step_1 = StepPresenter.new(create(:step, :radio, title: "First question", journey: journey)) - step_2 = StepPresenter.new(create(:step, :long_text, title: "Second question", journey: journey)) - step_3 = StepPresenter.new(create(:step, :short_text, title: "Third question", journey: journey)) - step_4 = StepPresenter.new(create(:step, :short_text, title: "Fourth question", journey: journey)) + journey = create(:journey, section_groups: "") + section = create(:section, journey: journey) + task = create(:task, section: section) + step_1 = StepPresenter.new(create(:step, :radio, title: "First question", task: task)) + step_2 = StepPresenter.new(create(:step, :long_text, title: "Second question", task: task)) + step_3 = StepPresenter.new(create(:step, :short_text, title: "Third question", task: task)) + step_4 = StepPresenter.new(create(:step, :short_text, title: "Fourth question", task: task)) section_groups = [ { diff --git a/spec/models/journey_spec.rb b/spec/models/journey_spec.rb index b4c1d1220..6ccd8a593 100644 --- a/spec/models/journey_spec.rb +++ b/spec/models/journey_spec.rb @@ -1,7 +1,6 @@ require "rails_helper" RSpec.describe Journey, type: :model do - it { should have_many(:steps) } it { should have_many(:sections) } describe "validations" do @@ -18,11 +17,13 @@ context "when all steps have been completed" do it "returns true" do journey = create(:journey) + section = create(:section, journey: journey) + task = create(:task, section: section) - step_1 = create(:step, :radio, journey: journey) + step_1 = create(:step, :radio, task: task) create(:radio_answer, step: step_1) - step_2 = create(:step, :radio, journey: journey) + step_2 = create(:step, :radio, task: task) create(:radio_answer, step: step_2) expect(journey.all_steps_completed?).to be true @@ -32,8 +33,10 @@ context "when no steps have been completed" do it "returns false " do journey = create(:journey) + section = create(:section, journey: journey) + task = create(:task, section: section) - create_list(:step, 2, :radio, journey: journey) + create_list(:step, 2, :radio, task: task) expect(journey.all_steps_completed?).to be false end @@ -42,11 +45,13 @@ context "when only some steps have been completed" do it "returns false" do journey = create(:journey) + section = create(:section, journey: journey) + task = create(:task, section: section) - step_1 = create(:step, :radio, journey: journey) + step_1 = create(:step, :radio, task: task) create(:radio_answer, step: step_1) - create(:step, :radio, journey: journey) + create(:step, :radio, task: task) expect(journey.all_steps_completed?).to be false end @@ -55,11 +60,13 @@ context "when there are uncompleted hidden steps" do it "ignores them and returns true" do journey = create(:journey) + section = create(:section, journey: journey) + task = create(:task, section: section) - step_1 = create(:step, :radio, journey: journey) + step_1 = create(:step, :radio, task: task) create(:radio_answer, step: step_1) - _hidden_step_without_an_answer = create(:step, :radio, journey: journey, hidden: true) + _hidden_step_without_an_answer = create(:step, :radio, task: task, hidden: true) expect(journey.all_steps_completed?).to be true end diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb index 22206faa5..ef173661e 100644 --- a/spec/models/step_spec.rb +++ b/spec/models/step_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" RSpec.describe Step, type: :model do - it { should belong_to(:journey) } + it { should belong_to(:task) } it { should have_one(:radio_answer) } it { should have_one(:short_text_answer) } it { should have_one(:long_text_answer) } diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 984d007a1..bc194dc38 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -2,6 +2,7 @@ RSpec.describe Task, type: :model do it { should belong_to(:section) } + it { should have_many(:steps) } describe "validations" do it { is_expected.to validate_presence_of(:title) } diff --git a/spec/services/create_journey_step_spec.rb b/spec/services/create_journey_step_spec.rb index 7f8e772c7..faafd99df 100644 --- a/spec/services/create_journey_step_spec.rb +++ b/spec/services/create_journey_step_spec.rb @@ -5,11 +5,14 @@ context "when the new step is of type step" do it "creates a local copy of the new step" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/radio-question.json" ) - step = described_class.new(journey: journey, contentful_entry: fake_entry).call + step = described_class.new(task: task, contentful_entry: fake_entry).call expect(step.title).to eq("Which service do you need?") expect(step.help_text).to eq("Tell us which service you need.") @@ -64,22 +67,28 @@ context "when the question is of type 'short_text'" do it "sets help_text and options to nil" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/short-text-question.json" ) - step = described_class.new(journey: journey, contentful_entry: fake_entry).call + step = described_class.new(task: task, contentful_entry: fake_entry).call expect(step.options).to eq(nil) end it "replaces spaces with underscores" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/short-text-question.json" ) - step = described_class.new(journey: journey, contentful_entry: fake_entry).call + step = described_class.new(task: task, contentful_entry: fake_entry).call expect(step.contentful_type).to eq("short_text") end @@ -88,12 +97,15 @@ context "when the new entry has a body field" do it "updates the step with the body" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/static-content.json" ) step, _answer = described_class.new( - journey: journey, contentful_entry: fake_entry + task: task, contentful_entry: fake_entry ).call expect(step.body).to eq("Procuring a new catering contract can \ @@ -106,12 +118,15 @@ context "when the new entry has a 'primaryCallToAction' field" do it "updates the step with the body" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/primary-button.json" ) step, _answer = described_class.new( - journey: journey, contentful_entry: fake_entry + task: task, contentful_entry: fake_entry ).call expect(step.primary_call_to_action_text).to eq("Go onwards!") @@ -121,12 +136,15 @@ context "when no 'primaryCallToAction' is provided" do it "default copy is used for the button" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/no-primary-button.json" ) step, _answer = described_class.new( - journey: journey, contentful_entry: fake_entry + task: task, contentful_entry: fake_entry ).call expect(step.primary_call_to_action_text).to eq(I18n.t("generic.button.next")) @@ -136,12 +154,15 @@ context "when no 'skipCallToAction' is provided" do it "default copy is used for the button" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/skippable-checkboxes-question.json" ) step, _answer = described_class.new( - journey: journey, contentful_entry: fake_entry + task: task, contentful_entry: fake_entry ).call expect(step.skip_call_to_action_text).to eq("None of the above") @@ -151,12 +172,15 @@ context "when no 'alwaysShowTheUser' is provided" do it "default hidden to true" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/no-hidden-field.json" ) step, _answer = described_class.new( - journey: journey, contentful_entry: fake_entry + task: task, contentful_entry: fake_entry ).call expect(step.hidden).to eq(false) @@ -166,12 +190,15 @@ context "when 'showAdditionalQuestion' is provided" do it "stores the rule as JSON" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/show-one-additional-question.json" ) step, _answer = described_class.new( - journey: journey, contentful_entry: fake_entry + task: task, contentful_entry: fake_entry ).call expect(step.additional_step_rules).to eql([ @@ -186,16 +213,21 @@ context "when the new entry has an unexpected content model" do it "raises an error" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/unexpected-contentful-type.json" ) - expect { described_class.new(journey: journey, contentful_entry: fake_entry).call } + expect { described_class.new(task: task, contentful_entry: fake_entry).call } .to raise_error(CreateJourneyStep::UnexpectedContentfulModel) end it "raises a rollbar event" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/unexpected-contentful-type.json" @@ -212,7 +244,7 @@ allowed_content_models: CreateJourneyStep::ALLOWED_CONTENTFUL_MODELS.join(", "), allowed_step_types: CreateJourneyStep::ALLOWED_CONTENTFUL_ENTRY_TYPES.join(", ")) .and_call_original - expect { described_class.new(journey: journey, contentful_entry: fake_entry).call } + expect { described_class.new(task: task, contentful_entry: fake_entry).call } .to raise_error(CreateJourneyStep::UnexpectedContentfulModel) end end @@ -220,16 +252,21 @@ context "when the new step has an unexpected step type" do it "raises an error" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) + fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/unexpected-contentful-question-type.json" ) - expect { described_class.new(journey: journey, contentful_entry: fake_entry).call } + expect { described_class.new(task: task, contentful_entry: fake_entry).call } .to raise_error(CreateJourneyStep::UnexpectedContentfulStepType) end it "raises a rollbar event" do journey = create(:journey, :catering) + section = create(:section, journey: journey) + task = create(:task, section: section) fake_entry = fake_contentful_step_or_task( contentful_fixture_filename: "steps/unexpected-contentful-question-type.json" @@ -246,7 +283,7 @@ allowed_content_models: CreateJourneyStep::ALLOWED_CONTENTFUL_MODELS.join(", "), allowed_step_types: CreateJourneyStep::ALLOWED_CONTENTFUL_ENTRY_TYPES.join(", ")) .and_call_original - expect { described_class.new(journey: journey, contentful_entry: fake_entry).call } + expect { described_class.new(task: task, contentful_entry: fake_entry).call } .to raise_error(CreateJourneyStep::UnexpectedContentfulStepType) end end diff --git a/spec/services/delete_stale_journeys_spec.rb b/spec/services/delete_stale_journeys_spec.rb index aafe4cb5f..22bf8699a 100644 --- a/spec/services/delete_stale_journeys_spec.rb +++ b/spec/services/delete_stale_journeys_spec.rb @@ -6,8 +6,9 @@ describe "#call" do it "destroys a journey and all associated records" do - journey = create(:journey, started: false, last_worked_on: (1.month + 1.day).ago) - step = create(:step, :radio, journey: journey) + step = create(:step, :radio) + journey = step.journey + journey.update(started: false, last_worked_on: (1.month + 1.day).ago) _radio_answer = create(:radio_answer, step: step) _short_text_answer = create(:short_text_answer, step: step) diff --git a/spec/services/get_answers_for_steps_spec.rb b/spec/services/get_answers_for_steps_spec.rb index 57a4d2eba..f52403f17 100644 --- a/spec/services/get_answers_for_steps_spec.rb +++ b/spec/services/get_answers_for_steps_spec.rb @@ -195,9 +195,11 @@ context "when there is no answer for a step" do it "does not try to prepare that answer in the result" do journey = create(:journey) - answerable_step = create(:step, :radio, journey: journey) + section = create(:section, journey: journey) + task = create(:task, section: section) + answerable_step = create(:step, :radio, task: task) _answer = create(:radio_answer, step: answerable_step) - unanswerable_step = create(:step, :radio, journey: journey) + unanswerable_step = create(:step, :radio, task: task) result = described_class.new(visible_steps: [answerable_step]).call diff --git a/spec/services/save_answer_spec.rb b/spec/services/save_answer_spec.rb index 793912dbc..a12928a13 100644 --- a/spec/services/save_answer_spec.rb +++ b/spec/services/save_answer_spec.rb @@ -71,8 +71,9 @@ context "when the associated journey is unstarted" do it "updates the journey to be started" do - journey = create(:journey, started: false) - step = create(:step, :radio, journey: journey) + step = create(:step, :radio) + journey = step.journey + journey.update(started: false) answer = create(:short_text_answer, step: step) _result = described_class.new(answer: answer).call(params: {}) diff --git a/spec/services/toggle_additional_steps_spec.rb b/spec/services/toggle_additional_steps_spec.rb index b8af60be1..5da9827b2 100644 --- a/spec/services/toggle_additional_steps_spec.rb +++ b/spec/services/toggle_additional_steps_spec.rb @@ -3,6 +3,8 @@ RSpec.describe ToggleAdditionalSteps do describe "#call" do let(:journey) { create(:journey) } + let(:section) { create(:section, journey: journey) } + let(:task) { create(:task, section: section) } context "when the additional_step_rules field is nil" do it "returns nil" do @@ -16,13 +18,13 @@ it "only shows the additional steps within the same journey" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "Red", "question_identifiers" => ["123"]}]) create(:radio_answer, step: step, response: "Red") another_step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: true) @@ -40,7 +42,7 @@ it "shows all additional_steps within the same journey" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{ "required_answer" => "Red", "question_identifiers" => ["123", "456"] @@ -49,13 +51,13 @@ first_additional_step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: true) second_additional_step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "456", hidden: true) @@ -75,13 +77,13 @@ it "updates the referenced step's hidden field to false" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "Red", "question_identifiers" => ["123"]}]) create(:radio_answer, step: step, response: "Red") step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: true) @@ -95,13 +97,13 @@ it "returns the updated step" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "RED", "question_identifiers" => ["123"]}]) create(:radio_answer, step: step, response: "red") step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: true) @@ -115,14 +117,14 @@ it "shows both connected questions" do first_step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "Red", "question_identifiers" => ["123"]}], hidden: false) create(:radio_answer, step: first_step, response: "Red") second_step = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", additional_step_rules: [{"required_answer" => "Blue", "question_identifiers" => ["456"]}], hidden: true) @@ -130,7 +132,7 @@ third_step = create(:step, :radio, - journey: journey, + task: task, contentful_id: "456", hidden: true) @@ -146,7 +148,7 @@ it "should hide itself and all connected branches" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [ {"required_answer" => "Red", "question_identifiers" => ["123"]}, {"required_answer" => "Blue", "question_identifiers" => ["456"]} @@ -157,7 +159,7 @@ first_step_to_hide = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", additional_step_rules: [ {"required_answer" => "Yellow", "question_identifiers" => ["8"]}, @@ -167,14 +169,14 @@ second_step_to_hide = create(:step, :radio, - journey: journey, + task: task, contentful_id: "8", additional_step_rules: [], hidden: false) third_step_to_hide = create(:step, :radio, - journey: journey, + task: task, contentful_id: "9", additional_step_rules: [], hidden: false) @@ -191,7 +193,7 @@ it "continues to show the next step (rather than hiding it again when it doesn't match the second rule)" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [ {"required_answer" => "red", "question_identifiers" => ["123"]}, {"required_answer" => "blue", "question_identifiers" => ["123"]} @@ -200,7 +202,7 @@ step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: true) @@ -214,13 +216,13 @@ it "updates the referenced step's hidden field to true" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "Red", "question_identifiers" => ["123"]}]) create(:radio_answer, step: step, response: "Blue") step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: false) @@ -232,7 +234,7 @@ it "hides all additional_steps within the same journey" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{ "required_answer" => "Red", "question_identifiers" => ["123", "456"] @@ -241,13 +243,13 @@ first_additional_step_to_hide = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: false) second_additional_step_to_hide = create(:step, :radio, - journey: journey, + task: task, contentful_id: "456", hidden: false) @@ -267,13 +269,13 @@ it "does not hide the step" do step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "RED", "question_identifiers" => ["123"]}]) create(:radio_answer, step: step, response: "red") step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: false) @@ -287,14 +289,14 @@ it "hides both connected questions" do first_step = create(:step, :radio, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "Red", "question_identifiers" => ["123"]}], hidden: false) create(:radio_answer, step: first_step, response: "Changed from red") second_step = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", additional_step_rules: [{"required_answer" => "Blue", "question_identifiers" => ["456"]}], hidden: false) @@ -302,7 +304,7 @@ third_step = create(:step, :radio, - journey: journey, + task: task, contentful_id: "456", hidden: false) @@ -318,13 +320,13 @@ it "checks for a match against all answers" do step = create(:step, :checkbox_answers, - journey: journey, + task: task, additional_step_rules: [{"required_answer" => "Red", "question_identifiers" => ["123"]}]) create(:checkbox_answers, step: step, response: ["Blue", "Red"]) step_to_show = create(:step, :radio, - journey: journey, + task: task, contentful_id: "123", hidden: true) From fd18a20e7d998c4128eaf5123a17a6cc753c2f93 Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Mon, 19 Apr 2021 13:23:14 +0100 Subject: [PATCH 14/32] Add GetStepsFromTask service Now that Steps will be nested within Tasks in Contentful, we need to be able to get the Steps from a given task, using this service. This PR also includes a correction to an existing fixture checkboxes-task.json changing the "name" attribute to "title" (in line with other fixtures) and adding a fake contentful ID to the step. --- app/services/create_journey.rb | 2 +- app/services/get_steps_from_task.rb | 19 ++++++++++++++++++ .../contentful/tasks/checkboxes-task.json | 4 ++-- spec/services/get_steps_from_task_spec.rb | 20 +++++++++++++++++++ spec/support/contentful_helpers.rb | 19 ++++++++++++++++++ 5 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 app/services/get_steps_from_task.rb create mode 100644 spec/services/get_steps_from_task_spec.rb diff --git a/app/services/create_journey.rb b/app/services/create_journey.rb index 147b47d61..3cafc7987 100644 --- a/app/services/create_journey.rb +++ b/app/services/create_journey.rb @@ -50,7 +50,7 @@ def call end end end - + journey end diff --git a/app/services/get_steps_from_task.rb b/app/services/get_steps_from_task.rb new file mode 100644 index 000000000..f17449f9d --- /dev/null +++ b/app/services/get_steps_from_task.rb @@ -0,0 +1,19 @@ +class GetStepsFromTask + attr_accessor :task + + def initialize(task:) + self.task = task + end + + def call + return [] unless task.respond_to?(:steps) + step_ids = [] + task.steps.each do |step| + step_ids << step.id + end + + step_ids.map { |entry_id| + GetEntry.new(entry_id: entry_id).call + } + end +end diff --git a/spec/fixtures/contentful/tasks/checkboxes-task.json b/spec/fixtures/contentful/tasks/checkboxes-task.json index 73cb68edf..7b6e73c49 100644 --- a/spec/fixtures/contentful/tasks/checkboxes-task.json +++ b/spec/fixtures/contentful/tasks/checkboxes-task.json @@ -32,13 +32,13 @@ "locale": "en-US" }, "fields": { - "name": "Task 1", + "title": "Task 1", "steps": [ { "sys": { "type": "Link", "linkType": "Entry", - "id": "1DqhwF2XkJJ0Um6NSweWlZ" + "id": "checkboxes-question" } } ] diff --git a/spec/services/get_steps_from_task_spec.rb b/spec/services/get_steps_from_task_spec.rb new file mode 100644 index 000000000..6c60ddc21 --- /dev/null +++ b/spec/services/get_steps_from_task_spec.rb @@ -0,0 +1,20 @@ +require "rails_helper" + +RSpec.describe GetStepsFromTask do + describe "#call" do + it "returns the list of entry objects referenced by the step list" do + task = fake_contentful_section( + contentful_fixture_filename: "tasks/checkboxes-task.json" + ) + stub_contentful_task_steps(tasks: [task]) + + result = described_class.new(task: task).call + + expect(result).to be_kind_of(Array) + # INFO: We should test this is a Contentful::Entry however it wasn't + # possible to create an instance_double due to an unusual way the object + # is constructed within the gem. Creating the object seems overly complex. + expect(result.first.id).to eq("checkboxes-question") + end + end +end diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index df3dcc078..2c53aea82 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -109,6 +109,25 @@ def stub_contentful_section_tasks( end end + def stub_contentful_task_steps( + tasks:, + contentful_connector: instance_double(ContentfulConnector) + ) + allow(ContentfulConnector).to receive(:new) + .and_return(contentful_connector) + + tasks.each do |task| + fake_steps = task.steps.map { |step| + fake_step = fake_contentful_step_or_task(contentful_fixture_filename: "steps/#{step.id}.json") + expect(contentful_connector).to receive(:get_entry_by_id) + .with(fake_step.id) + .and_return(fake_step) + fake_step + } + allow(task).to receive(:steps).and_return(fake_steps) + end + end + def stub_contentful_connector contentful_connector = instance_double(ContentfulConnector) expect(ContentfulConnector).to receive(:new) From 0d2487e71e411bf1d65c01caa26760645b536ed5 Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Wed, 21 Apr 2021 14:09:56 +0100 Subject: [PATCH 15/32] Show Task list pattern on the Journey page This commit changes the journey show page substantially under the hood. If a Section has tasks which contain multiple steps, the task name will be shown in the section, instead of the step (question). If a task only contains one step, the step will be shown without an encapsulating task, to reduce clicks in the UI. This also encures our existing tests using the old, task-less, fixtures still work. --- app/controllers/journeys_controller.rb | 11 +--- app/models/task.rb | 4 ++ app/services/create_journey.rb | 2 +- app/views/journeys/_step_list.html.erb | 19 ------- app/views/journeys/show.html.erb | 33 ++++++++++-- .../view_a_list_of_tasks_spec.rb | 54 +++++++++++++++++++ .../section-with-multiple-tasks.json | 44 +++++++++++++++ .../section-with-single-hidden-task.json | 44 +++++++++++++++ .../categories/section-with-single-task.json | 44 +++++++++++++++ .../sections/hidden-task-section.json | 41 ++++++++++++++ .../sections/multiple-tasks-section.json | 49 +++++++++++++++++ ...-section.json => single-task-section.json} | 2 +- .../tasks/checkboxes-and-radio-task.json | 53 ++++++++++++++++++ .../contentful/tasks/checkboxes-task.json | 2 +- .../contentful/tasks/hidden-task.json | 46 ++++++++++++++++ spec/models/task_spec.rb | 10 ++++ spec/services/create_journey_step_spec.rb | 26 ++++----- spec/services/get_tasks_from_section_spec.rb | 2 +- spec/support/contentful_helpers.rb | 37 ++++++++++--- spec/support/journey_helpers.rb | 11 ++++ 20 files changed, 478 insertions(+), 56 deletions(-) delete mode 100644 app/views/journeys/_step_list.html.erb create mode 100644 spec/fixtures/contentful/categories/section-with-multiple-tasks.json create mode 100644 spec/fixtures/contentful/categories/section-with-single-hidden-task.json create mode 100644 spec/fixtures/contentful/categories/section-with-single-task.json create mode 100644 spec/fixtures/contentful/sections/hidden-task-section.json create mode 100644 spec/fixtures/contentful/sections/multiple-tasks-section.json rename spec/fixtures/contentful/sections/{tasks-section.json => single-task-section.json} (94%) create mode 100644 spec/fixtures/contentful/tasks/checkboxes-and-radio-task.json create mode 100644 spec/fixtures/contentful/tasks/hidden-task.json diff --git a/app/controllers/journeys_controller.rb b/app/controllers/journeys_controller.rb index 46a5581d8..f78a6eafd 100644 --- a/app/controllers/journeys_controller.rb +++ b/app/controllers/journeys_controller.rb @@ -30,16 +30,7 @@ def new def show @journey = current_journey - @visible_steps = @journey.visible_steps.includes([ - :radio_answer, - :short_text_answer, - :long_text_answer, - :single_date_answer, - :checkbox_answers, - :number_answer, - :currency_answer - ]) - @step_presenters = @visible_steps.map { |step| StepPresenter.new(step) } + @sections = @journey.sections.includes(:tasks) end private diff --git a/app/models/task.rb b/app/models/task.rb index 143f1cf48..55f695f1f 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -4,4 +4,8 @@ class Task < ApplicationRecord has_many :steps, dependent: :destroy validates :title, :contentful_id, presence: true + + def visible_steps + steps.where(hidden: false) + end end diff --git a/app/services/create_journey.rb b/app/services/create_journey.rb index 3cafc7987..6b3d15946 100644 --- a/app/services/create_journey.rb +++ b/app/services/create_journey.rb @@ -41,7 +41,7 @@ def call contentful_tasks.each do |contentful_task| task = CreateTask.new(section: section, contentful_task: contentful_task).call - question_entries = GetStepsFromTask(task: contentful_task).call + question_entries = GetStepsFromTask.new(task: contentful_task).call question_entries.each do |entry| CreateJourneyStep.new( contentful_entry: entry, task: task diff --git a/app/views/journeys/_step_list.html.erb b/app/views/journeys/_step_list.html.erb deleted file mode 100644 index 318223bed..000000000 --- a/app/views/journeys/_step_list.html.erb +++ /dev/null @@ -1,19 +0,0 @@ -
    - <% steps.each do |step| %> - <% if step.question? %> -
  • - <% if step.answer %> - - <%= link_to step.title, edit_journey_step_path(journey, step), class: "govuk-link", 'aria-describedby': step.id + "-status"%> - - "><%= I18n.t("task_list.status.completed") %> - <% else %> - - <%= link_to step.title, journey_step_path(journey, step), class: "govuk-link", 'aria-describedby': step.id + "-status"%> - - "><%= I18n.t("task_list.status.not_started") %> - <% end %> -
    • - <% end %> - <% end %> -
diff --git a/app/views/journeys/show.html.erb b/app/views/journeys/show.html.erb index ab5c5d4aa..572b11d12 100644 --- a/app/views/journeys/show.html.erb +++ b/app/views/journeys/show.html.erb @@ -4,10 +4,37 @@

<%= I18n.t("specifying.start_page.page_title") %>

    - <% section_group_with_steps(journey: @journey, steps: @step_presenters).each do |grouping| %> -

    <%= grouping["title"] %>

    + <% @sections.each do |section| %> +

    <%= section.title %>

  1. - <%= render "step_list", journey: @journey, steps: grouping["steps"] %> +
      + <% section.tasks.each do |task| %> + <% if task.visible_steps.count == 1 %> + <% step = StepPresenter.new(task.visible_steps.first) %> + <% if step.question? %> +
    • + <% if step.answer %> + + <%= link_to step.title, edit_journey_step_path(@journey, step), class: "govuk-link", 'aria-describedby': step.id + "-status"%> + + "><%= I18n.t("task_list.status.completed") %> + <% else %> + + <%= link_to step.title, journey_step_path(@journey, step), class: "govuk-link", 'aria-describedby': step.id + "-status"%> + + "><%= I18n.t("task_list.status.not_started") %> + <% end %> +
      • + <% end %> + <% elsif task.visible_steps.count > 1 %> +
    • + + <%= link_to task.title, journey_task_path(@journey, task), class: "govuk-link", 'aria-describedby': task.id + "-status"%> + +
      • + <% end %> + <% end %> +
    2. <% end %>
diff --git a/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb b/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb index d509dcfbc..5d0932989 100644 --- a/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb +++ b/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb @@ -68,4 +68,58 @@ expect(page).to have_content("You should be able to see this question") end end + + context "When the sections & tasks are retrieved from the database (new task list process)" do + context "When there is a task with a single step" do + it "shows the section title" do + start_journey_with_tasks_from_category(category: "section-with-single-task.json") + within(".app-task-list") do + expect(page).to have_content("Section with a single task") + end + end + + it "shows the step title, not the task title" do + start_journey_with_tasks_from_category(category: "section-with-single-task.json") + + within(".app-task-list") do + expect(page).to have_content("Everyday services that are required and need to be considered") + expect(page).to_not have_content("Task with a single step") + end + end + end + + context "When there is a task with a single HIDDEN step" do + it "shows the section title" do + start_journey_with_tasks_from_category(category: "section-with-single-hidden-task.json") + within(".app-task-list") do + expect(page).to have_content("Section with a hidden task") + end + end + + it "does not show the task nor step title" do + start_journey_with_tasks_from_category(category: "section-with-single-hidden-task.json") + + within(".app-task-list") do + expect(page).to_not have_content("Task with a hidden step") + end + end + end + + context "When there is a task with multiple steps" do + it "shows the section title" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + within(".app-task-list") do + expect(page).to have_content("Section with multiple tasks") + end + end + + it "shows the task titles within the section" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + + within(".app-task-list") do + expect(page).to have_content("Task with multiple steps") + end + end + end + end end diff --git a/spec/fixtures/contentful/categories/section-with-multiple-tasks.json b/spec/fixtures/contentful/categories/section-with-multiple-tasks.json new file mode 100644 index 000000000..977cc82a9 --- /dev/null +++ b/spec/fixtures/contentful/categories/section-with-multiple-tasks.json @@ -0,0 +1,44 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "contentful-category-entry", + "type": "Entry", + "createdAt": "2021-01-20T12:19:10.220Z", + "updatedAt": "2021-01-20T15:06:12.067Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "category" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Catering", + "sections": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "multiple-tasks-section" + } + } + ], + "specificationTemplate": "

Liquid {{templating}}

" + } +} diff --git a/spec/fixtures/contentful/categories/section-with-single-hidden-task.json b/spec/fixtures/contentful/categories/section-with-single-hidden-task.json new file mode 100644 index 000000000..1d25d141c --- /dev/null +++ b/spec/fixtures/contentful/categories/section-with-single-hidden-task.json @@ -0,0 +1,44 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "contentful-category-entry", + "type": "Entry", + "createdAt": "2021-01-20T12:19:10.220Z", + "updatedAt": "2021-01-20T15:06:12.067Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "category" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Catering", + "sections": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hidden-task-section" + } + } + ], + "specificationTemplate": "

Liquid {{templating}}

" + } +} diff --git a/spec/fixtures/contentful/categories/section-with-single-task.json b/spec/fixtures/contentful/categories/section-with-single-task.json new file mode 100644 index 000000000..cc5819702 --- /dev/null +++ b/spec/fixtures/contentful/categories/section-with-single-task.json @@ -0,0 +1,44 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "contentful-category-entry", + "type": "Entry", + "createdAt": "2021-01-20T12:19:10.220Z", + "updatedAt": "2021-01-20T15:06:12.067Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "category" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Catering", + "sections": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "single-task-section" + } + } + ], + "specificationTemplate": "

Liquid {{templating}}

" + } +} diff --git a/spec/fixtures/contentful/sections/hidden-task-section.json b/spec/fixtures/contentful/sections/hidden-task-section.json new file mode 100644 index 000000000..d4add894d --- /dev/null +++ b/spec/fixtures/contentful/sections/hidden-task-section.json @@ -0,0 +1,41 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "tasks-section", + "type": "Entry", + "createdAt": "2021-04-12T10:41:36.073Z", + "updatedAt": "2021-04-12T10:42:51.119Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "section" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Section with a hidden task", + "tasks": [{ + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hidden-task" + } + }] + } +} diff --git a/spec/fixtures/contentful/sections/multiple-tasks-section.json b/spec/fixtures/contentful/sections/multiple-tasks-section.json new file mode 100644 index 000000000..b37814c63 --- /dev/null +++ b/spec/fixtures/contentful/sections/multiple-tasks-section.json @@ -0,0 +1,49 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "tasks-section", + "type": "Entry", + "createdAt": "2021-04-12T10:41:36.073Z", + "updatedAt": "2021-04-12T10:42:51.119Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "section" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Section with multiple tasks", + "tasks": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "checkboxes-task" + } + }, + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "checkboxes-and-radio-task" + } + }] + } +} diff --git a/spec/fixtures/contentful/sections/tasks-section.json b/spec/fixtures/contentful/sections/single-task-section.json similarity index 94% rename from spec/fixtures/contentful/sections/tasks-section.json rename to spec/fixtures/contentful/sections/single-task-section.json index dae1a6245..e2c0a698d 100644 --- a/spec/fixtures/contentful/sections/tasks-section.json +++ b/spec/fixtures/contentful/sections/single-task-section.json @@ -29,7 +29,7 @@ "locale": "en-US" }, "fields": { - "title": "Section with tasks", + "title": "Section with a single task", "tasks": [{ "sys": { "type": "Link", diff --git a/spec/fixtures/contentful/tasks/checkboxes-and-radio-task.json b/spec/fixtures/contentful/tasks/checkboxes-and-radio-task.json new file mode 100644 index 000000000..aa70ce43b --- /dev/null +++ b/spec/fixtures/contentful/tasks/checkboxes-and-radio-task.json @@ -0,0 +1,53 @@ +{ + "metadata": { + "tags": [] + }, + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "checkboxes-task", + "type": "Entry", + "createdAt": "2021-04-12T09:38:48.990Z", + "updatedAt": "2021-04-12T10:11:05.670Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 3, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "task" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Task with multiple steps", + "steps": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "checkboxes-question" + } + }, + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "radio-question" + } + } + ] + } +} diff --git a/spec/fixtures/contentful/tasks/checkboxes-task.json b/spec/fixtures/contentful/tasks/checkboxes-task.json index 7b6e73c49..e5a0603f4 100644 --- a/spec/fixtures/contentful/tasks/checkboxes-task.json +++ b/spec/fixtures/contentful/tasks/checkboxes-task.json @@ -32,7 +32,7 @@ "locale": "en-US" }, "fields": { - "title": "Task 1", + "title": "Task with a single step", "steps": [ { "sys": { diff --git a/spec/fixtures/contentful/tasks/hidden-task.json b/spec/fixtures/contentful/tasks/hidden-task.json new file mode 100644 index 000000000..f9ae5bdf0 --- /dev/null +++ b/spec/fixtures/contentful/tasks/hidden-task.json @@ -0,0 +1,46 @@ +{ + "metadata": { + "tags": [] + }, + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "checkboxes-task", + "type": "Entry", + "createdAt": "2021-04-12T09:38:48.990Z", + "updatedAt": "2021-04-12T10:11:05.670Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 3, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "task" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Hidden task", + "steps": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hidden-field" + } + } + ] + } +} diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index bc194dc38..17a355227 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -8,4 +8,14 @@ it { is_expected.to validate_presence_of(:title) } it { is_expected.to validate_presence_of(:contentful_id) } end + + describe "#visible_steps" do + it "only returns steps which are not hidden" do + task = create(:task) + step_1 = create(:step, :radio, hidden: false, task: task) + _step_2 = create(:step, :radio, hidden: true, task: task) + + expect(task.visible_steps).to eq [step_1] + end + end end diff --git a/spec/services/create_journey_step_spec.rb b/spec/services/create_journey_step_spec.rb index faafd99df..d32668b74 100644 --- a/spec/services/create_journey_step_spec.rb +++ b/spec/services/create_journey_step_spec.rb @@ -8,7 +8,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/radio-question.json" ) @@ -70,7 +70,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/short-text-question.json" ) @@ -84,7 +84,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/short-text-question.json" ) @@ -100,7 +100,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/static-content.json" ) @@ -121,7 +121,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/primary-button.json" ) @@ -139,7 +139,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/no-primary-button.json" ) @@ -157,7 +157,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/skippable-checkboxes-question.json" ) @@ -175,7 +175,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/no-hidden-field.json" ) @@ -193,7 +193,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/show-one-additional-question.json" ) @@ -216,7 +216,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/unexpected-contentful-type.json" ) @@ -229,7 +229,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/unexpected-contentful-type.json" ) @@ -255,7 +255,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/unexpected-contentful-question-type.json" ) @@ -268,7 +268,7 @@ section = create(:section, journey: journey) task = create(:task, section: section) - fake_entry = fake_contentful_step_or_task( + fake_entry = fake_contentful_step( contentful_fixture_filename: "steps/unexpected-contentful-question-type.json" ) diff --git a/spec/services/get_tasks_from_section_spec.rb b/spec/services/get_tasks_from_section_spec.rb index 8c62399bd..a31c49881 100644 --- a/spec/services/get_tasks_from_section_spec.rb +++ b/spec/services/get_tasks_from_section_spec.rb @@ -4,7 +4,7 @@ describe "#call" do it "returns the list of entry objects referenced by the task list" do section = fake_contentful_section( - contentful_fixture_filename: "sections/tasks-section.json" + contentful_fixture_filename: "sections/multiple-tasks-section.json" ) stub_contentful_section_tasks(sections: [section]) diff --git a/spec/support/contentful_helpers.rb b/spec/support/contentful_helpers.rb index 2c53aea82..e8ccd4bd3 100644 --- a/spec/support/contentful_helpers.rb +++ b/spec/support/contentful_helpers.rb @@ -4,7 +4,7 @@ def stub_contentful_entry( fixture_filename: "radio-question-example.json" ) contentful_connector = stub_contentful_connector - contentful_response = fake_contentful_step_or_task(contentful_fixture_filename: fixture_filename) + contentful_response = fake_contentful_step(contentful_fixture_filename: fixture_filename) allow(contentful_connector).to receive(:get_entry_by_id) .with(entry_id) .and_return(contentful_response) @@ -14,6 +14,7 @@ def stub_contentful_category( fixture_filename:, stub_sections: true, stub_steps: true, + stub_tasks: false, contentful_connector: instance_double(ContentfulConnector) # TODO: I suspect the double doesn't do anything and we need stub_contentful_connector ) category = fake_contentful_category(contentful_fixture_filename: fixture_filename) @@ -30,6 +31,9 @@ def stub_contentful_category( if stub_steps stub_contentful_section_steps(sections: sections, contentful_connector: contentful_connector) end + if stub_tasks + stub_contentful_section_tasks(sections: sections, contentful_connector: contentful_connector) + end end category @@ -63,7 +67,7 @@ def stub_contentful_section_steps( sections.each do |section| fake_steps = section.steps.map { |step| - fake_step = fake_contentful_step_or_task(contentful_fixture_filename: "steps/#{step.id}.json") + fake_step = fake_contentful_step(contentful_fixture_filename: "steps/#{step.id}.json") expect(contentful_connector).to receive(:get_entry_by_id) .with(fake_step.id) .and_return(fake_step) @@ -83,7 +87,7 @@ def stub_contentful_category_steps( .and_return(contentful_connector) category.steps.each do |step| - step = fake_contentful_step_or_task(contentful_fixture_filename: "steps/#{step.id}.json") + step = fake_contentful_step(contentful_fixture_filename: "steps/#{step.id}.json") allow(contentful_connector).to receive(:get_entry_by_id) .with(step.id) .and_return(step) @@ -99,13 +103,14 @@ def stub_contentful_section_tasks( sections.each do |section| fake_tasks = section.tasks.map { |task| - fake_task = fake_contentful_step_or_task(contentful_fixture_filename: "tasks/#{task.id}.json") + fake_task = fake_contentful_task(contentful_fixture_filename: "tasks/#{task.id}.json") expect(contentful_connector).to receive(:get_entry_by_id) .with(fake_task.id) .and_return(fake_task) fake_task } allow(section).to receive(:tasks).and_return(fake_tasks) + stub_contentful_task_steps(tasks: fake_tasks, contentful_connector: contentful_connector) end end @@ -118,8 +123,8 @@ def stub_contentful_task_steps( tasks.each do |task| fake_steps = task.steps.map { |step| - fake_step = fake_contentful_step_or_task(contentful_fixture_filename: "steps/#{step.id}.json") - expect(contentful_connector).to receive(:get_entry_by_id) + fake_step = fake_contentful_step(contentful_fixture_filename: "steps/#{step.id}.json") + allow(contentful_connector).to receive(:get_entry_by_id) .with(fake_step.id) .and_return(fake_step) fake_step @@ -187,7 +192,25 @@ def fake_contentful_section(contentful_fixture_filename:) ) end - def fake_contentful_step_or_task(contentful_fixture_filename:) + def fake_contentful_task(contentful_fixture_filename:) + raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{contentful_fixture_filename}") + hash_response = JSON.parse(raw_response) + + task = double( + Contentful::Entry, + id: hash_response.dig("sys", "id"), + title: hash_response.dig("fields", "title"), + raw: hash_response, + content_type: double(id: hash_response.dig("sys", "contentType", "sys", "id")) + ) + + steps = hash_response.dig("fields", "steps").map { |step_hash| double(id: step_hash.dig("sys", "id")) } + + allow(task).to receive(:steps).and_return steps + task + end + + def fake_contentful_step(contentful_fixture_filename:) raw_response = File.read("#{Rails.root}/spec/fixtures/contentful/#{contentful_fixture_filename}") hash_response = JSON.parse(raw_response) diff --git a/spec/support/journey_helpers.rb b/spec/support/journey_helpers.rb index cce75d329..b0d71d939 100644 --- a/spec/support/journey_helpers.rb +++ b/spec/support/journey_helpers.rb @@ -13,6 +13,17 @@ def start_journey_from_category(category:) user_signs_in_and_starts_the_journey end + def start_journey_with_tasks_from_category(category:) + stub_contentful_category( + fixture_filename: category, + stub_sections: true, + stub_steps: false, + stub_tasks: true + ) + + user_signs_in_and_starts_the_journey + end + def start_journey_from_category_and_go_to_question(category:) start_journey_from_category(category: category) From 555ea171334e6840b2ed9fc3a9956c31f5c54305 Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Wed, 21 Apr 2021 14:48:22 +0100 Subject: [PATCH 16/32] View a Task on its own page Add a controller, route and view for a task. In a future commit the task show page will contain a list of steps/questions --- CHANGELOG.md | 1 + app/controllers/tasks_controller.rb | 12 +++++++ app/views/tasks/show.html.erb | 4 +++ config/routes.rb | 1 + .../view_a_task_spec.rb | 31 +++++++++++++++++++ 5 files changed, 49 insertions(+) create mode 100644 app/controllers/tasks_controller.rb create mode 100644 app/views/tasks/show.html.erb create mode 100644 spec/features/school_buying_professionals/view_a_task_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bcba5a74..8ea37b701 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog 1.0.0]. database. - Break direct association between Journey and Steps. Create new association between Tasks and Steps. +- Show Tasks on Journey page; clicking on a Task name takes you to a task page. ## [release-009] - 2021-05-21 diff --git a/app/controllers/tasks_controller.rb b/app/controllers/tasks_controller.rb new file mode 100644 index 000000000..26c91ccae --- /dev/null +++ b/app/controllers/tasks_controller.rb @@ -0,0 +1,12 @@ +class TasksController < ApplicationController + def show + @journey = current_journey + @task = Task.find(task_id) + end + + private + + def task_id + params[:id] + end +end diff --git a/app/views/tasks/show.html.erb b/app/views/tasks/show.html.erb new file mode 100644 index 000000000..989348a40 --- /dev/null +++ b/app/views/tasks/show.html.erb @@ -0,0 +1,4 @@ +<%= content_for :title, @task.title %> +<%= link_to I18n.t("generic.button.back"), journey_path(@journey), class: "govuk-back-link" %> + +

<%= @task.title %>

diff --git a/config/routes.rb b/config/routes.rb index 5e0b32555..83b497b30 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,6 +19,7 @@ resources :steps, only: [:new, :show, :edit] do resources :answers, only: [:create, :update] end + resources :tasks, only: [:show] end namespace :preview do diff --git a/spec/features/school_buying_professionals/view_a_task_spec.rb b/spec/features/school_buying_professionals/view_a_task_spec.rb new file mode 100644 index 000000000..0d945f601 --- /dev/null +++ b/spec/features/school_buying_professionals/view_a_task_spec.rb @@ -0,0 +1,31 @@ +require "rails_helper" + +feature "Users can view a task" do + let(:user) { create(:user) } + before { user_is_signed_in(user: user) } + + context "When there is a task with multiple steps" do + it "the Task title takes you to a Task page" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + + within(".app-task-list") do + click_on "Task with multiple steps" + end + + expect(page).to have_content "Task with multiple steps" + end + + it "the back link takes you to the journey page" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + + within(".app-task-list") do + click_on "Task with multiple steps" + end + + expect(page).to have_content "Back" + + click_on "Back" + expect(page).to have_content "Task with multiple steps" + end + end +end From ba457d0f9abe84c509dcbcf8764f80fee3a014b6 Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Thu, 22 Apr 2021 16:22:15 +0100 Subject: [PATCH 17/32] Add a helper method to Task to indicate if it has only one step Task#has_one_visible_step? will be used to redirect the user appropriately when they answer a question. If the question belongs to a task with only one step, then the user will be redirected back to the journey show page. If the question belongs to a task with more than one step, the user will be redirected back to the task show page. --- app/models/task.rb | 4 ++++ spec/models/task_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/app/models/task.rb b/app/models/task.rb index 55f695f1f..de484c626 100644 --- a/app/models/task.rb +++ b/app/models/task.rb @@ -8,4 +8,8 @@ class Task < ApplicationRecord def visible_steps steps.where(hidden: false) end + + def has_single_visible_step? + visible_steps.count == 1 + end end diff --git a/spec/models/task_spec.rb b/spec/models/task_spec.rb index 17a355227..641e4fcf9 100644 --- a/spec/models/task_spec.rb +++ b/spec/models/task_spec.rb @@ -18,4 +18,26 @@ expect(task.visible_steps).to eq [step_1] end end + + describe "#has_single_visible_step?" do + context "when the task has one visible step" do + it "returns true" do + task = create(:task) + _step_1 = create(:step, :radio, hidden: false, task: task) + _step_2 = create(:step, :radio, hidden: true, task: task) + + expect(task.has_single_visible_step?).to be true + end + end + + context "when the task has more than one visible step" do + it "returns true" do + task = create(:task) + _step_1 = create(:step, :radio, hidden: false, task: task) + _step_2 = create(:step, :radio, hidden: false, task: task) + + expect(task.has_single_visible_step?).to be false + end + end + end end From 76d2623ff15b3a5616681f7377fa4e13cba85845 Mon Sep 17 00:00:00 2001 From: Laura Porter Date: Thu, 22 Apr 2021 16:03:49 +0100 Subject: [PATCH 18/32] Show a list of steps on the task page In a previous commit we added a task#show page which was empty. This commit adds the task's steps to this page, using the same UI pattern that was previously used on the journey#show page. Also, when a user answers a question from the task#show page, they will be redirected back to the task#show page. If they answer a question from the journey#show page, they will be redirected to the journey#show page --- CHANGELOG.md | 3 +- app/controllers/answers_controller.rb | 16 ++++- app/controllers/steps_controller.rb | 14 +++++ app/controllers/tasks_controller.rb | 10 ++++ app/views/steps/_edit_form_wrapper.html.erb | 2 + app/views/steps/_new_form_wrapper.html.erb | 2 + app/views/tasks/show.html.erb | 28 +++++++++ .../view_a_list_of_tasks_spec.rb | 28 +++++++++ .../view_a_task_spec.rb | 52 ++++++++++++++++ ...section-with-visible-and-hidden-tasks.json | 44 ++++++++++++++ ...iple-visible-and-hidden-tasks-section.json | 42 +++++++++++++ .../tasks/radio-and-hidden-task.json | 60 +++++++++++++++++++ 12 files changed, 298 insertions(+), 3 deletions(-) create mode 100644 spec/fixtures/contentful/categories/section-with-visible-and-hidden-tasks.json create mode 100644 spec/fixtures/contentful/sections/multiple-visible-and-hidden-tasks-section.json create mode 100644 spec/fixtures/contentful/tasks/radio-and-hidden-task.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ea37b701..7dfbe9e0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,8 @@ The format is based on [Keep a Changelog 1.0.0]. database. - Break direct association between Journey and Steps. Create new association between Tasks and Steps. -- Show Tasks on Journey page; clicking on a Task name takes you to a task page. +- Show Tasks on Journey page; clicking on a Task name takes you to a task page. +- Show Steps on Task page and allow users to answer questions from Task page ## [release-009] - 2021-05-21 diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 528abb3e4..f7f80fe0c 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -18,7 +18,11 @@ def create @answer = result.object if result.success? - redirect_to journey_path(@journey, anchor: @step.id) + if parent_task.has_single_visible_step? + redirect_to journey_path(@journey, anchor: @step.id) + else + redirect_to journey_task_path(@journey, parent_task) + end else render "steps/#{@step.contentful_type}", locals: {layout: "steps/new_form_wrapper"} end @@ -33,7 +37,11 @@ def update @answer = result.object if result.success? - redirect_to journey_path(@journey, anchor: @step.id) + if parent_task.has_single_visible_step? + redirect_to journey_path(@journey, anchor: @step.id) + else + redirect_to journey_task_path(@journey, parent_task) + end else render "steps/#{@step.contentful_type}", locals: {layout: "steps/edit_form_wrapper"} end @@ -41,6 +49,10 @@ def update private + def parent_task + @step.task + end + def journey_id params[:journey_id] end diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index 2e84ee991..c0e3c8cdd 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -10,6 +10,11 @@ def show @step_presenter = StepPresenter.new(@step) @answer = AnswerFactory.new(step: @step).call + @back_url = if !parent_task || parent_task.has_single_visible_step? + journey_path(@journey, anchor: @step.id) + else + journey_task_path(@journey, parent_task) + end render @step.contentful_type, locals: {layout: "steps/new_form_wrapper"} end @@ -21,12 +26,21 @@ def edit @step_presenter = StepPresenter.new(@step) @answer = @step.answer + @back_url = if !parent_task || parent_task.has_single_visible_step? + journey_path(@journey, anchor: @step.id) + else + journey_task_path(@journey, parent_task) + end render "steps/#{@step.contentful_type}", locals: {layout: "steps/edit_form_wrapper"} end private + def parent_task + @step.task + end + def journey_id params[:journey_id] end diff --git a/app/controllers/tasks_controller.rb b/app/controllers/tasks_controller.rb index 26c91ccae..018e84816 100644 --- a/app/controllers/tasks_controller.rb +++ b/app/controllers/tasks_controller.rb @@ -2,6 +2,16 @@ class TasksController < ApplicationController def show @journey = current_journey @task = Task.find(task_id) + steps = @task.visible_steps.includes([ + :short_text_answer, + :long_text_answer, + :radio_answer, + :checkbox_answers, + :currency_answer, + :number_answer, + :single_date_answer + ]) + @steps = steps.map { |step| StepPresenter.new(step) } end private diff --git a/app/views/steps/_edit_form_wrapper.html.erb b/app/views/steps/_edit_form_wrapper.html.erb index 8e174312a..6c51793b4 100644 --- a/app/views/steps/_edit_form_wrapper.html.erb +++ b/app/views/steps/_edit_form_wrapper.html.erb @@ -1,4 +1,6 @@ <%= content_for :title, @step_presenter.title %> +<%= link_to I18n.t("generic.button.back"), @back_url, class: "govuk-back-link" %> + <%= 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" %> diff --git a/app/views/steps/_new_form_wrapper.html.erb b/app/views/steps/_new_form_wrapper.html.erb index fc0bbb909..da2383a4e 100644 --- a/app/views/steps/_new_form_wrapper.html.erb +++ b/app/views/steps/_new_form_wrapper.html.erb @@ -1,4 +1,6 @@ <%= content_for :title, @step_presenter.title %> +<%= link_to I18n.t("generic.button.back"), @back_url, class: "govuk-back-link" %> + <%= form_for @answer, as: :answer, url: journey_step_answers_path(@journey, @step), method: "post" do |f| %> <%= yield f %> <%= f.submit @step_presenter.primary_call_to_action_text, class: "govuk-button" %> diff --git a/app/views/tasks/show.html.erb b/app/views/tasks/show.html.erb index 989348a40..81ae2753d 100644 --- a/app/views/tasks/show.html.erb +++ b/app/views/tasks/show.html.erb @@ -2,3 +2,31 @@ <%= link_to I18n.t("generic.button.back"), journey_path(@journey), class: "govuk-back-link" %>

<%= @task.title %>

+ +
    +

    <%= @task.title %>

    +
  1. +
      + <% @steps.each do |step| %> + <% if step.question? %> +
    • + <% if step.answer %> + + <%= link_to step.title, edit_journey_step_path(@journey, step), class: "govuk-link", 'aria-describedby': step.id + "-status"%> + + "><%= I18n.t("task_list.status.completed") %> + <% else %> + + <%= link_to step.title, journey_step_path(@journey, step), class: "govuk-link", 'aria-describedby': step.id + "-status"%> + + "><%= I18n.t("task_list.status.not_started") %> + <% end %> +
      • + <% end %> +
    +
    2. + <% end %> +
+ +<%= link_to I18n.t("journey.specification.button"), journey_specification_path(@journey), class: "govuk-button" %> + diff --git a/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb b/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb index 5d0932989..69547d9db 100644 --- a/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb +++ b/spec/features/school_buying_professionals/view_a_list_of_tasks_spec.rb @@ -86,6 +86,34 @@ expect(page).to_not have_content("Task with a single step") end end + + it "has a back link on the step page that takes you to the journey page" do + start_journey_with_tasks_from_category(category: "section-with-single-task.json") + + within(".app-task-list") do + click_on "Everyday services that are required and need to be considered" + end + + click_on "Back" + + expect(page).to have_content "Section with a single task" + end + + it "allows the user to complete the step, and returns to the journey page" do + start_journey_with_tasks_from_category(category: "section-with-single-task.json") + + within(".app-task-list") do + click_on "Everyday services that are required and need to be considered" + end + + check "Lunch" + click_on "Continue" + + expect(page).to have_content "Section with a single task" + within(".app-task-list") do + expect(page).to have_content("Complete") + end + end end context "When there is a task with a single HIDDEN step" do diff --git a/spec/features/school_buying_professionals/view_a_task_spec.rb b/spec/features/school_buying_professionals/view_a_task_spec.rb index 0d945f601..349c07b83 100644 --- a/spec/features/school_buying_professionals/view_a_task_spec.rb +++ b/spec/features/school_buying_professionals/view_a_task_spec.rb @@ -27,5 +27,57 @@ click_on "Back" expect(page).to have_content "Task with multiple steps" end + + it "shows a list of the task's steps" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + + within(".app-task-list") do + click_on "Task with multiple steps" + end + + expect(page).to have_content "Which service do you need?" + expect(page).to have_content "Everyday services that are required and need to be considered" + end + + it "does not show hidden steps" do + start_journey_with_tasks_from_category(category: "section-with-visible-and-hidden-tasks.json") + + within(".app-task-list") do + click_on "Task with visible and hidden steps" + end + + expect(page).to_not have_content "You should NOT be able to see this question?" + expect(page).to have_content "Which service do you need?" + end + + it "has a back link on the step page that takes you to the task list" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + + within(".app-task-list") do + click_on "Task with multiple steps" + end + + click_on "Which service do you need?" + click_on "Back" + + expect(page).to have_content "Task with multiple steps" + end + + it "allows the user to click on a step to supply an answer, and returns to the task page" do + start_journey_with_tasks_from_category(category: "section-with-multiple-tasks.json") + + within(".app-task-list") do + click_on "Task with multiple steps" + end + + click_on "Which service do you need?" + choose "Catering" + click_on "Continue" + + expect(page).to have_content "Task with multiple steps" + within(".app-task-list") do + expect(page).to have_content("Complete") + end + end end end diff --git a/spec/fixtures/contentful/categories/section-with-visible-and-hidden-tasks.json b/spec/fixtures/contentful/categories/section-with-visible-and-hidden-tasks.json new file mode 100644 index 000000000..84c2fcab0 --- /dev/null +++ b/spec/fixtures/contentful/categories/section-with-visible-and-hidden-tasks.json @@ -0,0 +1,44 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "contentful-category-entry", + "type": "Entry", + "createdAt": "2021-01-20T12:19:10.220Z", + "updatedAt": "2021-01-20T15:06:12.067Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "category" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Catering", + "sections": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "multiple-visible-and-hidden-tasks-section" + } + } + ], + "specificationTemplate": "

Liquid {{templating}}

" + } +} diff --git a/spec/fixtures/contentful/sections/multiple-visible-and-hidden-tasks-section.json b/spec/fixtures/contentful/sections/multiple-visible-and-hidden-tasks-section.json new file mode 100644 index 000000000..3e57a581e --- /dev/null +++ b/spec/fixtures/contentful/sections/multiple-visible-and-hidden-tasks-section.json @@ -0,0 +1,42 @@ +{ + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "tasks-section", + "type": "Entry", + "createdAt": "2021-04-12T10:41:36.073Z", + "updatedAt": "2021-04-12T10:42:51.119Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 2, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "section" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Section with visible and hidden tasks", + "tasks": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "radio-and-hidden-task" + } + }] + } +} diff --git a/spec/fixtures/contentful/tasks/radio-and-hidden-task.json b/spec/fixtures/contentful/tasks/radio-and-hidden-task.json new file mode 100644 index 000000000..6442b9777 --- /dev/null +++ b/spec/fixtures/contentful/tasks/radio-and-hidden-task.json @@ -0,0 +1,60 @@ +{ + "metadata": { + "tags": [] + }, + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "rwl7tyzv9sys" + } + }, + "id": "checkboxes-task", + "type": "Entry", + "createdAt": "2021-04-12T09:38:48.990Z", + "updatedAt": "2021-04-12T10:11:05.670Z", + "environment": { + "sys": { + "id": "develop", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 3, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "task" + } + }, + "locale": "en-US" + }, + "fields": { + "title": "Task with visible and hidden steps", + "steps": [ + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "hidden-field" + } + }, + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "radio-with-separator-question" + } + }, + { + "sys": { + "type": "Link", + "linkType": "Entry", + "id": "radio-question" + } + } + ] + } +} From a9c86a67d9c05c16dacb2fcf0ddc02511fe78a05 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 11:46:14 +0100 Subject: [PATCH 19/32] Docker build works when RAILS_ENV is production When Redis was added as a session store CI successfully built and tested the image. When merging this - and all subsequent work - into `develop` the build failed https://github.com/DFE-Digital/buy-for-your-school/actions/runs/773887911 This happened because CI builds docker images with RAILS_ENV=test (for testing) and on deploy to devleop RAILS_ENV=production is used. This causes this asset:precompile to run for the first time https://github.com/DFE-Digital/buy-for-your-school/blob/develop/Dockerfile#L66. Guard against this error by providing a default. The app just needs to start, it doesn't need to actually connect to redis. --- config/initializers/session_store.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 6c55fe999..ee674e810 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -1,4 +1,4 @@ -redis_session_uri = URI(ENV.fetch("REDIS_URL")) +redis_session_uri = URI(ENV.fetch("REDIS_URL", "redis://localhost:6379")) BuyForYourSchool::Application.config.session_store :redis_store, servers: ["#{redis_session_uri}/0/session"], From 0f4518db846ac663da1e9a0d8f6d28da64833501 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 11:53:30 +0100 Subject: [PATCH 20/32] Include REDIS_URL as a critical environment variable for app start --- config/initializers/_dotenv.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/initializers/_dotenv.rb b/config/initializers/_dotenv.rb index e81407eef..d5dd68c01 100644 --- a/config/initializers/_dotenv.rb +++ b/config/initializers/_dotenv.rb @@ -10,6 +10,7 @@ "CONTENTFUL_DEFAULT_CATEGORY_ENTRY_ID", "CONTENTFUL_PREVIEW_APP", "CONTENTFUL_ENTRY_CACHING", - "SUPPORT_EMAIL" + "SUPPORT_EMAIL", + "REDIS_URL" ) end From f1e5d0ad018d9e112c7f45ab6de35ccd5904a9da Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 13:05:32 +0100 Subject: [PATCH 21/32] Fix changelog line spacing --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dfbe9e0c..423a6ddde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,17 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog 1.0.0]. ## [Unreleased] -- add header and footer information for feedback and data requests +- add header and footer information for feedback and data requests - force SSL in production to only accept HTTPS traffic, enable HSTS and secure tower cookies - - prevent concurrent sign ins - - Create Task model; fetch tasks from Contentful and create them in the database. - Break direct association between Journey and Steps. Create new association between Tasks and Steps. -- Show Tasks on Journey page; clicking on a Task name takes you to a task page. +- Show Tasks on Journey page; clicking on a Task name takes you to a task page. - Show Steps on Task page and allow users to answer questions from Task page ## [release-009] - 2021-05-21 From 953a784df0c81db7f90924598644234fd02baf64 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 13:00:30 +0100 Subject: [PATCH 22/32] Spec files are copied into the Docker image so Rspec runs them Before this change CI was silently passing because rspec was not running any tests since no files were available to it. The longer term approach would be to use a multi stage docker file as seen in this Rails Template proposal [1]. For now though, we need CI to run our tests. [1] https://github.com/dxw/rails-template/pull/211/commits/5966236a533f07d330a5d24cb9ecc9d6fd52aa89#diff-dd2c0eb6ea5cfc6c4bd4eac30934e2d5746747af48fef6da689e85b752f39557R112 --- CHANGELOG.md | 1 + Dockerfile | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 423a6ddde..ce1a4aa47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog 1.0.0]. between Tasks and Steps. - Show Tasks on Journey page; clicking on a Task name takes you to a task page. - Show Steps on Task page and allow users to answer questions from Task page +- fix CI not running RSpec ## [release-009] - 2021-05-21 diff --git a/Dockerfile b/Dockerfile index a07a6abd3..ffd1c31eb 100644 --- a/Dockerfile +++ b/Dockerfile @@ -44,6 +44,7 @@ COPY lib ${INSTALL_PATH}/lib COPY config ${INSTALL_PATH}/config COPY db ${INSTALL_PATH}/db COPY script ${INSTALL_PATH}/script +COPY spec ${INSTALL_PATH}/spec COPY app ${INSTALL_PATH}/app # End From 022c3f8a27ff1633992701876f6a3fce6c9e87cf Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 13:02:35 +0100 Subject: [PATCH 23/32] Fix test suite to make redis available through docker compose No tests were being run so we didn't notice that redis wasn't able to establish connections. This change matches redis with the redis process that's called test-redis. Magic Docker DNS resolution is now able to make a connection. --- docker-compose.ci.yml | 2 +- docker-compose.test.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docker-compose.ci.yml b/docker-compose.ci.yml index 71ae4de43..4e9260185 100644 --- a/docker-compose.ci.yml +++ b/docker-compose.ci.yml @@ -16,7 +16,7 @@ services: - .env.test environment: DATABASE_URL: postgres://postgres:password@test-db:5432/buy-for-your-school-test - REDIS_URL: redis://redis:6379 + REDIS_URL: redis://test-redis:6379 DATABASE_CLEANER_ALLOW_REMOTE_DATABASE_URL: "true" networks: - test diff --git a/docker-compose.test.yml b/docker-compose.test.yml index a7a2c37f1..841f123ec 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -16,7 +16,7 @@ services: - .env.test environment: DATABASE_URL: postgres://postgres:password@test-db:5432/buy-for-your-school-test - REDIS_URL: redis://redis:6379 + REDIS_URL: redis://test-redis:6379 DATABASE_CLEANER_ALLOW_REMOTE_DATABASE_URL: "true" volumes: - .:/srv/app From 46c2600b2910a6edf5e376a6cd2b1fad02d2fd91 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 13:04:52 +0100 Subject: [PATCH 24/32] Align script/test so it's closer with scripts to rule them all When we created this project the Rails Template didn't have the Scripts to Rule Them All merged in. Though the current version of script/test on Rails Template [1] includes many more things we would have to change our Dockerfile. This change adjusts it so we reach parity + run standardrb. CI added to docker-compose.ci.yml so we don't run linting in auto fix mode. [1] https://github.com/dxw/rails-template/blob/develop/script/test --- docker-compose.ci.yml | 1 + script/test | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/docker-compose.ci.yml b/docker-compose.ci.yml index 4e9260185..97865e5c3 100644 --- a/docker-compose.ci.yml +++ b/docker-compose.ci.yml @@ -18,6 +18,7 @@ services: DATABASE_URL: postgres://postgres:password@test-db:5432/buy-for-your-school-test REDIS_URL: redis://test-redis:6379 DATABASE_CLEANER_ALLOW_REMOTE_DATABASE_URL: "true" + CI: "true" networks: - test diff --git a/script/test b/script/test index 92dd1c595..dab93225a 100755 --- a/script/test +++ b/script/test @@ -1,9 +1,35 @@ #!/bin/sh -# script/server: Launch the application and any extra required processes -# locally. +# script/test: Run the test suite for the application. Optionally pass in a path +# to an individual test file to run a single test. set -e -bundle exec rake -bundle exec brakeman -o /dev/stdout +cd "$(dirname "$0")/.." + +if [ -n "$DEBUG" ]; then + set -x +fi + +echo "==> Updating..." + +TEST_FILE=$1 + +if [ -n "$TEST_FILE" ]; then + echo "==> Running the tests matching '$TEST_FILE'..." + bundle exec rspec --pattern "$TEST_FILE" +else + if [ -n "$CI" ]; then + echo "==> Linting Ruby..." + bundle exec standardrb + else + echo "==> Linting Ruby in fix mode..." + bundle exec standardrb --fix + fi + + echo "==> Running the tests..." + bundle exec rspec + + echo "==> Running Brakeman" + bundle exec brakeman +fi From f3507a459802f26d8cde6e0476d665743632ea1a Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 13:54:37 +0100 Subject: [PATCH 25/32] Remove unused lines to get code coverage about 99% --- app/controllers/answers_controller.rb | 4 ---- app/controllers/journeys_controller.rb | 6 ------ app/controllers/specifications_controller.rb | 6 ------ app/controllers/steps_controller.rb | 4 ---- 4 files changed, 20 deletions(-) diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index f7f80fe0c..bf7af37e2 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -53,10 +53,6 @@ def parent_task @step.task end - def journey_id - params[:journey_id] - end - def step_id params[:step_id] end diff --git a/app/controllers/journeys_controller.rb b/app/controllers/journeys_controller.rb index f78a6eafd..13454c397 100644 --- a/app/controllers/journeys_controller.rb +++ b/app/controllers/journeys_controller.rb @@ -32,10 +32,4 @@ def show @journey = current_journey @sections = @journey.sections.includes(:tasks) end - - private - - def journey_id - params[:id] - end end diff --git a/app/controllers/specifications_controller.rb b/app/controllers/specifications_controller.rb index 99b066a16..037bface1 100644 --- a/app/controllers/specifications_controller.rb +++ b/app/controllers/specifications_controller.rb @@ -25,10 +25,4 @@ def show end end end - - private - - def journey_id - params[:journey_id] - end end diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index c0e3c8cdd..06797057c 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -40,8 +40,4 @@ def edit def parent_task @step.task end - - def journey_id - params[:journey_id] - end end From 72b21953ca4b9f4f01f64edca8621a3c779a6ddf Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 14:02:18 +0100 Subject: [PATCH 26/32] Use new github action to tell the team of failed deployments Using this Action: https://github.com/8398a7/action-slack --- .github/workflows/continuous-integration.yml | 8 ++++++++ .github/workflows/deploy.yml | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index da57487dc..6ca7b41eb 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -17,3 +17,11 @@ jobs: run: docker-compose -f docker-compose.ci.yml build - name: Test run: docker-compose -f docker-compose.ci.yml run --rm test script/test + - name: Notify + uses: 8398a7/action-slack@v3 + if: failure() + with: + fields: workflow,job,commit,repo,ref,author,took + status: ${{ job.status }} + env: + SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 20a2e7bda..5a175e6b2 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -36,6 +36,14 @@ jobs: run: echo "::set-output name=value::$TF_VAR_docker_image" - name: Build run: script/docker-push-ghcr + - name: Notify + uses: 8398a7/action-slack@v3 + if: failure() + with: + fields: workflow,job,commit,repo,ref,author,took + status: ${{ job.status }} + env: + SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} deploy: needs: build @@ -69,3 +77,11 @@ jobs: run: | script/deploy-terraform if: github.ref == 'refs/heads/main' + - name: Notify + uses: 8398a7/action-slack@v3 + if: failure() + with: + fields: workflow,job,commit,repo,ref,author,took + status: ${{ job.status }} + env: + SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} From 281b70587d3bc4c67c87561412b84355ee23026d Mon Sep 17 00:00:00 2001 From: Lorna Date: Mon, 26 Apr 2021 11:42:19 +0100 Subject: [PATCH 27/32] Existing Specification page displays useful message when no specs exist --- CHANGELOG.md | 1 + app/views/journeys/index.html.erb | 35 +++++++++++-------- config/locales/en.yml | 1 + .../view_a_dashboard_spec.rb | 10 ++++++ 4 files changed, 33 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce1a4aa47..d8a1345d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog 1.0.0]. - Show Tasks on Journey page; clicking on a Task name takes you to a task page. - Show Steps on Task page and allow users to answer questions from Task page - fix CI not running RSpec +- existing specification page displays useful message when no specs exist ## [release-009] - 2021-05-21 diff --git a/app/views/journeys/index.html.erb b/app/views/journeys/index.html.erb index 106a7646f..a92d76627 100644 --- a/app/views/journeys/index.html.erb +++ b/app/views/journeys/index.html.erb @@ -3,19 +3,26 @@

<%= I18n.t("journey.index.existing.header") %>

- - - - - - - - - <% @journeys.each do |journey| %> +<% unless @journeys.empty? %> +
Date started
+ - - + + - <% end %> - -
<%= journey.created_at.strftime("%e %B %Y") %>Review and editDate started
+ + + <% @journeys.each do |journey| %> + + <%= journey.created_at.strftime("%e %B %Y") %> + Review and edit + + <% end %> + + +<% else %> +

<%= I18n.t("journey.index.existing.empty") %>

+ +<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index bba9160db..8ea1dcf2a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -63,6 +63,7 @@ en: index: existing: header: "Existing specifications" + empty: "You currently have no existing specifications, return to create a new specification" specification: header: "Your specification" warning: "You have not completed all the tasks. There may be information missing from your specification." diff --git a/spec/features/school_buying_professionals/view_a_dashboard_spec.rb b/spec/features/school_buying_professionals/view_a_dashboard_spec.rb index a55be68e5..d3f8f50f4 100644 --- a/spec/features/school_buying_professionals/view_a_dashboard_spec.rb +++ b/spec/features/school_buying_professionals/view_a_dashboard_spec.rb @@ -33,6 +33,16 @@ expect(page).to have_content("15 February 2021") end + scenario "when a user has no specifications a message is shown" do + user = create(:user) + user_is_signed_in(user: user) + + visit journeys_path + + expect(page).to have_content(I18n.t("journey.index.existing.empty")) + expect(page).to have_content(I18n.t("dashboard.create.link")) + end + scenario "user can start a new specification" do stub_contentful_category(fixture_filename: "radio-question.json") From 31375f878749433b6159c04c18e780008af74c98 Mon Sep 17 00:00:00 2001 From: Lorna Date: Mon, 26 Apr 2021 16:00:30 +0100 Subject: [PATCH 28/32] Text input field width set to `full` --- CHANGELOG.md | 1 + app/views/steps/long_text.html.erb | 2 +- app/views/steps/short_text.html.erb | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8a1345d3..218cc6ec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ The format is based on [Keep a Changelog 1.0.0]. - Show Steps on Task page and allow users to answer questions from Task page - fix CI not running RSpec - existing specification page displays useful message when no specs exist +- fix text input field width to fit full screen width ## [release-009] - 2021-05-21 diff --git a/app/views/steps/long_text.html.erb b/app/views/steps/long_text.html.erb index 8cd8d3079..325fd6cb2 100644 --- a/app/views/steps/long_text.html.erb +++ b/app/views/steps/long_text.html.erb @@ -2,7 +2,7 @@ <%= f.govuk_text_area :response, label: { text: @step_presenter.title, size: "l" }, hint: -> { @step_presenter.help_text_html }, - width: "one-third", + width: "full", rows: 9 %> <% end %> diff --git a/app/views/steps/short_text.html.erb b/app/views/steps/short_text.html.erb index 04a495804..499c76e6a 100644 --- a/app/views/steps/short_text.html.erb +++ b/app/views/steps/short_text.html.erb @@ -2,6 +2,6 @@ <%= f.govuk_text_field :response, label: { text: @step_presenter.title, size: "l" }, hint: -> { @step_presenter.help_text_html }, - width: "one-third" + width: "full" %> <% end %> From 16cb9fbe0ebeca267c27b719fe1d4da1461d260e Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Tue, 27 Apr 2021 11:11:18 +0100 Subject: [PATCH 29/32] Log webhook information to Rollbar for debugging --- app/controllers/api/contentful/entries_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/api/contentful/entries_controller.rb b/app/controllers/api/contentful/entries_controller.rb index 9cffe3fb0..867848e77 100644 --- a/app/controllers/api/contentful/entries_controller.rb +++ b/app/controllers/api/contentful/entries_controller.rb @@ -5,6 +5,8 @@ class Api::Contentful::EntriesController < ApplicationController skip_before_action :verify_authenticity_token def changed + Rollbar.info("Accepted request to cache bust key: #{cache_key}", params: params) + Cache.delete(key: cache_key) render json: {status: "OK"}, status: :ok end From 5eb0b7c68cee10921093db69fba75d347f1955cc Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 17:11:11 +0100 Subject: [PATCH 30/32] Don't notify on every failed builds, only those during deployments --- .github/workflows/continuous-integration.yml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/.github/workflows/continuous-integration.yml b/.github/workflows/continuous-integration.yml index 6ca7b41eb..da57487dc 100644 --- a/.github/workflows/continuous-integration.yml +++ b/.github/workflows/continuous-integration.yml @@ -17,11 +17,3 @@ jobs: run: docker-compose -f docker-compose.ci.yml build - name: Test run: docker-compose -f docker-compose.ci.yml run --rm test script/test - - name: Notify - uses: 8398a7/action-slack@v3 - if: failure() - with: - fields: workflow,job,commit,repo,ref,author,took - status: ${{ job.status }} - env: - SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} From 5abbb6a1a9d91c5b61ea9cd4f0ed60b2371e11d5 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 18:15:40 +0100 Subject: [PATCH 31/32] docs: where to find the service --- CHANGELOG.md | 1 + README.md | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 218cc6ec7..b82b3f384 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog 1.0.0]. - fix CI not running RSpec - existing specification page displays useful message when no specs exist - fix text input field width to fit full screen width +- document where to find the service in the readme ## [release-009] - 2021-05-21 diff --git a/README.md b/README.md index d13e3475b..ff28b10ef 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,13 @@ in doc/architecture/decisions and contributed to with the ## Access -TODO: Where can people find the service and the different environments? +| Environment | URL | +| :------------- | :------------------------------------------------------------: | +| Development | http://localhost:3000 | +| Research | https://buy-for-your-school-research.london.cloudapps.digital | +| Preview | https://buy-for-your-school-preview.london.cloudapps.digital | +| Staging | https://staging-get-help-buying-for-schools.education.gov.uk | +| Production | https://get-help-buying-for-schools.education.gov.uk | ## Source From 02b95b25083b4f5c82f650dadc39b2ae3c560505 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Mon, 26 Apr 2021 18:20:27 +0100 Subject: [PATCH 32/32] release-010 - add header and footer information for feedback and data requests - force SSL in production to only accept HTTPS traffic, enable HSTS and secure tower cookies - prevent concurrent sign ins - Create Task model; fetch tasks from Contentful and create them in the database. - Break direct association between Journey and Steps. Create new association between Tasks and Steps. - Show Tasks on Journey page; clicking on a Task name takes you to a task page. - Show Steps on Task page and allow users to answer questions from Task page - fix CI not running RSpec - existing specification page displays useful message when no specs exist - fix text input field width to fit full screen width - document where to find the service in the readme --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b82b3f384..5c550876d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog 1.0.0]. ## [Unreleased] +## [release-010] - 2021-05-27 + - add header and footer information for feedback and data requests - force SSL in production to only accept HTTPS traffic, enable HSTS and secure tower cookies - prevent concurrent sign ins @@ -147,7 +149,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-009...HEAD +[unreleased]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-010...HEAD +[release-010]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-009...release-010 [release-009]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-008...release-009 [release-008]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-007...release-008 [release-007]: https://github.com/DFE-Digital/buy-for-your-school/compare/release-006...release-007