From 9d7e7447a1bf54e78c18599e6d3d17df95290e13 Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Mon, 23 Aug 2021 14:09:00 -0500 Subject: [PATCH 1/4] Use synchronous answering machine detection on Twilio channels --- core/ivr/twiml/twiml.go | 33 ++++------- web/ivr/ivr_test.go | 121 ++++++++++++---------------------------- 2 files changed, 47 insertions(+), 107 deletions(-) diff --git a/core/ivr/twiml/twiml.go b/core/ivr/twiml/twiml.go index a54b51479..5cb05f29c 100644 --- a/core/ivr/twiml/twiml.go +++ b/core/ivr/twiml/twiml.go @@ -6,7 +6,6 @@ import ( "crypto/hmac" "crypto/sha1" "encoding/base64" - "encoding/json" "encoding/xml" "fmt" "net/http" @@ -177,8 +176,9 @@ func (c *client) URNForRequest(r *http.Request) (urns.URN, error) { // CallResponse is our struct for a Twilio call response type CallResponse struct { - SID string `json:"sid"` - Status string `json:"status"` + SID string `json:"sid" validate:"required"` + Status string `json:"status"` + AnsweredBy string `json:"answered_by"` } // RequestCall causes this client to request a new outgoing call for this provider @@ -191,8 +191,6 @@ func (c *client) RequestCall(number urns.URN, callbackURL string, statusURL stri if machineDetection { form.Set("MachineDetection", "Enable") - form.Set("AsyncAmd", "true") - form.Set("AsyncAmdStatusCallback", statusURL) } sendURL := c.baseURL + strings.Replace(callPath, "{AccountSID}", c.accountSID, -1) @@ -206,16 +204,17 @@ func (c *client) RequestCall(number urns.URN, callbackURL string, statusURL stri return ivr.NilCallID, trace, errors.Errorf("received non 201 status for call start: %d", trace.Response.StatusCode) } - // parse out our call sid + // parse the response from Twilio call := &CallResponse{} - err = json.Unmarshal(trace.ResponseBody, call) - if err != nil || call.SID == "" { - return ivr.NilCallID, trace, errors.Errorf("unable to read call id") + if err := utils.UnmarshalAndValidate(trace.ResponseBody, call); err != nil { + return ivr.NilCallID, trace, errors.Wrap(err, "unable parse Twilio response") } - if call.Status == statusFailed { return ivr.NilCallID, trace, errors.Errorf("call status returned as failed") } + if call.AnsweredBy == "machine_start" || call.AnsweredBy == "fax" { + return ivr.NilCallID, trace, errors.Errorf("call answered by machine") + } return ivr.CallID(call.SID), trace, nil } @@ -293,25 +292,13 @@ func (c *client) ResumeForRequest(r *http.Request) (ivr.Resume, error) { // StatusForRequest returns the call status for the passed in request, and if it's an error the reason, // and if available, the current call duration func (c *client) StatusForRequest(r *http.Request) (models.ConnectionStatus, models.ConnectionError, int) { - // we re-use our status callback for AMD results which will have an AnsweredBy field but no CallStatus field - answeredBy := r.Form.Get("AnsweredBy") - if answeredBy != "" { - switch answeredBy { - case "machine_start", "fax": - return models.ConnectionStatusErrored, models.ConnectionErrorMachine, 0 - } - return models.ConnectionStatusInProgress, "", 0 - } - status := r.Form.Get("CallStatus") switch status { case "queued", "ringing": return models.ConnectionStatusWired, "", 0 - case "in-progress", "initiated": return models.ConnectionStatusInProgress, "", 0 - case "completed": duration, _ := strconv.Atoi(r.Form.Get("CallDuration")) return models.ConnectionStatusCompleted, "", duration @@ -321,7 +308,7 @@ func (c *client) StatusForRequest(r *http.Request) (models.ConnectionStatus, mod case "no-answer": return models.ConnectionStatusErrored, models.ConnectionErrorNoAnswer, 0 case "canceled", "failed": - return models.ConnectionStatusErrored, "", 0 + return models.ConnectionStatusErrored, models.ConnectionErrorProvider, 0 default: logrus.WithField("call_status", status).Error("unknown call status in status callback") diff --git a/web/ivr/ivr_test.go b/web/ivr/ivr_test.go index 2ef31f101..2e7aa0a5e 100644 --- a/web/ivr/ivr_test.go +++ b/web/ivr/ivr_test.go @@ -123,8 +123,6 @@ func TestTwilioIVR(t *testing.T) { testdata.Cathy.ID, models.ConnectionStatusWired, "Call1").Returns(1) testsuite.AssertQuery(t, db, `SELECT COUNT(*) FROM channels_channelconnection WHERE contact_id = $1 AND status = $2 AND external_id = $3`, testdata.Bob.ID, models.ConnectionStatusWired, "Call2").Returns(1) - testsuite.AssertQuery(t, db, `SELECT COUNT(*) FROM channels_channelconnection WHERE contact_id = $1 AND status = $2 AND external_id = $3`, - testdata.George.ID, models.ConnectionStatusWired, "Call3").Returns(1) tcs := []struct { label string @@ -252,51 +250,6 @@ func TestTwilioIVR(t *testing.T) { expectedResponse: `An error has occurred, please try again later.`, expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "W"}, }, - { - label: "call3 started", - url: fmt.Sprintf("/ivr/c/%s/handle?action=start&connection=3", testdata.TwilioChannel.UUID), - form: nil, - expectedStatus: 200, - expectedContains: []string{`Hello there. Please enter one or two. This flow was triggered by Cathy`}, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "I"}, - }, - { - label: "answer machine detection sent to tell us we're talking to a voicemail", - url: fmt.Sprintf("/ivr/c/%s/status", testdata.TwilioChannel.UUID), - form: url.Values{ - "CallSid": []string{"Call3"}, - "AccountSid": []string{"sid"}, - "AnsweredBy": []string{"machine_start"}, - "MachineDetectionDuration": []string{"2000"}, - }, - expectedStatus: 200, - expectedContains: []string{"An error has occurred, please try again later.`, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E"}, - }, - { - label: "then Twilio will call the status callback to say that we're done but don't overwrite the error status", - url: fmt.Sprintf("/ivr/c/%s/status", testdata.TwilioChannel.UUID), - form: url.Values{ - "CallSid": []string{"Call3"}, - "CallStatus": []string{"completed"}, - "CallDuration": []string{"50"}, - }, - expectedStatus: 200, - expectedResponse: ``, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E"}, - }, { label: "we don't have a call trigger so incoming call creates a missed call event", url: fmt.Sprintf("/ivr/c/%s/incoming", testdata.TwilioChannel.UUID), @@ -307,7 +260,7 @@ func TestTwilioIVR(t *testing.T) { }, expectedStatus: 200, expectedResponse: ``, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E", "Call4": "I"}, + expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "W", "Call4": "I"}, }, { label: "", @@ -319,7 +272,7 @@ func TestTwilioIVR(t *testing.T) { }, expectedStatus: 200, expectedResponse: ``, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E", "Call4": "F"}, + expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "W", "Call4": "F"}, }, } @@ -363,8 +316,41 @@ func TestTwilioIVR(t *testing.T) { testsuite.AssertQuery(t, db, `SELECT count(*) FROM msgs_msg WHERE contact_id = $1 AND msg_type = 'V' AND connection_id = 2 AND ((status = 'H' AND direction = 'I') OR (status = 'W' AND direction = 'O'))`, testdata.Bob.ID).Returns(2) +} - testsuite.AssertQuery(t, db, `SELECT status, error_reason FROM channels_channelconnection WHERE contact_id = $1 AND next_attempt IS NOT NULL`, testdata.George.ID).Columns(map[string]interface{}{"status": "E", "error_reason": "M"}) +func mockVonageHandler(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("recording") != "" { + w.WriteHeader(http.StatusOK) + w.Write([]byte{}) + } else { + type CallForm struct { + To []struct { + Number string `json:"number"` + } `json:"to"` + Action string `json:"action,omitempty"` + } + body, _ := io.ReadAll(r.Body) + form := &CallForm{} + json.Unmarshal(body, form) + logrus.WithField("method", r.Method).WithField("url", r.URL.String()).WithField("body", string(body)).WithField("form", form).Info("test server called") + + // end of a leg + if form.Action == "transfer" { + w.WriteHeader(http.StatusNoContent) + } else if form.To[0].Number == "16055741111" { + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{ "uuid": "Call1","status": "started","direction": "outbound","conversation_uuid": "Conversation1"}`)) + } else if form.To[0].Number == "16055743333" { + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{ "uuid": "Call2","status": "started","direction": "outbound","conversation_uuid": "Conversation2"}`)) + } else if form.To[0].Number == "2065551212" { + // start of a transfer leg + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{ "uuid": "Call3","status": "started","direction": "outbound","conversation_uuid": "Conversation3"}`)) + } else { + w.WriteHeader(http.StatusNotFound) + } + } } func TestVonageIVR(t *testing.T) { @@ -384,40 +370,7 @@ func TestVonageIVR(t *testing.T) { db.MustExec(`UPDATE channels_channel SET config = '{"nexmo_app_id": "app_id", "nexmo_app_private_key": "-----BEGIN PRIVATE KEY-----\nMIICdgIBADANBgkqhkiG9w0BAQEFAASCAmAwggJcAgEAAoGBAKNwapOQ6rQJHetP\nHRlJBIh1OsOsUBiXb3rXXE3xpWAxAha0MH+UPRblOko+5T2JqIb+xKf9Vi3oTM3t\nKvffaOPtzKXZauscjq6NGzA3LgeiMy6q19pvkUUOlGYK6+Xfl+B7Xw6+hBMkQuGE\nnUS8nkpR5mK4ne7djIyfHFfMu4ptAgMBAAECgYA+s0PPtMq1osG9oi4xoxeAGikf\nJB3eMUptP+2DYW7mRibc+ueYKhB9lhcUoKhlQUhL8bUUFVZYakP8xD21thmQqnC4\nf63asad0ycteJMLb3r+z26LHuCyOdPg1pyLk3oQ32lVQHBCYathRMcVznxOG16VK\nI8BFfstJTaJu0lK/wQJBANYFGusBiZsJQ3utrQMVPpKmloO2++4q1v6ZR4puDQHx\nTjLjAIgrkYfwTJBLBRZxec0E7TmuVQ9uJ+wMu/+7zaUCQQDDf2xMnQqYknJoKGq+\noAnyC66UqWC5xAnQS32mlnJ632JXA0pf9pb1SXAYExB1p9Dfqd3VAwQDwBsDDgP6\nHD8pAkEA0lscNQZC2TaGtKZk2hXkdcH1SKru/g3vWTkRHxfCAznJUaza1fx0wzdG\nGcES1Bdez0tbW4llI5By/skZc2eE3QJAFl6fOskBbGHde3Oce0F+wdZ6XIJhEgCP\niukIcKZoZQzoiMJUoVRrA5gqnmaYDI5uRRl/y57zt6YksR3KcLUIuQJAd242M/WF\n6YAZat3q/wEeETeQq1wrooew+8lHl05/Nt0cCpV48RGEhJ83pzBm3mnwHf8lTBJH\nx6XroMXsmbnsEw==\n-----END PRIVATE KEY-----", "callback_domain": "localhost:8090"}', role='SRCA' WHERE id = $1`, testdata.VonageChannel.ID) // start test server - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Query().Get("recording") != "" { - w.WriteHeader(http.StatusOK) - w.Write([]byte{}) - } else { - type CallForm struct { - To []struct { - Number string `json:"number"` - } `json:"to"` - Action string `json:"action,omitempty"` - } - body, _ := io.ReadAll(r.Body) - form := &CallForm{} - json.Unmarshal(body, form) - logrus.WithField("method", r.Method).WithField("url", r.URL.String()).WithField("body", string(body)).WithField("form", form).Info("test server called") - - // end of a leg - if form.Action == "transfer" { - w.WriteHeader(http.StatusNoContent) - } else if form.To[0].Number == "16055741111" { - w.WriteHeader(http.StatusCreated) - w.Write([]byte(`{ "uuid": "Call1","status": "started","direction": "outbound","conversation_uuid": "Conversation1"}`)) - } else if form.To[0].Number == "16055743333" { - w.WriteHeader(http.StatusCreated) - w.Write([]byte(`{ "uuid": "Call2","status": "started","direction": "outbound","conversation_uuid": "Conversation2"}`)) - } else if form.To[0].Number == "2065551212" { - // start of a transfer leg - w.WriteHeader(http.StatusCreated) - w.Write([]byte(`{ "uuid": "Call3","status": "started","direction": "outbound","conversation_uuid": "Conversation3"}`)) - } else { - w.WriteHeader(http.StatusNotFound) - } - } - })) + ts := httptest.NewServer(http.HandlerFunc(mockVonageHandler)) defer ts.Close() wg := &sync.WaitGroup{} From c748bc916552e701beb78fd1a2c4602ba0b22eea Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Tue, 24 Aug 2021 12:10:34 -0500 Subject: [PATCH 2/4] Handle AMD result in first callback rather than call request response --- core/ivr/ivr.go | 26 ++++++++++++++++++++++---- core/ivr/twiml/twiml.go | 17 +++++++++++------ core/ivr/vonage/vonage.go | 4 ++++ core/tasks/ivr/worker_test.go | 4 ++++ web/ivr/ivr_test.go | 27 +++++++++++++++++++++++++-- 5 files changed, 66 insertions(+), 12 deletions(-) diff --git a/core/ivr/ivr.go b/core/ivr/ivr.go index 79202ec5d..5b5f53c5a 100644 --- a/core/ivr/ivr.go +++ b/core/ivr/ivr.go @@ -9,6 +9,7 @@ import ( "strconv" "time" + "github.com/nyaruka/gocommon/dates" "github.com/nyaruka/gocommon/httpx" "github.com/nyaruka/gocommon/storage" "github.com/nyaruka/gocommon/urns" @@ -82,6 +83,9 @@ type Client interface { // and if available, the current call duration StatusForRequest(r *http.Request) (models.ConnectionStatus, models.ConnectionError, int) + // CheckStartRequest checks the start request from the provider is as we expect and if not returns an error reason + CheckStartRequest(r *http.Request) models.ConnectionError + PreprocessResume(ctx context.Context, db *sqlx.DB, rp *redis.Pool, conn *models.ChannelConnection, r *http.Request) ([]byte, error) PreprocessStatus(ctx context.Context, db *sqlx.DB, rp *redis.Pool, r *http.Request) ([]byte, error) @@ -302,7 +306,7 @@ func StartIVRFlow( channel *models.Channel, conn *models.ChannelConnection, c *models.Contact, urn urns.URN, startID models.StartID, r *http.Request, w http.ResponseWriter) error { - // connection isn't in a wired status, that's an error + // connection isn't in a wired or in-progress status then we shouldn't be here if conn.Status() != models.ConnectionStatusWired && conn.Status() != models.ConnectionStatusInProgress { return WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Errorf("connection in invalid state: %s", conn.Status())) } @@ -312,12 +316,26 @@ func StartIVRFlow( if err != nil { return errors.Wrapf(err, "unable to load start: %d", startID) } - flow, err := oa.FlowByID(start.FlowID()) if err != nil { return errors.Wrapf(err, "unable to load flow: %d", startID) } + // check that call on provider side is in the state we need to continue + if errorReason := client.CheckStartRequest(r); errorReason != "" { + err := conn.MarkErrored(ctx, rt.DB, dates.Now(), flow.IVRRetryWait(), errorReason) + if err != nil { + return errors.Wrap(err, "unable to mark connection as errored") + } + + errMsg := fmt.Sprintf("status updated: %s", conn.Status()) + if conn.Status() == models.ConnectionStatusErrored { + errMsg = fmt.Sprintf("%s, next_attempt: %s", errMsg, conn.NextAttempt()) + } + + return client.WriteErrorResponse(w, errors.New(errMsg)) + } + // our flow contact contact, err := c.FlowContact(oa) if err != nil { @@ -620,10 +638,10 @@ func HandleIVRStatus(ctx context.Context, rt *runtime.Runtime, oa *models.OrgAss return errors.Wrapf(err, "unable to load flow: %d", start.FlowID()) } - conn.MarkErrored(ctx, rt.DB, time.Now(), flow.IVRRetryWait(), errorReason) + conn.MarkErrored(ctx, rt.DB, dates.Now(), flow.IVRRetryWait(), errorReason) if conn.Status() == models.ConnectionStatusErrored { - return client.WriteEmptyResponse(w, fmt.Sprintf("status updated: %s next_attempt: %s", conn.Status(), conn.NextAttempt())) + return client.WriteEmptyResponse(w, fmt.Sprintf("status updated: %s, next_attempt: %s", conn.Status(), conn.NextAttempt())) } } else if status == models.ConnectionStatusFailed { diff --git a/core/ivr/twiml/twiml.go b/core/ivr/twiml/twiml.go index 5cb05f29c..e9efbf1ca 100644 --- a/core/ivr/twiml/twiml.go +++ b/core/ivr/twiml/twiml.go @@ -148,6 +148,15 @@ func (c *client) DownloadMedia(url string) (*http.Response, error) { return http.Get(url) } +func (c *client) CheckStartRequest(r *http.Request) models.ConnectionError { + r.ParseForm() + answeredBy := r.Form.Get("AnsweredBy") + if answeredBy == "machine_start" || answeredBy == "fax" { + return models.ConnectionErrorMachine + } + return "" +} + func (c *client) PreprocessStatus(ctx context.Context, db *sqlx.DB, rp *redis.Pool, r *http.Request) ([]byte, error) { return nil, nil } @@ -176,9 +185,8 @@ func (c *client) URNForRequest(r *http.Request) (urns.URN, error) { // CallResponse is our struct for a Twilio call response type CallResponse struct { - SID string `json:"sid" validate:"required"` - Status string `json:"status"` - AnsweredBy string `json:"answered_by"` + SID string `json:"sid" validate:"required"` + Status string `json:"status"` } // RequestCall causes this client to request a new outgoing call for this provider @@ -212,9 +220,6 @@ func (c *client) RequestCall(number urns.URN, callbackURL string, statusURL stri if call.Status == statusFailed { return ivr.NilCallID, trace, errors.Errorf("call status returned as failed") } - if call.AnsweredBy == "machine_start" || call.AnsweredBy == "fax" { - return ivr.NilCallID, trace, errors.Errorf("call answered by machine") - } return ivr.CallID(call.SID), trace, nil } diff --git a/core/ivr/vonage/vonage.go b/core/ivr/vonage/vonage.go index 80783b55a..fda8a257e 100644 --- a/core/ivr/vonage/vonage.go +++ b/core/ivr/vonage/vonage.go @@ -170,6 +170,10 @@ func (c *client) DownloadMedia(url string) (*http.Response, error) { return http.DefaultClient.Do(req) } +func (c *client) CheckStartRequest(r *http.Request) models.ConnectionError { + return "" +} + func (c *client) PreprocessStatus(ctx context.Context, db *sqlx.DB, rp *redis.Pool, r *http.Request) ([]byte, error) { // parse out the call status, we are looking for a leg of one of our conferences ending in the "forward" case // get our recording url out diff --git a/core/tasks/ivr/worker_test.go b/core/tasks/ivr/worker_test.go index c0958db76..deac9c75a 100644 --- a/core/tasks/ivr/worker_test.go +++ b/core/tasks/ivr/worker_test.go @@ -106,6 +106,10 @@ func (c *MockClient) StatusForRequest(r *http.Request) (models.ConnectionStatus, return models.ConnectionStatusFailed, models.ConnectionErrorProvider, 10 } +func (c *MockClient) CheckStartRequest(r *http.Request) models.ConnectionError { + return "" +} + func (c *MockClient) PreprocessResume(ctx context.Context, db *sqlx.DB, rp *redis.Pool, conn *models.ChannelConnection, r *http.Request) ([]byte, error) { return nil, nil } diff --git a/web/ivr/ivr_test.go b/web/ivr/ivr_test.go index 2e7aa0a5e..5f0e50f1d 100644 --- a/web/ivr/ivr_test.go +++ b/web/ivr/ivr_test.go @@ -250,6 +250,29 @@ func TestTwilioIVR(t *testing.T) { expectedResponse: `An error has occurred, please try again later.`, expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "W"}, }, + { + label: "call3 started with answered_by telling us it's a machine", + url: fmt.Sprintf("/ivr/c/%s/handle?action=start&connection=3", testdata.TwilioChannel.UUID), + form: url.Values{ + "CallStatus": []string{"in-progress"}, + "AnsweredBy": []string{"machine_start"}, + }, + expectedStatus: 200, + expectedContains: []string{``, + expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E"}, + }, { label: "we don't have a call trigger so incoming call creates a missed call event", url: fmt.Sprintf("/ivr/c/%s/incoming", testdata.TwilioChannel.UUID), @@ -260,7 +283,7 @@ func TestTwilioIVR(t *testing.T) { }, expectedStatus: 200, expectedResponse: ``, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "W", "Call4": "I"}, + expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E", "Call4": "I"}, }, { label: "", @@ -272,7 +295,7 @@ func TestTwilioIVR(t *testing.T) { }, expectedStatus: 200, expectedResponse: ``, - expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "W", "Call4": "F"}, + expectedConnStatus: map[string]string{"Call1": "D", "Call2": "D", "Call3": "E", "Call4": "F"}, }, } From cb9c0897c9eb2093c27448f98d5af8f66c9c2833 Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Tue, 24 Aug 2021 12:32:11 -0500 Subject: [PATCH 3/4] Rename WriteErrorResponse to HandleAsFailure for clarity --- core/ivr/ivr.go | 24 ++++++++---------------- web/ivr/ivr.go | 20 ++++++-------------- web/ivr/ivr_test.go | 2 ++ 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/core/ivr/ivr.go b/core/ivr/ivr.go index 5b5f53c5a..401d69ddc 100644 --- a/core/ivr/ivr.go +++ b/core/ivr/ivr.go @@ -41,9 +41,6 @@ const ( ErrorMessage = "An error has occurred, please try again later." ) -// CallEndedError is our constant error for when a call has ended -var CallEndedError = fmt.Errorf("call ended") - // our map of client constructors var constructors = make(map[models.ChannelType]ClientConstructor) @@ -291,11 +288,11 @@ func RequestCallStartForConnection(ctx context.Context, config *config.Config, d return nil } -// WriteErrorResponse marks the passed in connection as errored and writes the appropriate error response to our writer -func WriteErrorResponse(ctx context.Context, db *sqlx.DB, client Client, conn *models.ChannelConnection, w http.ResponseWriter, rootErr error) error { +// HandleAsFailure marks the passed in connection as errored and writes the appropriate error response to our writer +func HandleAsFailure(ctx context.Context, db *sqlx.DB, client Client, conn *models.ChannelConnection, w http.ResponseWriter, rootErr error) error { err := conn.MarkFailed(ctx, db, time.Now()) if err != nil { - logrus.WithError(err).Error("error when trying to mark connection as errored") + logrus.WithError(err).Error("error marking connection as failed") } return client.WriteErrorResponse(w, rootErr) } @@ -308,7 +305,7 @@ func StartIVRFlow( // connection isn't in a wired or in-progress status then we shouldn't be here if conn.Status() != models.ConnectionStatusWired && conn.Status() != models.ConnectionStatusInProgress { - return WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Errorf("connection in invalid state: %s", conn.Status())) + return HandleAsFailure(ctx, rt.DB, client, conn, w, errors.Errorf("connection in invalid state: %s", conn.Status())) } // get the flow for our start @@ -426,14 +423,14 @@ func ResumeIVRFlow( } if session == nil { - return WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Errorf("no active IVR session for contact")) + return HandleAsFailure(ctx, rt.DB, client, conn, w, errors.Errorf("no active IVR session for contact")) } if session.ConnectionID() == nil { - return WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Errorf("active session: %d has no connection", session.ID())) + return HandleAsFailure(ctx, rt.DB, client, conn, w, errors.Errorf("active session: %d has no connection", session.ID())) } if *session.ConnectionID() != conn.ID() { - return WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Errorf("active session: %d does not match connection: %d", session.ID(), *session.ConnectionID())) + return HandleAsFailure(ctx, rt.DB, client, conn, w, errors.Errorf("active session: %d does not match connection: %d", session.ID(), *session.ConnectionID())) } // check if connection has been marked as errored - it maybe have been updated by status callback @@ -480,12 +477,7 @@ func ResumeIVRFlow( // get the input of our request ivrResume, err := client.ResumeForRequest(r) if err != nil { - // call has ended, so will our session - if err == CallEndedError { - WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Wrapf(err, "call already ended")) - } - - return WriteErrorResponse(ctx, rt.DB, client, conn, w, errors.Wrapf(err, "error finding input for request")) + return HandleAsFailure(ctx, rt.DB, client, conn, w, errors.Wrapf(err, "error finding input for request")) } var resume flows.Resume diff --git a/web/ivr/ivr.go b/web/ivr/ivr.go index c77b256b3..3cf5bd4ff 100644 --- a/web/ivr/ivr.go +++ b/web/ivr/ivr.go @@ -3,13 +3,13 @@ package ivr import ( "context" "database/sql" - "encoding/json" "fmt" "net/http" "net/url" "time" "github.com/nyaruka/gocommon/httpx" + "github.com/nyaruka/gocommon/jsonx" "github.com/nyaruka/gocommon/urns" "github.com/nyaruka/goflow/assets" "github.com/nyaruka/mailroom/config" @@ -173,7 +173,7 @@ func handleIncomingCall(ctx context.Context, rt *runtime.Runtime, r *http.Reques } // try to handle it, this time looking for a missed call event - session, err = handler.HandleChannelEvent(ctx, rt, models.MOMissEventType, event, nil) + _, err = handler.HandleChannelEvent(ctx, rt, models.MOMissEventType, event, nil) if err != nil { logrus.WithError(err).WithField("http_request", r).Error("error handling missed call") return channel, conn, client.WriteErrorResponse(w, errors.Wrapf(err, "error handling missed call")) @@ -196,19 +196,11 @@ type IVRRequest struct { } // writeClientError is just a small utility method to write out a simple JSON error when we don't have a client yet -// to do it on our behalf func writeClientError(w http.ResponseWriter, err error) error { w.Header().Set("Content-type", "application/json") w.WriteHeader(http.StatusBadRequest) - response := map[string]string{ - "error": err.Error(), - } - serialized, err := json.Marshal(response) - if err != nil { - return errors.Wrapf(err, "error serializing error") - } - _, err = w.Write([]byte(serialized)) - return errors.Wrapf(err, "error writing error") + _, err = w.Write(jsonx.MustMarshal(map[string]string{"error": err.Error()})) + return err } func buildResumeURL(cfg *config.Config, channel *models.Channel, conn *models.ChannelConnection, urn urns.URN) string { @@ -322,7 +314,7 @@ func handleFlow(ctx context.Context, rt *runtime.Runtime, r *http.Request, w htt // had an error? mark our connection as errored and log it if err != nil { logrus.WithError(err).WithField("http_request", r).Error("error while handling IVR") - return channel, conn, ivr.WriteErrorResponse(ctx, rt.DB, client, conn, w, err) + return channel, conn, ivr.HandleAsFailure(ctx, rt.DB, client, conn, w, err) } return channel, conn, nil @@ -397,7 +389,7 @@ func handleStatus(ctx context.Context, rt *runtime.Runtime, r *http.Request, w h // had an error? mark our connection as errored and log it if err != nil { logrus.WithError(err).WithField("http_request", r).Error("error while handling status") - return channel, conn, ivr.WriteErrorResponse(ctx, rt.DB, client, conn, w, err) + return channel, conn, ivr.HandleAsFailure(ctx, rt.DB, client, conn, w, err) } return channel, conn, nil diff --git a/web/ivr/ivr_test.go b/web/ivr/ivr_test.go index 5f0e50f1d..142928d7c 100644 --- a/web/ivr/ivr_test.go +++ b/web/ivr/ivr_test.go @@ -123,6 +123,8 @@ func TestTwilioIVR(t *testing.T) { testdata.Cathy.ID, models.ConnectionStatusWired, "Call1").Returns(1) testsuite.AssertQuery(t, db, `SELECT COUNT(*) FROM channels_channelconnection WHERE contact_id = $1 AND status = $2 AND external_id = $3`, testdata.Bob.ID, models.ConnectionStatusWired, "Call2").Returns(1) + testsuite.AssertQuery(t, db, `SELECT COUNT(*) FROM channels_channelconnection WHERE contact_id = $1 AND status = $2 AND external_id = $3`, + testdata.George.ID, models.ConnectionStatusWired, "Call3").Returns(1) tcs := []struct { label string From a642841ae41dd477e3b49eae83446d4dc0dd4c79 Mon Sep 17 00:00:00 2001 From: Rowan Seymour Date: Tue, 24 Aug 2021 13:17:29 -0500 Subject: [PATCH 4/4] Rename writeClientError to writeGenericErrorResponse for clarity --- web/ivr/ivr.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/web/ivr/ivr.go b/web/ivr/ivr.go index 3cf5bd4ff..80acbde18 100644 --- a/web/ivr/ivr.go +++ b/web/ivr/ivr.go @@ -75,25 +75,25 @@ func handleIncomingCall(ctx context.Context, rt *runtime.Runtime, r *http.Reques // load the org id for this UUID (we could load the entire channel here but we want to take the same paths through everything else) orgID, err := models.OrgIDForChannelUUID(ctx, rt.DB, channelUUID) if err != nil { - return nil, nil, writeClientError(w, err) + return nil, nil, writeGenericErrorResponse(w, err) } // load our org assets oa, err := models.GetOrgAssets(ctx, rt.DB, orgID) if err != nil { - return nil, nil, writeClientError(w, errors.Wrapf(err, "error loading org assets")) + return nil, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "error loading org assets")) } // and our channel channel := oa.ChannelByUUID(channelUUID) if channel == nil { - return nil, nil, writeClientError(w, errors.Wrapf(err, "no active channel with uuid: %s", channelUUID)) + return nil, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "no active channel with uuid: %s", channelUUID)) } // get the right kind of client client, err := ivr.GetClient(channel) if client == nil { - return channel, nil, writeClientError(w, errors.Wrapf(err, "unable to load client for channel: %s", channelUUID)) + return channel, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "unable to load client for channel: %s", channelUUID)) } // validate this request's signature @@ -195,8 +195,8 @@ type IVRRequest struct { Action string `form:"action" validate:"required"` } -// writeClientError is just a small utility method to write out a simple JSON error when we don't have a client yet -func writeClientError(w http.ResponseWriter, err error) error { +// writeGenericErrorResponse is just a small utility method to write out a simple JSON error when we don't have a client yet +func writeGenericErrorResponse(w http.ResponseWriter, err error) error { w.Header().Set("Content-type", "application/json") w.WriteHeader(http.StatusBadRequest) _, err = w.Write(jsonx.MustMarshal(map[string]string{"error": err.Error()})) @@ -233,25 +233,25 @@ func handleFlow(ctx context.Context, rt *runtime.Runtime, r *http.Request, w htt // load our org assets oa, err := models.GetOrgAssets(ctx, rt.DB, conn.OrgID()) if err != nil { - return nil, nil, writeClientError(w, errors.Wrapf(err, "error loading org assets")) + return nil, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "error loading org assets")) } // and our channel channel := oa.ChannelByID(conn.ChannelID()) if channel == nil { - return nil, nil, writeClientError(w, errors.Errorf("no active channel with id: %d", conn.ChannelID())) + return nil, nil, writeGenericErrorResponse(w, errors.Errorf("no active channel with id: %d", conn.ChannelID())) } // get the right kind of client client, err := ivr.GetClient(channel) if client == nil { - return channel, conn, writeClientError(w, errors.Wrapf(err, "unable to load client for channel: %d", conn.ChannelID())) + return channel, conn, writeGenericErrorResponse(w, errors.Wrapf(err, "unable to load client for channel: %d", conn.ChannelID())) } // validate this request's signature if relevant err = client.ValidateRequestSignature(r) if err != nil { - return channel, conn, writeClientError(w, errors.Wrapf(err, "request failed signature validation")) + return channel, conn, writeGenericErrorResponse(w, errors.Wrapf(err, "request failed signature validation")) } // load our contact @@ -330,31 +330,31 @@ func handleStatus(ctx context.Context, rt *runtime.Runtime, r *http.Request, w h // load the org id for this UUID (we could load the entire channel here but we want to take the same paths through everything else) orgID, err := models.OrgIDForChannelUUID(ctx, rt.DB, channelUUID) if err != nil { - return nil, nil, writeClientError(w, err) + return nil, nil, writeGenericErrorResponse(w, err) } // load our org assets oa, err := models.GetOrgAssets(ctx, rt.DB, orgID) if err != nil { - return nil, nil, writeClientError(w, errors.Wrapf(err, "error loading org assets")) + return nil, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "error loading org assets")) } // and our channel channel := oa.ChannelByUUID(channelUUID) if channel == nil { - return nil, nil, writeClientError(w, errors.Wrapf(err, "no active channel with uuid: %s", channelUUID)) + return nil, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "no active channel with uuid: %s", channelUUID)) } // get the right kind of client client, err := ivr.GetClient(channel) if client == nil { - return channel, nil, writeClientError(w, errors.Wrapf(err, "unable to load client for channel: %s", channelUUID)) + return channel, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "unable to load client for channel: %s", channelUUID)) } // validate this request's signature if relevant err = client.ValidateRequestSignature(r) if err != nil { - return channel, nil, writeClientError(w, errors.Wrapf(err, "request failed signature validation")) + return channel, nil, writeGenericErrorResponse(w, errors.Wrapf(err, "request failed signature validation")) } // preprocess this status