[FYST-1089] refactor twilio service #4944

jnf · 2024-11-01T23:16:05Z

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

Yes - don't merge until JIRA issue is accepted!
Reminder: merge main into this branch and get green tests before merging to main

What was done?

Updates credentials file structure

The structure below would appear in each environment's credentials file. This structure reflects a simplified Twilio organization that isn't reflective of the current setup; restructuring the Twilio services and setup is a blocking dependency of this refactor.

twilio:
  voice_phone_number: <i think there's just the one>
  gyr:
    account_sid: <value>
    auth_token: <value>
    messaging_service_id: <value>
  statefile:
    account_sid: <value>
    auth_token: <value>
    messaging_service_id: <value>

How to test?

Describe the testing approach taken to verify the changes, including:
- Unit/integration/manual tests
- Test data used
Specify any relevant testing environments used (e.g., development, staging, demo, Heroku).
Risk Assessment
- Risks or side effects associated with the changes and how they were mitigated.
- Highlight areas that may need extra attention during code review or testing.
- Paste SQL queries or output where relevant

github-actions · 2024-11-01T23:20:20Z

Heroku app: https://gyr-review-app-4944-3d8f2d5ce12a.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4944 (optionally add --tail)

…ve space for a twilio account per service type; update the MultiTenantService to know about twilio.

…sier review. if folks like the direction, i'll turn it into a proper refactor

…n start fixing tests and callsites

…wise; specs to support

…e only callsite of the #valid_request? method

…re's not tests for this, which worries me, but we shouldn't ever need to run this again, right? ...right?

…tes relevant specs

…'t look like there are any related tests. that's probably fine, right

… am _guessing_ that ctc is the right tenant for this use case.

…job. specs are green because of the use of a clever mock (MockTwilio). neat.

…pdated TwilioService api. wee bit of code formatting cleanup too

…e the updated TwilioService api

…actored (and improved upon) in the main TwilioService

jenny-heath · 2024-11-06T18:28:36Z

app/services/twilio_service.rb

@@ -12,104 +13,101 @@ class TwilioService
    delivered
    undelivered
    failed
-  ].freeze
+  ).unshift(nil) # why do we need nil in this list, and why must it be first?


[DUST] i assume you're planning to remove this comment before merging? would prefer not to leave stuff like this around the codebase

yes! for sure. i literally don't know the answer but would like to. 😅

jenny-heath · 2024-11-06T19:35:49Z

spec/services/twilio_service_spec.rb

                                                                                        filename: "a_real_image.jpg",
                                                                                        body: "image body"
                                                                                      })
