Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions api/notification-service.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,33 @@ paths:
schema:
$ref: '#/components/schemas/Error'

/sms/send:
post:
summary: Send an SMS
operationId: sendSMS
tags:
- sms
requestBody:
description: Parameters of the message to send
required: true
content:
application/json:
schema:
$ref: '#/components/schemas/SendSMSRequest'
responses:
200:
description: OK
content:
application/json:
schema:
type: string
default:
description: unexpected error
content:
application/json:
schema:
$ref: '#/components/schemas/Error'

/notification/slack/send:
post:
summary: Send a Slack message
Expand Down Expand Up @@ -254,6 +281,23 @@ components:
description: Schedule these mesages to go out at the time specified by this UNIX timestamp
format: int64

SendSMSRequest:
type: object
required:
- recipient
- message
properties:
recipient:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

type: string
message:
type: string

SendSMSResponse:
type: object
properties:
message:
type: string

SlackRecipient:
type: object
required:
Expand Down
5 changes: 4 additions & 1 deletion cmd/server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,16 @@ func main() {
EmailApiService := service.NewEmailApiService(config)
EmailApiController := server.NewEmailApiController(EmailApiService)

SmsApiService := service.NewSmsApiService(config)
SmsApiController := server.NewSmsApiController(SmsApiService)

HealthApiService := service.NewHealthApiService(config)
HealthApiController := server.NewHealthApiController(HealthApiService)

NotificationApiService := service.NewNotificationApiService(config)
NotificationApiController := server.NewNotificationApiController(NotificationApiService)

router := server.NewRouter(EmailApiController, HealthApiController, NotificationApiController)
router := server.NewRouter(EmailApiController, SmsApiController, HealthApiController, NotificationApiController)

serverAddress := fmt.Sprintf("0.0.0.0:%d", config.Port)
server := &http.Server{Addr: serverAddress, Handler: router}
Expand Down
18 changes: 18 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ type Config struct {
Port int
SendgridAPIKey string
SlackAPIKey string
TwilioAccID string
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to TwilioAccountID wherever it's used if "Acc" is short for "Account"?

Copy link
Contributor

@dtoki dtoki Apr 9, 2021

Choose a reason for hiding this comment

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

@bmonkman okay to leave the env variables as TWILIO_ACC_ID or should that be updated as well?

TwilioAuthToken string
TwilioPhoneNumber string
GracefulShutdownTimeout time.Duration
StructuredLogging bool
}
Expand All @@ -22,6 +25,9 @@ const (
Port
SendgridAPIKey
SlackAPIKey
TwilioAccID
TwilioAuthToken
TwilioPhoneNumber
GracefulShutdownTimeout
StructuredLogging
)
Expand All @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


viper.SetDefault(TwilioAuthToken, "")
viper.BindEnv(TwilioAuthToken, "TWILIO_AUTH_TOKEN")

viper.SetDefault(TwilioPhoneNumber, "")
viper.BindEnv(TwilioPhoneNumber, "TWILIO_PHONE_NUMBER")

viper.SetDefault(GracefulShutdownTimeout, "10")
viper.BindEnv(GracefulShutdownTimeout, "GRACEFUL_SHUTDOWN_TIMEOUT_SECONDS")

Expand All @@ -54,6 +69,9 @@ func loadConfig() *Config {
Port: viper.GetInt(Port),
SendgridAPIKey: viper.GetString(SendgridAPIKey),
SlackAPIKey: viper.GetString(SlackAPIKey),
TwilioAccID: viper.GetString(TwilioAccID),
TwilioAuthToken: viper.GetString(TwilioAuthToken),
TwilioPhoneNumber: viper.GetString(TwilioPhoneNumber),
GracefulShutdownTimeout: viper.GetDuration(GracefulShutdownTimeout),
StructuredLogging: viper.GetBool(StructuredLogging),
}
Expand Down
61 changes: 61 additions & 0 deletions internal/service/api_sms_service.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package service

import (
"context"
"fmt"
"net/http"
"net/url"
"strings"

"github.com/commitdev/zero-notification-service/internal/config"
"github.com/commitdev/zero-notification-service/internal/server"
)

// EmailApiService is a service that implents the logic for the EmailApiServicer
// This service should implement the business logic for every endpoint for the EmailApi API.
// Include any external packages or services that will be required by this service.
type SmsApiService struct {
config *config.Config
}

// NewSmsApiService creates a default api service
func NewSmsApiService(c *config.Config) server.SmsApiServicer {
return &SmsApiService{c}
}

// SendSMS - Send an email
func (s *SmsApiService) SendSMS(ctx context.Context, sendSMSRequest server.SendSmsRequest) (server.ImplResponse, error) {
// Set initial variables
accountSid := s.config.TwilioAccID
authToken := s.config.TwilioAuthToken
urlStr := "https://api.twilio.com/2010-04-01/Accounts/" + accountSid + "/Messages.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use fmt.Sprintf()


// Build out the data for our message
v := url.Values{}
v.Set("To", sendSMSRequest.Recipient)
v.Set("From", s.config.TwilioPhoneNumber)
v.Set("Body", sendSMSRequest.Message)
rb := *strings.NewReader(v.Encode())

// Create Client
client := &http.Client{}

req, _ := http.NewRequest("POST", urlStr, &rb)
req.SetBasicAuth(accountSid, authToken)
req.Header.Add("Accept", "application/json")
req.Header.Add("Content-Type", "application/x-www-form-urlencoded")

// Make request
resp, err := client.Do(req)
fmt.Print(resp.Status)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove


if err != nil {
fmt.Printf("Error sending SMS: %v\n", resp)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

return server.Response(http.StatusBadRequest, server.SendSmsResponse{Message: "Error sending SMS"}), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
return server.Response(http.StatusOK, server.SendSmsResponse{Message: "SMS Sent"}), nil
}