feat: Added filtering by date support for persons API

[benjackwhite]

Problem

Related to #12962

Customer requested the ability to filter persons based on the creation date via the API. We support for our other
primitives so why not here.

Changes

• Extends the list persons endpoint to filter on created_at_from and created_at_to

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

How did you test this code?

Tests

[neilkakkar]

I don't think this will close the issue you linked, since that is about exposing this in the UI as well.

Feedback on the PR:

I think these parameters should be a part of the filter, rather than becoming an extra parameter to PersonQuery. This ensures we support GET and POST both, and adhere to one place for all filters.

I'm guessing you decided to put this here because it won't be used anywhere but for persons filtering? I think it makes sense for this to be in the filter, so in the future someone can also filter insights based on this person property.

Also see https://github.com/PostHog/posthog/blob/master/posthog/models/filters/mixins/common.py#L354 for helper function for parsing dates relative_date_parse_with_delta_mapping

[benjackwhite]

I don't think this will close the issue you linked, since that is about exposing this in the UI as well.

Yeah changed to "related"

I think these parameters should be a part of the filter, rather than becoming an extra parameter to PersonQuery. This ensures we support GET and POST both, and adhere to one place for all filters.
I'm guessing you decided to put this here because it won't be used anywhere but for persons filtering? I think it makes sense for this to be in the filter, so in the future someone can also filter insights based on this person property.

Here I actually started with the filter using the date_from and date_to params but this caused issues all over the place as we pass filters in related to trends etc. Are you suggesting I use the same params or add more params specifically for the created_at value? Could do that but feels like it is too specific to Persons in that case?

[Twixes]

This looks pretty good, though I do agree with Neil it might make sense to make this a part of filters. I requested @mariusandra's review, as he's working on a new querying API scheme which will include a PersonsNode, so this seems of interest.

[neilkakkar]

Definitely don't re-use date_from and date_to, but create the created_at_from parameter

[neilkakkar]

Could do that but feels like it is too specific to Persons in that case?

Which is perfectly fine, because all insights can filter on persons as well, without calling the Persons endpoint directly.

[neilkakkar]

Another option might be to fold it under property filters, and special case that everywhere since it's not a regular property, but that's a much bigger chunk of work, although it makes a bit more conceptual sense

[mariusandra]

We should fold this under property filters. Here's my main motivation. This is an event that's sent back to us:

Currently you can't filter the fields "id", "distinct_id", "timestamp" or "event" in the interface. You can here:

but what if you want event name starts with "toolbar" or "everything that's not a pageview, made between Oct 31 and Dec 25". You can't.

The solution is to add a new pill to the filter called "event fields", with fields "event name", "timestamp", "distinct id" and perhaps something else. Similar to the "elements" tab we now have.

A similar category can be created for person properties, and perhaps recordings. The API does return a few things:

[benjackwhite]

@mariusandra - I'm worried this could lead to greater confusion...

How do I filter differently between person.created_at and person.properties.created_at ? Given how common a field this is it feels like this clash will definitely happen.

If we had a separate tab of "person_fields", what would be there other than created_at? I guess we could have the actual Person ID?

Happy to put this in property filters but feels like I might need some guidance of how we want to do that in a manageable way...

[mariusandra]

In the hypothetical case, when you have a custom person property created_at, then yes, you might confuse people. However even then, separating the filter into "person metadata" ("id", "distinct_id", "created_at") and "person properties" (stuff you have sent along) should IMO reduce confusion, rather than increase it.

I'd put the "person properties" as the first pill in the filters (you usually want a property), and then "person metadata" as the second pill. It'll look good enough.

[neilkakkar]

I think that makes sense.

Implementation wise, this should be a different type of Property, Ben:
https://github.com/PostHog/posthog/blob/master/posthog/models/property/property.py#L33

so we can distinguish these everywhere;

and then wherever a property filter is used, it always calls this function: https://github.com/PostHog/posthog/blob/master/posthog/models/property/util.py#L201 , so we'd need to add code here for handling the new type appropriately, i.e. instead of JSONExtractRaw(person.properties, 'whatver prop'), it's person.created_at.

---

The same is true for event metadata, where people want to filter on distinct IDs / created_at / etc. etc.

Relevant: #7810 and #8250 (wow, I didn't realise we've had this convo before!)

[posthog-bot]

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.

[mariusandra]

I'm happy to take this over and own it, as it's very much connected to my work this sprint.

[posthog-bot]

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]

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