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

Automatically switch to the right project if possible #8681

Merged
merged 12 commits into from
Feb 18, 2022
Merged

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Feb 17, 2022

Changes

Sometimes you get sent a link to PostHog that points to an item from a different project than the one you are currently in. Right now this results in a 404. With this solution, if you have access to the target project, you'll be seamlessly switched to it.

2022-02-17 17 56 49

This was kind of tackled already by the solution from #8262, but that required user interaction and worked only for dashboards. This solution instead "asks for forgiveness" (which seems to be the more intuitive choice here) and works for dashboards, insights, feature flags, actions, and cohorts.

Note that this does not fix using multiple projects at once (#5363), nonetheless it's a significant improvement to the experience of using multiple projects.

How did you test this code?

Added a bunch of cases to test_middleware.py (TestAutoProjectMiddleware).

Copy link
Contributor

@clarkus clarkus left a comment

Choose a reason for hiding this comment

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

Looks good to me. We might consider a toast instead of the banner. That would have the benefit of being lower priority, dismissing automatically, while still offering a prominent undo or go back action that helps the user recover just in case this was a navigational error. The key thing to communicate is that the user is in a new context - "you've switched to project ABC". Not so much why it happened, just that it did and that there is an easy way to go back. Thoughts?

@Twixes
Copy link
Collaborator Author

Twixes commented Feb 17, 2022

That's some good points, +1 on autodismiss and being less verbose – a toast sounds like a good solution. But I'm not sure what should it look like – our toast are kind of all over the place currently, and I don't think we have a good pattern for info-level ones (as opposed to the more common success/error).

@Twixes
Copy link
Collaborator Author

Twixes commented Feb 17, 2022

Currently looks like this:
Screen Shot 2022-02-17 at 19 19 06

@clarkus
Copy link
Contributor

clarkus commented Feb 17, 2022

Screen Shot 2022-02-17 at 10 47 33 AM

Here's a work-in-progress toast design in two variations. Thoughts on either option?

@Twixes
Copy link
Collaborator Author

Twixes commented Feb 17, 2022

The light one looks pretty great (black seems extremely contrasty compared to the rest of the app).
One thought is that "Undo" is pretty ambiguous, plus a user is quite likely to not remember what project they'd be switching back to. My proposal is "Switch back to <previous project>", though that could require putting the button below the text.

@clarkus
Copy link
Contributor

clarkus commented Feb 17, 2022

The light one looks pretty great (black seems extremely contrasty compared to the rest of the app). One thought is that "Undo" is pretty ambiguous, plus a user is quite likely to not remember what project they'd be switching back to. My proposal is "Switch back to ", though that could require putting the button below the text.

Agree with the approach. Naming where they're returning to would be great, but it's going to eat up a lot of space. We could say "go back" instead of "undo"? It would achieve the same intent but in fewer characters.

@hazzadous
Copy link
Contributor

@Twixes I don't have much context on this, but could we just encode the organization and project into the url and remove the state we're storing on the user db model altogether? Or is this something we have considered and rejected for some reason?

@Twixes
Copy link
Collaborator Author

Twixes commented Feb 18, 2022

Absolutely @hazzadous, that's the next step which will solve the multiple projects at once case. The difference is that this is a single middleware with some notification code, while putting the project in the URL will take updating many logics in the frontend and ensuring a backwards compatibility layer.

@hazzadous
Copy link
Contributor

@Twixes do you know when the next steps are likely to happen?

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@hazzadous the project ID is already embedded in the URL, just behind a foreign key relationship:

In these cases, if the user has access to the object they're trying to open, we can just swap the project behind the scenes when opening the URL... as that's very likely what they're trying to do.

We'll theoretically only need the project ID in the URLs if we ever want to share index pages like:

Putting this in the URL has been suggested as the correct implementation for this task, but that has been a pending/stalling todo item for months now, so there's some friction in getting this going. Personally I think we can stall longer, as the problem has always been about opening a certain insight/dashboard that are shared with you, not settings pages. This PR solves that beautifully.

Having two projects in two tabs side by side in one browser will still have some issues and requires rewriting some API calls. That'll be the next step to work on here... and finally we can see if making all URLs in posthog longer by adding a project ID is worth the tradeoff in friendliness.

@Twixes Twixes marked this pull request as ready for review February 18, 2022 10:53
@Twixes Twixes enabled auto-merge (squash) February 18, 2022 11:10
@Twixes Twixes disabled auto-merge February 18, 2022 11:13
@Twixes Twixes enabled auto-merge (squash) February 18, 2022 12:35
@Twixes Twixes merged commit 058b65a into master Feb 18, 2022
@Twixes Twixes deleted the project-from-url branch February 18, 2022 12:47
@Twixes
Copy link
Collaborator Author

Twixes commented Feb 18, 2022

Merged with that basic toast, I'm making a separate PR to overhaul the toast patterns.

EDsCODE added a commit that referenced this pull request Feb 18, 2022
* master:
  hobby: Wait for ClickHouse and for Postgres before starting (#8686)
  Part 2: Deprecate old tags and upgrade to new tags Backend (#8529)
  Remove flake8-commas (#8695)
  Update Breakdown props to use filter groups (#8679)
  Automatically switch to the right project if possible (#8681)
  Super Lazy VMs (#8609)
  .github/workflows/ci-backend.yml: fix flake8 config (#8676)
  Fix recording page refresh loop (#8685)
  Instance status configuration (#8096)
paolodamico pushed a commit that referenced this pull request Feb 23, 2022
* Remove `ObjectExistsInOtherProject`

* Automatically switch to the right project if possible

* Refactor `get_target_queryset` out

* Switch to toast and fix errors

* Add `IconSwapHoriz`

* Break out toast style updates

* Add tests

* Fix typing

* Optimize slightly

* Fix bug with non-numeric pseudo-IDs

* Add ✨ docstring ✨
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

5 participants