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: Bulk Delete page not showing full information #15757

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

akemidx
Copy link
Collaborator

@akemidx akemidx commented Oct 30, 2024

Description

The Bulk Delete page had missing information:

  1. showed user id instead of the person's name
  2. showed incomplete information for a user's asset in they did not have a location set

Before:
Screenshot 2024-10-30 at 7 29 51 PM

After:
Screenshot 2024-10-30 at 7 29 29 PM

The location will be listed to match the location of the asset as it appears on the Assets index page.

Fixes # 26944

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

manual tests and test suite

  • PHP version: 8.1
  • OS version: Sonoma 14.6

Checklist:

@akemidx akemidx requested a review from snipe as a code owner October 30, 2024 23:34
Copy link

what-the-diff bot commented Oct 30, 2024

PR Summary

  • Updated Condition for Location Identification
    The update changes how we identify a location in our system. We are now using a specific 'location_id' instead of a generic 'location'. This improves the accuracy and efficiency of locating information.

  • Added Clause to Check for Real-Time Data Location Identification
    An additional clause was added to check for 'rtd_location_id'. Now, if no specific location is provided, the system is capable of showing the default location name. This makes our interface more user-friendly and helps prevent confusion in case of missing data.

  • Improvement in Task Assignment Identification
    The way we check and confirm task assignments has been enhanced by referencing 'assigned_to'. Furthermore, when displaying task assignment, we now use a more user-friendly name. This change enhances the clarity and readability of our information presentation.

@akemidx akemidx requested a review from marcusmoore October 31, 2024 00:44
@@ -42,15 +42,15 @@
<td>{{ $asset->id }}</td>
<td>{{ $asset->present()->name() }}</td>
<td>
@if ($asset->location)
@if ($asset->location_id)
Copy link
Owner

Choose a reason for hiding this comment

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

Why was this line changed? There is a relationship ($asset->location) which is being checked for, which is generally better than just checking for the ID, since the ID could be invalid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so, the two things we were checking against are the location_id and the rtd_location_id. these are the two values in the assets table when we looked for location. currently, we are ending up with blanks for the location of an asset on the bulk delete page, this is weird behavior, since it won't match the assets index page. since through deployedLocationFormatter we return the row.name off the row.id, and if not, then we pull the rtd_location.name from the rtd_location.id. its this if/else that we tried to parity.

Copy link
Owner

Choose a reason for hiding this comment

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

Right, but if the location_id or rtd_location_id reference a non-existent location (bad data, etc), that will 500. We should be checking against the relations to make sure those locations actually exist in the database.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

righto, i'll get to this on monday and get that switched over. ty for the explaination.

@snipe
Copy link
Owner

snipe commented Nov 13, 2024

Looks like we've got some tests failing here - can you take a look?

@akemidx akemidx requested a review from snipe November 13, 2024 23:47
@snipe snipe merged commit 8f72231 into snipe:develop Nov 19, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants