-
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
Quick Start for Existing Users V2 - Refactor code to support different quick start types #16500
Quick Start for Existing Users V2 - Refactor code to support different quick start types #16500
Conversation
QuickStartTask is now an interface and currently tasks exists only for QuickStartNewSiteTask. All occurrences of new site tasks in the code should now reference QuickStartNewSiteTask for the code to compile.
Fixes error as per suggestion: Type is not directly supported by Parcelize Annotate the parameter with @rawvalue if you want it to be serialized via writeValue()
Currently same set of tasks are shown irrespective of whether user creates a new site or chooses an existing site. These tasks are currently part of NewSiteQuickStartType which is set quick start type in qs repo.
…fication based on quick start type
…-wpandroid # Conflicts: # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
Since we do not save quick start type in db, it makes more sense to keep it in WPAndroid.
Hey @ashiagr, thanks for the details PR description and testing notes, those are very helpful! Some notes from testing:
delay.quick.start.prompt.mp4Note: Spoke to @ashiagr offline about this, this is from this PR. I also checked the current beta build and the same is reproducible there, so isn't related to the PR.
dynamic.card.no.qs.prompt.mp4
pinned.card.still.removed.mp4
pinned.card.jumping.mp4I have not checked the dynamic cards observations against a production build (I'm not sure how to turn this on for production since the debug option is not there...) so thought I'd share them here so you can have a look first. |
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 tackling this task @ashiagr . Awesome job. I have reviewed the code changes and I have done regression testing on quick start. I have also tested with dynamic cards as well. Everything looks good. 👍🏼 . I have done the review for the Corresponding fluxC PR and everything looks good there as well.
I have left two nitpicks for you to consider. As those are non blocking, I am approving the PR. Please feel free to merge if you think those are not needed.
WordPress/src/main/java/org/wordpress/android/ui/quickstart/QuickStartNoticeDetails.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/quickstart/QuickStartTaskDetails.kt
Show resolved
Hide resolved
Generated by 🚫 dangerJS |
Found 1 violations: The PR caused the following dependency changes:-+--- org.wordpress:fluxc:{strictly trunk-1ee442f447044438c9b17068cb224cc162cd5912} -> trunk-1ee442f447044438c9b17068cb224cc162cd5912
-| +--- org.wordpress:wellsql:1.7.0
-| | \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
-| +--- org.wordpress.fluxc:fluxc-annotations:trunk-1ee442f447044438c9b17068cb224cc162cd5912
-| +--- 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 trunk-6fcf6007126537fa66dbe6aacc439ab0f3aa30db} -> trunk-6fcf6007126537fa66dbe6aacc439ab0f3aa30db
+| +--- org.wordpress:wellsql:1.7.0
+| | \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
+| +--- org.wordpress.fluxc:fluxc-annotations:trunk-6fcf6007126537fa66dbe6aacc439ab0f3aa30db
+| +--- 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 -> trunk-1ee442f447044438c9b17068cb224cc162cd5912 (*)
+ \--- org.wordpress:fluxc:trunk-1436f46fc296ede0e65e117bbfced38fa34cec6d -> trunk-6fcf6007126537fa66dbe6aacc439ab0f3aa30db (*)
Please review and act accordingly
|
Thanks, @jostnes for a detailed review! Really appreciate it. Please find my answers below:
That's correct. The PR is not yet merged to the
Quick Start Dynamic Cards are not enabled in production. I tested and found the same behavior in the existing code in dynamic.card.no.qs.prompt.trunk.mp4
I think this is expected behavior. I tested in the existing pinned.card.still.removed.trunk.mp4
This one looks like a potential bug. I'll create an issue and link it here. Since dynamic cards are not enabled in production, I'll assign a low priority to it.
Thank you so much for the detailed notes. I've attached videos from the trunk branch. You can also try the APK from the previous PR: #16473 (comment) which will allow you to enable the flag in case you want to be sure. 🙂 |
…-wpandroid # Conflicts: # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt
I'll go ahead and merge the PR as the issues reported are non-blocking and mostly related to QS dynamic cards which are not enabled in production and not related to changes in this PR. Thanks once again for your inputs. 🙇♀️ 🙌 |
Parent #16350
Corresponding FluxC PR: wordpress-mobile/WordPress-FluxC-Android#2384
This PR refactors quick start logic to consider tasks and task types based on quick start type.
Note 1: No new functionality is added in this PR. It needs a careful code review and regression testing.
Note 2: I've tried to split changes into several commits and provided an overview of the changes in our sync call but let me know if I can help in reviewing the PR.
Details
Changes can be grouped under the following heads:
QuickStartRepository.quickStartType
Currently, the same set of tasks is shown irrespective of whether the user creates a new site or chooses an existing site. These tasks are currently part of
NewSiteQuickStartType
which is set as qs type in the qs repo in a73828b.Trackers and Illustrations
Updated to compare task strings in trackers and task illustrations as a common task like
check_stats
needs the same tracking, and illustrations irrespective of quick start type.QuickStartMySitePrompts, QuickStartNoticeDetails, QuickStartTaskDetails
Updated to get the corresponding enum by comparing task strings.
As a common task like
check_stats
needs the same prompt, notice details, task details, and illustrations irrespective of quick start type, there was no need to split them based on quick start type.Quick Start Functions
Quick Start Card
To Test
Test.1 Regression test Quick Start
Test.2 Dynamic Cards
PS: Test 5 - Quick Start Cards disappear in
wpandroid-quick-start-testing-instructions.txt
will not work asPlans
were removed (internal ref: p1651587009445519-slack-C011BKNU1V5) but the corresponding Quick Start task remains which will not mark the card as complete.Test.3 (optional) To test that qs functions consider tasks and task types based on quick start type.
(I added this test to share the patch I used to find out qs functions we needed to update based on quick start type but it can be skipped to a future PR when we actually add the tasks)
ExistingSiteQuickStartType
in qs repo and adds common taskscheck_stats
,view_site
temporarily inQuickStartStore
. Enable local fluxc in local-build.gradle.Merge Instructions
FluxC
version inbuilds.gradle
Not Ready for Merge
labelRegression Notes
Potential unintended areas of impact
Quick Start functionality does not work as expected
What I did to test those areas of impact (or what existing automated tests I relied on)
See
To Test
sectionWhat automated tests I added (or what prevented me from doing so)
Updated existing tests for the code to compile after refactor (no changes to existing functionality in this PR).
PR submission checklist:
RELEASE-NOTES.txt
if necessary.