Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: conditional routing #1728

Merged
merged 1 commit into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks intentional, why was this put in place? Did I miss sth. @mattwr18 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roschaefer we already had a conversation about this code in another PR; unfortunately, I can't find it. It was actually you that added this code in 5845dc4... You did not go into great detail why you added it, but the commit message states that you were fixing ci. It then has been refactored and copy/pasted throughout the spec file.

I can say for my part that I find the code useful the first time I record the cassette so that I can get some actual data back when I run the receive messages rake task. After the cassette is recorded, I think we can and probably should delete the code.

I think just having the line:

allow(Setting).to receive(:signal_server_phone_number).and_return('SIGNAL_SERVER_PHONE_NUMBER')

was enough to get the ci to pass 2 years ago, but I think you maybe wanted to continue using what you had configured in .env 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in our pairing, it's too confusing, so let's just remove it. Maybe we can check if we're in record mode instead of checking the configuration.

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