Twilio sms #15
Twilio sms #15
Conversation
|
Running docker run openapitools/openapi-generator-cli:v5.0.0-beta <openapi args> |
| - recipient | ||
| - message | ||
| properties: | ||
| recipient: |
There was a problem hiding this comment.
Could you indicate that the recipient is a phone number either by adding a description or by changing the name to something like recipientPhoneNumber? Or if it's possible for a recipient to be something other than a phone number, give some indictation here so the consumer knows what this is supposed to contain.
| @@ -11,6 +11,9 @@ type Config struct { | |||
| Port int | |||
| SendgridAPIKey string | |||
| SlackAPIKey string | |||
| TwilioAccID string | |||
There was a problem hiding this comment.
Can you change this to TwilioAccountID wherever it's used if "Acc" is short for "Account"?
There was a problem hiding this comment.
@bmonkman okay to leave the env variables as TWILIO_ACC_ID or should that be updated as well?
| @@ -44,6 +50,15 @@ func loadConfig() *Config { | |||
| viper.SetDefault(SlackAPIKey, "") | |||
| viper.BindEnv(SlackAPIKey, "SLACK_API_KEY") | |||
|
|
|||
| viper.SetDefault(TwilioAccID, "") | |||
| viper.BindEnv(TwilioAccID, "TWILIO_ACC_ID") | |||
There was a problem hiding this comment.
Please add these env vars to the helm chart as well. They should be referenced in values.yaml and then added to secret.yaml, and then the chart version can be bumped in Chart.yaml.
This will allow us to use these options immediately in staging/production systems running in Kubernetes.
| // Set initial variables | ||
| accountSid := s.config.TwilioAccID | ||
| authToken := s.config.TwilioAuthToken | ||
| urlStr := "https://api.twilio.com/2010-04-01/Accounts/" + accountSid + "/Messages.json" |
| fmt.Print(resp.Status) | ||
|
|
||
| if err != nil { | ||
| fmt.Printf("Error sending SMS: %v\n", resp) |
There was a problem hiding this comment.
Should use Zap logger, for example:
zap.S().Errorf("Error sending SMS: %v", resp)
| return server.Response(http.StatusInternalServerError, nil), fmt.Errorf("Unable to send SMS: %v", err) | ||
| } | ||
| if !(resp.StatusCode >= 200 && resp.StatusCode <= 299) { | ||
| fmt.Printf("Failure from Twilio when sending SMS: %v", resp) |
|
|
||
| // Make request | ||
| resp, err := client.Do(req) | ||
| fmt.Print(resp.Status) |
| } | ||
| if !(resp.StatusCode >= 200 && resp.StatusCode <= 299) { | ||
| fmt.Printf("Failure from Twilio when sending SMS: %v", resp) | ||
| return server.Response(http.StatusBadRequest, server.SendSmsResponse{Message: "Error sending SMS"}), nil |
There was a problem hiding this comment.
Let's also add the response code and body here so the user can debug. See how it's done in the email service. Also we should standardize on the response code. Email uses StatusInternalServerError where this is using StatusBadRequest. We can't really know if it should be 400-level or 500-level so we really could use either one. Maybe let's stick with Bad Request to encourage the user to check their request, and hopefully the response from Twilio will be enough to tell them if it's actually a server-side misconfiguration.
|
Completed in #25 |
Added Twilio SMS API to provide SMS notification service.
3 env variables are required for this service to function:
TWILIO_ACC_IDTWILIO_AUTH_TOKENTWILIO_PHONE_NUMBER1 new endpoint
POST sms/sendis added. Following input format is expected:{ "recipient": string (valid phone number), "message": string }Following output format can be expected:
{ "message": string }Note that the current
make generatecommand may not work on certain platforms. Trying runningnpx @openapitools/openapi-generator-cli generate -i api/notification-service.yaml -g go-server -o ./ -p sourceFolder=internal/server -p packageName=server --git-user-id=commitdev --git-repo-id=zero-notification-serviceinstead ofopenapi-generator generate -i api/notification-service.yaml -g go-server -o ./ -p sourceFolder=internal/server -p packageName=server --git-user-id=commitdev --git-repo-id=zero-notification-servicein themake generatecommand if on Windows.