Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] SMS & Email Invite for Contacts #11
[New] SMS & Email Invite for Contacts #11
Changes from all commits
394511e
6f252ab
12a5eb1
493d784
d92164b
00552d6
0efc206
f7e2b49
eb4d691
9179d15
75eb6ca
bd69cc1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's do it in another PR.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a note of this and track for future PR?
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.
Sounds good.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do that in another PR where we handle the Native language case as well.