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

Fixed Asset acceptance error when user company and asset company don't match #13400

Merged
merged 5 commits into from
Aug 22, 2023

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented Aug 3, 2023

Description

With full company support activated if an asset is checked out from company A to an user from company B, then when the user from B tries to accept the asset, it appears the 'Error: insufficient permission' sign.

In this PR I tried to mitigate the issue by not showing any asset to accept/deny if this case already happened, so the users doesn't have to deal with that. And in the asset checkout method I check if full company support is enabled, and if it is now it doesn't allow the checkout to happen. Also it shows a more explicit error message "User and asset companies missmatch".

Fixes internal freshdesk 37131

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Test Configuration:

  • PHP version: 8.1
  • MySQL version: 8.0.31
  • Webserver version: PHP dev server
  • OS version: Debian 12

@what-the-diff
Copy link

what-the-diff bot commented Aug 3, 2023

PR Summary

  • Enhancement to User Permissions Handling
    The application now includes an additional validation in the account acceptance process. If a user doesn't have the needed permissions, the system will redirect them to the account acceptance page and display an appropriate error message.

  • Multi-Company Support and Company Matching Validation
    Adjustments were made in asset checkout procedure. When full multiple companies support is enabled, there is an added validation. It ensures that the target company ID and the asset company ID match. If they do not, the user is redirected with an error message.

  • New Translation Added
    A new error message translation has been introduced - "User and Asset companies mismatch", improving the localization of the application.

  • UI Improvements in Account Acceptance
    To increase resilience, changes were introduced to the account acceptance page. Precautions are now in place to check if acceptance 'checkoutable' exists before rendering specific table data, eliminating potential render errors.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

I like this idea 👍🏾

Two things that stand out regarding "I tried to mitigate the issue by not showing any asset to accept/deny if this case already happened":

The landing page still shows they have an item to accept or decline:
CleanShot 2023-08-03 at 12 49 54@2x

And, a smaller issue, is although the item isn't displayed on the accept assets screen, the number still shows:
CleanShot 2023-08-03 at 12 56 22@2x

I'm not sure those are easy fixes but it would be nice to not show it if they cannot accept the item. Keeping in mind that this is only for the users that have had assets checked out to them before this PR goes in, it might be ok to skip the fixes if they are time consuming?

I'm gonna approve this code-wise but @snipe should decide on the UX.

@inietov
Copy link
Collaborator Author

inietov commented Aug 15, 2023

@marcusmoore thanks for your review! I just add a message so the number doesn't feel like an error, because it could happen that a user have 3 assets assigned and for this 3, for some reason, 2 are from another company so they can't accept/deny it, and the number wouldn't match. So now looks something like this:
image

@inietov
Copy link
Collaborator Author

inietov commented Aug 15, 2023

Kindly pinging @snipe, this looks ok to you? or need some more work?

@@ -452,6 +452,8 @@
'serial_number' => 'Serial Number',
'item_notes' => ':item Notes',
'item_name_var' => ':item Name',
'error_user_company' => 'User and Asset companies missmatch',
'error_user_company_accpept_view' => 'An Asset assigned to you belongs to a different company so you can\'t accept nor deny it, please check with your manager',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor typo in this string: accpept

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:S thanks

@snipe snipe merged commit 79b330f into snipe:develop Aug 22, 2023
4 checks passed
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.

None yet

3 participants