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 😄

akemidx · 2024-08-15T19:34:00Z

this is still a bug on v7. confirmed on develop. it looks a tad different, but I did still manage to export a blank report.

akemidx · 2024-08-15T19:51:27Z

no code changes, only merge conflicts resolved, and tests are failing. the tests are newer than this branch, but still we don't want em failing. so, gonna have to figure those out.

snipe · 2024-08-17T14:01:52Z

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

-                    {{ (\App\Models\Statuslabel::find($status_id)) ? \App\Models\Statuslabel::find($status_id)->name : '' }}
-                </option>
-            @else
-                <option value=""  role="option">{{ trans('general.select_status') }}</option>


IIRC, we have that here because in some places these fields are required and in some they are not, and the dropdowns don't provide a "Select blah" option automatically. This would need to be tested across other things that use these dropdown partials.

snipe · 2024-08-17T14:02:39Z

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

@@ -5,7 +5,7 @@
    <div class="col-md-7{{  ((isset($required)) && ($required=='true')) ? ' required' : '' }}">
        <select class="js-data-ajax" data-endpoint="users" data-placeholder="{{ trans('general.select_user') }}" name="{{ $fieldname }}" style="width: 100%" id="assigned_user_select" aria-label="{{ $fieldname }}">
            @if ($user_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : ''))
-                <option value="{{ $user_id }}" selected="selected" role="option" aria-selected="true"  role="option">


Good catch on the duplicate code!

…_fix

akemidx · 2024-09-03T19:47:17Z

fixed in PR-15418

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

Merge branch 'develop' into blank_select_bug_fix

8cfef65

snipe reviewed Aug 17, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into blank_select_bug…

c22dd94

…_fix

akemidx closed this Sep 3, 2024

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.

akemidx commented Aug 15, 2024

akemidx commented Aug 15, 2024

snipe Aug 17, 2024

snipe Aug 17, 2024

akemidx commented Sep 3, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusmoore commented Jan 30, 2024

marcusmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcusmoore left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

akemidx commented Aug 15, 2024

akemidx commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akemidx commented Sep 3, 2024