Fixed #9422: pivot ID was being used as a user_id #9512
Conversation
|
💖 Thanks for this pull request! 💖 We use semantic commit messages to streamline the release process and easily generate changelogs between versions. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix if it doesn't have one already. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
|
@inietov can you confirm you can reproduce this original issue? I'm not sure how accessories ever worked if this assertion is correct, and we've never gotten a bug report about them checking in the wrong items. |
Just FYI, it's not that the wrong items would get checked in, but they would show up in the activity report as checked in by the wrong user. Also, it would only happen on accessory checkins happening through the API. As an example, I checked out a bunch of Magic Mouse accessories, then checked them back one at a time through the API using the command below, putting in the Pivot IDs (2, 3, 4, 5, 6, 7) from the checkout recordss in the path (7 in the example below): curl --request POST --url https://develop.snipeitapp.com/api/v1/accessories/7/checkin --header "$auth" --header "Content-Type: application/json" --header "Accepts: application/json" This is the activity report:
|
There was a problem hiding this comment.
You know what? I think your change does fix the issue, but the thing is... we already are expecting a variable called $accessoryUserId and if it isn't getting the correct value, I think is 'more correct' to fix it at the moment where the value is passed incorrectly.
|
Understood - remember this case is a little jenky to start with, because we need to know both the user ID and the checkout record that we're to be operating on. Since checkout records (ie: rows in the accessories_users table) can have "notes" attached to them, we have the situation where one user can have multiple checkouts of an accessory, and they're not all the same. As such, the /:id parameter in the URL is not a :user id: but rather an :accessory user id: per the documentation. Other commands provide a user ID in the POST body (such as the checkout operation "assign_to" parameter), but if we require that we change the API (and break backward compatibility). So one option would be to provide for an optional post body with the checkin user ID and default to the accessory_user->user_id when not provided (IMO: that seems even jenkier). Another option would be to find the user instance that owns the API key in the header. If you feel this is the better solution, are you able to provide me a place to look to figure out how to do that? That's a new part of the code for me. I'm happy to chase it down, but a little guidance would be greatly appreciated. |
|
You know what now? 😝 You were totally correct in the beginning. I wasn't understanding the issue in the beginning but today I actually read the issue #9422 where you describe it in detail, your change is absolutely correct and its a great catch. Your issue is beautifully described and the PR is clean and simple. I'm feeling inclined to approve, and I extend you an apology for not being careful yesterday reviewing your work. Only thing is that probably you need to retarget this same PR to develop. As stated in our collaboration guide: https://snipe-it.readme.io/docs/contributing-overview#pull-request-guidelines Thanks @markbrule!! |
|
Thanks @inietov ... thought I had targeted develop originally - my bad. Rebased now. |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
Description
In the API call for an Accessory checkin, the User table is queried but it was passed the accessory_user_id - which is the pivot ID of the accessory_user table, and not a user ID.
Fixes # 9422
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
On a linux shell, this can be tested by finding an accessory in the database and doing the following.
% export token="enter-an-API-token"
-- in the following curl command, put a valid accessory ID in the URL .../accessories/1/checkout
-- in the following curl command, put a valid user ID in for "1" in the assigned_to body
-- do this several times to see multiple checkout records
% curl --request POST http:https://localhost/api/v1/accessories/1/checkout --data '{"assigned_to": 1}' --header "Authorization: Bearer $token" --header "Accept: application/json" --header "Content-Type: application/json"
-- in the following curl command, use the same accessory ID in the URL as above
-- you should see the different checkout records for this accessory
-- note the different "assigned_pivot_id" values in the records
% curl --request GET http:https://localhost/api/v1/accessories/1/checkedout --header "Authorization: Bearer $token" --header "Accept: application/json" --header "Content-Type: application/json"
-- in the following curl command, put one of the assigned_pivot_id's in the checkout records above in the url for the "6" as .../accessories/6/checkin
% curl --request POST http:https://localhost/api/v1/accessories/6/checkin --header "Authorization: Bearer $token" --header "Accept: application/json" --header "Content-Type: application/json"
You should now see the records checked in the UI; re-run the checkedout curl command above to also verify they have been checked in. Check the activity report in the UI to see the records checked in. If you happen to have users in your database with User::id matching the assigned_pivot_id you should note that user in the checkin record is the correct user.
Test Configuration:
Checklist: