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

Allow CompanyableTrait trait on users #14801

Merged
merged 18 commits into from
Jun 12, 2024
Merged

Conversation

snipe
Copy link
Owner

@snipe snipe commented May 31, 2024

This should allow us to automatically scope users to company instead of having to manually do it. Previously, adding the use CompanyableTrait; statement would get caught in an infinite loop, since Auth::check() internally calls Auth::user(), which makes the scope get caught on which user you want to be looking at for scoping.

Using Auth::hasUser() just returns the boolean value, which prevents this looping problem.

To test

  • enable full multiple company support
  • create a few users in a few different companies. give them permission to see assets, users, etc, but not superadmin
  • create some assets in those same companies
  • login as each of those users, and see that the results are scoped to the company the respective users belong to

Copy link

what-the-diff bot commented May 31, 2024

PR Summary

  • Reworking of User Controller
    Removed dependencies on Company model in various functions of UsersController. It seems to streamline the code and make it less reliant on the Company model for certain operations.

  • Refactoring of Company Model
    Changed the authentication checks in Company model for better security. Now it checks if there is an authenticated user rather than just performing a generic authentication check. The code also uses the authenticated user object directly, instead of referencing the user's company id. This seems to optimize the user authentication process in the application.

  • Addition of CompanyableTrait to User Model
    Adding CompanyableTrait to the User model suggests increased modularity. This makes the User model more efficient and reduces redundancy by using traits to share common functionality across models. It ensures cleaner and more manageable code.

Signed-off-by: snipe <[email protected]>
@snipe
Copy link
Owner Author

snipe commented May 31, 2024

One thing I'm seeing here in the tests that I'm unsure about is that I'm getting:

   WARN  Tests\Feature\Users\ViewUserTest
  ✓ user without permissions cannot view user detail page
  ! user without permissions cannot view print all inventory page → Test code or tested code did not (only) close its own output buffers
  ✓ user without permissions cannot send inventory
  ✓ user without permissions cannot delete user

But I can't tell what's causing that warning.

Signed-off-by: snipe <[email protected]>
@marcusmoore
Copy link
Collaborator

@snipe I tracked it down. That Test code or tested code did not (only) close its own output buffers is caused by a missing @endpush to close the @push('js') in users/print.blade.php.

@marcusmoore
Copy link
Collaborator

(I only looked into the test issues so far. Going to give it a full review soon.)

@snipe
Copy link
Owner Author

snipe commented Jun 4, 2024

Omg, first, thank you, second fml

@snipe
Copy link
Owner Author

snipe commented Jun 5, 2024

I tracked it down. That Test code or tested code did not (only) close its own output buffers is caused by a missing @endpush to close the push('js') in users/print.blade.php.

I don't even see a @push('js') in that file?

@snipe
Copy link
Owner Author

snipe commented Jun 5, 2024

Nevermind, I'm an idiot lol

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

Since this touches the API controller can we add tests for that as well? I think it would have caught the missing ->find() I mentioned below.

It'll be good to catch up to develop so the test is in the correct directory too 😄 (#14825)

app/Http/Controllers/Api/UsersController.php Outdated Show resolved Hide resolved
tests/Feature/Users/ViewUserTest.php Outdated Show resolved Hide resolved
tests/Feature/Users/ViewUserTest.php Outdated Show resolved Hide resolved
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
Signed-off-by: snipe <[email protected]>
@snipe snipe merged commit a8b47a5 into develop Jun 12, 2024
7 of 8 checks passed
@snipe snipe deleted the fixes/companyable_trait_on_users branch June 12, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants