Feature #9514, Feature #9378 Unaccepted Assets Report Actions, Fixed [ch16410]

[dampfklon]

Description

Added unaccepted Assets Report Actions (send reminder, delete) added, this allows to resend the Asset acceptence notification to the user and to delete a unaccepted entry.
Fixed unaccepted Assets Export, report is currently broken, this error also occurs on the demo instance https://demo.snipeitapp.com/reports/export/unaccepted_assets

Adds #9378
Adds #9514

EDIT: This should also Fixes #2294 as this is the last missing feature in this request.

This is my first bigger PR, I am not sure the functions sentAssetAcceptanceReminder and deleteAssetAcceptance are at the right place. There are also no added tests for the functions as I am not sure how to implement them.

Feedback would be appreciated.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

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

• Test Unaccepted Assets Report Action
• Open Unaccepted Assets Report
• Actions send reminder, delete are displayed
• click send reminder
• "Asset delivery confirmation" mail is sent to user, same mail as on checkout
• Unaccepted Assets Report is displayed with the success banner
• click delete
• confirmation modal is displayed
• confirm
• Unaccepted Assets Report is displayed with the success banner and deleted entry not longer in list
• Test Fix Unaccepted Assets Export
• Open Unaccepted Assets Report
• click the "Download All" Button in the top right
• download CSV file
• contents "match" Unaccepted Assets Report

Test Configuration:

• Alpine Dockerimage

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

[dampfklon]

different approach to #9508

[inietov]

It works as described, only have a couple observations. Which is why I would like @uberbrady and @snipe input.

• I'm not sure we need to delete the Acceptance Requests?. I can think of a couple scenarios where deleting them could cause some kind of dispute in the organization using the feature.
• When a user with pending acceptance is deleted, it appears in the report as 'deleted user'. I think we need to define what we want to happen in that case, the acceptance request doesn't need to appear at all? or putting this label is the correct and expected behaviour?

Otherwise I think it's a solid PR and I'm willing to accept it once these subjects are clarified. Thanks for your work on this @dampfklon!!

[dampfklon]

I'm not sure we need to delete the Acceptance Requests?. I can think of a couple scenarios where deleting them could cause some kind of dispute in the organization using the feature.

Good point, i think it would be at least necessary to have a dedicated permission to this function.

When a user with pending acceptance is deleted, it appears in the report as 'deleted user'. I think we need to define what we want to happen in that case, the acceptance request doesn't need to appear at all? or putting this label is the correct and expected behavior?

I think this should actually never happen. A user can't be deleted as long as assets are still checked out to him and I would think the pending acceptance should be removed when a asset get checked in.
Then there wouldn't be any pending acceptance for deleted users.

[snipe]

Good point, i think it would be at least necessary to have a dedicated permission to this function.

I'm kinda loathe to add more permissions, tbh. Perhaps something more like we don't show deleted ones by default, but there's an option to show deleted, as we do for Users?

When a user with pending acceptance is deleted, it appears in the report as 'deleted user'. I think we need to define what we want to happen in that case, the acceptance request doesn't need to appear at all? or putting this label is the correct and expected behavior?

I think in the activity report, we should show the user's name with a strikethrough, as we do elsewhere in the system. If the user has been deleted and purged, we show "deleted user", again, as we do elsewhere in the system.

Agreed that if the item has been checked out to a user, they never accepted it, and they were subsequently checkin-and-deleted (so, a user got something checked out, never accepted it, and was fired), we probably want to keep track of that information, I'm just not exactly sure where that makes the most sense. If legal action needed to be taken against the employee, for example, we wouldn't want to lose the fact that they never accepted that item, even though it was checked out to them.

[snipe]

Hey there - just following up on this - any additional thoughts on the last part of our convo?

[dampfklon]

I'm kinda loathe to add more permissions, tbh. Perhaps something more like we don't show deleted ones by default, but there's an option to show deleted, as we do for Users?

So, no new permissions 😄 I think it should be a valid solution to have the option to show deleted requests.

I think in the activity report, we should show the user's name with a strikethrough, as we do elsewhere in the system. If the user has been deleted and purged, we show "deleted user", again, as we do elsewhere in the system.

I will change the display as suggested. Do you have a place where I could look for this solution?

Agreed that if the item has been checked out to a user, they never accepted it, and they were subsequently checkin-and-deleted (so, a user got something checked out, never accepted it, and was fired), we probably want to keep track of that information, I'm just not exactly sure where that makes the most sense. If legal action needed to be taken against the employee, for example, we wouldn't want to lose the fact that they never accepted that item, even though it was checked out to them.

Currently it’s not possible to delete a user who as checked out assets. In the case the user who did not accept the asset and also did not return the asset could not be deleted. This results in no unaccepted requests with deleted users except for deleted requests where the rendering as described above is in place.

[dampfklon]

Suggested Changes are done.

[inietov]

I think it's ok to merge this, the new changes made sense, only observation:

- [x]  Test Fix Unaccepted Assets Export

Call http://example.com/reports/export/unaccepted_assets
download CSV file
contents "match" Unaccepted Assets Report

The route described in that use case doesn't exist anymore, which I don't find problematic, since the report is downloadable on other route without the Asset::unaccepted() method. Just wanted to mention as the test case still appears in your description, great job!!

[dampfklon]

Thanks for the heads-up, you are right in the refactoring i changed the route to match the activity report.
Reviewing the changes i notices i accedentialy commited chnages to the docker.env and migrations which should be in the PR.

I will push a update wihtout those changes.

[dampfklon]

Updated the PR to fix the merge conflicts from the v6 merge to develop.
Would be nice if this could be merged.

[snipe]

I'll approve this PR, but man is this going to be ugly when we merge to master. :( We've done a lot of hot fixes on master for asset acceptance and stuff.

[dampfklon]

That sucks :( Thanks for merging nevertheless, I will happily test the changes on RC when it drops.

[AlexanderWPapyrus]

Can't wait for a resend and delete unaccapted asset funcionality 😍