[NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. #2196

ear-dev · 2019-04-04T16:13:09Z

@RocketChat/android

Changes:

Adds support for Firebase DynamicLinks as discussed in the forum here: https://forums.rocket.chat/t/improved-deep-linking-support/1433
This PR replaces the current LoginDeepLinkInfo class with a more generic, DeepLinkInfo class that can handle all of the previous deepLinks supported, as well as DynamicLinks.
DynamicLinks needs to be configured in your firebase account for this to work in the dynamicLinks case. If it is not then the default will be same as current behavior in the app.
For 'play' builds: Invites via another app will include a dynamicLink that when clicked will bring the recipient to the play store to download the app. Once downloaded and logged in, the app will open immediately to a DM with the sender. If the recipient already has the app installed, clicking the dynamicLink will automatically open the app to a DM with the sender.
For 'foss' builds: Invites will auto-populate with the hard-coded link to the play store, as it currently does.

Video Demo here: https://drive.google.com/file/d/1t244q_G4Kls_iWzb4EMqA8eBCg94j1bq/view?usp=sharing

…amicLinks manager and resolve imports

…ing the widechat google-services and appName for testing.

…DM from dynamicLinks

…ndroid into ear_develop_w_share_and_dynamic_links

ear-dev · 2019-06-04T14:37:21Z

@filipedelimabrito I can fix the conflicts in this PR, but I would prefer to wait until you are ready to review so that we don't keep going around in circles.

This is our Phase 1 to support the Contact Sync feature. Do you think we'll get that feature in the next milestone? We are prepared to make all the PRs and present a design doc if needed. Thanks...

engelgabriel · 2019-06-13T20:21:07Z

@filipedelimabrito can we give this PR some visibility on what is missing for it to get merged if the conflicts are resolved?

philipbrito · 2019-06-17T23:47:13Z

app/src/play/java/chat/rocket/android/dynamiclinks/DynamicLinksForFirebase.kt

+
+                }.addOnFailureListener {
+                    // Error
+                    Toast.makeText(context, "Error dynamic link", Toast.LENGTH_SHORT).show()


Hard-coded string! 🛑

Please remove and make use of the strings.xml!

These toast messages were just for debugging. I will remove them and replace with timber logging without string translations. If you would rather we do it a different way please let me know.

Timber is enough but if it is there because you are still debugging it I'll make this as a WIP PR, ok?

Anyway, I think that showing an error message to the user when there is a failure would be better. In this case make sure to implement the MessageView on the fragment (if needed) and make use of our ext. function showToast(resId) and/or showToast(message) plus log it with timber.

Is it supposed to show the deeplink url on the toast in this line (on the addOnSuccessListener):
Toast.makeText(context, newDeepLink, Toast.LENGTH_SHORT).show()
?

Only a toast with the deeplink url? 🤔
If so, better to add more context on the message.

My thought is this.... I don't think we want to show any info to the user about the deepLink. Users have no idea what a deepLink is, and there is no requirement that they do. I added the logging with Timber just to have a record in the server logs for admins as needed. If the deepLink creation fails, then it uses the default message, and the user has not been affected. Thoughts?

FYI - I did push a bunch of updates to the PR and the Toast messages are now removed.

app/src/play/java/chat/rocket/android/dynamiclinks/DynamicLinksForFirebase.kt

app/src/main/java/chat/rocket/android/authentication/presentation/AuthenticationNavigator.kt

philipbrito · 2019-06-18T00:21:24Z

app/src/main/java/chat/rocket/android/authentication/loginoptions/ui/LoginOptionsFragment.kt

@@ -181,7 +181,7 @@ class LoginOptionsFragment : Fragment(), LoginOptionsView {
            totalSocialAccountsEnabled = getInt(TOTAL_SOCIAL_ACCOUNTS)
            isLoginFormEnabled = getBoolean(IS_LOGIN_FORM_ENABLED)
            isNewAccountCreationEnabled = getBoolean(IS_NEW_ACCOUNT_CREATION_ENABLED)
-            deepLinkInfo = getParcelable(DEEP_LINK_INFO)
+            deepLinkInfo = getParcelable(Constants.DEEP_LINK_INFO)


Remove the Constants object please (a comment was left on it).

philipbrito · 2019-06-18T00:21:29Z

app/src/main/java/chat/rocket/android/authentication/loginoptions/ui/LoginOptionsFragment.kt

@@ -115,7 +115,7 @@ fun newInstance(
        putInt(TOTAL_SOCIAL_ACCOUNTS, totalSocialAccountsEnabled)
        putBoolean(IS_LOGIN_FORM_ENABLED, isLoginFormEnabled)
        putBoolean(IS_NEW_ACCOUNT_CREATION_ENABLED, isNewAccountCreationEnabled)
-        putParcelable(DEEP_LINK_INFO, deepLinkInfo)
+        putParcelable(Constants.DEEP_LINK_INFO, deepLinkInfo)


Remove the Constants object please (a comment was left on it).

app/src/main/java/chat/rocket/android/authentication/domain/model/DeepLinkInfo.kt

app/src/foss/java/chat/rocket/android/dynamiclinks/DynamicLinksForFirebase.kt

…share_and_dynamic_links

ear-dev · 2019-06-25T13:50:08Z

Hi Filipe..... I did manage to test this end to end using our Firebase dynamic links... I'm pretty much done with this PR, unless you have some suggestion about how to best handle the Constants.kt file (I posed the question in the thread)... Thanks.

Let me know if there are other issues you would like resolved before merge...

philipbrito · 2019-06-26T15:38:49Z

@ear-dev It is not ready to be merged yet. There is still some hard-coded strings on this PR making it unready to be integrated here - for instance.

ear-dev · 2019-06-26T15:48:22Z

Thanks Filipe. I see that there are new conflicting files.... I'll fix those. That const you pointed out is fixed.... let me know if you have any thoughts about the Constants.kt file question I posed....

app/src/main/java/chat/rocket/android/authentication/domain/model/DeepLinkInfo.kt

philipbrito · 2019-06-26T16:07:06Z

app/src/main/java/chat/rocket/android/authentication/domain/model/DeepLinkInfo.kt

+private inline fun Uri.isWebSchemeRoomLink(): Boolean {
+    val roomType = path.split("/")[1]
+    if (scheme.startsWith("http") &&
+            (roomType == "channel" || roomType == "group" || roomType == "direct"))


You can make use of roomTypeOf(roomType) here to check is the room type is channel, group, etc.

Ex.:

... roomTypeOf(roomType) is RoomType.DirectMessage

@filipedelimabrito will roomTypeOf() just accept the lower case strings we've parsed from the path? Or does it need to be converted into some object or capitalized string? It's not clear to me right away. Thanks.

philipbrito · 2019-06-26T16:12:53Z

app/src/main/java/chat/rocket/android/authentication/ui/AuthenticationActivity.kt

-            allowStateLoss = true
-        ) {
-            chat.rocket.android.authentication.server.ui.newInstance()
+    private fun routeNoLink() {


@ear-dev Can you add a comment to make it clear to anyone reading that code or change the function name to something more meaningful? It will help us a lot here.

philipbrito · 2019-06-26T16:18:50Z

app/src/main/java/chat/rocket/android/helper/Constants.kt

@@ -0,0 +1,5 @@
+package chat.rocket.android.helper
+
+object Constants {


Let me take a look in a deep way here. But since we are going to use a Constant class with, well... our constants we will need to move all the stuffs declared on our code here to avoid people declaring it in multiple places of our code for instance.

ear-dev · 2019-06-26T16:34:57Z

Thanks for thinking about the constants.kt issue..... I honestly do not know what the best practice should be in this case. Most of your constants are scoped to just a single class I believe. Are there any other examples of a constant like this one that is shared among multiple classes and activities?

…ndroid into ear_develop_w_share_and_dynamic_links

philipbrito · 2019-06-29T18:51:39Z

app/src/main/java/chat/rocket/android/analytics/Analytics.kt

+     *
+     * @param inviteType The method of invite to log, currently only 'share' using the share intent.
+     */
+    fun logInviteSent(inviteType: String, inviteSucceeded: Boolean) {}


We can make the inviteType an enum here.

philipbrito · 2019-06-29T18:52:57Z

app/src/main/java/chat/rocket/android/authentication/presentation/AuthenticationNavigator.kt

+    fun saveDeepLinkInfo(deepLinkInfo: DeepLinkInfo) {
+        savedDeepLinkInfo = deepLinkInfo
+    }
+    fun toOnBoarding() {


Missing white line above function.

philipbrito · 2019-06-29T18:54:08Z

app/src/main/java/chat/rocket/android/authentication/server/ui/ServerFragment.kt

-
-private const val DEEP_LINK_INFO = "DeepLinkInfo"
+fun newInstance(deepLinkInfo: DeepLinkInfo?): ServerFragment {
+    val fragment = ServerFragment()


Code can be improved here. I'll work on it.

philipbrito · 2019-06-29T19:01:37Z

app/src/main/java/chat/rocket/android/settings/ui/SettingsFragment.kt

-            putExtra(Intent.EXTRA_TEXT, getString(R.string.play_store_link))
-            startActivity(Intent.createChooser(this, getString(R.string.msg_share_using)))
-        }
+        // We can't know for sure at this point that the invitation was sent successfully since they will now be outside our app


Right. But having it logged by our analytic tool will let us know how many people do this action even though the invitation was not sent successfully as explained on your comment above (thanks for adding it). This way I'll remove the inviteSucceeded which indicate if the invitation was sent successfully or not - since we don't know about it.

philipbrito · 2019-06-29T19:24:38Z

app/src/main/java/chat/rocket/android/util/extensions/Uri.kt

+            DeepLinkInfo(url, userId, token, null, null, null)
+        } catch (ex: Exception) {
+            Timber.d(ex, "Error parsing auth deeplink")


For error used the proper Timber function: e.

philipbrito · 2019-06-29T19:58:53Z

app/src/main/java/chat/rocket/android/analytics/Analytics.kt

+    /**
+     * Logs the invitation sent event.
+     *
+     * @param inviteType The method of invite to log, currently only 'share' using the share intent.


Where is the inviteSucceeded param?

philipbrito · 2019-06-29T20:06:38Z

app/src/play/java/chat/rocket/android/analytics/GoogleAnalyticsForFirebase.kt

@@ -73,6 +73,13 @@ class GoogleAnalyticsForFirebase @Inject constructor(val context: Context) :
        })
    }

+    override fun logInviteSent(inviteType: String, inviteSucceeded: Boolean) {


You forgot to log it on AnswersAnalytics too...

philipbrito · 2019-06-29T20:09:15Z

app/src/main/java/chat/rocket/android/authentication/presentation/AuthenticationNavigator.kt

 import chat.rocket.android.main.ui.MainActivity
 import chat.rocket.android.server.ui.changeServerIntent
 import chat.rocket.android.util.extensions.addFragmentBackStack
 import chat.rocket.android.util.extensions.toPreviousView
 import chat.rocket.android.webview.ui.webViewIntent

 class AuthenticationNavigator(internal val activity: AuthenticationActivity) {
+    var savedDeepLinkInfo: DeepLinkInfo? = null


Variable can be private.

philipbrito · 2019-06-29T20:17:57Z

app/src/main/java/chat/rocket/android/main/ui/MainActivity.kt

+        intent?.let {
+            val deepLinkInfo = it.getParcelableExtra<DeepLinkInfo>(Constants.DEEP_LINK_INFO)
+            if (deepLinkInfo != null) {
+                val chatRoomsFragment = supportFragmentManager.findFragmentByTag(TAG_CHAT_ROOMS_FRAGMENT) as ChatRoomsFragment


Please, respect our code style.

philipbrito · 2019-06-29T20:36:57Z

app/src/main/java/chat/rocket/android/authentication/presentation/AuthenticationNavigator.kt

+    fun saveDeepLinkInfo(deepLinkInfo: DeepLinkInfo) {
+        savedDeepLinkInfo = deepLinkInfo
+    }
+    fun toOnBoarding() {


Also, where is this function being called?

philipbrito

@ear-dev I have fixed some issues here. Thanks for sending this PR!

ear-dev · 2019-07-01T16:35:12Z

@filipedelimabrito Awesome!! Thanks!!

ericrosenthal added 8 commits April 2, 2019 16:36

WIP - just added the new shareViaApp function - still need to add dyn…

709f883

…amicLinks manager and resolve imports

WIP - dynamic link creation during shareViaApp intent. Temporarily us…

55994ce

…ing the widechat google-services and appName for testing.

Replace LoginDeepLinkInfo with DeepLinkInfo and handle a deepLink to …

7810a67

…DM from dynamicLinks

cleanup

cbbe6da

revert back to RC google-services.json for PR

fad99ae

build.gradle use rocket app name

6964f3d

revert to rc default strings

fce180f

cleanup

fc0a9f3

ear-dev changed the title ~~Send dynamicLinks with invites; Support the handling of dynamicLinks in the app.~~ [NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. Apr 10, 2019

ericrosenthal added 2 commits April 17, 2019 22:11

Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat.A…

b826ece

…ndroid into ear_develop_w_share_and_dynamic_links

cleanup

55bd98f

engelgabriel requested a review from philipbrito June 13, 2019 20:17

engelgabriel added this to the 3.5.0 milestone Jun 13, 2019

philipbrito suggested changes Jun 18, 2019

View reviewed changes

ericrosenthal added 2 commits June 20, 2019 14:39

first pass at code review cleanup

09bd4c5

Merge remote-tracking branch 'rocketchat/develop' into ear_develop_w_…

19fe905

…share_and_dynamic_links

philipbrito changed the title ~~[NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app.~~ [WIP][NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. Jun 21, 2019

ericrosenthal added 2 commits June 21, 2019 12:16

code review cleanup

b7f1d62

refactor processDeepLink

048ea21

philipbrito suggested changes Jun 26, 2019

View reviewed changes

ericrosenthal and others added 3 commits June 26, 2019 13:45

Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat.A…

e8d6b51

…ndroid into ear_develop_w_share_and_dynamic_links

Review changes

e917eff

Improves code.

6d56e7d

philipbrito suggested changes Jun 29, 2019

View reviewed changes

philipbrito added 2 commits June 29, 2019 18:00

Improves code.

d7cff48

Improves code.

057be55

philipbrito changed the title ~~[WIP][NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app.~~ [NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. Jun 29, 2019

Merge branch 'develop' into ear_develop_w_share_and_dynamic_links

a3ef0a4

philipbrito approved these changes Jun 29, 2019

View reviewed changes

philipbrito merged commit 2e3fcc3 into RocketChat:develop Jun 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. #2196

[NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. #2196

ear-dev commented Apr 4, 2019 •

edited

Loading

ear-dev commented Jun 4, 2019

engelgabriel commented Jun 13, 2019

philipbrito Jun 17, 2019

ear-dev Jun 20, 2019

philipbrito Jun 21, 2019

philipbrito Jun 21, 2019

ear-dev Jun 21, 2019

philipbrito Jun 18, 2019

philipbrito Jun 18, 2019

ear-dev commented Jun 25, 2019

philipbrito commented Jun 26, 2019

ear-dev commented Jun 26, 2019

philipbrito Jun 26, 2019

ear-dev Jun 26, 2019

philipbrito Jun 26, 2019

philipbrito Jun 26, 2019

ear-dev commented Jun 26, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito Jun 29, 2019

philipbrito left a comment

ear-dev commented Jul 1, 2019

		@@ -0,0 +1,5 @@
		package chat.rocket.android.helper

		object Constants {

[NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. #2196

[NEW] Send dynamicLinks with invites; Support the handling of dynamicLinks in the app. #2196

Conversation

ear-dev commented Apr 4, 2019 • edited Loading

Changes:

ear-dev commented Jun 4, 2019

engelgabriel commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ear-dev commented Jun 25, 2019

philipbrito commented Jun 26, 2019

ear-dev commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ear-dev commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipbrito left a comment

Choose a reason for hiding this comment

ear-dev commented Jul 1, 2019

ear-dev commented Apr 4, 2019 •

edited

Loading