Skip to content
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

Store full enrollment date on a/b/n and truncate before sending pixels #5535

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

marcosholgado
Copy link
Contributor

@marcosholgado marcosholgado commented Jan 24, 2025

Task/Issue URL: https://app.asana.com/0/1125189844152671/1209213126411295/f

Description

This PR stores the full enrollment date for better comparison but still truncates it when sending pixels.

Steps to test this PR

  • Tests should pass
  • Load https://www.jsonblob.com/api/1304478638945460224 to enroll in an experiment
  • Filter the logcat by "pixel sent"
  • You should see an experiment_enroll pixel sent with the enrollment date truncated
  • Make a search
  • You should see an experiment_metric... pixel sent with the enrollment date truncated
  • Check shared prefs com.duckduckgo.feature.toggle.blocklist. The enrollment date for blockList_tdsNextExperimentBaselineBackup should have the full date rather than be truncated to days.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@marcosholgado marcosholgado requested a review from aitorvs January 24, 2025 16:51
@marcosholgado marcosholgado force-pushed the feature/marcos/store_full_date branch from 109d4af to fcd5b7e Compare January 24, 2025 17:15
Comment on lines 86 to 89
private fun daysBetweenTodayAnd(date: String): Long {
val today = ZonedDateTime.now(ZoneId.of("America/New_York"))
val localDate = LocalDate.parse(date)
val zoneDateTime: ZonedDateTime = localDate.atStartOfDay(ZoneId.of("America/New_York"))
return ChronoUnit.DAYS.between(zoneDateTime, today)
val localDate = ZonedDateTime.parse(date)
return ChronoUnit.DAYS.between(localDate, today)
Copy link
Contributor

@LukasPaczos LukasPaczos Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now returns different values than it used to.

Here's an example:

    @Test
    fun test() {
        val enrollmentTruncatedZDT = "2025-01-30"
        val enrollmentNotTruncatedZDT = "2025-01-30T18:05:28.548199-05:00[America/New_York]"

        // check on the next day when MORE than 24 hours since enrollment elapsed, both methods return 1 day diff
        var currentZDT = "2025-01-31T18:06:07.467047-05:00[America/New_York]"
        val old1 = oldDaysBetweenTodayAnd(enrollmentDate = enrollmentTruncatedZDT, currentZDTString = currentZDT)
        val new1 = daysBetweenTodayAnd(enrollmentDate = enrollmentNotTruncatedZDT, currentZDTString = currentZDT)
        assertEquals(old1, new1) // passes

        // check on the next day when LESS than 24 hours since enrollment elapsed, old method returns 1 while the new returns 0
        currentZDT = "2025-01-31T18:04:07.467047-05:00[America/New_York]"
        val old2 = oldDaysBetweenTodayAnd(enrollmentDate = enrollmentTruncatedZDT, currentZDTString = currentZDT)
        val new2 = daysBetweenTodayAnd(enrollmentDate = enrollmentNotTruncatedZDT, currentZDTString = currentZDT)
        assertEquals(old2, new2) // fails
    }

    private fun oldDaysBetweenTodayAnd(enrollmentDate: String, currentZDTString: String): Long {
        val today = ZonedDateTime.parse(currentZDTString)
        val localDate = LocalDate.parse(enrollmentDate)
        val zoneDateTime: ZonedDateTime = localDate.atStartOfDay(ZoneId.of("America/New_York"))
        return ChronoUnit.DAYS.between(zoneDateTime, today)
    }

    private fun daysBetweenTodayAnd(enrollmentDate: String, currentZDTString: String): Long {
        val today = ZonedDateTime.parse(currentZDTString)
        val localDate = ZonedDateTime.parse(enrollmentDate)
        return ChronoUnit.DAYS.between(localDate, today)
    }

If the aim of the framework was only to check for calendar day changes without accounting for hours, then the new iteration would regress by being too precise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need to be more precise, should be 24h diff rather than calendar days so the new one seems to work as expected compared to the old.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 In this case, #5535 (comment) would need to get updated in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rolling back on this thought, it shouldn't be 24h so I'm changing this bit and leaving the other one as it is.

@@ -85,8 +85,7 @@ class RetentionMetricsAtbLifecyclePlugin @Inject constructor(

private fun daysBetweenTodayAnd(date: String): Long {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one more copy of this function in MetricsPixelInterceptor that we should also adjust, shouldn't we?

Copy link
Contributor

@LukasPaczos LukasPaczos Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks fine to stay because it takes truncated days as parameter 👍 It would be great to add a small comment to the function parameter so that someone stumbling on this in the future will immediately understand the difference between the way we parse and use the various enrollmentDate instances. Needs to get updated in the end, refs #5535 (comment).

@marcosholgado marcosholgado force-pushed the feature/marcos/store_full_date branch from ae412b2 to 9b0bea8 Compare January 28, 2025 10:14
Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the default browser prompts feature, LGTM

@marcosholgado marcosholgado merged commit cd17533 into develop Jan 31, 2025
7 checks passed
@marcosholgado marcosholgado deleted the feature/marcos/store_full_date branch January 31, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants