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
Adds days since install calculation to the SDK #4491
Conversation
@@ -207,17 +220,43 @@ impl NimbusClient { | |||
|
|||
pub fn apply_pending_experiments(&self) -> Result<Vec<EnrollmentChangeEvent>> { | |||
log::info!("updating experiment list"); | |||
// If the application did not pass in an installation date, |
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.
Added this logic here because it happens off the main thread... Not sure if it's the best place for it
Codecov Report
@@ Coverage Diff @@
## nimbus-targeting-object #4491 +/- ##
==========================================================
Coverage ? 76.71%
==========================================================
Files ? 48
Lines ? 4496
Branches ? 0
==========================================================
Hits ? 3449
Misses ? 1047
Partials ? 0
Continue to review full report at Codecov.
|
8927d2d
to
50d0e3b
Compare
Huh https://app.circleci.com/pipelines/github/mozilla/application-services/28408/workflows/235fedb9-adfe-4dc6-92a8-2c1980dde3e7/jobs/116169 is very weird... It's a test failure in |
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.
Main changes needed:
- make
AppContext
immutable again. - make a new struct with
app_directory_path: string
anddays_since_install: i32?
and pass it in as part of aNimbus
constructor. - calculate
install_date
andlast_update_date
and store them in the database. - calculate
days_since_update
anddays_since_install
from the stored dates andnow()
. - Add a
TimeBasedAttributes
toTargetingAttributes
with thedays_since_update
anddays_since_install
.
50d0e3b
to
a632b8e
Compare
a632b8e
to
972fca5
Compare
Added tests, not 100% happy with how they look, but they test both the targeting and the fallback (it doesn't test that the actual fallback works, but it tests that we end up doing the fallback when we need to) I'll do a second go through the tests to see if I can make them better or cover more logic, but opening this up for more reviews! |
3e7469e
to
4ec7c56
Compare
f739139
to
aa2847c
Compare
4ec7c56
to
46266d5
Compare
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 looking lovely. There are a few bits I was confused about which you should address/answer/comment, but I don't think I should hold you up in another review cycle.
Good job!
90ef6b4
to
a3120c2
Compare
components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt
Show resolved
Hide resolved
a3120c2
to
20042b6
Compare
20042b6
to
21bbe92
Compare
Part of https://mozilla-hub.atlassian.net/browse/SDK-384
Some important TODOs before this can land:
installation_date
, then address the actual values on another PR