Non-Simple Profile page does not work in MultiTenancy Bug - Need Advice For Best Solution for PR
When using a profile page (custom or default) on a panel with Multitenancy using the non simple layout it throws an exception. I want to submit a PR to fix this but I have a few different ways I can handle this and I want some feedback on which I should do.
In my Panel Provider class I have this:
This results in the Exception of: "Filament\FilamentManager::getTenantName(): Argument #1 ($tenant) must be of type Illuminate\Database\Eloquent\Model, null given, called in /path/to/project/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php on line 363"
It works fine if I just call
->profile(Profile::class) and thus use the simple layout.
I haven't dug into the exception yet, but my assumption is the tenant switcher on the non-simple layout is unable to find what the current tenant is because the profile page is defined outside the tenant aware set of routes.
I could just use the simple layout; however, I think for anyone else who tries to setup a profile page on a panel with Multitenancy without the simple layout, it should work without throwing this exception. So I have a couple of ideas on how to address this in a bugfix PR, but want some other opinions on which solution you all think is best. The possible solutions I have come up with:
1. Exclude the tenant switcher from the layout unless it is on a panel with multitenancy AND there is a tenant selected.
2. Make the tenant switcher just treat the first tenant as the "selected" one
3. Make the tenant switcher work without a tenant selected, it would just be blank, but you could select a tenant and move to the dashboard for that tenant.
4. Update Filament's routing so that it will setup the Profile page as a tenant aware route when using the non-simple layout on a multitenant panel (For example this is how Stripe does their user profile pages, and what I am personally leaning towards)17 Replies
I haven't dug into the exception yet, but my assumption is the tenant switcher on the non-simple layout is unable to find what the current tenant is because the profile page is defined outside the tenant aware set of routes.That's correct.
Exclude the tenant switcher from the layout unless it is on a panel with multitenancy AND there is a tenant selected.I thought about this too, but though it might confuse people. @Dan Harrin What do you think? If somebody want the non-simple layout, what should be the best behaviour?
Yeah I think 1 is my least favorite solution for sure, not only because of that but the layout shift from all the menu links moving up to fill the void.
I also don't think I like 2 very much either, because if that is not the tenant which they were using that could be odd that it would just seemingly switch on the user.
So I think its going to come down to either leaving the tenant switcher with a blank selection and the profile route exists outside of the scope of a tenant, or else 4 where the profile is part of a tenant so it can maintain the previously selected tenant.
I am curious to see what Dan thinks on this for sure though.
Having had the chance to think on this more deeply, I would actually change my recommendation from #4 to #3.
#4 is technically already possible by making a regular Custom Page and customizing the Profile link to point to it. So while I still like #4 and will probably just do that in my app, I think making #3 the fix here gives developers both options. And maybe the PR could also add a small call out in the docs (say around here: https://filamentphp.com/docs/4.x/users/overview#using-a-sidebar-on-the-profile-page) to point out the possibility of making a tenant aware profile page using a regular Custom Page and overriding the profile link?
But I am still going to wait on Dan's feedback here to make sure I don't waste my time PRing a fix that is not what he wants.
i think #3
It seems its a bit deeper than just fixing the tenant selection menu. I get exceptions for building the tenant billing URl, and other side menu URLs which of course require a tenant for the path, can't believe I didn't think of that.
I'm not sure there's an easy way out of this, other than just throwing a custom Filament exception when trying to use the non simple layout for the profile page on a panel with Tenancy that tells the developer that multitenant panels must use the simple layout for the profile page or use a custom page that exists within the scope of a tenant?
I can also hide the tenant switcher and disable the navigation for a non simple view in a multitenant panel. I got that to work with some smalll tweaks.
I guess the biggest issue is the concept. A users profile isn’t based on the tenant and a user can belong to more than one tenant. So how can the profile architecturally be under a tenant?
I wonder if the active tenant could be placed in Session or Cache and updated if a new tenant is discovered in the url?
Not sure of any other internal implications of that approach though.
That's the way Stripe does it, You just see the same profile regardless of what Account (tenant) you are accessing. So they still have a current Account (tenant) to scope the navigation links to. And you end up with a profile URL of: https://dashboard.stripe.com/(account_id)/settings/user
Stripe Login | Sign in to the Stripe Dashboard
Sign in to the Stripe Dashboard to manage business payments and operations in your account. Manage payments and refunds, respond to disputes and more.
Maybe that’s the solution then. Use the tenant url for the profile if tenancy exists.
The simple answer might to either make the non simple view on a multitenant panel this no navigation view so there are no tenant specific links that need to be generated
Yeah so I'm not sure whats the best way foward as of now.
Could just be a check in the Profile url route in filament to added the tenant if it exists, otherwise fallback to how it’s working now.
It’s still a PR either way. I think that would be the simplest solution. Just not sure if there’s any side effects to it.
Yeah I think both are technically valid ways, I think its just a matter of which Dan prefers at this point.
Agreed
If we go the disable navigation and tenant switcher for the non simple view it looks like this:
It does end up looking very similar to the simple view to be fair.
I know for my use case I prefered having the normal navigation which is why I tried to use the non simple view in the first place, but even if we went with this no navigation approach one could still make a custom page to act as the profile page which is scoped to a tenant and could get working navigation.
the whole reason that the simple profile page exists is because it works with tenancy
its default because it works in all cases
we actually introduced the non-simple version later on with the caveat that it was not meant to be used with tenancy
i definitely dont think that the profile url should contain the tenant id
If you just don't want to support the non simple view with multitenancy I could just PR a docs note stating that, and maybe a custom exception that references that docs note if you try to use the non simple view with multitenancy.
depends how large the pr to fix it is