Added #10454: Bulk Checkin #10462

adagioajanes · 2021-12-24T00:40:11Z

Description

This will add an additional menu option and page for 'Bulk Checkin'. This page will allow you to bulk checkin assets in a similar way to bulk checkout.

Adds #10454 #9551

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
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

Check in multiple assets utilizing bulk checkin
Attempt and fail to check in an asset that is not currently checked out (Test passed because I can't select a checked in asset. I did not test just forcing the post data to have an asset that is not checked out.)
Ensure all (within reason) existing functionality is not affected

Test Configuration:

PHP version: 8.0.14
MySQL version: 5.7.36
Webserver version: PHPStorm Built in Web Server
OS version: Windows 10 WSL

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

adagioajanes · 2021-12-28T20:44:38Z

app/Models/Asset.php

@@ -280,6 +281,7 @@ public function availableForCheckout()
     * @param Carbon $expected_checkin
     * @param string $note
     * @param null $name
+     * @param null $location


I noticed that this was not in the notes at this line. So I added it.

…elected

adagioajanes · 2021-12-28T21:02:41Z

resources/lang/en/admin/hardware/message.php

@@ -7,6 +7,7 @@
    'does_not_exist' 	=> 'Asset does not exist.',
    'does_not_exist_or_not_requestable' => 'Nice try. That asset does not exist or is not requestable.',
    'assoc_users'	 	=> 'This asset is currently checked out to a user and cannot be deleted. Please check the asset in first, and then try deleting again. ',
+    'no_assets_selected' => 'You must select at least one asset from the list',


Refactoring this made the most sense to me. But let me know if there is a different way.

adagioajanes · 2021-12-28T21:05:41Z

app/Models/Asset.php

+     * @author [A. Janes] [<[email protected]>]
+     * @param Carbon $checkin_at
+     * @param string $note
+     * @param null $name


I included the $name variable to keep it consistent with checkOut(). It isn't going to be used by this PR. But, it seemed like it made sense to keep if this method is reused.

adagioajanes · 2021-12-28T21:08:41Z

resources/views/layouts/default.blade.php

+                            </a>
+                        </li>
+                    @endcan
+


Putting the menu item above bulk checkout made the most sense to me. But again, feel free to offer a different suggestion.

Develop

adagioajanes · 2021-12-28T23:07:59Z

Unit testing successful. Ready for review.

resources/views/hardware/bulk-checkin.blade.php

Sxderp · 2022-01-07T17:31:23Z

Your checkin function on the asset doesn't do all the same checks and debugging as the asset checkin controller.

--

As an aside, I'd request a followup PR that updates the asset checkin controller to use the new function on the model.

…n_id.

adagioajanes · 2022-01-07T18:13:44Z

Your checkin function on the asset doesn't do all the same checks and debugging as the asset checkin controller.

I am trying to find those checks you are referring to. Perhaps you could give me an example or two?
EDIT: @Sxderp AH NEVERMIND! I found it. I was looking at the wrong controller...

As an aside, I'd request a followup PR that updates the asset checkin controller to use the new function on the model.

That would be nice. This PR isn't going to do that since it serves a different purpose. But I like the idea. :)

adagioajanes · 2022-01-11T20:01:58Z

Your checkin function on the asset doesn't do all the same checks and debugging as the asset checkin controller.

@Sxderp Alright, I've been doing some thinking and reviewing on this. Take all of this with a grain of salt, as I could be wrong about design standards...

checkIn() on the model shouldn't be responsible for these checks, right? When designing an eloquent model, the functions on it should rely on the controller to perform the checks for authorization and existence, not the model itself. It is assumed that once you can instantiate a model, it exists. If we couldn't instantiate it, then we wouldn't be able to get as far as checking it in.

That being said, the checks you are referring to are performed at different parts of the application, outside of the model. First, the view itself won't let you select an asset that doesn't exist, or isn't eligible for check in. Next, the BulkAssetsController storeCheckin() function is performing a check to ensure we are even calling assets, and whether we are authorized to work with them. I will put code reviews for you on those sections.

Thoughts?

EDIT: Oh, also, this was designed to work very nearly exactly how the bulk checkout function currently operates. I assume if bulk checkout is written this way, then this is how @snipe wanted bulk checkin written? (Please don't let me put words in your mouth @snipe. I am referring to #10455 (comment))

adagioajanes

@Sxderp Added comments to the lines of code that perform these checks. Please let me know your thoughts at your convenience. :)

app/Http/Controllers/Assets/BulkAssetsController.php

resources/views/hardware/bulk-checkin.blade.php

app/Models/Asset.php

Sxderp · 2022-01-12T02:13:09Z

I was mostly talking about this bit:

snipe-it/app/Http/Controllers/Assets/AssetCheckinController.php

Lines 86 to 107 in 2d304ec

    
           if ($asset->rtd_location_id == '0') { 
        
               \Log::debug('Manually override the RTD location IDs'); 
        
               \Log::debug('Original RTD Location ID: '.$asset->rtd_location_id); 
        
               $asset->rtd_location_id = ''; 
        
               \Log::debug('New RTD Location ID: '.$asset->rtd_location_id); 
        
           } 
        
           if ($asset->location_id == '0') { 
        
               \Log::debug('Manually override the location IDs'); 
        
               \Log::debug('Original Location ID: '.$asset->location_id); 
        
               $asset->location_id = ''; 
        
               \Log::debug('New RTD Location ID: '.$asset->location_id); 
        
           } 
        
           $asset->location_id = $asset->rtd_location_id; 
        
           \Log::debug('After Location ID: '.$asset->location_id); 
        
           \Log::debug('After RTD Location ID: '.$asset->rtd_location_id); 
        
           if ($request->filled('location_id')) { 
        
               \Log::debug('NEW Location ID: '.$request->get('location_id')); 
        
               $asset->location_id = e($request->get('location_id')); 
        
           }

It's backwards compatibility code that likely needs to remain even for bulk checkin.

adagioajanes · 2022-01-12T17:23:45Z

It's backwards compatibility code that likely needs to remain even for bulk checkin.

@Sxderp Got ya. I will work on implementing that. Thank you so much for clarifying. :)

You are correct. Implementing it into the model itself would be ideal, especially because it's to correct a backwards compatibility issue that affects the model itself.

adagioajanes · 2022-01-13T14:09:23Z

Alright, I pushed that update, but I haven't had the chance to test it. I will try to get testing in today.

…nnot_be_null Fixes: Column activated cannot be null [sc-18528]

adagioajanes · 2022-02-02T20:42:47Z

Because #10455 was merged, there is the potential that this PR may be un-needed entirely now.

However, continuing to operate under the assumption that this COULD be potentially merged in the future, what would make the most sense in terms of menu arrangement for this? Would it make sense to rename "Quick Scan Checkin" to "Bulk Checkin" and offer the option of which experience the user wants?

Just spit balling UI/UX ideas here.

adagioajanes · 2022-03-21T18:27:24Z

Because #10455 was merged, this PR may be unnecessary now. Unless there is some additional demand for this in the near future, or a desire from @snipe to implement this, I won't be putting any additional work into it.

jose-duarte · 2024-11-03T11:05:53Z

Apologies for reopening this but in my opinion there's still space for bulk checkin since it still differs from the "Quick Scan Checkin" menu.

The biggest point is for people not using a barcode scanner, when you do a bulk checkout, you can easily use your numpad to write down the X digits of your asset tag and press enter (it will search on the background for the best match).
For the "Quick Scan Checkin" you don't have that option which pushes you to write down the whole asset tag, including prefix. This, as you can imagine, consumes a lot of time.

Hopefully you consider moving forward with this feature which could still co-live with the quick scan.

adagioajanes added 2 commits December 23, 2021 15:25

not done

63aeb68

Now it's checking things in

ae8e9a4

adagioajanes requested a review from snipe as a code owner December 24, 2021 00:40

probot-autolabeler bot added backend frontend labels Dec 24, 2021

adagioajanes mentioned this pull request Dec 24, 2021

Added #10454: Quick Scan Checkin #10455

Merged

18 tasks

