-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Remove explore plans from Quick start #16582
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
Remove explore plans from Quick start #16582
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APKs: |
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 for this PR, @AjeshRPai!
Overall I find the changes fine, except that in the migration test - step 4 since all Quick Start tasks are complete on installing the app from this PR branch, I would have expected following actions:
- Show
Quick Startcompletion notification. - Hide
Quick Startcard.
Currently, I see that Quick Start card is shown with all items stuck off. It's not hidden even on pull-to-refresh.
qs-card-shown-all-tasks-completed.mp4
It looks like
-
When we install an app having the
Explore Planslisted, and only thePlanstask is left incomplete,Quick Startis not marked complete in the DB which should have happened here. -
Then when we launch the app from this PR branch, we show a completion notification if every task is marked complete for the site and the completion notification is not yet received. On refreshing the screen, since
Quick Startis still in progress, the card keeps displaying.
I think we can resolve this issue if we mark Quick Start complete explicitly (if not already complete) when we check that all tasks are complete on app launch before sending the completion notification. Let me know if it makes sense. 😅
|
Hey @ashiagr , Thanks for reviewing the PR. I was expecting that scenario to work properly as I had tested the scenario you had described in my local and I got
I am not sure what went wrong here, I will look into this on priority
I think this will work. Working on a fix 👍🏼 |
|
Found 1 violations: The PR caused the following dependency changes:-+--- org.wordpress:fluxc:{strictly 1.42.0} -> 1.42.0
-| +--- org.wordpress:wellsql:1.7.0
-| | \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
-| +--- org.wordpress.fluxc:fluxc-annotations:1.42.0
-| +--- org.greenrobot:eventbus:3.3.1 (*)
-| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
-| +--- com.android.volley:volley:1.1.1 -> 1.2.0
-| +--- androidx.paging:paging-runtime:2.1.2
-| | +--- androidx.paging:paging-common:2.1.2
-| | | +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
-| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
-| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
-| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.3.1 (*)
-| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.2.0 (*)
-| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-| +--- com.goterl:lazysodium-android:5.0.2
-| +--- net.java.dev.jna:jna:5.5.0
-| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
-| +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
-| +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
-| +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.1.0-beta01 (*)
-| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
-| | +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
-| +--- com.google.code.gson:gson:2.8.5
-| +--- org.apache.commons:commons-text:1.1 (*)
-| +--- androidx.room:room-runtime:2.4.2 (*)
-| +--- androidx.room:room-ktx:2.4.2
-| | +--- androidx.room:room-common:2.4.2 (*)
-| | +--- androidx.room:room-runtime:2.4.2 (*)
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
-| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
-| +--- com.google.dagger:dagger:2.29.1 -> 2.41 (*)
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
-| \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
++--- org.wordpress:fluxc:{strictly 1.42.1} -> 1.42.1
+| +--- org.wordpress:wellsql:1.7.0
+| | \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
+| +--- org.wordpress.fluxc:fluxc-annotations:1.42.1
+| +--- org.greenrobot:eventbus:3.3.1 (*)
+| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
+| +--- com.android.volley:volley:1.1.1 -> 1.2.0
+| +--- androidx.paging:paging-runtime:2.1.2
+| | +--- androidx.paging:paging-common:2.1.2
+| | | +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
+| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
+| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
+| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.3.1 (*)
+| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.2.0 (*)
+| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+| +--- com.goterl:lazysodium-android:5.0.2
+| +--- net.java.dev.jna:jna:5.5.0
+| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
+| +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
+| +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
+| +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.1.0-beta01 (*)
+| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
+| | +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
+| +--- com.google.code.gson:gson:2.8.5
+| +--- org.apache.commons:commons-text:1.1 (*)
+| +--- androidx.room:room-runtime:2.4.2 (*)
+| +--- androidx.room:room-ktx:2.4.2
+| | +--- androidx.room:room-common:2.4.2 (*)
+| | +--- androidx.room:room-runtime:2.4.2 (*)
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
+| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
+| +--- com.google.dagger:dagger:2.29.1 -> 2.41 (*)
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
+| \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
\--- org.wordpress:login:0.14.0
- \--- org.wordpress:fluxc:trunk-1436f46fc296ede0e65e117bbfced38fa34cec6d -> 1.42.0 (*)
+ \--- org.wordpress:fluxc:trunk-1436f46fc296ede0e65e117bbfced38fa34cec6d -> 1.42.1 (*)
Please review and act accordingly
|
|
@ashiagr Thanks for the Input and all the help. The issue you have mentioned in the PR have been fixed by the commit b7dfd15. I have implemented the solution you had described. Thanks for the help. Now, if only the plans item is remaining before this PR, then after installing the app from this PR the user will
|
ashiagr
left a comment
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 for the fix! All looks good to me now.
Fixes #16477
This PR removes the Explore Plans task from the quick start list and card.
Internal Reference: p1651666551282059/1651587009.445519-slack-C011BKNU1V5
Corresponding FluxC PR: wordpress-mobile/WordPress-FluxC-Android#2409
To test:
Test 1
Explore Plans is Not Shown on Quickstart list
Test 2 - Migration
Scenario - Test the case where all Quickstart tasks other than Explore Plan is completed, In this task completed notification should be shown to the user in Notifications tab
Pre- requisites
For this test, install an app having the
Explore Planslisted,Merge Instructions
Regression Notes
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.