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

General cleanup - Docblock fixes, adding type hinting, added tests, added asset model form validator #15029

Merged
merged 42 commits into from
Jul 8, 2024

Conversation

snipe
Copy link
Owner

@snipe snipe commented Jul 4, 2024

This touches a buttload of files, but as you can see, the vast majority are just small changes like changing Auth::user()->id to auth()->id(), etc to modernize the codebase a little. I also pulled out references to use statements that the code wasn't actually using, and added type-hinting to a lot of the controller methods (that last bit is the majority of the files changed).

I also added a bunch of tests. We do seem to have a few different naming conventions going on in there and I wasn't sure which one we wanted to settle on.

Not sure if we want to go with:

  • IndexObjectTest.php
  • CreateObjectTest.php

Or

  • ObjectIndexTest.php
  • ObjectCreateTest.php

I don't have strong feelings, but they should be consistent.

Some of these tests only cover the basics - can the index be loaded, etc - but they're at least something more than what we had.

In writing those tests, I did find a few bugs which I also fixed. Those will be the only changes that stand out here, as everything else is pretty straightforward.

For some reason, these changes have made our codacy code quality plummet, even though most of these things are just tests and type-hinting. Seems the type-hinting is somehow increasing our cyclomatic and Npath complexity somehow?

Copy link

what-the-diff bot commented Jul 4, 2024

PR Summary

  • Added return type to several controller methods
    Some of the main methods in the AccessoriesController, AssetMaintenancesController, AssetModelsController, and Assets/AssetCheckinController, and others, have been updated to return \Illuminate\Contracts\View\View instead of plain string. It provides better consistency in our code and specifies what to expect from these methods.

  • Replaced 'Auth::user()->id' with 'auth()->id()'
    The pull request refactored the authentication method used across several files, replacing Auth::user()->id to auth()->id(), to improve readability and simplicity.

  • Added new methods to various Controllers
    Several new functionalities have been added to the system such as Two-Factor Authentication, Google Login Settings, Asset Request, and Depreciation Report among others. These were implemented through the addition of new methods in the respective controllers.

  • Updated SettingsController class.
    The class was updated with more clearly defined methods for get and post requests, streamlining the process of updating settings within the application, and increasing its overall functionality.

  • Added and Updated Test Cases
    Several test cases like AssetModelStoreTest, AssetModelIndexTest, and others have been added or updated to test the new functionalities, ensuring system robustness.

  • Standardized method of getting current user
    Frequently used Auth::user() now replaced with auth()->user() throughout the codebase. This helps in increasing code readability and maintainability.

  • Miscellaneous Changes
    Lastly, this PR contains many other minor modifications, additions, and deletions aiming for better code structure and increased performance. This includes changing logging levels, importing classes using relative namespaces, removing unused import statements, renaming functions among others.

@snipe snipe marked this pull request as ready for review July 5, 2024 11:06
@snipe snipe changed the title WIP Docblock fixes, adding type hinting, added tests, added asset model form validator General cleanup - Docblock fixes, adding type hinting, added tests, added asset model form validator Jul 5, 2024
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This feels like we're moving into really modern code, and I'm hoping it helps us catch things at interpret-time or develop-time way, way before we make it into production. You sneaked a few changes I wouldn't have minded seeing separately into this one, but they're all good fixes so I'm not going to fight too hard. This looks like it was a tremendous amount of work. Thank you for dedicating the time to do it - I'm really hoping it pays off in the future - as we stave off bugs we didn't need to write, or catch them way, way before they even make it into CI/CD testing.

app/Http/Controllers/Api/AssetsController.php Show resolved Hide resolved
app/Http/Controllers/CategoriesController.php Show resolved Hide resolved
app/Http/Controllers/LabelsController.php Outdated Show resolved Hide resolved
app/Http/Controllers/SettingsController.php Show resolved Hide resolved
app/Http/Middleware/CheckPermissions.php Show resolved Hide resolved
app/Models/Category.php Show resolved Hide resolved
@snipe snipe merged commit 9c304f2 into develop Jul 8, 2024
8 of 9 checks passed
@snipe snipe deleted the fixes/small_docblock_fixes branch July 8, 2024 16:43
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.

2 participants