posthog icon indicating copy to clipboard operation
posthog copied to clipboard

refactor(insightTooltipUtils): Splits getFormattedDate to new getFormattedTimeInterval method

Open matzexcom opened this issue 1 year ago • 2 comments

"Problem"

In insightTooltipUtils.tsx, the method getFormattedDate may be overloaded. The method generates a formatted date, as the name implies, but it also generates an interval amount like "2 days, 4 weeks, ..." depending on the type of the first argument, which may not be intuitive for future maintainers.

Propose

Extract the part for the interval amount from the method getFormattedDate into a new method getFormattedTimeInterval

Changes

  • Created a new method `getFormattedTimeInterval(intervalAmount: number, interval: IntervalType = 'day')``
  • Changed Input type from getFormattedDate to string and make it required
  • Changed the InsightTooltipProps.date to string | number to reflect the possibility that the date can either be a string (hopefully a date string) or a number
  • Changed the getTooltipTitle method a bit to reflect the change that the date can be undefined or a number, which would result in a "bad" title
  • Adds unit tests for getFormattedTimeInterval and getTooltipTitle

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

Yes

How did you test this code?

Add a new insight on a dashboard, test the different tabs, and check if the tooltip is the same as before.

matzexcom avatar May 22 '24 19:05 matzexcom

@thmsobrmlr I proposed a refactor, which we discussed in #22304, and I created this separate PR. My refactor clarifies the purpose of the insightTooltipUtil methods. I had to adjust some types because there are cases where the date variable is a number, that was missed in the typing. I changed the nearest type definition because changing the definition of days in the TrendResult interface would result in a big refactor.

I hope my changes don't add more complexity and reduce tech dept for the future.

matzexcom avatar May 22 '24 20:05 matzexcom

Thanks for the catch; I thought I had checked that; my mistake. I wrote an e2e test to reflect my introduced bug 🙈 and fixed it. I am not very happy with the nested ternary. I don't know if it is allowed in the coding convention or if I should write a "helper" method for the InsightTooltip Component to get the title.

matzexcom avatar May 23 '24 20:05 matzexcom

I am not very happy with the nested ternary. I don't know if it is allowed in the coding convention or if I should write a "helper" method for the InsightTooltip Component to get the title.

The nested ternary is ok, we have many of those in the codebase.

thmsobrmlr avatar May 27 '24 11:05 thmsobrmlr

@thmsobrmlr Yes not related to my changes here.

Since commit 31f97c11 the tooltip in stickiness is broken again

the interval type in seriesData?.[0]?.filter?.interval is undefined, the filter property is an empty object {}

I have to comment out/delete this two lines

#     "hogql-in-insight-serialization",
#     "hogql-insights-preview",

in posthog/settings/feature_flags.py after that the filter property is filled again and the tooltip is shown correctly

matzexcom avatar May 27 '24 15:05 matzexcom

@thmsobrmlr Any idea where I can investigate the issue? I tried to find my way in the backend and found out that the filter parameter is built within this method _query_to_filter in posthog/hogql_queries/insights/trends/trends_query_runner.py (maybe)

I tried to start the django server in debug mode but I can not start it and get weird ModuleNotFoundError: No module named 'errors'

matzexcom avatar Jun 05 '24 18:06 matzexcom

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

posthog-bot avatar Jun 18 '24 07:06 posthog-bot

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

posthog-bot avatar Jun 25 '24 07:06 posthog-bot