Review the message-sending code path

[stvnrlly]

Goal: to check for (1) addressable complexity and (2) trap doors in the steps a sent message takes.

Multiple developers should share this task, both to get a range of opinions and to build familiarity with this core bit of the system.

Questions:

1. What are the entry points to the API for sending a message?
2. What are the exit points from the API to the provider?
3. Where does the code branch between those points?

To complete:

• A brief summary in docs/all.md of the findings
• Docstrings and code comments added through the path reviewed
• Recommendations for streamlining opportunities
• Identification of any areas that need repair or updating

Reviewers:

• @terrazoon
• @A-Shumway42
• @xlorepdarkhelm
• @anagradova

[ccostino]

Notes from our discussion:

• Entry-point is the same as one-off and batch CSVs; that's the only real branching point
• Goes into processing the task and creating the job and works its way through SNS to send the messages
• There are some edge cases beyond the jobs; easier to track it on the method send_notification_to_queue - we never send it directly, we drop it into a queue
• Things like reset password, account verification - these are all still email notifications
• Noting that MFA codes also go through this path but will go away with Login.gov integration
• Seems well-designed and simple enough, generally not hard to follow; overall looks very clean
  • Can't go to one file to see most things, but it's expected to be spread around
  • More complexity getting the data from the API to the admin then with this
• One exit point, everything goes to deliver_sms once out of the queue
• Regarding the emails, whoever clears this out even though we don’t send notifications, we still have the user verification
• Different types ubiquitous through the whole system
• Some left-over letter sending code
• Looking at ROI;
  • Every time on the front end, where is this data coming from; not with the message sending flow
• Can’t go to one file
• Potential benefits
  • Modernizing it to modern Python standards could help make things clearer and more concise; even just updating Python
  • e.g., the enum changes; other things that could be done
  • Going through and getting all the API endpoint paths into a single file
  • Typing could be helpful here; not a small under-taking

Things we'd like to capture with this:

• Getting comments and docstrings in place
  • Could bring in a flake8 plugin for checking for this going forward
• The checks for sending test messages appear to be removed; can we get those back in?
  • Need to investigate how the simulated_numbers flag is used, if at all
  • Need to also investigate V2 API - this is not a code path we currently use at all, should we have for this?
  • This was also part of the research mode that was mostly removed
  • We are now using 2 real simulated numbers from AWS Pinpoint
• Are we making the database calls that we should be making or if they should stay there and be cleaned up a little bit, especially after the switching over to things in the CSVs
  • Also need to check on the personalization PR that's open and check the user table as well

[ccostino]

• @A-Shumway42 will tackle documentation, code comments, and docstrings
• @terrazoon will tackle the simulated numbers investigation
• @xlorepdarkhelm will tackle the DB investigation and reviewing the final personalization pieces

In addition, @xlorepdarkhelm and anyone else, if there are other things we can or should do to improve our Python code here, or other tooling we should get into place (or configure more), please list them here! I've captured some notes above already.

[ccostino]

All things have been accounted for in the new issues linked above! Closing this one out.