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.

Sign up for GitHub

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

Jump to bottom

Prevent setting assigned_to without setting assigned_type #15629

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Prevent setting assigned_to without setting assigned_type #15629

wants to merge 6 commits into from

Conversation

uberbrady
Copy link
Collaborator

@uberbrady uberbrady commented Oct 7, 2024 •
edited
Loading

This adds a simple rule to Asset to disallow setting an assigned_to without setting an assigned_type.

I added a test to make sure it works. Without the rule, the test fails. With the rule, the test passes.

(edit) I also found some incorrectly-indented lines of code that were pretty confusing to me, so I fixed that. Sorry for the noise but it was making it so that I couldn't read what was going on.

(edit) I also deleted an extra return, after the 'real' return. That's just noise.

We should probably think about propagating the rule 'upwards' into the appropriate FormRequests, but I think we automagically do that anyways? Either way, this protects the database from that happening.

Unfortunately, tests aren't passing yet, so I'm going to start this off in Draft until I can get those fixed.

@what-the-diff What The Diff
Copy link

what-the-diff bot commented Oct 7, 2024

PR Summary

  • Refinement of Assets Management:

    • Simplified the structure in which our asset management system conducts data updates, which enhances data stability.
    • Enhanced legibility in relation to how data on users, assets and their locations are documented, maintaining a smoother user interaction.
    • Reduced unnecessary information in our asset management API response, boosting efficiency.

  • Improved Rules for Assets Assignments:

    • Incorporated a condition that checks for the presence of 'assigned type' whenever an asset is assigned to a user. This change ensures the completeness of information and makes asset tracking more reliable.

  • Augmentation of Asset Testing:

    • Expanded our asset testing capabilities by adding a test that validates that an asset can be created without an assigned type if there is no assigned user. This ensures our system's flexibility to accommodate a wider range of real-life scenarios.

@uberbrady uberbrady marked this pull request as ready for review October 7, 2024 17:16
@uberbrady uberbrady requested a review from snipe as a code owner October 7, 2024 17:16
@uberbrady
Copy link
Collaborator Author

@snipe suggests that I add some API tests to make sure it fails at that layer too - and that makes sense to me. I'll push up some new tests.

@uberbrady
Copy link
Collaborator Author

I added those API tests and they fail - because of another bug that we introduced somewhere else. So I'm going to have to fix that as well and include it here.

@uberbrady
Copy link
Collaborator Author

@snipe I think this one is ready now!

snipe
snipe requested changes Oct 7, 2024
View reviewed changes
Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

One small change

snipe
snipe requested changes Oct 10, 2024
View reviewed changes
Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

Please put that last return back in when you get a moment.

@snipe
Copy link
Owner

snipe commented Oct 10, 2024 •
edited
Loading

Looks like we're getting some failing tests now, possibly related to this:

OK, but there were issues!
Tests: 771, Assertions: 2995, Skipped: 1, Incomplete: 25.

Fatal error: Uncaught Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php:1126
Stack trace:
#0 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(921): Illuminate\Container\Container->notInstantiable()
#1 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(795): Illuminate\Container\Container->build()
#2 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(961): Illuminate\Container\Container->resolve()
#3 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(731): Illuminate\Foundation\Application->resolve()
#4 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(946): Illuminate\Container\Container->make()
#5 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(288): Illuminate\Foundation\Application->make()
#6 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(207): Illuminate\Foundation\Bootstrap\HandleExceptions->getExceptionHandler()
#7 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(189): Illuminate\Foundation\Bootstrap\HandleExceptions->renderForConsole()
#8 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(255): Illuminate\Foundation\Bootstrap\HandleExceptions->handleException()
#9 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->Illuminate\Foundation\Bootstrap\{closure}()
#10 {main}
  thrown in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php on line 1126
PHP Fatal error:  Uncaught Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php:1126
Stack trace:
#0 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(921): Illuminate\Container\Container->notInstantiable()
#1 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(795): Illuminate\Container\Container->build()
#2 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(961): Illuminate\Container\Container->resolve()
#3 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(731): Illuminate\Foundation\Application->resolve()
#4 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(946): Illuminate\Container\Container->make()
#5 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(288): Illuminate\Foundation\Application->make()
#6 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(207): Illuminate\Foundation\Bootstrap\HandleExceptions->getExceptionHandler()
#7 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(189): Illuminate\Foundation\Bootstrap\HandleExceptions->renderForConsole()
#8 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(255): Illuminate\Foundation\Bootstrap\HandleExceptions->handleException()
#9 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->Illuminate\Foundation\Bootstrap\{closure}()
#10 {main}
  thrown in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php on line 1126

Fatal error: Uncaught Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php:1126
Stack trace:
#0 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(921): Illuminate\Container\Container->notInstantiable()
#1 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(795): Illuminate\Container\Container->build()
#2 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(961): Illuminate\Container\Container->resolve()
#3 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(731): Illuminate\Foundation\Application->resolve()
#4 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(946): Illuminate\Container\Container->make()
#5 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(288): Illuminate\Foundation\Application->make()
#6 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(207): Illuminate\Foundation\Bootstrap\HandleExceptions->getExceptionHandler()
#7 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(189): Illuminate\Foundation\Bootstrap\HandleExceptions->renderForConsole()
#8 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(231): Illuminate\Foundation\Bootstrap\HandleExceptions->handleException()
#9 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(255): Illuminate\Foundation\Bootstrap\HandleExceptions->handleShutdown()
#10 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->Illuminate\Foundation\Bootstrap\{closure}()
#11 {main}
  thrown in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php on line 1126
PHP Fatal error:  Uncaught Illuminate\Contracts\Container\BindingResolutionException: Target [Illuminate\Contracts\Debug\ExceptionHandler] is not instantiable. in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php:1126
Stack trace:
#0 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(921): Illuminate\Container\Container->notInstantiable()
#1 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(795): Illuminate\Container\Container->build()
#2 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(961): Illuminate\Container\Container->resolve()
#3 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php(731): Illuminate\Foundation\Application->resolve()
#4 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Application.php(946): Illuminate\Container\Container->make()
#5 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(288): Illuminate\Foundation\Application->make()
#6 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(207): Illuminate\Foundation\Bootstrap\HandleExceptions->getExceptionHandler()
#7 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(189): Illuminate\Foundation\Bootstrap\HandleExceptions->renderForConsole()
#8 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(231): Illuminate\Foundation\Bootstrap\HandleExceptions->handleException()
#9 /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php(255): Illuminate\Foundation\Bootstrap\HandleExceptions->handleShutdown()
#10 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->Illuminate\Foundation\Bootstrap\{closure}()
#11 {main}
  thrown in /home/runner/work/snipe-it/snipe-it/vendor/laravel/framework/src/Illuminate/Container/Container.php on line 1126
Error: Process completed with exit code 255.

@snipe snipe mentioned this pull request Oct 11, 2024
Merged
@uberbrady
Copy link
Collaborator Author

Make sure to check importer - assigned_to without assigned_type is allowed there? Does this break that?

@uberbrady
Copy link
Collaborator Author

(defaults to Users)

@snipe
Copy link
Owner

snipe commented Nov 13, 2024

Tests are still failing here :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snipe snipe snipe requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@uberbrady @snipe