From b92128b2681adb96290641ab09af575ba935c05e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Thu, 23 Nov 2023 22:58:26 +0530 Subject: [PATCH] refactor: conditional routing Motivation ---------- It took me a while to debug the 404 error when clicking on the link to: ``` http://localhost:3000/onboarding/whats-app?jwt=eyJhbGc ``` I was expecting to see a problem in `config/routes.rb` but couldn't find anything. It was surprising to me that we programmatically throw a 404 in one of the controllers. How to test ----------- 1. `bin/rspec ./spec/adapters/signal_adapter/api_spec.rb` 2. Tests pass Requested changes ----------------- @mattwr18 asked me to implement this consistently across our codebase. I have seen this only once, here's the commit that introduced it: 9a541d69fd8b41ce269f67eb11dc2fbccf9f5160. Thankfully there is already a test (that still fails if you remove conditional routing). In the tests there was: ``` unless Setting.signal_server_phone_number allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') # .. end ``` from 07694a6fe58e1b7b47abf0e5eda1e90663f0a519. @mattwr18 this looks intentional, why was that? Did I miss anything? --- .env.template | 7 +++- .../onboarding/signal_controller.rb | 7 ---- .../onboarding/whats_app_controller.rb | 8 ----- config/routes.rb | 14 +++++--- .../receive_polling_job_spec.rb | 17 +++------- spec/requests/onboarding/whatsapp_spec.rb | 33 +++++++++++++++++++ 6 files changed, 53 insertions(+), 33 deletions(-) create mode 100644 spec/requests/onboarding/whatsapp_spec.rb diff --git a/.env.template b/.env.template index ccf6bd9ab..de6a20730 100644 --- a/.env.template +++ b/.env.template @@ -12,6 +12,11 @@ THREEMARB_API_IDENTITY= THREEMARB_API_SECRET= THREEMARB_PRIVATE= +TWILIO_ACCOUNT_SID= +TWILIO_API_KEY_SID= +TWILIO_API_KEY_SECRET= +WHATS_APP_SERVER_PHONE_NUMBER= + # These environment variables are optional during # development, but required when running 100eyes # in a production environment. @@ -27,4 +32,4 @@ THREEMARB_PRIVATE= # # THREE_SIXTY_DIALOG_PARTNER_ID= # THREE_SIXTY_DIALOG_PARTNER_USERNAME= -# THREE_SIXTY_DIALOG_PARTNER_PASSWORD= \ No newline at end of file +# THREE_SIXTY_DIALOG_PARTNER_PASSWORD= diff --git a/app/controllers/onboarding/signal_controller.rb b/app/controllers/onboarding/signal_controller.rb index 5a1b4955d..bb7d82f1a 100644 --- a/app/controllers/onboarding/signal_controller.rb +++ b/app/controllers/onboarding/signal_controller.rb @@ -3,7 +3,6 @@ module Onboarding class SignalController < OnboardingController skip_before_action :verify_jwt, only: :link - before_action :ensure_signal_is_set_up def link; end @@ -20,11 +19,5 @@ def redirect_to_success def complete_onboarding(contributor) SignalAdapter::CreateContactJob.perform_later(contributor) end - - def ensure_signal_is_set_up - return if Setting.signal_server_phone_number.present? - - raise ActionController::RoutingError, 'Not Found' - end end end diff --git a/app/controllers/onboarding/whats_app_controller.rb b/app/controllers/onboarding/whats_app_controller.rb index ea550ff00..38c32a210 100644 --- a/app/controllers/onboarding/whats_app_controller.rb +++ b/app/controllers/onboarding/whats_app_controller.rb @@ -2,18 +2,10 @@ module Onboarding class WhatsAppController < OnboardingController - before_action :ensure_whats_app_is_set_up - private def attr_name :whats_app_phone_number end - - def ensure_whats_app_is_set_up - return if Setting.whats_app_server_phone_number.present? || Setting.three_sixty_dialog_client_api_key.present? - - raise ActionController::RoutingError, 'Not Found' - end end end diff --git a/config/routes.rb b/config/routes.rb index 9c5b59267..4cb5c8708 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -20,9 +20,11 @@ get '/email', to: 'email#show' post '/email', to: 'email#create' - get '/signal/', to: 'signal#show' - get '/signal/link/', to: 'signal#link', as: 'signal_link' - post '/signal/', to: 'signal#create' + constraints(-> { Setting.signal_server_phone_number.present? }) do + get '/signal/', to: 'signal#show' + get '/signal/link/', to: 'signal#link', as: 'signal_link' + post '/signal/', to: 'signal#create' + end get '/threema/', to: 'threema#show' post '/threema/', to: 'threema#create' @@ -32,8 +34,10 @@ get '/telegram/fallback/:telegram_onboarding_token', to: 'telegram#fallback', as: 'telegram_fallback' post '/telegram/', to: 'telegram#create' - get '/whats-app/', to: 'whats_app#show' - post '/whats-app/', to: 'whats_app#create' + constraints(-> { Setting.whats_app_server_phone_number.present? || Setting.three_sixty_dialog_client_api_key.present? }) do + get '/whats-app/', to: 'whats_app#show' + post '/whats-app/', to: 'whats_app#create' + end end end diff --git a/spec/jobs/signal_adapter/receive_polling_job_spec.rb b/spec/jobs/signal_adapter/receive_polling_job_spec.rb index 2543857fa..631fba005 100644 --- a/spec/jobs/signal_adapter/receive_polling_job_spec.rb +++ b/spec/jobs/signal_adapter/receive_polling_job_spec.rb @@ -45,10 +45,7 @@ before do create(:request) - unless Setting.signal_server_phone_number - allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') - end - + allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') allow(job).to receive(:ping_monitoring_service).and_return(nil) end @@ -174,10 +171,8 @@ describe 'given a known contributor requests to unsubscribe', vcr: { cassette_name: :receive_signal_message_to_unsubscribe } do before do - unless Setting.signal_server_phone_number - allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') - allow(Setting).to receive(:signal_cli_rest_api_endpoint).and_return('http://signal:8080') - end + allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') + allow(Setting).to receive(:signal_cli_rest_api_endpoint).and_return('http://signal:8080') end let!(:contributor) { create(:contributor, signal_phone_number: '+4915112345789', signal_onboarding_completed_at: Time.zone.now) } @@ -187,10 +182,8 @@ describe 'given a contributor who has unsubscribed and requests to resubscribe', vcr: { cassette_name: :receive_signal_message_to_resubscribe } do before do - unless Setting.signal_server_phone_number - allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') - allow(Setting).to receive(:signal_cli_rest_api_endpoint).and_return('http://signal:8080') - end + allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER') + allow(Setting).to receive(:signal_cli_rest_api_endpoint).and_return('http://signal:8080') end let!(:contributor) do create(:contributor, signal_phone_number: '+4915112345789', signal_onboarding_completed_at: Time.zone.now, diff --git a/spec/requests/onboarding/whatsapp_spec.rb b/spec/requests/onboarding/whatsapp_spec.rb new file mode 100644 index 000000000..f2c369584 --- /dev/null +++ b/spec/requests/onboarding/whatsapp_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Onboarding::Whatsapp', type: :routing do + describe 'GET /onboarding/whatsapp' do + subject { { get: '/onboarding/whats-app' } } + + describe 'when no Whatsapp number was configured' do + before { allow(Setting).to receive(:whats_app_server_phone_number).and_return('') } + it { should_not be_routable } + end + + describe 'but when a Whatsapp number was configured' do + before { allow(Setting).to receive(:whats_app_server_phone_number).and_return('+49123456789') } + it { should be_routable } + end + end + + describe 'POST /onboarding/whatsapp' do + subject { { post: '/onboarding/whats-app' } } + + describe 'when no Whatsapp number was configured' do + before { allow(Setting).to receive(:whats_app_server_phone_number).and_return('') } + it { should_not be_routable } + end + + describe 'but when a Whatsapp number was configured' do + before { allow(Setting).to receive(:whats_app_server_phone_number).and_return('+49123456789') } + it { should be_routable } + end + end +end