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

User V2 API endpoint #3855

Merged
merged 32 commits into from
Apr 5, 2021
Merged

User V2 API endpoint #3855

merged 32 commits into from
Apr 5, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented Apr 1, 2021

Changes

This PR introduces a brand-new /api/users/@me/ endpoint. Instead of a full refactor, we introduce the new endpoint while keeping support for the old one for a little while. While we've known for a while this refactor is needed, this is motivated because now it has become a blocker for some teams that have a ton of events and/or event properties (see Slack thread).

This PR is already on the larger end of things, as such, I've decided to split this whole refactor, and this particular PR only includes backend changes.

Please don't be scared by the number of file changes or lines. Most lines are tests and the large number of file changes is mostly due to a simple name refactoring of the UserSerializer (see below).

Detailed changes

  • The new /api/users/@me/ endpoint is now built with DRF serializers and views. Among other things we pay special attention to the nested properties included (to reduce them to the minimum). A random sample in production shows a 50x+ reduction in payload size (while this data is still loaded in the frontend, it is no longer blocking to render the app).
    • Particularly worth noting, we no longer include heavy properties in the payload [particular related to the Team object] (e.g. team.event_names, team.event_properties, team.event_properties_numerical, team.event_names_with_usage).
    • The endpoint fully supports the relevant parts of the user object, including changing the current organization, current team & password (with special logic handling, i.e. password changing requires sending the current_password).
    • Related to the above, we make sure users cannot set current organization or teams that don't make sense (e.g. setting a team that doesn't belong to the organization being set).
    • Nested updates are no longer supported (deliberate decision). To update the Team or Organization the respective endpoints need to be used.
  • Instance-based properties that were previously being sent in the legacy endpoint are now sent in the preflight check response (e.g. is_debug, email_service_available, is_event_property_usage_enabled, posthog_version).
  • All event reporting and instrumentation is kept as is (i.e. we send an async $identify to update the user on every API hit). We also introduce a user updated backend event.
  • We add a uuid property (UUIDT) to the User model to start using as key in API requests.
  • The previously existing UserSerializer is refactored to UserBasicSerializer to properly reflect the goal and composition of this serializer. This serializer is now used for any nested properties (e.g. used for created_by in multiple endpoints).
  • Removed the unused last_name attribute from User model (part of Django's base AbstractUser).

New API response (GET /api/users/@me/)

{
  "id": "01788ed8-6d2d-0000-ccdb-d8e17589b1ea",
  "distinct_id": "kOyAmTq2McfCD5XvqwjKf8m6GVecm9uN0L0ypEoRN50",
  "first_name": "Jane",
  "email": "test@posthog.com",
  "email_opt_in": true,
  "anonymize_data": false,
  "toolbar_mode": "toolbar",
  "has_password": true,
  "is_staff": false,
  "is_impersonated": false,
  "team": {
    "id": 1,
    "uuid": "01787e47-55d7-0000-6976-b893a03aaba8",
    "organization": "01787e47-5558-0000-bae1-6bc513b42abe",
    "api_token": "dtrwW0uEC7LwVx64RmZmqog_XjU0nXWuEK1d1ZpjteQ",
    "name": "HogFlix Demo App",
    "completed_snippet_onboarding": true,
    "ingested_event": true,
    "is_demo": true,
    "timezone": "UTC"
  },
  "organization": {
    "id": "01787e47-5558-0000-bae1-6bc513b42abe",
    "name": "Hogflix Movies",
    "created_at": "2021-03-29T13:58:27.416616Z",
    "updated_at": "2021-03-29T13:58:57.067131Z",
    "membership_level": 15,
    "personalization": {
      "role": "engineer",
      "products": ["web", "website"],
      "technical": "technical"
    },
    "setup": {
      "is_active": true,
      "current_section": 1,
      "any_project_ingested_events": false,
      "any_project_completed_snippet_onboarding": false,
      "non_demo_team_id": null,
      "has_invited_team_members": true
    },
    "plugins_access_level": 9,
    "teams": [
      {
        "id": 1,
        "uuid": "01787e47-55d7-0000-6976-b893a03aaba8",
        "organization": "01787e47-5558-0000-bae1-6bc513b42abe",
        "api_token": "dtrwW0uEC7LwVx64RmZmqog_XjU0nXWuEK1d1ZpjteQ",
        "name": "HogFlix Demo App",
        "completed_snippet_onboarding": true,
        "ingested_event": true,
        "is_demo": true,
        "timezone": "UTC"
      }
    ],
    "available_features": []
  },
  "organizations": [
    { "id": "01787e47-5558-0000-bae1-6bc513b42abe", "name": "Hogflix Movies" }
  ]
}

Additional context

  • What this will permit on the front-end is that we'll only require the /api/user/ & /_preflight endpoints to be loaded to be able to display the app, instead of requiring this plus, /api/projects/@current/, /api/organizations/@current/ plus the billing information. These two required endpoints are significantly lighter and faster than they were before. The rest of the info will be loaded without blocking the entire page render. Relevant local components will display a local loading state.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests. Not applicable.
  • Cypress end-to-end tests. Not applicable.

@sentry-io
Copy link

sentry-io bot commented Apr 1, 2021

Sentry issue: POSTHOG-2KR

@timgl timgl temporarily deployed to posthog-pr-3855 April 1, 2021 23:48 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 02:41 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 15:39 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 15:40 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 15:41 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 15:51 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 16:07 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 18:13 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 2, 2021 23:13 Inactive
@timgl timgl temporarily deployed to posthog-pr-3855 April 5, 2021 13:49 Inactive
Copy link
Member

@EDsCODE EDsCODE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small comments but lgtm!

from posthog.test.base import APIBaseTest


class TestUser(APIBaseTest):
class TestUserAPI(APIBaseTest):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could move this in separate file to keep files smaller

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I actually think this is the class that makes sense keeping here. The other one is just a legacy class to be kept around until we remove the old endpoint.

posthog/api/team.py Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-pr-3855 April 5, 2021 14:30 Inactive
@paolodamico
Copy link
Contributor Author

Thanks for the review @EDsCODE! Would you mind taking another quick look? I've fixed merged conflicts, address your feedback, and some minor tweaks that came up while updating the frontend.

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, just got some questions regarding routing

posthog/api/user.py Show resolved Hide resolved
posthog/urls.py Outdated Show resolved Hide resolved
@paolodamico paolodamico requested a review from Twixes April 5, 2021 16:58
@paolodamico
Copy link
Contributor Author

Ready for one final look @Twixes

@timgl timgl temporarily deployed to posthog-pr-3855 April 5, 2021 17:14 Inactive
Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some picking of nits

posthog/views.py Show resolved Hide resolved
posthog/migrations/0143_user_uuid.py Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-pr-3855 April 5, 2021 19:13 Inactive
@paolodamico paolodamico requested a review from Twixes April 5, 2021 19:14
@Twixes Twixes temporarily deployed to posthog-pr-3855 April 5, 2021 19:32 Inactive
Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Twixes Twixes removed the request for review from mariusandra April 5, 2021 19:42
@Twixes Twixes merged commit 9865bbc into master Apr 5, 2021
@Twixes Twixes deleted the user-endpoint-refactor branch April 5, 2021 19:42
@paolodamico paolodamico mentioned this pull request Apr 5, 2021
4 tasks
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

4 participants