Added #14979: add checkout to location and assets functionality to accessories #15185

arne-kroeger · 2024-07-29T09:24:22Z

Added #14979: add checkout functionality to accessories

Description

Currently it is just possible to checkout accessories to a user and not to a location or asset like it is possible for assets. This would cover many use cases where a workplace definition can be done with a location or an assignment of accessories to a room. There are several uses case descriptions in the linked issues (see below).

Added #14665
Added #12381
Added #12452
Added #11820
Added #10956
Added #5140 (parts of it)
Added #14979 (parts of it)

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Tests\Feature\Checkouts\Ui\AccessoryCheckoutTest::testAccessoryCanBeCheckedOutToLocationWithQuantity
Tests\Feature\Checkouts\Ui\AccessoryCheckoutTest::testAccessoryCanBeCheckedOutToAssetWithQuantity

Test Configuration:

PHP version: 8.3.9
MariaDB version - 10.11
Webserver version - php server
OS version - Mac OS

Checklist:

I have read the Contributing documentation available here: https://snipe-it.readme.io/docs/contributing-overview
I have formatted this PR according to the project guidelines: https://snipe-it.readme.io/docs/contributing-overview#pull-request-guidelines
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Added snipe#14979: add checkout functionality to accessoires

what-the-diff · 2024-07-29T09:25:19Z

PR Summary

Refactor of Accessory Checkouts
The main functionality this PR introduces is renaming the 'users' relationship to 'checkouts' in the accessory model. This rework improves the clarity and purpose of the relationship, making it more intuitive to understand for developers and users alike.
Enhancement in Accessory Controllers, Helper, and Transformers Files
The code which used to interact with the 'users' relationship in these files is now updated to work with the new 'checkouts' relationship. This ensures that the changes in the data model maintain their consistency throughout the application.
Introduces 'AccessoryCheckout' Model
A new file, 'AccessoryCheckout.php', has been added to the model folder. This model serves as the representation of a 'checkout' instance in our application and formalizes the relationship between a user and accessories.
Update in 'User.php' Relationship
We also updated the user model to replace the previous relationship, 'accessories_users', to 'accessories_checkout'. This aligns with the newly introduced Accessory checkout model and maintains the coherence in the data representation.
Changes in 'AccessoryPresenter.php' and 'AccessoryFactory.php'
Both of these files now refer to the updated 'checkouts' relationship and the newly named 'checkouts_count' instead of the former 'users_count'.
Database Migration Included
To make the shift from 'accessories_users' to 'accessories_checkout', a new migration file has been created. This migration will rename the existing table and add a new 'assigned_type' column.
Changes in Views and Tests
The views have been updated to reflect the new changes. Alongside the visualization changes, the test files have likewise been updated to ensure that they continue to validate the application correctly utilizing the new 'checkouts' relationship.
Improvement in Checkout Logic
The checkout process sees significant improvements. Instead of only checking out to users, it can now handle asset and location checkouts. Also introduced are the changes in validation and assertion techniques which makes the process more agile and aligns with the introduced representation changes.

snipe · 2024-07-29T09:27:20Z

We haven't built this functionality because we don't really want it. Our position has always been that you should be checking items out to users, since an asset or location cannot be held responsible for missing items.

app/Http/Controllers/Api/AccessoriesController.php

snipe · 2024-07-29T09:33:08Z

Let me spend some time looking at this and we'll consider whether we want to take this. As I mentioned, we didn't really want this functionality since it goes against the principles of what an asset system is supposed to do, but I'll consider it.

arne-kroeger · 2024-07-29T09:38:54Z

We haven't built this functionality because we don't really want it. Our position has always been that you should be checking items out to users, since an asset or location cannot be held responsible for missing items.

As there is an option to define a manager for a location I see a good way to assign a responsiblity here.

Our use case is also important to desk sharing topics where some of the accessories are part of a bookable working place for the employees. To give these items out on a daily basis is a lot of work. The idea is to assign locations via an additional workplace booking system on a time based ticket and handle the responsibility by it.

snipe · 2024-07-29T09:40:22Z

As there is an option to define a manager for a location I see a good way to assign a responsibility here.

