Refactor analytics queries #1280
Conversation
| self.assertEqual(action_response[0]["data"][1], 1.0) | ||
| self.assertEqual(action_response[0]["labels"][1], "Thu. 2 January") | ||
|
|
||
| self.assertTrue(self._compare_entity_response(action_response, event_response)) |
There was a problem hiding this comment.
@EDsCODE I might remove all of these comparisons and just the actions functionality once to simplify and speed up tests. Thoughts? I'm not super sure what they were originally supposed to test
There was a problem hiding this comment.
the compare entity response was meant to make sure the responses for actions and event responses don't differ if the entities are the same
There was a problem hiding this comment.
Yep, i've left most of those in noww
posthog/queries/trends.py
Outdated
|
|
||
| return entities_list | ||
|
|
||
| def run(self, filter: Filter, team: Team, actions: QuerySet): |
There was a problem hiding this comment.
This pattern helps a lot to unload logic from the /api.
I think for the case of reusability it may help to provide detailed comments above some of the utility functions (such as describing the params needed and the shape of the output). Mainly suggesting this because I was reading a function handle_compare and there was just a bit too much time required to remember what the function actually produces and how to use the output. This would allow us to reference the functions more easily and know what the inputs and outputs of a function are and more easily reuse them heavily.
There was a problem hiding this comment.
Yep, I've added comments. I'm still not really happy with these functions but I want to minimise the actual code changed in this PR, but will tackle those in the next ones
| class Retention(BaseQuery): | ||
| def calculate_retention(self, filter: Filter, team: Team, start_entity: Optional[Entity] = None, total_days=11): | ||
| date_from: datetime.datetime = filter.date_from # type: ignore | ||
| filter._date_to = (date_from + timedelta(days=total_days)).isoformat() |
There was a problem hiding this comment.
Perhaps in a followup refactor since this refactor is overall restructuring. We should standardize how/when to handle filters/params. For example, in this instance above we don't check any conditions and just write in something extra to the filter whereas in trends.py line 298 we do explicit checks before reassigning
There was a problem hiding this comment.
Yep agree, something I will tackle. It's annoying b/c the way they change the dates isn't consistent
Changes
Move most of the analytics logic into a
queriesfolder. This will make sure we have all of our analytics logic in one place, and will make it much easier to change and extend that logic.Checklist