-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[NEW] Twilio MMS support for LiveChat integration #7964
Conversation
Sorry about the codacy messages. First time messing with those. I'll stop on that front unless it's required. |
return returndata; | ||
} | ||
|
||
returndata.hasMedia = true; |
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 are always setting hasMedia
as true
which is not correct.
for (let mediaIndex = 0; mediaIndex < NumMedia; mediaIndex++) { | ||
const media = { | ||
'url': '', | ||
'contenttype': '' |
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.
please use camel case properties, i.e: contentType
@@ -44,6 +44,30 @@ RocketChat.API.v1.addRoute('livechat/sms-incoming/:service', { | |||
sendMessage.message.msg = sms.body; | |||
sendMessage.guest = visitor; | |||
|
|||
if (sms.hasMedia) { |
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.
it will always enter on this if
because hasMedia
is always true
, so it is not really needed.
@@ -44,6 +44,30 @@ RocketChat.API.v1.addRoute('livechat/sms-incoming/:service', { | |||
sendMessage.message.msg = sms.body; | |||
sendMessage.guest = visitor; | |||
|
|||
if (sms.hasMedia) { | |||
sendMessage.message.attachments = []; | |||
for (let mediaIndex = 0; mediaIndex < sms.media.length; mediaIndex++) { |
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 would suggest to use sms.media.map()
to populate the attachments
field to help code reading. but not really a must.
return { | ||
let NumMedia = 0; | ||
|
||
const returndata = { |
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.
please use camel case variables, i.e: returnData
First of all, thank you for your contribution, it looks like a very cool feature to have 😄 I have made some comments, please let me know if you need any help.. |
* Switch to camelcase * Changed hasMedia (removed) * use map instead of for loop for populating attachments
Updated and pushed changes based on feedback. Hopefully this is more in line with standards and CI and lint all report back fine. |
@renatobecker @sampaiodiego looks like feedback has been addressed here. But enough time has passed it may need another looking. |
wow.. I missed the update here.. sry about that 😞 for sure I'll take a look soon |
Would love to see this integrated ASAP! |
@RocketChat/core
This pull request adds the ability for the Twilio integration to accept MMS attachments and display the attachment in the LiveChat window.