Fixed: Blank select on custom reports causing empty exports #14110

akemidx · 2024-01-08T22:51:02Z

Description

Custom reports have some fields that are multi select. It was originally possible to select an option, and then x out that option, but leave a "blank" choice in that field. Generating the report would end up passing that blank option as a search parameter, and returning an empty report.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

created and ran tests to confirm payload is as expected
practical testing for visual confirmation

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

what-the-diff · 2024-01-08T22:51:24Z

PR Summary

Enhanced Company Selections Functionality
Updated the 'company-select.blade.php' file. Now it can handle multiple company selections more effectively due to the added conditional logic.
Improved Location Selections Process
A modification in the 'location-select.blade.php' file has led to a better management of multiple location selections, thanks to the newly implemented conditional logic.
Upgraded Model Selections Capability
The changes in the 'model-select.blade.php' file now allow for more streamlined handling of multiple model selections by incorporating conditional logic.
Better Status Selections Management
Integration of additional conditional logic in the 'status-select.blade.php' file enhances functionality when dealing with multiple status selections.
Optimized Supplier Selections Method
The 'supplier-select.blade.php' file was improved and can now efficiently handle multiple supplier selections due to the newly added conditional logic.

resources/views/partials/forms/edit/company-select.blade.php

snipe

A few questions/fixes requested.

snipe · 2024-01-24T13:38:32Z

resources/views/partials/forms/edit/location-select.blade.php

-            @else
-                <option value=""  role="option">{{ trans('general.select_location') }}</option>
-            @endif
+            {{  ((isset($multiple)) && ($multiple=='true')) ? " multiple='multiple'" : '' }}


I'm not sure what this check does?

So, what we want to do is check that there is a multiple value set, so ignore anything that does NOT ever deal with multiple values. and then also that that value is true.

The original way I wrote it (the in red bits) was how I figured out how to get rid of the bug. It took some trial and error and that's how I came up with the fix, tho it was a bit dupilcative with how it was written.

Addmittedly I am still a bit new with ternary syntax, so I might have oopsed.

That line ends up writing multiple=multiple onto the page without any effect:

Did you mean to make that an @isset blade directive to match the one added below on line 12?

snipe · 2024-01-24T13:38:50Z

resources/views/partials/forms/edit/location-select.blade.php

-            @endif
+            {{  ((isset($multiple)) && ($multiple=='true')) ? " multiple='multiple'" : '' }}
+                @if($location_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : ''))
+                    <option value="{{ $location_id }}" selected="selected" role="option" aria-selected="true"  role="option">


role="option" is in here twice

I can remove that, but that is actually in the selects a LOT. including blades I never touched.
for example: resources/views/partials/forms/edit/asset-select.blade.php or resources/views/partials/forms/edit/user-select.blade.php

I'll update those too.

marcusmoore · 2024-01-30T19:34:11Z

Ah! It took me a minute to realize what the issue is. Here's a video and screenshot demonstrating the problem:

CleanShot.2024-01-30.at.11.24.42.mp4

marcusmoore

This PR actually fixes the bug but there are some changes that should be made before merging.

marcusmoore · 2024-01-30T19:31:00Z

resources/views/partials/forms/edit/location-select.blade.php

-            @else
-                <option value=""  role="option">{{ trans('general.select_location') }}</option>
-            @endif
+            {{  ((isset($multiple)) && ($multiple=='true')) ? " multiple='multiple'" : '' }}


That line ends up writing multiple=multiple onto the page without any effect:

Did you mean to make that an @isset blade directive to match the one added below on line 12?

marcusmoore · 2024-01-30T19:32:23Z

resources/views/partials/forms/edit/location-select.blade.php

-                <option value=""  role="option">{{ trans('general.select_location') }}</option>
-            @endif
+            {{  ((isset($multiple)) && ($multiple=='true')) ? " multiple='multiple'" : '' }}
+                @if($location_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : ''))


This @if isn't closed properly. I'm not sure if it's working because @endisset is closing it but it should have a matching @endif added.

(the other selects in this PR should be updated as well)

marcusmoore

Looks good! Thanks for the fix 😄

blank select bug fix for custom reports

aea9883

akemidx requested a review from snipe as a code owner January 8, 2024 22:51

probot-autolabeler bot added the frontend label Jan 8, 2024

snipe reviewed Jan 9, 2024

View reviewed changes

resources/views/partials/forms/edit/company-select.blade.php Outdated Show resolved Hide resolved

snipe changed the title ~~BUG FIX: Blank select on custom reports causing empty exports~~ Fixed: Blank select on custom reports causing empty exports Jan 9, 2024

akemidx requested a review from uberbrady January 9, 2024 20:05

cleaning up code

4d977dc

snipe requested changes Jan 24, 2024

View reviewed changes

duplicate role=options

0c064a3

marcusmoore requested changes Jan 30, 2024

View reviewed changes

akemidx added 2 commits January 30, 2024 18:50

fixing if/set tags

61cbdcb

else

4a18405

marcusmoore approved these changes Feb 1, 2024

View reviewed changes

marcusmoore requested a review from snipe February 6, 2024 17:54

This comment was marked as spam.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed: Blank select on custom reports causing empty exports #14110

Fixed: Blank select on custom reports causing empty exports #14110

akemidx commented Jan 8, 2024

what-the-diff bot commented Jan 8, 2024

snipe left a comment

snipe Jan 24, 2024

akemidx Jan 24, 2024

marcusmoore Jan 30, 2024

snipe Jan 24, 2024

akemidx Jan 24, 2024

marcusmoore commented Jan 30, 2024

marcusmoore left a comment

marcusmoore Jan 30, 2024

marcusmoore Jan 30, 2024

marcusmoore left a comment

This comment was marked as spam.

Fixed: Blank select on custom reports causing empty exports #14110

Are you sure you want to change the base?

Fixed: Blank select on custom reports causing empty exports #14110

Conversation

akemidx commented Jan 8, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

what-the-diff bot commented Jan 8, 2024

PR Summary

snipe left a comment

Choose a reason for hiding this comment

snipe Jan 24, 2024

Choose a reason for hiding this comment

akemidx Jan 24, 2024

Choose a reason for hiding this comment

marcusmoore Jan 30, 2024

Choose a reason for hiding this comment

snipe Jan 24, 2024

Choose a reason for hiding this comment

akemidx Jan 24, 2024

Choose a reason for hiding this comment

marcusmoore commented Jan 30, 2024

marcusmoore left a comment

Choose a reason for hiding this comment

marcusmoore Jan 30, 2024

Choose a reason for hiding this comment

marcusmoore Jan 30, 2024

Choose a reason for hiding this comment

marcusmoore left a comment

Choose a reason for hiding this comment

This comment was marked as spam.