Sure, but we don't have anything built in for them to be able to accept, etc. Right now it's just a field for reference, there are no controls there, and very few people use managers for locations anyway.

snipe · 2024-07-29T09:42:11Z

To give these items out on a daily basis is a lot of work. The idea is to assign locations via an additional workplace booking system on a time based ticket and handle the responsibility by it.

I get that, but it opens the doors to people in other situations who don't use an external "responsible for" application.

As I said, let me think about it.

snipe · 2024-07-29T09:49:33Z

database/migrations/2024_07_26_143301_add_checkout_for_all_types_to_accessories.php

+    public function down(): void
+    {
+        if (Schema::hasTable('accessories_checkout')) {
+            Schema::rename('accessories_checkout', 'accessories_users');


We probably want to drop the column created in the up migration as well, no?

Yes, I will adjust that.

uberbrady

This is some genuine good-looking code - I have a few small nits I've pointed out, but I feel like 80% of this is just handling that the table name has changed. This really does look pretty good to me. Wonderful, very well-written, tight, careful code. Thank you so much for your contribution!

uberbrady · 2024-07-29T14:09:14Z

app/Http/Controllers/Api/AccessoriesController.php

@@ -316,19 +314,19 @@ public function checkout(AccessoryCheckoutRequest $request, Accessory $accessory
     */
    public function checkin(Request $request, $accessoryUserId = null)
    {
-        if (is_null($accessory_user = DB::table('accessories_users')->find($accessoryUserId))) {
+        if (is_null($accessory_checkout = DB::table('accessories_checkout')->find($accessoryUserId))) {


Since we're already modifying this line, is there maybe a way for us to not say DB::table() here? That always feels to me like a code smell. And the line's already being altered, is there a way to do this properly polymorphic and make it a little cleaner?

app/Http/Controllers/Api/AccessoriesController.php

app/Models/Accessory.php

snipe · 2024-07-29T17:17:32Z

@arne-kroeger I'm seeing some failing tests here - do you think you can take a look at this. I'm reluctantly inclined to take this (not because your code is bad - it's great - just from a product perspective), but we do need the tests to pass.

welcome · 2024-07-29T17:50:50Z

Congrats on merging your first pull request! 🎉🎉🎉

snipe · 2024-07-29T19:19:21Z

I think you might have missed a bit in the seeder, but I think I fixed it

snipe-it/database/seeders/AccessorySeeder.php

Line 19 in 3972782

DB::table('accessories_users')->truncate();

snipe · 2024-07-29T19:35:12Z

Something still seems off here. On develop.snipeitapp.com (the live dev branch) I just checked out an accessory to a location, and now the accessories page loads with an empty table, and the location does not show any accessories assigned.

snipe · 2024-07-29T19:38:46Z

False alarm on accessories not loading (weirdly, it was a 429 error, which probably bears investigation), but the location doesn't show anything checked out to it.

arne-kroeger · 2024-07-29T20:17:28Z

False alarm on accessories not loading (weirdly, it was a 429 error, which probably bears investigation), but the location doesn't show anything checked out to it.

Thats a good point.

Currently the location would show Accessories which are placed there (in terms of the field location inside the accessory). What would be you're recommendation?

A mixed list of placed and checked out accessories?

snipe · 2024-07-30T10:39:33Z

I think you're starting to see why we didn't build this. It's always more complicated than it seems. I suppose we should handle it the way we do locations, where there are assets that belong to that location, and then there are assets that are checked out to that location. Different tabs. I don't think there's a way to make it not-confusing.

arne-kroeger · 2024-07-30T13:04:14Z

@snipe - Yeah, but I think in the end it will be a good enhancement. I will prepare an integration for it and prepare it.

snipe · 2024-08-03T11:14:45Z

This also misses the "Print all assigned" views on locations, etc. We need to pull accessories into there as well.

snipe · 2024-08-03T11:26:01Z

This may also require a rework of the locations layout altogether, as you can see adding additional tabs here causes them to wrap, and this could potentially only get much worse with other languages that use longer words.

I may have to revert this PR until it's more complete and thought through. :( Right now, letting people check things out to a location but not actually seeing them on the locations page will be a very confusing UI.

snipe · 2024-08-03T11:29:14Z

app/Http/Controllers/Users/BulkUsersController.php

@@ -219,7 +219,7 @@ public function destroy(Request $request)

        $users = User::whereIn('id', $user_raw_array)->get();
        $assets = Asset::whereIn('assigned_to', $user_raw_array)->where('assigned_type', \App\Models\User::class)->get();
-        $accessories = DB::table('accessories_users')->whereIn('assigned_to', $user_raw_array)->get();
+        $accessories = DB::table('accessories_checkout')->whereIn('assigned_to', $user_raw_array)->get();


I think this query will also not work, since it's not constraining to users. Location ID could get pulled from that list as well as User ID 4.

arne-kroeger added 2 commits July 29, 2024 10:54

added options to checkout accessoires to locations and assets

b18baf7

Added snipe#14979: add checkout functionality to accessoires

added additional tests

3a0b033

arne-kroeger requested a review from snipe as a code owner July 29, 2024 09:24

probot-autolabeler bot added api backend backups cli / artisan frontend restore tests labels Jul 29, 2024

snipe reviewed Jul 29, 2024

View reviewed changes

app/Http/Controllers/Api/AccessoriesController.php Show resolved Hide resolved

snipe reviewed Jul 29, 2024

View reviewed changes

snipe changed the title ~~Added #14979: add checkout functionality to accessories~~ Added #14979: add checkout to location and assets functionality to accessories Jul 29, 2024

uberbrady reviewed Jul 29, 2024

View reviewed changes

arne-kroeger added 2 commits July 29, 2024 19:04

Adjusted missing down on migration and code smells

e8d0147

Adjusted newly added tests to new checkout form

3c3b922

snipe merged commit 4eccb5f into snipe:develop Jul 29, 2024
8 of 9 checks passed

snipe reviewed Aug 3, 2024

View reviewed changes

This was referenced Aug 7, 2024

Partial revert for checkout accessories to non user #15241

Merged

WIP - Fixes #15185 - adds accessories to locations and asset screens #15235

Closed

snipe mentioned this pull request Dec 2, 2024

Refactor of #15235 - accessory checkout to locations and assets #15913

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added #14979: add checkout to location and assets functionality to accessories #15185

Added #14979: add checkout to location and assets functionality to accessories #15185

arne-kroeger commented Jul 29, 2024

what-the-diff bot commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

arne-kroeger commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe Jul 29, 2024

arne-kroeger Jul 29, 2024

arne-kroeger Jul 29, 2024

uberbrady left a comment

uberbrady Jul 29, 2024

arne-kroeger Jul 29, 2024

snipe commented Jul 29, 2024

welcome bot commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

arne-kroeger commented Jul 29, 2024 •

edited by snipe

Loading

snipe commented Jul 30, 2024

arne-kroeger commented Jul 30, 2024

snipe commented Aug 3, 2024

snipe commented Aug 3, 2024

snipe Aug 3, 2024

Added #14979: add checkout to location and assets functionality to accessories #15185

Added #14979: add checkout to location and assets functionality to accessories #15185

Conversation

arne-kroeger commented Jul 29, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

what-the-diff bot commented Jul 29, 2024

PR Summary

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

arne-kroeger commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe Jul 29, 2024

Choose a reason for hiding this comment

arne-kroeger Jul 29, 2024

Choose a reason for hiding this comment

arne-kroeger Jul 29, 2024

Choose a reason for hiding this comment

uberbrady left a comment

Choose a reason for hiding this comment

uberbrady Jul 29, 2024

Choose a reason for hiding this comment

arne-kroeger Jul 29, 2024

Choose a reason for hiding this comment

snipe commented Jul 29, 2024

welcome bot commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

snipe commented Jul 29, 2024

arne-kroeger commented Jul 29, 2024 • edited by snipe Loading

snipe commented Jul 30, 2024

arne-kroeger commented Jul 30, 2024

snipe commented Aug 3, 2024

snipe commented Aug 3, 2024

snipe Aug 3, 2024

Choose a reason for hiding this comment

arne-kroeger commented Jul 29, 2024 •

edited by snipe

Loading