-
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
Subscribers line chart #20706
Subscribers line chart #20706
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
55ca517
to
39d117c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #20706 +/- ##
==========================================
- Coverage 40.62% 40.60% -0.03%
==========================================
Files 1485 1488 +3
Lines 68330 68436 +106
Branches 11302 11315 +13
==========================================
+ Hits 27760 27787 +27
- Misses 38054 38129 +75
- Partials 2516 2520 +4 ☔ 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.
I'm seeing this chart on the mobile.blog
site as well as a few others I tested with, which looks incorrect 🤔 I tested using a local build as well as the apk downloaded from this PR. I haven't spent too much time debugging this yet, but the y-axis is missing and tapping on the line isn't scrollable + only shows the value for Apr 25:
@@ -76,6 +78,48 @@ class UiModelMapper | |||
return mapStatsWithOverview(TimeStatsType.OVERVIEW, useCaseModels, showError) | |||
} | |||
|
|||
@Suppress("CyclomaticComplexMethod") | |||
fun mapSubscribers(useCaseModels: List<UseCaseModel>, showError: (Int) -> Unit): UiModel { |
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 noticed a lot of this function's contents are similar to mapInsights()
and mapStats()
. Do you think we could consolidate some of the repetition into helper functions?
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 consolidated (fd93f16) some parts of it, there are slight differences in other parts regarding handling nullability. I don't want to make further changes to avoid affecting the production.
} | ||
val allFailingWithoutData = useCaseModels.fold(true) { acc, useCaseModel -> | ||
acc && useCaseModel.state == ERROR && useCaseModel.data == null | ||
} |
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 is being used in both mapInsights()
and in mapSubscribers()
for example, and could probably be brought out into their own short functions. 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.
Done: fd93f16
# Conflicts: # build.gradle
Good catch! I addressed it (60b9408) by matching the behavior with the web. I set the min value to 0 and the max value to twice of subscribers count if there is no change on the line. |
Quality Gate failedFailed conditions |
Found 1 violations: The PR caused some dependency changes (expand to see details)
-+--- org.wordpress:fluxc:{strictly trunk-e6bb1ab9b9b73ee49ca380e8e3b665358922e01e} -> trunk-e6bb1ab9b9b73ee49ca380e8e3b665358922e01e
-| +--- org.wordpress:wellsql:2.0.0
-| | +--- androidx.annotation:annotation:1.2.0 -> 1.6.0 (*)
-| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
-| +--- org.wordpress.fluxc:fluxc-annotations:trunk-e6bb1ab9b9b73ee49ca380e8e3b665358922e01e
-| +--- org.greenrobot:eventbus:3.3.1
-| | \--- org.greenrobot:eventbus-java:3.3.1
-| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
-| +--- com.android.volley:volley:1.1.1 -> 1.2.1
-| +--- androidx.paging:paging-runtime:2.1.2
-| | +--- androidx.paging:paging-common:2.1.2
-| | | +--- androidx.annotation:annotation:1.0.0 -> 1.6.0 (*)
-| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
-| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.6.2 (*)
-| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.6.2 (*)
-| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.0 (*)
-| +--- com.goterl:lazysodium-android:5.0.2
-| +--- net.java.dev.jna:jna:5.5.0
-| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.9.10 (*)
-| +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.20 -> 1.9.22 (*)
-| +--- androidx.appcompat:appcompat:1.0.2 -> 1.6.1 (*)
-| +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.0 (*)
-| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6 (*)
-| +--- androidx.security:security-crypto:1.0.0
-| | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
-| | \--- com.google.crypto.tink:tink-android:1.5.0
-| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2 (*)
-| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
-| +--- org.apache.commons:commons-text:1.10.0
-| | \--- org.apache.commons:commons-lang3:3.12.0
-| +--- androidx.room:room-runtime:2.4.2 -> 2.5.0
-| | +--- androidx.annotation:annotation-experimental:1.1.0 -> 1.3.1 (*)
-| | +--- androidx.arch.core:core-runtime:2.0.1 -> 2.2.0 (*)
-| | +--- androidx.room:room-common:2.5.0
-| | | +--- androidx.annotation:annotation:1.3.0 -> 1.6.0 (*)
-| | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.20 -> 1.9.10 (*)
-| | +--- androidx.sqlite:sqlite:2.3.0
-| | | +--- androidx.annotation:annotation:1.0.0 -> 1.6.0 (*)
-| | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
-| | \--- androidx.sqlite:sqlite-framework:2.3.0
-| | +--- androidx.annotation:annotation:1.2.0 -> 1.6.0 (*)
-| | +--- androidx.sqlite:sqlite:2.3.0 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
-| +--- androidx.room:room-ktx:2.4.2 -> 2.5.0
-| | +--- androidx.room:room-common:2.5.0 (*)
-| | +--- androidx.room:room-runtime:2.5.0 (*)
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
-| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.7.3 (*)
-| +--- com.google.dagger:dagger:2.42 -> 2.50
-| | \--- javax.inject:javax.inject:1
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.7.3 (*)
-| \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.7.3 (*)
++--- org.wordpress:fluxc:{strictly 2993-d6cb0eded15c3dbbb924172ab3d1c5d8431f1be3} -> 2993-d6cb0eded15c3dbbb924172ab3d1c5d8431f1be3
+| +--- org.wordpress:wellsql:2.0.0
+| | +--- androidx.annotation:annotation:1.2.0 -> 1.6.0 (*)
+| | \--- org.wordpress.wellsql:wellsql-annotations:2.0.0
+| +--- org.wordpress.fluxc:fluxc-annotations:2993-d6cb0eded15c3dbbb924172ab3d1c5d8431f1be3
+| +--- org.greenrobot:eventbus:3.3.1
+| | \--- org.greenrobot:eventbus-java:3.3.1
+| +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.12.0 (*)
+| +--- com.android.volley:volley:1.1.1 -> 1.2.1
+| +--- androidx.paging:paging-runtime:2.1.2
+| | +--- androidx.paging:paging-common:2.1.2
+| | | +--- androidx.annotation:annotation:1.0.0 -> 1.6.0 (*)
+| | | \--- androidx.arch.core:core-common:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.arch.core:core-runtime:2.0.0 -> 2.2.0 (*)
+| | +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.6.2 (*)
+| | +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.6.2 (*)
+| | \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.0 (*)
+| +--- com.goterl:lazysodium-android:5.0.2
+| +--- net.java.dev.jna:jna:5.5.0
+| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.9.10 (*)
+| +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.20 -> 1.9.22 (*)
+| +--- androidx.appcompat:appcompat:1.0.2 -> 1.6.1 (*)
+| +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.3.0 (*)
+| +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.3.6 (*)
+| +--- androidx.security:security-crypto:1.0.0
+| | +--- androidx.annotation:annotation:1.1.0 -> 1.6.0 (*)
+| | \--- com.google.crypto.tink:tink-android:1.5.0
+| +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2 (*)
+| +--- com.google.code.gson:gson:2.8.5 -> 2.10.1
+| +--- org.apache.commons:commons-text:1.10.0
+| | \--- org.apache.commons:commons-lang3:3.12.0
+| +--- androidx.room:room-runtime:2.4.2 -> 2.5.0
+| | +--- androidx.annotation:annotation-experimental:1.1.0 -> 1.3.1 (*)
+| | +--- androidx.arch.core:core-runtime:2.0.1 -> 2.2.0 (*)
+| | +--- androidx.room:room-common:2.5.0
+| | | +--- androidx.annotation:annotation:1.3.0 -> 1.6.0 (*)
+| | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.20 -> 1.9.10 (*)
+| | +--- androidx.sqlite:sqlite:2.3.0
+| | | +--- androidx.annotation:annotation:1.0.0 -> 1.6.0 (*)
+| | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
+| | \--- androidx.sqlite:sqlite-framework:2.3.0
+| | +--- androidx.annotation:annotation:1.2.0 -> 1.6.0 (*)
+| | +--- androidx.sqlite:sqlite:2.3.0 (*)
+| | \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
+| +--- androidx.room:room-ktx:2.4.2 -> 2.5.0
+| | +--- androidx.room:room-common:2.5.0 (*)
+| | +--- androidx.room:room-runtime:2.5.0 (*)
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.20 -> 1.9.22 (*)
+| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.7.3 (*)
+| +--- com.google.dagger:dagger:2.42 -> 2.50
+| | \--- javax.inject:javax.inject:1
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.7.3 (*)
+| \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.7.3 (*)
\--- org.wordpress:login:1.15.0
- \--- org.wordpress:fluxc:trunk-ed60798b4d96ec19863c74b0f525e2e20f4525db -> trunk-e6bb1ab9b9b73ee49ca380e8e3b665358922e01e (*)
+ \--- org.wordpress:fluxc:trunk-ed60798b4d96ec19863c74b0f525e2e20f4525db -> 2993-d6cb0eded15c3dbbb924172ab3d1c5d8431f1be3 (*)
Please review and act accordingly
|
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.
Looks good to me 👍
Fixes #20687
This adds a line graph card to the Subscribers tab of Stats.
It displays a graph of the total subscriber count for the last 30 days. If you tap on line, you'll see the data for that specific point. The marker view is the same as the marker on the "Views & Visitors" card. Most of the design details were taken from the "Views & Visitors" card.
Tip
Review tip
Some classes are copied from existing classes, but they also include some modifications. I copied some same functions intentionally, since it will be easier to remove old classes when we remove
stats_traffic_subscribers_tab
flag.LineChartMarkerView
→SubscribersMarkerView
LineChartViewHolder
→SubscribersChartViewHolder
LineChartLabelFormatter
→SubscribersChartLabelFormatter
ViewsAndVisitorsUseCaseTest
→SubscribersChartUseCaseTest
To Test:
stats_traffic_subscribers_tab
from My site → Debug settings.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)
SubscribersChartUseCaseTest
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):
WordPress.com sites and self-hosted Jetpack sites.Large and small screen sizes. (Tablet and smaller phones)