Skip to content

Conversation

@thapacodes4u
Copy link
Contributor

@thapacodes4u thapacodes4u commented Jun 12, 2025

EC-187

What was done in this PR?

Role create/edit:

  • We already have site_id column in roles table. So add a site field in Create & Edit user page were user can select. (Completed)

User create/edit/view:

  • Set user's global roles (when editing or creating user). (Completed)
    • Here I have added a way to identify Global or Site-specific role in the dropdown
  • Set user's roles for each site. (Completed)
    • Here I have added a new multi-select field were you can select which site access you want to give to the particular user
  • When saved, the site_has_user table should also be updated. (Completed)
  • View user's global and site roles on the view page (only for sites the admin has access to). (Completed)
    • I have added a new secction called Access Information that basically provide info about what site they have access to and what roles they have.

User control:

  • User can login only on his sites.
    • This was already applied. Don't know if you mean any specific implementation here.
  • New users must get the panel_user role, no matter where they came from (registration, seeder...)

Site management:

  • When a new site is created, all users with global roles should be added to it, so they have the role on the new site also
    • I assumed global role means those whose site_id is null. So anyone having this role should have access to any new site that gets created.

User list:

  • By default, the users list should display only users for the current site
  • Add columns for user's global and (current) site roles
  • A filter can be set to see users from all sites (that the admin has the access to)
  • A filter for global and site roles (any role/specific role)

Some Preview

image image image image image

@SlimDeluxe SlimDeluxe self-requested a review June 12, 2025 11:41
@SlimDeluxe SlimDeluxe assigned SlimDeluxe and unassigned SlimDeluxe Jun 12, 2025
Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

Looks mostly done 🙂
But I found some problems in the logic. So apart from these changes, I will add a comment in the PR for further changes.

Comment on lines 6 to 12
test('new user automatically gets panel_user role', function () {

});

test('user from seeder gets panel_user role', function () {

});
Copy link
Member

Choose a reason for hiding this comment

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

These tests are empty?

@SlimDeluxe
Copy link
Member

Additional Todos:

  1. In the Create Role form, the "Select a site" value should be changed to "Global (all sites)"
    image

  2. When viewing a global role, the site select option is empty. Should say the same as above.
    image

  3. When creating a new role with a site selected, the query fails:
    image
    Global roles can be created with no problem.

  4. Some tests fail. This is a new problem, since now, the "hasRole()" is checking for site/tenant role, but it ignores if the user has the global role.
    Eg. checking for $user->hasRole('super_admin') will return false and already some stuff is not working anymore.
    I think we should override the hasRole() method in User class so that it also checks global roles — what do you think?

I still need to check the other features, but I think there's not a lot more to do.

@SlimDeluxe
Copy link
Member

SlimDeluxe commented Jun 13, 2025

@thapacodes4u what about number 4? What do you suggest?

Edit: I see error from number 3 is still present.

@thapacodes4u
Copy link
Contributor Author

@thapacodes4u what about number 4? What do you suggest?

Edit: I see error from number 3 is still present.

I am not fully done yet still working on some of the things. By the way I am not getting that error when I create new role with some site being selected.

(Ignore the site id in the view its not fixed yet)

Screen.Recording.2025-06-13.at.6.39.00.PM.mov

@thapacodes4u
Copy link
Contributor Author

@thapacodes4u what about number 4? What do you suggest?

Edit: I see error from number 3 is still present.

As for number 4. I am working on it. I think its better to just create a new function called hasRoleWithoutScopes(). I don't know if something will break if we modify like hasRole it self which might be being used in filamentphp shield.

@SlimDeluxe
Copy link
Member

Ah ok, sorry, I thought you were done 🙂
Let me know when I should review.

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe By the way I am getting this weird issue related to spatie/health. I just added hasRoleGlobally now if we use $user->hasRoleGlobally('super_admin') we should be fine in the test it should pass as it skips the tenant scope and checks everything.

But I couldn't run composer run test due to this spatie/heath issue. Maybe its how I have setup the projects that creating the problem. I am using herd for this rather than lando or docker

@SlimDeluxe
Copy link
Member

Don't worry about the health plugin problem, just commit whatever you have and I'll look into it.

@SlimDeluxe
Copy link
Member

I see a new commit, but it doesn't affect the hasRole / hasRoleGlobally methods, as we talked yesterday. I guess you are still working on that?

@thapacodes4u
Copy link
Contributor Author

thapacodes4u commented Jun 17, 2025 via email

@thapacodes4u
Copy link
Contributor Author

thapacodes4u commented Jun 17, 2025 via email

@SlimDeluxe
Copy link
Member

Ok, no problem. We are building a long-term product and this needs to be done the best way possible :)

@SlimDeluxe
Copy link
Member

@thapacodes4u actually I found out that hasRole does not work with site roles, only global.
E.g. this test fails:

test('user is correctly given site role', function () {
    $site = Site::factory()->create();
    $siteRole = Role::create(['name' => 'site_admin', 'guard_name' => 'web', 'site_id' => $site->id]);

    $user = User::factory()->create();
    $user->assignRole($siteRole);
    $this->actingAs($user);

    Filament::setTenant($site);

    expect($user->hasRole('site_admin'))->toBeTrue();
});

But hold for now, don't fix anything, let me rethink this... I think we can make it better.

@thapacodes4u
Copy link
Contributor Author

thapacodes4u commented Jun 18, 2025

@SlimDeluxe sure let me know if you want me to further work on this. I am currently working on different PR ( #10 )

@SlimDeluxe
Copy link
Member

  1. We need to merge the "global roles" and "site roles" into one thing to make things more streamlined.
    Role locking to 1 site is not a thing we need. All roles can be used either in a global context or for any of the sites.
    The column in the table can stay, we just don't need to use it.

  2. On the user edit page, roles can be given for the global context, or for any specific site.
    When assigned globally, the user has the role (and corresponding permissions) on all sites, without having it set for a specific site.
    When assigned for a site, well, it's obvious :)
    Bottom line, it's set either globally, or for one or more specific sites.

  3. In the code, there should be an option to test while on site 1 (Filament tenant) if the user has the role on site 2, without switching tenants, e.g. hasSiteRole($site_2, 'super_admin').

  4. In the UI, there are different ways to do it, but I expect you to choose one that you think is the best, since you are much more experienced in the Filament UI.
    But it has to allow:

  • adding role globally or for one or more sites
  • removing -- || --
    Should they (global and sites) be separate inputs/sections or not, I leave up to you to suggest.
    As a reference and a reminder, this is how it was done in the old app:
    image
    image
    (this was fine in the beginning but when a project has more roles and reaches 3+ sites it gets nasty)

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe Sure I am about to be done with #10 . Maybe by tomorrow morning or evening and then I will jump into this.

This looks straight forward. I will let you know if I find any part confusing.

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe I found a core issue with the unified role approach we discussed.

When a user has the same role (e.g., "Admin") assigned both globally AND to a specific site, the UI becomes confusing and data gets corrupted:

  • User has "Admin" globally (applies everywhere)
  • User also has "Admin" for Site A specifically
  • Result: Both "Global Roles" and "Site A" sections show "Admin" as checked
  • Issue: When saving, we can't tell which "Admin" assignment the user wants to keep or remove
  • Outcome: Editing one section accidentally wipes out the other

What happens:
Edit global roles → site-specific roles get deleted
Edit site roles → global roles get deleted
Form data conflicts with each other

Suggested fix:
Use the existing site_id column to distinguish role types:

site_id = null → Global role
site_id = specific_id → Site-specific role

This way "Admin" can exist in both contexts without data conflicts, and the form sections won't overwrite each other.

Spent time trying to make the current approach work, but it requires hacky workarounds. The site_id distinction would be much more robust.

Heres a screenshot (I haven't pushed this because its not a working solution As I have mention above I tried for a while to get this working with a unified role approach it doesn't look like we can do it this way)

image

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe I found a core issue with the unified role approach we discussed.

When a user has the same role (e.g., "Admin") assigned both globally AND to a specific site, the UI becomes confusing and data gets corrupted:

  • User has "Admin" globally (applies everywhere)
  • User also has "Admin" for Site A specifically
  • Result: Both "Global Roles" and "Site A" sections show "Admin" as checked
  • Issue: When saving, we can't tell which "Admin" assignment the user wants to keep or remove
  • Outcome: Editing one section accidentally wipes out the other

What happens: Edit global roles → site-specific roles get deleted Edit site roles → global roles get deleted Form data conflicts with each other

Suggested fix: Use the existing site_id column to distinguish role types:

site_id = null → Global role site_id = specific_id → Site-specific role

This way "Admin" can exist in both contexts without data conflicts, and the form sections won't overwrite each other.

Spent time trying to make the current approach work, but it requires hacky workarounds. The site_id distinction would be much more robust.

Heres a screenshot (I haven't pushed this because its not a working solution As I have mention above I tried for a while to get this working with a unified role approach it doesn't look like we can do it this way)

image

Maybe we can discuss about this in todays meeting

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe The role and permission in user create/edit is not completed yet there is still weird bug on it. I add is_global field and using that as identifier between role assigned by global role section and site role section.

@thapacodes4u thapacodes4u marked this pull request as draft August 7, 2025 03:23
@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 81.97183% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.52%. Comparing base (bc76271) to head (4a4309a).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/Filament/Resources/UserResource.php 79.14% 49 Missing ⚠️
src/Models/Site.php 0.00% 14 Missing ⚠️
src/Models/User/Role.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main       #8      +/-   ##
============================================
+ Coverage     73.10%   73.52%   +0.41%     
- Complexity      289      334      +45     
============================================
  Files            54       56       +2     
  Lines          1848     2115     +267     
============================================
+ Hits           1351     1555     +204     
- Misses          497      560      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thapacodes4u thapacodes4u marked this pull request as ready for review August 7, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants