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

fix(stickiness): display correct interval for dates #22304

Merged
merged 4 commits into from
May 21, 2024

Conversation

matzexcom
Copy link
Contributor

@matzexcom matzexcom commented May 15, 2024

Datapoint labels on stickiness graphs always count in days (#22209)

Problem

The tooltip does not replicate the correct "group by" interval in the insights stickiness line graph.

Changes

Bugfix: The getFormattedDate() method in insightTooltipUtils.tsx had a bug for numeric first param input. The pluralize() method had a fixed second argument instead of the interval param.

Nit: Changed the name for the first parameter from dayInput to input, because it can vary between the IntervalType. (Other possible names could be intervalAmountor timeQuantity)

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

  • open a dashboard
  • click Add Insight in top right corner
  • choose the Stickiness tab
  • set the group by interval to weeks for example
  • mouseover the line graph and the heading is now corresponding to the interval

Added automated unit tests for the getFormattedDate() method.
SCR-20240515-tvrz

@matzexcom
Copy link
Contributor Author

I would like to complete the unit tests with a string input for the first parameter in getFormattedDate(). I think the string input is used in the FunnelLineGraph. Could you please guide me to where in the front end the FunnelLineGraph is used 🙈 so I can see which strings are typical inputs?

@thmsobrmlr
Copy link
Contributor

I would like to complete the unit tests with a string input for the first parameter in getFormattedDate(). I think the string input is used in the FunnelLineGraph. Could you please guide me to where in the front end the FunnelLineGraph is used 🙈 so I can see which strings are typical inputs?

Thanks for taking this on @matzexcom! You can find the FunnelLineGraph by creating a funnel insight with "Historical trends" graph type.

Screenshot 2024-05-16 at 00 10 07

@matzexcom
Copy link
Contributor Author

matzexcom commented May 16, 2024

Thanks, @thmsobrmlr, for the guide. I added the remaining test cases.

I have a proposal; I would like to split the function in getFormatttedTimeInterval(intervalAmount: number, interval: IntervalType = 'day') and omit the type number from getFormattedDate(dateString: string, interval: IntervalType = 'day')

I would also like to make the first input required for a better type check because I think we don't want the string "undefined" in the front end to be displayed.

Is that ok?

@thmsobrmlr
Copy link
Contributor

Thanks, @thmsobrmlr, for the guide. I added the remaining test cases.

I have a proposal; I would like to split the function in getFormatttedTimeInterval(intervalAmount: number, interval: IntervalType = 'day') and omit the type number from getFormattedDate(dateString: string, interval: IntervalType = 'day')

I would also like to make the first input required for a better type check because I think we don't want the string "undefined" in the front end to be displayed.

Is that ok?

Hey @matzexcom, yeah that makes sense to me. Should we get this PR in first and do that in a follow up? (Triggered a CI run right now & will review/merge if that passes)

@thmsobrmlr thmsobrmlr changed the title Bugfix/issue 22209 fix(stickiness): display correct interval for dates May 21, 2024
@thmsobrmlr thmsobrmlr enabled auto-merge (squash) May 21, 2024 13:39
auto-merge was automatically disabled May 21, 2024 17:56

Head branch was pushed to by a user without write access

@matzexcom
Copy link
Contributor Author

@thmsobrmlr, sure I will create a follow-up PR for the little refactor.

Do you know if there is anything I can do here? The failing cypress test earlier today seems unrelated to this PR. I rebased the branch, maybe it will succeed now.

@thmsobrmlr
Copy link
Contributor

@matzexcom Let me try to re-run the tests right now. Some of them are flaky and just need to be ran again.

@thmsobrmlr thmsobrmlr merged commit f40e14b into PostHog:master May 21, 2024
78 of 79 checks passed
@thmsobrmlr
Copy link
Contributor

Just merged this in. Thanks again for the PR!

@matzexcom matzexcom deleted the bugfix/issue-22209 branch May 22, 2024 17:51
timgl pushed a commit that referenced this pull request May 24, 2024
---------

Co-authored-by: Matthias Vogel <hub@birdsview.dev>
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.

None yet

2 participants