-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Feature: IN-APP UPDATES #20822
Feature: IN-APP UPDATES #20822
Conversation
Moves: the dependency version and implementation code change for better dependency management visibility
# Conflicts: # WordPress/src/main/java/org/wordpress/android/modules/ApplicationModule.java # WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java # WordPress/src/main/res/values/strings.xml # build.gradle # version.properties
# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java # version.properties
# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java # version.properties
Generated by 🚫 Danger |
Found 1 violations: The PR caused some dependency changes (expand to see details)
++--- com.google.android.play:app-update:2.1.0
+| +--- com.google.android.gms:play-services-basement:18.1.0 -> 18.2.0 (*)
+| +--- com.google.android.gms:play-services-tasks:18.0.2 (*)
+| \--- com.google.android.play:core-common:2.0.3
++--- com.google.android.play:app-update-ktx:2.1.0
+| +--- androidx.core:core:1.1.0 -> 1.12.0 (*)
+| +--- androidx.fragment:fragment:1.1.0 -> 1.6.2 (*)
+| +--- com.google.android.gms:play-services-basement:18.1.0 -> 18.2.0 (*)
+| +--- com.google.android.gms:play-services-tasks:18.0.2 (*)
+| +--- com.google.android.play:app-update:2.1.0 (*)
+| +--- com.google.android.play:core-common:2.0.3
+| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.3.72 -> 1.9.10 (*)
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.0 -> 1.7.3 (*)
+| \--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.5.0 -> 1.7.3 (*)
+--- com.google.android.play:review:2.0.1
-| \--- com.google.android.play:core-common:2.0.2
+| \--- com.google.android.play:core-common:2.0.2 -> 2.0.3
Please review and act accordingly
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #20822 +/- ##
==========================================
+ Coverage 40.66% 40.69% +0.03%
==========================================
Files 1490 1498 +8
Lines 68629 68757 +128
Branches 11340 11364 +24
==========================================
+ Hits 27911 27984 +73
- Misses 38198 38241 +43
- Partials 2520 2532 +12 ☔ View full report in Codecov by Sentry. |
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.
Hey @pantstamp
Awesome job on integrating the api to the app.
Overall the flow looks good to me. I tested both the flows (immediate and flexible) and it looks good to me. Also I tested the tracking logic which I think we should add to the PR testing instructions so that we are on the same page.
Things I noticed while testing the flow.
-
During the flexible update flow a snackbar is shown with Restart action when the download is completed.
There was no tracking for clicking on this snackbar action button. I think its good to have a tracking there as well. -
During the immediate update flow
After clicking accepted on the update modal, I couldn't see the tracking in the log console. But I couldn't test it again as the link for the first uploaded app version(79999) is returning error after I updated the app to 8000000. I tried re-uploading the 799999 and retrying the workflow but it didn't work. If you can confirm there is tracking on accepting the update on the immediate update flow. You can go ahead and do the merge.
WordPress/src/main/java/org/wordpress/android/inappupdate/InAppUpdateManagerNoop.kt
Show resolved
Hide resolved
@Override public void onAppUpdateDownloaded() { | ||
popupSnackbarForCompleteUpdate(); | ||
} | ||
@Override public void onAppUpdateStarted(int type) { |
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.
❓ Do we need all these callbacks on the interface and the listener if we are not using them?
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.
Nice catch here @AjeshRPai ! I want to keep the callbacks in case we need them for a future "client". I proceeded with a small refactoring, providing default empty implementations, so each client will be able to implement only the callbacks they need.
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.
Thanks Makes sense
@AjeshRPai thank you for reviewing the PR! I will reply to all of your comments as I address them 🙌 |
@AjeshRPai added analytics event for app restart -> |
@AjeshRPai, I can confirm that no tracking is logged for accepting the update in the "Immediate" flow. I intended to add this as a "known issue," but I forgot. Thanks for reminding me! I tried to make this work but without success. The problem lies in the fact that in the immediate flow, when the full-screen modal appears and the user accepts the update, the flow of execution never returns to our app until the install is complete and the app is restarted. So our activity's onResult is never called. We could calculate this number by using: |
Quality Gate passedIssues Measures |
Got it Pantelis. I thought this would be the case. You can go ahead and merge the PR |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR implements the In-app updates feature on Android.
More details about Google's in-app updates can be found here. Please read the contents of the link first to get more context. We use the Google Play Core libraries to support UX flows for in-app updates, allowing users to keep our app up to date on their devices.
To Test:
Testing this feature requires many steps. We need to test both the "immediate update" flow and the "flexible update" flow.
When an update is available, we need to decide which flow to use. We utilize a remote config value from the backend representing the last version of our app that requires an immediate update. This is usually a version that contains a critical bug fix or a new feature that our app cannot function properly without. If the current version of the installed app on the user's device is lower than the remote config value, then the immediate update flow is launched. Otherwise, we go with the flexible one.
ATTENTION: The current remote config value is 80000. This means that when you try to update a version of the app that is < 80000, the immediate flow will be launched.
To test this feature, we use Internal app sharing as Google suggests here.
Please read how Internal app sharing works here.
ATTENTION: As app bundle links in Internal app sharing might expire, you may need to reupload an app bundle to get a new link to use. The page for uploading is here. If you have the Release apps to testing tracks permission, you’re authorized to upload app bundles and APKs for internal sharing by default. If you don't, you can make a request in Systems P2 like this.
Attention: If the app is in the background when the update is downloaded, the update is installed silently without notifying the user.
Please feel free to explore unhappy paths, like turning off the internet connection while an update is being downloaded and then trying to restart the app.
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):