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

feat: add sms provider seven #120

Closed
wants to merge 1 commit into from
Closed

Conversation

matthiez
Copy link
Contributor

Hi,
I added a new a new SMS provider seven.

Hope this helps!

Copy link
Member

@BDav24 BDav24 left a comment

Choose a reason for hiding this comment

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

Hi @matthiez, thanks for your PR!
It looks good to me, just a comment about boolean parameters.

Comment on lines +19 to +26
const params = {
flash: messageClass === 0,
from,
text,
to,
ttl,
unicode: type === 'unicode'
}
Copy link
Member

@BDav24 BDav24 Jan 31, 2024

Choose a reason for hiding this comment

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

It seems they use 0 or 1 in place of booleans in the documentation:

Suggested change
const params = {
flash: messageClass === 0,
from,
text,
to,
ttl,
unicode: type === 'unicode'
}
const params = {
flash: messageClass === 0 ? 1 : 0,
from,
text,
to,
ttl,
unicode: type === 'unicode' ? 1 : 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both works :)

Copy link
Member

Choose a reason for hiding this comment

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

You can remove this file from the PR, it'll be generated during deployment.

Comment on lines +63 to +74
headers: expect.objectContaining({
Accept: ['application/json'],
'Content-Length': ['102'],
'Content-Type': ['application/json'],
SentWith: ['Notifme'],
'User-Agent': ['notifme-sdk/v1 (+https://github.com/notifme/notifme-sdk)'],
'X-Api-Key': ['apiKey']
})
}))
expect(mockHttp.body).toEqual(
'{"flash":false,"from":"Notifme","text":"Hello John! How are you?","to":"+15000000001","unicode":false}'
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
headers: expect.objectContaining({
Accept: ['application/json'],
'Content-Length': ['102'],
'Content-Type': ['application/json'],
SentWith: ['Notifme'],
'User-Agent': ['notifme-sdk/v1 (+https://github.com/notifme/notifme-sdk)'],
'X-Api-Key': ['apiKey']
})
}))
expect(mockHttp.body).toEqual(
'{"flash":false,"from":"Notifme","text":"Hello John! How are you?","to":"+15000000001","unicode":false}'
)
headers: expect.objectContaining({
Accept: ['application/json'],
'Content-Length': ['94'],
'Content-Type': ['application/json'],
SentWith: ['Notifme'],
'User-Agent': ['notifme-sdk/v1 (+https://github.com/notifme/notifme-sdk)'],
'X-Api-Key': ['apiKey']
})
}))
expect(mockHttp.body).toEqual(
'{"flash":0,"from":"Notifme","text":"Hello John! How are you?","to":"+15000000001","unicode":0}'
)

Comment on lines +85 to +93
await sdk.send({
sms: {
...request.sms,
customize: async (provider, request) => ({ ...request, text: 'Hello John! How are you?' })
}
})
expect(mockHttp).lastCalledWith(expect.objectContaining({
headers: expect.objectContaining({ 'Content-Length': ['103'] })
}))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await sdk.send({
sms: {
...request.sms,
customize: async (provider, request) => ({ ...request, text: 'Hello John! How are you?' })
}
})
expect(mockHttp).lastCalledWith(expect.objectContaining({
headers: expect.objectContaining({ 'Content-Length': ['103'] })
}))
await sdk.send({
sms: {
...request.sms,
customize: async (provider, request) => ({
...request,
text: 'a totally new message',
messageClass: 0,
type: 'unicode'
})
}
})
expect(mockHttp.body).toEqual(
'{"flash":1,"from":"Notifme","text":"a totally new message","to":"+15000000001","unicode":1}'
)

@BDav24 BDav24 changed the title add sms provider seven feat: add sms provider seven Feb 13, 2024
@BDav24
Copy link
Member

BDav24 commented Mar 26, 2024

Merged in #151

@BDav24 BDav24 closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants