Skip to content

feat: use tz_offset when doing deployment dau query #7736

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

Merged
merged 7 commits into from
Jun 1, 2023

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented May 31, 2023

Fixes #7275

@Emyrk Emyrk requested a review from johnstcn May 31, 2023 16:53
@@ -453,7 +453,7 @@ func (q *fakeQuerier) GetTemplateDAUs(_ context.Context, arg database.GetTemplat
continue
}

date := as.CreatedAt.UTC().Add(time.Duration(arg.TzOffset) * time.Hour).Truncate(time.Hour * 24)
date := as.CreatedAt.UTC().Add(time.Duration(arg.TzOffset) * time.Hour * -1).Truncate(time.Hour * 24)
Copy link
Member

@johnstcn johnstcn May 31, 2023

Choose a reason for hiding this comment

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

🤔 This change didn't break anything... which means that our unit tests probably aren't checking something that they should.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are half right.

I forgot to change it in the Dau dbfake function, which does break the unit test.

The template test does not break because I reverted the Template DAU to only use UTC again for performance reasons. So this is always 0 as the offset.


tl;dr fixing it in the spot that is used does break the test

@Emyrk Emyrk requested a review from johnstcn May 31, 2023 18:53
@ammario ammario changed the title feat: Have FE use tz_offset when doing deployment dau query. feat: have FE use tz_offset when doing deployment dau query May 31, 2023
@ammario ammario changed the title feat: have FE use tz_offset when doing deployment dau query feat: Have FE use tz_offset when doing deployment dau query May 31, 2023
@Emyrk Emyrk changed the title feat: Have FE use tz_offset when doing deployment dau query feat: use tz_offset when doing deployment dau query Jun 1, 2023
@Emyrk Emyrk merged commit cf8d2bc into main Jun 1, 2023
@Emyrk Emyrk deleted the stevenmasley/dau_fe_timezone branch June 1, 2023 13:23
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: daily active users modal in Deployment tab is incorrect
2 participants