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

Added #10454: Quick Scan Checkin #10455

Merged
merged 16 commits into from
Feb 2, 2022

Conversation

adagioajanes
Copy link
Contributor

@adagioajanes adagioajanes commented Dec 19, 2021

Description

This will add an additional menu option and page for 'Quick Scan Checkin'. This page will allow quick scanning (much like audit) with the functionality of checking in each asset as it is scanned.

A menu option was added for Quick Scan Checkin.
image

Here is the page in action...
image

((Some comments may refer to this as 'Bulk Checkin'. This PR and function was renamed to 'Quick Scan Checkin' during its development.))

Adds #10454

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

  • Setup default install to make sure no core functionality is broken
  • Load with sample data
  • Attempt to checkin assets with the new check in screen
  • Successfully checkin an asset that was deployed.
  • Fail to checkin an asset that is already checked in.
  • Fail to checkin an asset that doesn't exist.
  • Fail to checkin an asset I don't have permission to.

Test Configuration:

  • PHP version: 7.4.26
  • MySQL version: 5.7.36
  • Webserver version: PHPStorm Built in Web Server
  • OS version: Windows 10 - WSL

Checklist:

* @since [v6.0]
* @return JsonResponse
*/
public function checkinByTag(Request $request)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than using javascript to get the asset id by asset tag, this was the cleanest way of doing it in my mind. I'd rather the page not rely on a javascript call to get the asset id before pressing enter.

Thoughts?

@adagioajanes adagioajanes marked this pull request as ready for review December 20, 2021 00:25
@adagioajanes adagioajanes requested a review from snipe as a code owner December 20, 2021 00:25
@adagioajanes adagioajanes mentioned this pull request Dec 21, 2021
13 tasks
@adagioajanes
Copy link
Contributor Author

Added screenshots

@snipe
Copy link
Owner

snipe commented Dec 21, 2021

I feel like we should probably be using the select2 typeahead stuff like we do for bulk checkout. That lets people enter a comma-separated list (which space-completes), or use a barcode scanner to grab them all at once in that asset tags field. Thoughts?

@adagioajanes
Copy link
Contributor Author

adagioajanes commented Dec 21, 2021

I feel like we should probably be using the select2 typeahead stuff like we do for bulk checkout. That lets people enter a comma-separated list (which space-completes), or use a barcode scanner to grab them all at once in that asset tags field. Thoughts?

I think both approaches make sense. If we look at Bulk Checkout, the idea there is that you are trying to apply several properties to MULTIPLE assets in one transaction.

If we were to make a one-to-one replica of that feature, then I agree, the select2 typeahead approach makes more sense for a 'Bulk Checkin'. So, that will be the next PR I do. ;) (But, please read on...)

All of that being said, I want to be able to fly through checkins as fast as possible (and yes, set a location). So, perhaps, it is better to rename this PR from 'Bulk Checkins' to 'Quick Scan Checkins'.

Here's the 'story'/use case that drove the development approach for this PR...
In the before times (pre-pandemic), I would be in the office M-F. Assets that were returned were processed at that moment, forms filled out and approved, and the assets made it to their correct home/shelf/new user in the same day (usually same hour).
In the now now times (post March 2020), I am going into the office once per week. Asset returns has devolved into, "Leave it on my chair or desk. I will get to it when I come in." As much as I would like to go through each asset and properly update every element in the database, I just need it off my desk, and I need to not really worry if what I scanned actually exists or not. So I need to check it in ASAP, get the asset responsibility (so to speak) off of the user, and just toss it in a literal "In" box to be properly checked for damage/maintenance later.

With that use case in mind, I would prefer the usability advantages of just, scan it, its checked in. No thinking about it. The user gets an email saying its not theirs anymore right on the spot. I get to look at it and worry about more details (like whether its still usable) later.

There's one main reason the select2 typeahead approach wouldn't work for this 'Quick Scan Checkin' feature. When a barcode/rfid scanner scans an asset tag on the 'Bulk Checkout' page, and it either doesn't exist, or isn't available to checkout, you now have to delete the input that the scanner put into the text box. This is inconvenient for someone who is just trying to get through a stack of 25 laptops. Additionally, in that 25 laptop example, I prefer the list of successes and failures, with the count at the top. It allows me to quickly identify the problems after I have scanned them all and move on, rather than having to stop every time there is a problem.

@adagioajanes adagioajanes changed the title Added #10454: Bulk checkin for assets Added #10454: Quickscan checkin for assets Dec 21, 2021
@adagioajanes adagioajanes changed the title Added #10454: Quickscan checkin for assets Added #10454: Quick Scan checkin for assets Dec 21, 2021
@adagioajanes adagioajanes changed the title Added #10454: Quick Scan checkin for assets Added #10454: Quick Scan Checkin Dec 21, 2021
@adagioajanes
Copy link
Contributor Author

adagioajanes commented Dec 23, 2021

So, I have been continuing to work on an alternative select2 approach setup, and had to stop myself to check some things.

