-
Notifications
You must be signed in to change notification settings - Fork 939
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
Add AppTP WAU pixel #5292
Add AppTP WAU pixel #5292
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
...ection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/pixels/DeviceShieldPixels.kt
Outdated
Show resolved
Hide resolved
815f236
to
8d59648
Compare
val daysBetween = ChronoUnit.DAYS.between(firstDate, secondDate).absoluteValue | ||
|
||
// Check if the difference is more than 28 days | ||
return daysBetween > 28 |
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.
The condition daysBetween > 28
will fire pixels every 29 days, while the test expects firing at 28 days. Changing to daysBetween >= 28
would align the implementation with the test and maintain the intended monthly cadence.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
verifyNoMoreInteractions(pixel) | ||
} | ||
|
||
@Test | ||
fun whenReportEnable28DaysApartReportMonthlyPixel() { |
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.
The test name appears to be incorrect. This test verifies that a pixel is fired when exactly 28 days apart, but the name suggests the opposite. Consider renaming to whenReportExactly28DaysApartThenFireMonthlyPixel
to match the test's actual behavior.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
8d59648
to
221b4a5
Compare
The PR title implies weekly, not monthly. Maybe just change WAU to MAU. |
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.
Just rename tests for clarity and everything looks good and works as expected.
Task/Issue URL: https://app.asana.com/0/69071770703008/1208808769852413/f
Description
Add a pixel for AppTP MAU.
The pixel fires at most once every 28 days
Steps to test this PR
You may want to change the polici to
REPLACE
inscheduleDeviceShieldStatusReporting()
to trigger the pixel on every apponCreate
Test
m_atp_ev_enabled_monthly
is sentm_atp_ev_enabled_monthly
is NOT sentm_atp_ev_enabled_monthly
is NOT sentm_atp_ev_enabled_monthly
is sent