refactor: conditional routing #1728
Conversation
merge after #1728 Motivation ---------- So this is a follow up for #1728. It will prevent rendering the whatsapp link unless activated. We still have an inconsistency, because the channel links depend on `Setting.channels` whereas the routing depends on `Setting.whatsapp_configured?`. Ie. `Setting.channels` can be activated for whatsapp although `Setting.whatsapp_configured?` is false. How to test ----------- 1. `bin/rspec spec/system/onboarding/index.spec.rb` 2. See: ``` Onboarding visit /onboarding/ if WhatsApp was explicitly activated or activated by configuration renders invitation link for WhatsApp but if WhatsApp is deactivated renders no invitation link for WhatsApp ```
roschaefer
force-pushed
the
refactor-conditional-routing
branch
from
a526c24 to
2fe370b
Compare
| @@ -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 | |||
There was a problem hiding this comment.
This looks intentional, why was this put in place? Did I miss sth. @mattwr18 ?
There was a problem hiding this comment.
@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 🤷
There was a problem hiding this comment.
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.
merge after #1728 Motivation ---------- So this is a follow up for #1728. It will prevent rendering the whatsapp link unless activated. We still have an inconsistency, because the channel links depend on `Setting.channels` whereas the routing depends on `Setting.whatsapp_configured?`. Ie. `Setting.channels` can be activated for whatsapp although `Setting.whatsapp_configured?` is false. How to test ----------- 1. `bin/rspec spec/system/onboarding/index.spec.rb` 2. See: ``` Onboarding visit /onboarding/ if WhatsApp was explicitly activated or activated by configuration renders invitation link for WhatsApp but if WhatsApp is deactivated renders no invitation link for WhatsApp ```
| @@ -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 | |||
There was a problem hiding this comment.
@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 🤷
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?
roschaefer
force-pushed
the
refactor-conditional-routing
branch
from
2fe370b to
b92128b
Compare
refactor: conditional routing
Motivation
It took me a while to debug the 404 error when clicking on the link to:
I was expecting to see a problem in
config/routes.rbbut couldn't find anything. It was surprising to me that we programmatically throw a 404 in one of the controllers.How to test
bin/rspec ./spec/adapters/signal_adapter/api_spec.rbRequested 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:
from 07694a6.
@mattwr18 this looks intentional, why was that? Did I miss anything?