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

Update M* handlers to use new send #699

Merged
merged 17 commits into from
Feb 23, 2024
Merged

Update M* handlers to use new send #699

merged 17 commits into from
Feb 23, 2024

Conversation

norkans7
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Attention: Patch coverage is 48.57143% with 72 lines in your changes are missing coverage. Please review.

Project coverage is 74.73%. Comparing base (34cdc2f) to head (d15ff31).
Report is 21 commits behind head on main.

Files Patch % Lines
handlers/meta/handlers.go 51.42% 24 Missing and 10 partials ⚠️
handlers/messangi/handler.go 36.36% 4 Missing and 3 partials ⚠️
handlers/m3tech/handler.go 25.00% 3 Missing and 3 partials ⚠️
handlers/mtarget/handler.go 45.45% 3 Missing and 3 partials ⚠️
handlers/macrokiosk/handler.go 54.54% 3 Missing and 2 partials ⚠️
handlers/mblox/handler.go 50.00% 3 Missing and 2 partials ⚠️
handlers/mtn/handler.go 44.44% 3 Missing and 2 partials ⚠️
handlers/messagebird/handler.go 60.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #699      +/-   ##
==========================================
+ Coverage   73.89%   74.73%   +0.84%     
==========================================
  Files          98       98              
  Lines       13053    13115      +62     
==========================================
+ Hits         9645     9802     +157     
+ Misses       2722     2626      -96     
- Partials      686      687       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

clog.Error(courier.ErrorResponseValueUnexpected("status", "OK"))
break
return courier.ErrFailedWithReason("", "response returned non-OK status")
Copy link
Member

Choose a reason for hiding this comment

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

seems like both of these can be replaced with return courier.ErrResponseStatus

}

if respPayload.ExternalID == "" {
clog.Error(courier.ErrorResponseValueMissing("message_id"))
return status, nil
return courier.ErrFailedWithReason("", "response missing message_id")
Copy link
Member

Choose a reason for hiding this comment

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

ErrFailedWithReason is specifically for when the channel gives an external error code..

both of these lines could just be replaced with return courier.ErrResponseUnexpected

recipientID := respPayload.RecipientID
if recipientID == "" {
clog.Error(courier.ErrorResponseValueMissing("recipient_id"))
return courier.ErrFailedWithReason("", "response missing recipient_id")
Copy link
Member

Choose a reason for hiding this comment

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

see above

@@ -885,7 +873,7 @@ func (h *handler) sendWhatsAppMsg(ctx context.Context, msg courier.MsgOut, clog

payload.Interactive = &interactive
} else {
return nil, fmt.Errorf("too many quick replies WAC supports only up to 10 quick replies")
return fmt.Errorf("too many quick replies WAC supports only up to 10 quick replies")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be a terminal error - we can send the message with 10 quick replies and add a channel log error to say that

@@ -1040,7 +1025,7 @@ func (h *handler) sendWhatsAppMsg(ctx context.Context, msg courier.MsgOut, clog

payload.Interactive = &interactive
} else {
return nil, fmt.Errorf("too many quick replies WAC supports only up to 10 quick replies")
return fmt.Errorf("too many quick replies WAC supports only up to 10 quick replies")
Copy link
Member

Choose a reason for hiding this comment

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

see above

func setSendURL(s *httptest.Server, h courier.ChannelHandler, c courier.Channel, m courier.MsgOut) {
sendURL = s.URL
}

var defaultSendTestCases = []OutgoingTestCase{
{Label: "Plain Send",
Copy link
Member

Choose a reason for hiding this comment

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

nice to fix this formatting while you're in here

@rowanseymour rowanseymour merged commit c6ea483 into main Feb 23, 2024
6 of 7 checks passed
@rowanseymour rowanseymour deleted the m-sends branch February 23, 2024 14:53
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants