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

Adjust requested assets and models workflow a little bit. #11190

Merged
merged 6 commits into from
May 24, 2022

Conversation

inietov
Copy link
Collaborator

@inietov inietov commented May 24, 2022

Description

This changes doesn't change behavior a lot, but there was a Requestable Asset Models view that doesn't links to the proper model if you click to their name.

Also when checking this I hit a DB exception if in Requestable Assets view I try to order columns, so the fix is also in this PR, and finally, a tiny tiny fix on the Requested Assets views, where we were showing two blank columns instead of the one that contains the checkin/checkout actions.

Fixes freshdesk 27421

Type of change

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

How Has This Been Tested?

Test Configuration:

  • PHP version: 7.4.16
  • MySQL version: 8.0.23
  • Webserver version: nginx/1.19.8
  • OS version: Debian 10

Checklist:

@snipe
Copy link
Owner

snipe commented May 24, 2022

So IIRC, we don't link to those model listings because the person logged in may not have the ability to view asset model details (i.e. a "regular" user), which would result in 403 errors on everything they click on.

@inietov
Copy link
Collaborator Author

inietov commented May 24, 2022

Yeah, just tested it and that's the case. You think it's a good idea to check for permissions to show the link? or I just rollback that change?

@snipe
Copy link
Owner

snipe commented May 24, 2022

Yeah, from a UX perspective, being presented with a link that you can't click on without an error is a bad experience. I'd say check for the gate before presenting the link.

@@ -109,8 +109,14 @@ class="table table-striped snipe-table"

</td>

<td>
@if (Gate::allows('superadmin'))
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be superadmin, or just the ability to view asset models?

@can('view', \App\Models\AssetModel::class)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's a lot better, yes.

@@ -109,8 +109,14 @@ class="table table-striped snipe-table"

</td>

<td>
@can('view', \App\Models\AssetModel::class)
<a href='{{ url("/models/{$requestableModel->id}") }}' > {{ $requestableModel->name }} </a></td>
Copy link
Owner

Choose a reason for hiding this comment

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

Can we clean up the extra spaces and the extra </td> here? We already close the <td> outside of the loop.

<a href='{{ url("/models/{$requestableModel->id}") }}' > {{ $requestableModel->name }} </a></td>

vs

<a href="{{ url("/models/{$requestableModel->id}") }}">{{ $requestableModel->name }}</a>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, I have it written differently and the tag remained 😬

@snipe snipe merged commit a243823 into snipe:develop May 24, 2022
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

2 participants