[New] SMS & Email Invite for Contacts #11
Conversation
…nto sms-invite
kb0304
changed the title
|
|
||
| Meteor.methods({ | ||
| sendInvitationSMS(phones) { | ||
| const twilioService = RocketChat.SMS.getService('twilio'); |
There was a problem hiding this comment.
This hard codes twilio as the only sms service? Are there other services that admins might want to use? Is it possible to generalize this and pass the desired service into the method somehow?
There was a problem hiding this comment.
Depending on what I saw, right now in RC there is only option for Twilio as the supported SMS service. They have made it partially modular, where anyone if wants to implement another service can implement it, but currently only Twilio is implemented. If we want to support multiple services, I can add a setting via which you can select which provider to use for inviting purpose, but that is only usable when other SMS services are implemented.
There was a problem hiding this comment.
So upstream has the ability to configure another SMS service, but no way to use it? Is that what you mean by "partially modular"?
Is it a lot of work to "add a setting via which you can select which provider to use for inviting purpose"?
There was a problem hiding this comment.
https://github.com/RocketChat/Rocket.Chat/tree/develop/packages/rocketchat-sms/server
Here, only the Twilio is implemented. To use another, one will have to make service and register.
We can create an option to select from the available services (not a lot of work). Should we use Twilio as the default? There is nothing like default/active service for SMS.
There was a problem hiding this comment.
If it's not a lot of work, I think it's best. Do you agree? Just create the option, and use Twilio as the default. That way it is at least capable of using another service if an admin wants to make another service and register it. Thanks!
There was a problem hiding this comment.
I'm thinking that we might want to merge this PR and make notes on work like this that we'd like to do as enhancements. What do you think?
There was a problem hiding this comment.
Sure, let's do it in another PR.
| if (RocketChat.settings.get('Invitation_SMS_Customized')) { | ||
| body = RocketChat.settings.get('Invitation_SMS_Customized_Body'); | ||
| } else { | ||
| const lng = user.language || RocketChat.settings.get('language') || 'en'; |
There was a problem hiding this comment.
Can you clarify this for me please?
Is "user.language" in this case the user who is sending the invitation? If so, is there some condition when we won't be getting that flag from the user, so that we need these '||' statements?
Is RocketChat.settings.get('language') the language that the admin uses on the RC server? (Which we know will be English because we are going to be the admins)?
There was a problem hiding this comment.
I replicated it from the RC code here. https://github.com/RocketChat/Rocket.Chat/blob/14f47007aec90aee4ed810d1250bc71f328df7f0/packages/rocketchat-livechat/messageTypes.js#L37
I will checkout for the cases (if they exist) when the first two will not be set.
There was a problem hiding this comment.
Interesting..... the file you linked to is "live chat", and the user will be on a web browser I believe.... so we can likely expect that we'll get "user.language", because web browsers do that by default. We should verify what this looks like when it's coming from the android client, because we most definitely want to send the invite in the native language.
There was a problem hiding this comment.
Make a note of this and track for future PR?
| @@ -35,5 +35,31 @@ Meteor.startup(function() { | |||
| i18nLabel: 'Auth_Token', | |||
| }); | |||
| }); | |||
|
|
|||
| this.section('Invitation', function() { | |||
There was a problem hiding this comment.
This PR does not include anything related to the deep link to a DM with the original inviter, in the invitation message, is that correct?
There was a problem hiding this comment.
How about any default SMS message at all? Should that be implemented here, or will that come in a different PR? As it is, only "Invitation_SMS_Default_Body" shows up in the sms invite message.
There was a problem hiding this comment.
Lets do that in another PR where we handle the Native language case as well.
Closes WideChat/Rocket.Chat.Android#121