Bugfix/issue 22209

[matzexcom]

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.

[matzexcom]

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]

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.

[matzexcom]

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]

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)

[matzexcom]

@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]

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

[thmsobrmlr]

Just merged this in. Thanks again for the PR!