-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Multiple choice options for selectable fields on custom reports #13482
Conversation
…electing the placeholder value in some circumstances
PR Summary
|
Please outline in a little more detail how this was tested, specifically in ways where the files changed are used in areas outside of the custom report, which use cases you explored, etc. |
of course. wasn't sure what else to say, so I'll update the description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can compact a lot of this code down to just use a ternary statement, the same way as we do in the location-select.blade.php
<select class="js-data-ajax" data-endpoint="locations" data-placeholder="{{ trans('general.select_location') }}" name="{{ $fieldname }}" style="width: 100%" id="{{ $fieldname }}_location_select" aria-label="{{ $fieldname }}" {!! ((isset($item)) && (Helper::checkIfRequired($item, $fieldname))) ? ' data-validation="required" required' : '' !!}{{ (isset($multiple) && ($multiple=='true')) ? " multiple='multiple'" : '' }}> |
did you quote which I need to change, or an example of what other stuff should be changed to be like? |
The link in my post above shows an example of the ternary |
@snipe This is finished and ready for review. I tested a few of our create and edit pages for our different top level models and was able to confirm they only allow the selection of one choice in the fields I made available to be a multiple choice on the custom reports. Since I had used the isset(multiple), I believe this will work, and ONLY in spots in the code where we explicitly state "multiple" will be the fields where you can choose multiple options. The custom reports blade was the only spot i changed to allow multiple choices. Custom reports -> multiple is set -> multiple choice allowed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks excellent - very nice work! If I'm reading it right - and I'm not sure that I am - it looks to me like the <option>
tags look the same whether something is multiple
or not. If so, can we squish those up into just one <option>
tag value? e.g. I don't think we need the if($multiple)
or if(!$multiple)
branches - they look identical to me. Another thing - which might be completely fine, mind you - is that I don't see any changes to the controller to accept arrays - does that already 'automagically' work? If so that's great! Once we can get those if($multiple)
/if!($multiple)
branches squished together (assuming they can be squished together), I think we'll be good to go!
@uberbrady I will go back and test on Monday. I believe that when I originally made the PR, the reason for the way it is now, is to fix a bug in the fields you can select. #13482 (comment) <--example here This was causing a situation where a report could end up empty. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a couple of those drop-downs got their contents flipped around - to Category from various things like Company, Location, Department - once we can get those back to where they should be (or if you can explain what I'm misunderstanding!) I think we should be in good shape.
@if($multiple) | ||
@if($company_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $company_id }}" selected="selected" role="option" aria-selected="true" role="option"> | ||
{{ (\App\Models\Category::find($company_id)) ? \App\Models\Category::find($company_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm, we sure we want 'Category' here? Shouldn't that be Company?
@if(!$multiple) | ||
@if($company_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $company_id }}" selected="selected" role="option" aria-selected="true" role="option"> | ||
{{ (\App\Models\Category::find($company_id)) ? \App\Models\Category::find($company_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
@if($multiple ?? '') | ||
@if($company_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $company_id }}" selected="selected" aria-selected="true"> | ||
{{ (\App\Models\Category::find($company_id)) ? \App\Models\Category::find($company_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this too?
@if(!$multiple ?? '') | ||
@if($company_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $company_id }}" selected="selected"> | ||
{{ (\App\Models\Category::find($company_id)) ? \App\Models\Category::find($company_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here?
@if($multiple) | ||
@if($department_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $department_id }}" selected="selected" role="option" aria-selected="true" role="option"> | ||
{{ (\App\Models\Category::find($department_id)) ? \App\Models\Category::find($cdepartment_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Department->Category here as well? Or maybe I'm missing something.
@if($multiple) | ||
@if($location_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $location_id }}" selected="selected" role="option" aria-selected="true" role="option"> | ||
{{ (\App\Models\Category::find($location_id)) ? \App\Models\Category::find($location_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here?
@if(!$multiple) | ||
@if($location_id = old($fieldname, (isset($item)) ? $item->{$fieldname} : '')) | ||
<option value="{{ $location_id }}" selected="selected" role="option" aria-selected="true" role="option"> | ||
{{ (\App\Models\Category::find($location_id)) ? \App\Models\Category::find($location_id)->name : '' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also here? (Locations)
@uberbrady I adjusted those. It seems that some of the multi searches were working even with those erroneously being "Category" instead of their proper paths. The code is built to bypass the "blank search" bug I found. But as of right now, the three choices I made newly multi searchable, are not working. so that's Department, Manufacturer, and Company Currently, it is only returning the FIRST choice in the multi select, and I'm not sure why right now. Need to do some more testing and see what is different between the ones that work, and the ones that don't. I'll update if I can figure out a fix. Worst case I will make a new PR that is specifically fixing that bug. |
When running custom reports, users can now select multiple options from available choices in their DB.
Fields for multi select are:
Type of Change:
How Has This Been Tested?
Tested locally in test instance
Test Configuration:
PHP version: php8.0
Snipe-IT dev instance
Checklist: