-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/pn 310 sms ttl #15
base: master
Are you sure you want to change the base?
Conversation
server/sms/nexmo_sms_sender_test.go
Outdated
countCh := make(chan bool) | ||
doneCh := make(chan struct{}) | ||
// no request should be made in case the sms is expired | ||
go dummyNexmoEndpointWithHandlerFunc(t, countCh, port, func(t *testing.T, countCh chan bool) http.HandlerFunc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better way to do it.Make a new NexmoHandler and if a request reaches the http server.
a.FailNow() the request.In this way no doneCh is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean another function that handles it? Because we have no NexmoHandler
atm.
IsError: true, | ||
} | ||
|
||
assert.Equal(t, "!"+ERROR_BAD_REQUEST+" "+"you are so bad.", string(msg.Bytes())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) ha ha ha ..Such Badness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was there before. I just moved it. :))
…tea/gobbler into feature/pn-310-sms-ttl
protocol/message.go
Outdated
@@ -150,7 +150,7 @@ func (m *Message) SetFilter(key, value string) { | |||
// Checks are made using `Expires` field timezone | |||
func (m *Message) IsExpired() bool { | |||
if m.Expires == nil { | |||
return true | |||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
server/sms/nexmo_sms_sender.go
Outdated
"Expires": msg.Expires.Format(time.RFC3339), | ||
"Created": time.Unix(msg.Time, 0).Format(time.RFC3339), | ||
}).Info("Expired message received") | ||
mTotalExpiredMessages.Add(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a prometheus equivalent for this in another PR
server/sms/nexmo_sms_sender_test.go
Outdated
@@ -35,3 +36,26 @@ func TestNexmoSender_SendWithError(t *testing.T) { | |||
a.Error(err) | |||
a.Equal(ErrRetryFailed, err) | |||
} | |||
|
|||
func TestNexmoSender_SendExpiredMessage(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I moved the test to sms_integration_test.go
Changes Unknown when pulling bfc87cf on feature/pn-310-sms-ttl into ** on master**. |
… feature/pn-310-ttl-change-to-timestamp
…tea/gobbler into feature/pn-310-sms-ttl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too long ago. I can not review this request anymore. Sorry.
This PR should be merged after: #7