adagioajanes marked this pull request as draft December 24, 2021 00:42

adagioajanes added 3 commits December 23, 2021 19:51

added menu option

c787a24

Corrected an issue with not passing checkin dates

671921e

unnecessary 'use' statements removed

49ebeb2

adagioajanes commented Dec 28, 2021

View reviewed changes

refactored message.checkout.no_assets_selected to message.no_assets_s…

6dcefe3

…elected

adagioajanes commented Dec 28, 2021

View reviewed changes

Merge pull request #9 from snipe/develop

ae8c611

Develop

adagioajanes marked this pull request as ready for review December 28, 2021 23:08

Sxderp reviewed Jan 7, 2022

View reviewed changes

resources/views/hardware/bulk-checkin.blade.php Outdated Show resolved Hide resolved

Removed requirement to assign a location. Changed variable to locatio…

e2e12b8

…n_id.

adagioajanes commented Jan 11, 2022

View reviewed changes

app/Http/Controllers/Assets/BulkAssetsController.php Show resolved Hide resolved

app/Http/Controllers/Assets/BulkAssetsController.php Show resolved Hide resolved

resources/views/hardware/bulk-checkin.blade.php Show resolved Hide resolved

app/Models/Asset.php Show resolved Hide resolved

Merge branch 'develop' into features/bulk_checkin

2d304ec

Added checks for legacy location issues on the checkin method

c00b973

Add a casting to a truthy/falsy that needs to be integer

eac8e0b

Merge pull request snipe#10604 from inietov/fixes/column_activated_ca…

8add477

…nnot_be_null Fixes: Column activated cannot be null [sc-18528]

adagioajanes added 2 commits February 2, 2022 16:03

Merge branch 'develop' into features/bulk_checkin

29ee559

Merge branch 'snipe:master' into features/bulk_checkin

5292249

snipe closed this May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added #10454: Bulk Checkin #10462

Added #10454: Bulk Checkin #10462

adagioajanes commented Dec 24, 2021 •

edited

Loading

adagioajanes Dec 28, 2021 •

edited

Loading

adagioajanes Dec 28, 2021

adagioajanes Dec 28, 2021

adagioajanes Dec 28, 2021

adagioajanes commented Dec 28, 2021

Sxderp commented Jan 7, 2022

adagioajanes commented Jan 7, 2022 •

edited

Loading

adagioajanes commented Jan 11, 2022 •

edited

Loading

adagioajanes left a comment

Sxderp commented Jan 12, 2022

adagioajanes commented Jan 12, 2022 •

edited

Loading

adagioajanes commented Jan 13, 2022

adagioajanes commented Feb 2, 2022 •

edited

Loading

adagioajanes commented Mar 21, 2022

jose-duarte commented Nov 3, 2024

Added #10454: Bulk Checkin #10462

Added #10454: Bulk Checkin #10462

Conversation

adagioajanes commented Dec 24, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

adagioajanes Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

adagioajanes Dec 28, 2021

Choose a reason for hiding this comment

adagioajanes Dec 28, 2021

Choose a reason for hiding this comment

adagioajanes Dec 28, 2021

Choose a reason for hiding this comment

adagioajanes commented Dec 28, 2021

Sxderp commented Jan 7, 2022

adagioajanes commented Jan 7, 2022 • edited Loading

adagioajanes commented Jan 11, 2022 • edited Loading

adagioajanes left a comment

Choose a reason for hiding this comment

Sxderp commented Jan 12, 2022

adagioajanes commented Jan 12, 2022 • edited Loading

adagioajanes commented Jan 13, 2022

adagioajanes commented Feb 2, 2022 • edited Loading

adagioajanes commented Mar 21, 2022

jose-duarte commented Nov 3, 2024

adagioajanes commented Dec 24, 2021 •

edited

Loading

adagioajanes Dec 28, 2021 •

edited

Loading

adagioajanes commented Jan 7, 2022 •

edited

Loading

adagioajanes commented Jan 11, 2022 •

edited

Loading

adagioajanes commented Jan 12, 2022 •

edited

Loading

adagioajanes commented Feb 2, 2022 •

edited

Loading