The current bulk checkout is accessing and modifying the models directly. In an effort to better adopt the API, it would be better for me to rewrite the foreach loop on checkins to call the API, and record each returned result to monitor for errors. As I was trying to refactor bulk checkout to do that, I realized that what I have written here accomplishes that already, without the additional resource overhead that select2 presents.

I also think this is genuinely a better approach for a checkin workflow. When I am checking in, I want to know if I am throwing it into the 'Asset doesn't exist' box (to be corrected in our system), or the 'To audit' box (because it wasn't checked out in the first place), or just the 'Ready' box. The select2 approach doesn't allow me to quickly identify those issues. Is asset 123 not appearing because it isn't eligible to check in (in which case it needs to be audited)? Or is it not appearing because it doesn't exist (probably because we haven't got it into the system yet)? I'm checking in assets, just tell me so I can fix it later. ;)

I think this PR is the correct approach for bulk checkin.

@adagioajanes
Copy link
Contributor Author

adagioajanes commented Dec 24, 2021

I have added PR #10462 to utilize the select2 method of selecting assets. However, if you believe the select2 method is the one you would ultimately choose, then I propose BOTH PRs should be used. I seriously prefer this Quick Scan Checkin functionality over the other method. It is just so much faster and would make the workflow much easier for my team.

@adagioajanes
Copy link
Contributor Author

adagioajanes commented Dec 28, 2021

This PR is almost complete. But I am having alot of trouble setting up unit testing. Perhaps someone could take a look with me and just see if we can't figure out why none are running correctly?

EDIT: Figured it out. It was a configuration issue with my testing setup. Unit tests are mostly succeeding. But there are some (11) still failing, and now I am really unsure why. They are all failing for the same reason. An example is the AssetTest. The error I am getting is...

Illuminate\Database\QueryException : SQLSTATE[HY000] [1045] Access denied for user ''@'localhost' (using password: NO) (SQL: update settings set next_auto_tag_base = next_auto_tag_base + 1, settings.updated_at = 2021-12-28 14:28:26 where id = 1)

@adagioajanes
Copy link
Contributor Author

adagioajanes commented Dec 28, 2021

Alright, fixed my testing environment. All unit tests (except for some translation tests unrelated to this PR) are passing. There doesn't appear to be existing unit tests for this functionality in similar features, so I did not create any for this PR. Do you need some new unit tests in order to consider this for merging?

Otherwise, my unit tests are working.

@adagioajanes adagioajanes mentioned this pull request Jan 7, 2022
14 tasks
@adagioajanes
Copy link
Contributor Author

By the way, I am holding off on submitting any documentation changes for this and #10462. Just until we can get settled on the direction this is going to be implemented.

@betomail
Copy link

This is so awesome. I've been looking for this since we started using Snipe IT, one year ago. Good job man!! Do you know when it will be released? Is there a way to use it in our production already?
We had the same issue checking in our assets last year and we start getting the same issue this year because we see a lot of cases in this pandemic situation so I'm starting seeing the same issue now because we going to work remotely. it will be great if we could have this implemented this asap.

@adagioajanes
Copy link
Contributor Author

adagioajanes commented Jan 14, 2022

This is so awesome. I've been looking for this since we started using Snipe IT, one year ago. Good job man!! Do you know when it will be released? Is there a way to use it in our production already? We had the same issue checking in our assets last year and we start getting the same issue this year because we see a lot of cases in this pandemic situation so I'm starting seeing the same issue now because we going to work remotely. it will be great if we could have this implemented this asap.

This isn't finished yet. This is just a pull request.

Currently, I am trying to gather more feedback as to the approach that should be taken. Should it be quick scan like this PR? Or, should it be similar/identical to the current Bulk Checkout function like in PR #10462?

EDIT: You are free to take these changes and apply them to your own Snipe IT instance. But, that is NOT recommended as it can affect your ability to update in the future.

@betomail
Copy link

betomail commented Jan 15, 2022 via email

@TenOfTens
Copy link
Contributor

This feature is amazing! I love the simple ease of use it has. I just alter the 2 fields and when I scan it assigns the location and notes that are currently existing in the fields with the checkin event. Maybe set this up both ways? Quick scan check in and out? Use this to just get devices assigned and have the other method be the one that works great with the alert system? I don't know. My programming knowledge is zero, but this is super easy to use and I think a lot of people would love this. If only there was more attention to this. Awesome work @adagioajanes!

@snipe
Copy link
Owner

snipe commented Feb 2, 2022

I don’t know if it’s possible but when you scan an asset which is already checked in it should show up in another color, like blue.

We can't ever (and don't) rely on colors to indicate a status, since that puts color blind people in a bad spot. (We always use an icon/text and color). What's the use case for needing to know if something was already checked in? I'd think that would just be a failure indication, since the normal GUI behavior would be to throw an error saying the asset is already checked in.

@betomail
Copy link

betomail commented Feb 2, 2022 via email

@snipe snipe merged commit ce154a2 into snipe:develop Feb 2, 2022
@snipe
Copy link
Owner

snipe commented Feb 2, 2022

We can always tighten up that UX a little as we move forward

@adagioajanes adagioajanes deleted the features/quickscan_checkin branch February 2, 2022 21:04
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.

4 participants