-        allow(TwilioService).to receive(:fetch_attachment).with(pdf_url).and_return({
+        allow(@twilio_service).to receive(:fetch_attachment).with(pdf_url).and_return({


i'm unfamiliar with using instance variables in rspec specs—typically we'd put something like this in a let block (not to say it's better, just that it's the practice). happy to discuss!

jenny-heath · 2024-11-06T20:34:22Z

app/models/signup.rb

@@ -34,7 +34,7 @@ def self.send_message(message_name, batch_size=nil, after: nil)
      end

      if signup.phone_number.present?
-        TwilioService.send_text_message(to: signup.phone_number, body: message.sms_body)
+        TwilioService.new(:ctc).send_text_message(to: signup.phone_number, body: message.sms_body)


this is actually not just a CTC thing! see: https://www.getyourrefund.org/en/sign-up

that makes me moderately unhappy. but it's kinda sorta ok as right now ctc and gyr seem to be sharing the same twilio setup.

jenny-heath · 2024-11-06T20:40:50Z

app/jobs/send_outgoing_text_message_job.rb

@@ -4,7 +4,7 @@ class SendOutgoingTextMessageJob < ApplicationJob
  def perform(outgoing_text_message_id)
    outgoing_text_message = OutgoingTextMessage.find(outgoing_text_message_id)
    begin
-      message = TwilioService.send_text_message(
+      message = TwilioService.new.send_text_message( # TODO: set explicit service scope; is this job for :ctc or :gyr?


[DUST] again, would like to remove comments before merging

absolutely. do you know if/how this job is aware of it's tenant service/environment?

jenny-heath · 2024-11-06T20:50:22Z

just a general question about this PR: what was the thinking behind refactoring this from a singleton into a class that gets instantiated with a service type (as opposed to, say, passing the service type into send_text_message)? not opposed to this approach, just curious about the reasons for it.

related to that: do we want to consolidate the twilio subaccounts into just the main account? i feel like we talked about this on friday but wasn't sure how serious we are about it. this approach accounts for having different account info (account sid + auth token) per service type but if we just have the one maybe that would affect the implementation

anyway, assuming we will discuss verbally just wanted to write down my thoughts before i forget them

…eaching directly into the client (also updates specs)

…iated specs with what i hope is the final form of twilio ccount representation

jnf · 2024-11-07T21:22:07Z

I prefer an instantiated class for a couple of reasons:

send_text_message isn't the only method on TwilioService that needs info about which tenant service it's working with (client_metadata does too).
limits what caller's need to know about the internals. i like the general pattern of saying "i need to talk to twilio as fyst" and the TwilioService managing what that means behind the scenes (which is why i also replaced direct calls to Twilio::REST::Client with an instance of TwilioService

re: consolidating accounts. Yes, that's a very real thing and this PR assumes that work is done. I think we'll end up with 2 accounts (down from 5)--one main account that'll house the FYST-related services/assets, and one subaccount that'll do the same for GYR (with CTC as a technically also there deprecated sidecar). Each account will have many messaging services--one per supported environment--with each messaging service having a specific sender pool and environment configuration.

Hope this all makes sense and isn't too disrupting. Lemme know if you wanna talk through it more; happy to get on a call about any/all of it!

jnf added the wip denotes a work in progress that isn't ready for formal review label Nov 1, 2024

jnf added 3 commits November 5, 2024 09:19

change the structure of the credentials file (just dev for now) to ha…

7ebf9aa

…ve space for a twilio account per service type; update the MultiTenantService to know about twilio.

an idea of how to evolve the twilio service, presented as a v2 for ea…

a914b96

…sier review. if folks like the direction, i'll turn it into a proper refactor

tweak how the multi tenant service fetches twilio creds

505cdfc

jnf force-pushed the jnf/FYST-1089/refactor-twilio-service branch from dbee352 to 505cdfc Compare November 5, 2024 17:19

jnf added 17 commits November 5, 2024 10:02

bring the v2 version of TwilioClient into the original object so i ca…

623d5dd

…n start fixing tests and callsites

updates TwilioService specs to conform to new API

ae26344

don't need to clear a class variable in the mock twilio object anymore

c12385b

instantiate TwilioService as a gyr messenger unless caller says other…

530dcf4

…wise; specs to support

updates callsite and specs for twilio webhooks controller; this is th…

eacfb9d

…e only callsite of the #valid_request? method

updates callsites and specs for sms sending in the bulk signup

ceee3f7

updates the other #get_metadata callsite and associated specs

077654d

update the twilio status backfiller to the new TwilioService api. the…

64d8be2

…re's not tests for this, which worries me, but we shouldn't ever need to run this again, right? ...right?

updates verification code job to use new TwilioService api. also upda…

2a1a4cd

…tes relevant specs

updated TwilioService callsites in the CtcSignupMessage, but it doesn…

ee2f22b

…'t look like there are any related tests. that's probably fine, right

updates Signup to use updated TwilioService api. updates specs too. i…

bb05cb2

… am _guessing_ that ctc is the right tenant for this use case.

updates TwilioService callsite in the send_outgoing_text_message_job …

40e4dbd

…job. specs are green because of the use of a clever mock (MockTwilio). neat.

updates the gyr incoming text message job/handling/specs to use the u…

db5747d

…pdated TwilioService api. wee bit of code formatting cleanup too

update code in and specs for TextMessageVerificationCodeService to us…

6812f1e

…e the updated TwilioService api

updates the client login spec to work with the updated TwilioService

4bd6e7b

updates the login spec to work with the new TwilioService api

288ac36

removes the 'v2' twilio service as the changes proposed have been ref…

64c28b9

…actored (and improved upon) in the main TwilioService

jenny-heath reviewed Nov 6, 2024

View reviewed changes

jnf added 2 commits November 7, 2024 11:58

updates the hub outbound call from to use TwilioService rather than r…

4f92fea

…eaching directly into the client (also updates specs)

updates TwilioService, MultiTenantService, dev credentials, and assoc…

fa01ca8

…iated specs with what i hope is the final form of twilio ccount representation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FYST-1089] refactor twilio service #4944

[FYST-1089] refactor twilio service #4944

jnf commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024

jenny-heath Nov 6, 2024

jnf Nov 7, 2024

jenny-heath Nov 6, 2024

jenny-heath Nov 6, 2024

jnf Nov 7, 2024

jenny-heath Nov 6, 2024

jnf Nov 7, 2024

jenny-heath commented Nov 6, 2024 •

edited

Loading

jnf commented Nov 7, 2024

[FYST-1089] refactor twilio service #4944

Are you sure you want to change the base?

[FYST-1089] refactor twilio service #4944

Conversation

jnf commented Nov 1, 2024 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

Updates credentials file structure

How to test?

github-actions bot commented Nov 1, 2024

jenny-heath Nov 6, 2024

Choose a reason for hiding this comment

jnf Nov 7, 2024

Choose a reason for hiding this comment

jenny-heath Nov 6, 2024

Choose a reason for hiding this comment

jenny-heath Nov 6, 2024

Choose a reason for hiding this comment

jnf Nov 7, 2024

Choose a reason for hiding this comment

jenny-heath Nov 6, 2024

Choose a reason for hiding this comment

jnf Nov 7, 2024

Choose a reason for hiding this comment

jenny-heath commented Nov 6, 2024 • edited Loading

jnf commented Nov 7, 2024

jnf commented Nov 1, 2024 •

edited

Loading

jenny-heath commented Nov 6, 2024 •

edited

Loading