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

Insights short share URL #4513

Merged
merged 18 commits into from
Jun 1, 2021
Merged

Insights short share URL #4513

merged 18 commits into from
Jun 1, 2021

Conversation

paolodamico
Copy link
Contributor

@paolodamico paolodamico commented May 26, 2021

Changes

This PR sets some groundwork for saved reports & facilitating collaboration with insights (#3408).

  • Introduces a short_id (~8 alpha character) so saved insights can be shared easily (think like a YT video).
  • Saved insights can now be accessed via /i/{id}, making it easy to share them with other team members (e.g. https://app.posthog.com/i/DXQRFh3F.
  • Misc improvements.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious

@timgl timgl temporarily deployed to posthog-pr-4513 May 26, 2021 23:52 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 00:37 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 13:43 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 14:35 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 14:36 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 15:39 Inactive
@paolodamico paolodamico changed the title Saved Insights Insights short share URL May 27, 2021
@paolodamico paolodamico marked this pull request as ready for review May 27, 2021 15:45
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 15:58 Inactive
@paolodamico paolodamico requested review from timgl and liyiy May 27, 2021 16:01
@timgl timgl had a problem deploying to posthog-pr-4513 May 27, 2021 16:36 Failure
@timgl timgl had a problem deploying to posthog-pr-4513 May 27, 2021 16:40 Failure
@paolodamico
Copy link
Contributor Author

Ready for a review, not sure why tests are failing, they pass locally. Seems like the issue with webpack (related thread), see screenshot of failed tests.

Funnels -- Add 1 action to funnel and navigate to persons (failed) (attempt 3)

@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 19:25 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 27, 2021 22:57 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 May 31, 2021 20:34 Inactive
@timgl timgl temporarily deployed to posthog-pr-4513 June 1, 2021 01:54 Inactive
@paolodamico
Copy link
Contributor Author

Merging this,

  1. Tests seem to be failing related to the CSS bundling and/or caching issue, which is not actually a test failure. See archive screenshot with components not being rendered correctly.
  2. Tests pass consistently locally.
  3. No code has changed since tests were consistently passing (2dd4c8d) except branch merges.

image
image

@paolodamico paolodamico merged commit 1b0240d into master Jun 1, 2021
@paolodamico paolodamico deleted the saved-reports-I branch June 1, 2021 14:18
def create_short_ids(apps, schema_editor):
DashboardItem = apps.get_model("posthog", "DashboardItem")
for obj in DashboardItem.objects.all():
obj.short_id = posthog.models.dashboard_item.generate_short_id()
Copy link
Contributor

Choose a reason for hiding this comment

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

This will likely cause users to be unable to update in the future if we ever rename the method in the model

Rather than relying on app code in a migration it's smarter to inline the code, even if it means duplication.

paolodamico added a commit that referenced this pull request Jun 1, 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.

5 participants