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

Add slug field to Organization #6395

Merged
merged 18 commits into from
Oct 13, 2021
Merged

Add slug field to Organization #6395

merged 18 commits into from
Oct 13, 2021

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 12, 2021

Changes

Slugs. We all love these slimy creatures, and today I'm honored to introduce them into our backend.

awww

How so? Well, we want to point to organizations and projects with URLs instead of relying on User fields. We could just use IDs there… but they're completely not user-friendly. Obviously they are impenetrable for humans, but additionally:

  • UUIDs are needlessly long for this use case
  • autoincremented IDs are not professional, as they leak platform usage data

The solution that seems much better for humans is slug IDs. This PR adds them (inferring their values from entity display names) for orgs and projects, so that soon we can have URLs like this
app.posthog.com/orgs/hogflix/projects/foobar-test-123
instead of
app.posthog.com/orgs/12add3aa-5e5c-47dc-8bad-e39f83d45505/projects/666.

In the future slugs may also be fully customizable, but that's not important now, and the obstacle I encountered while trying that out is… our settings pages. They've been growing ad hoc with no layout system, and that just shows – it's getting awkward adding more options, as there's no structure to it all, and the number of Save buttons rises. I think it'll be worth thinking about a long-term system before adding more granular settings like this.

How did you test this code?

I ran the migration, including a case of duplicated names.
Also manually tried creating and renaming orgs/projects.
And there are some new tests including edge cases.

@Twixes Twixes temporarily deployed to posthog-pr-6395 October 12, 2021 20:29 Inactive
@Twixes
Copy link
Member Author

Twixes commented Oct 12, 2021

This causes the backend checks to fail:

Screen Shot 2021-10-12 at 22 39 56

This is non-null very deliberately, by necessity, on relatively small tables – so I don't think we're in danger here. At the same time I don't know how to disable this check for migration operations that use SQL generated by Django (as opposed to RunSQL). Can it be worked around somehow, or does it have to be removed for this to pass? CC @timgl

@Twixes Twixes temporarily deployed to posthog-pr-6395 October 12, 2021 21:18 Inactive
@Twixes Twixes changed the title Add slug fields to Organization and Team Add slug fields to Organization Oct 12, 2021
@Twixes Twixes temporarily deployed to posthog-pr-6395 October 12, 2021 21:25 Inactive
@Twixes
Copy link
Member Author

Twixes commented Oct 12, 2021

Actually I scaled this down to only add slug to Organization – I realized that the additional back-and-forth between the frontend and the backend to resolve project slug to project ID would be too impactful, while adding first-class project slug support to the API is not really feasible. It's definitely not ideal to expose serial project IDs, but I feel that for now it's a better option than slowing the app down.
Organization is less impactful for overall app performance and only relays on large UUIDs anyway, so slug is still the way forward there.

@Twixes Twixes temporarily deployed to posthog-pr-6395 October 12, 2021 22:55 Inactive
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

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

lgtm! I would only consider the migration question as potentially blocker

posthog/api/organization.py Outdated Show resolved Hide resolved
frontend/src/types.ts Outdated Show resolved Hide resolved
posthog/models/utils.py Outdated Show resolved Hide resolved
slugified_name = slugify(kwargs["name"])[:MAX_SLUG_LENGTH] if "name" in kwargs else default_slug
for i in range(10):
# This retry loop handles possible duplicates by appending `-\d` to the slug in case of an IntegrityError
slugified_name_i = slugified_name[: MAX_SLUG_LENGTH - 2] if i == 0 else f"{slugified_name}-{i}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a better approach would be to generate a random 3 or 4 integer number and only try like 5 times?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do something really similar in ee/clickhouse/materialized_columns/columns.py#materialized_column_name and I suggest that approach instead.

Basically if there's a conflict we generate a n-letter random ascii-letter suffix and append that. :)

Worth unifying the two approaches at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I unified the suffix logic using the materialized_column_name approach – more unique = better. Used the same in the migration.

posthog/models/organization.py Outdated Show resolved Hide resolved
posthog/migrations/0174_organization_slug.py Show resolved Hide resolved
@Twixes Twixes temporarily deployed to posthog-pr-6395 October 13, 2021 10:33 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-6395 October 13, 2021 10:37 Inactive
@Twixes Twixes changed the title Add slug fields to Organization Add slug field to Organization Oct 13, 2021
@Twixes Twixes temporarily deployed to posthog-pr-6395 October 13, 2021 11:00 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-6395 October 13, 2021 11:04 Inactive
@Twixes Twixes temporarily deployed to posthog-pr-6395 October 13, 2021 11:29 Inactive
@Twixes Twixes merged commit ec0f7ef into master Oct 13, 2021
@Twixes Twixes deleted the project-org-slugs branch October 13, 2021 12:09
@mariusandra
Copy link
Collaborator

If you change the organisation's name, and change its slug, all existing links/bookmarks for users will now be invalidated.

Do we want to

  1. Keep this as is
  2. Add logic to remember past slugs and redirect accordingly
  3. Never change the slug
  4. Add a large warning when editing the name

?

@Twixes
Copy link
Member Author

Twixes commented Oct 14, 2021

I think this will be rare enough that we can put this off a bit, but in the end I think it'll be best for the slug to only be manually changeable in org settings. Maybe with a nicer settings layout, though that's not a prerequisite, just a very-nice-to-have. As for past slugs, would love some automagicness with redirecting from them, but at org level I'd prefer to free unused slugs up instead.

@Twixes Twixes mentioned this pull request Oct 14, 2021
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.

4 participants