Skip to content

Commit

Permalink
refactor: conditional routing
Browse files Browse the repository at this point in the history
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: 9a541d6.
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 07694a6.

@mattwr18 this looks intentional, why was that? Did I miss anything?
  • Loading branch information
roschaefer authored Jan 9, 2024
2 parents ec91646 + b92128b commit 4ea4001
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 33 deletions.
7 changes: 6 additions & 1 deletion .env.template
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -27,4 +32,4 @@ THREEMARB_PRIVATE=
#
# THREE_SIXTY_DIALOG_PARTNER_ID=
# THREE_SIXTY_DIALOG_PARTNER_USERNAME=
# THREE_SIXTY_DIALOG_PARTNER_PASSWORD=
# THREE_SIXTY_DIALOG_PARTNER_PASSWORD=
7 changes: 0 additions & 7 deletions app/controllers/onboarding/signal_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
8 changes: 0 additions & 8 deletions app/controllers/onboarding/whats_app_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
14 changes: 9 additions & 5 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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

Expand Down
17 changes: 5 additions & 12 deletions spec/jobs/signal_adapter/receive_polling_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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) }
Expand All @@ -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,
Expand Down
33 changes: 33 additions & 0 deletions spec/requests/onboarding/whatsapp_spec.rb
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 4ea4001

Please sign in to comment.