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

Add alerts_twilio script, and prepare alerts_gcs script for future usage in a Windmill Flow #59

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rudokemper
Copy link
Member

@rudokemper rudokemper commented Jan 7, 2025

Goal

Closes #32.

The vision here is that these two scripts will be chained together with the CoMapeo alerts script in a Windmill Flow via a future PR:

  1. alerts_gcs runs. If new rows are written to the alerts or metadata tables, then the script returns an alerts_statistics dictionary; if not, it will return None.
  2. comapeo_alerts runs. Dependent on Add script to post alerts to CoMapeo Cloud API #60 being merged.
  3. alerts_twilio runs. I expect that in the Flow I can configure this not to run if alert_gcs does not return anything.

Screenshots

A WhatsApp message I received from our Twilio service bot:

image

What I changed

twilio_message_template

  • I added a twilio_message_template resource type.
  • I opted to create my own rather than use the verified twilio resource in windmill because this resource only provides auth_token and account_sid arguments, whereas we also need content_sid and could provide an additional message_service_sid for metrics tracking.
    • Additionally, I sought fit to add origin_number and recipients so as to bundle all of the relevant Twilio config parameters in one resource: to me, it seems more logical to configure all of the Twilio config (including phone numbers that will be receiving the message) in one screen, rather than to parse those between a resource and the script itself.

alerts_gcs

  • The script now compiles alert_statistics in prepare_alerts_metadata(), consisting of values that are inserted in the Twilio message.
    • Here, we face a conundrum of needing to choose a row to generate alerts statistics for, and need to make an assumption that the alerts provider is always posting new alerts for the last month only. Grounded in that assumption, I am grouping and sorting the alerts metadata Dataframe in descending order by year and month, so as to always have the latest row at iloc[0].
  • The DB Writer now returns a new boolean new_alerts_data which is set to true if any alerts rows or metadata rows were written.
  • If new_alerts_data is true, then the script as a whole returns alerts_statistics, with the expectation that the alerts_twilio script will be able to use this as the next step in a Flow.

alerts_twilio

  • This new script takes three properties:
    • an alerts_statistics object
    • a twilio object referring to the above resource type
    • a territory_name string that represents the slug used for subdomains. For example, demo for the demo, and m**a, t**p, ...
  • The script calls a send_twilio_message function which leverages the Twilio client library to send a message to the API with the expected variables.

Tests

  • I adapted the CSV fixture to include a row with the latest month and year somewhere in the middle, and added a test to assert that alerts_statistics nevertheless returns values from this new row (to test the Pandas Dataframe sorting behavior).
  • The alerts_gcs e2e test now checks to see the script returns an alerts_statistics the first time, and None the second time (since this second run will update 0 alerts and alerts__metadata rows, so the script should not return anything).

What I'm not doing here

  • Mocking or testing the Twilio API, since our usage of the API is entirely orthodox.
  • Setting up the Flow itself that will chain together alerts_gcs, the CoMapeo POST alerts script to be merged from Add script to post alerts to CoMapeo Cloud API #60, and alerts_twilio. When I do that, I expect that I will make some changes to what alerts_gcs actually returns in order to trigger runs for the other two scripts.

@rudokemper rudokemper requested a review from IamJeffG January 7, 2025 16:08
@@ -53,6 +55,21 @@ schema:
will be retrieved. This ID is used to filter and process relevant
alerts.
default: null
territory_name:
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not convinced on this variable name, as it is ambiguous.

A community operator might misunderstand this to refer to the real name of their territory, not the GC subdomain slug e.g. {slug}.guardianconnector.net.

On the other hand, "slug" is not a commonly known term for non-technical users.

Maybe "subdomain" is a better choice.

@rudokemper rudokemper reopened this Jan 16, 2025
@rudokemper rudokemper changed the title Add optional Twilio message operation to alerts script Add alerts_twilio script, and prepare alerts_gcs script for future usage in a Windmill Flow Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[alerts] Add an optional Twilio operation
1 participant