Custom Asset Field Reordering #17873
Conversation
cconard96
force-pushed
the
feature/custom_asset_field_order
branch
2 times, most recently
from
2cb5785
to
024575a
Compare
|
Now #17462 has been merged, here are a few notes on that feature as it's the continuation; Technically
Regarding UX/UI
That's all for the moment, I may have other minor suggestions later (after seeing a first iteration) |
cconard96
force-pushed
the
feature/custom_asset_field_order
branch
from
9b8edd1
to
40ba2a6
Compare
cconard96
force-pushed
the
feature/custom_asset_field_order
branch
from
3c9f33d
to
42e5059
Compare
|
A checklist based on previous review comment:
Also had to redo the field display as flexbox instead of grid for sorting to work properly when fields could be regular or full width. |
|
Tests need updated, but I think this is ready for a review of the initial implementation. |
cedric-anne
requested review from
orthagh,
AdrienClairembault and
cedric-anne
| http_response_code(404); | ||
| exit(); |
There was a problem hiding this comment.
| http_response_code(404); | |
| exit(); | |
| throw new NotFoundHttpException(); |
| 'results' => $field_results, | ||
| 'count' => count($all_fields) | ||
| ], JSON_THROW_ON_ERROR); | ||
| exit(); |
There was a problem hiding this comment.
We have removed all exit calls, see #18042.
| exit(); |
| } | ||
| http_response_code(400); |
There was a problem hiding this comment.
If we remove the exit statement, this need to be in an else.
| } | |
| http_response_code(400); | |
| } else { | |
| throw new BadRequestHttpException(); | |
| } |
| use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; | ||
|
|
There was a problem hiding this comment.
| use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; | |
| use Symfony\Component\HttpKernel\Exception\BadRequestHttpException; | |
| use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; | |
| throw new NotFoundHttpException(); | ||
| } | ||
| $asset_definition->showFieldOptionsForCoreField($_GET['key']); | ||
| exit(); |
There was a problem hiding this comment.
| exit(); |
| } | ||
| $custom_field->fields['field_options']['disabled'] = true; | ||
| echo $custom_field->getFieldType()->getFormInput('', null); | ||
| exit(); |
There was a problem hiding this comment.
| exit(); |
There was a problem hiding this comment.
Symfony controllers have been available for a while now, IMO we shouldn't create any new legacy front files.
| foreach ($all_fields as $k => $v) { | ||
| $field_info = is_array($v) ? $v : ['text' => $v]; | ||
| if (!empty($_POST['searchText']) && stripos($field_info['text'], $_POST['searchText']) === false) { | ||
| continue; | ||
| } | ||
| $field_info['id'] = $k; | ||
| $field_results[] = $field_info; | ||
| } |
There was a problem hiding this comment.
It is usually best to move this kind of code into the method of a class, as it allow to test it easily with phpunit (and it also make the code reusable and able to benefits from things like dependency injection if needed).
(Not required to change it, just a general comment about best practices for next time).
| {% set default_field_order = [ | ||
| 'name', 'firstname', 'template_name', '_template_is_active', 'states_id', item.getForeignKeyField(), 'is_helpdesk_visible', | ||
| '_dc_breadcrumbs', 'locations_id', 'item_type', 'itemtype', 'date_domaincreation', item.getTypeForeignKeyField(), | ||
| 'usertitles_id', 'registration_number', 'phone', 'phone2', 'phonenumber', 'mobile', 'fax', 'website', 'email', | ||
| 'address', 'postalcode', 'town', 'postcode', 'state', 'country', 'date_expiration', 'ref', 'users_id_tech', | ||
| 'manufacturers_id', 'groups_id_tech', item.getModelForeignKeyField(), 'contact_num', 'serial', 'contact', 'otherserial', | ||
| 'sysdescr', 'snmpcredentials_id', 'users_id', 'is_global', 'size', 'networks_id', 'groups_id', 'uuid', 'version', | ||
| 'comment', 'ram', 'alarm_threshold', 'brand', 'begin_date', 'autoupdatesystems_id', 'pictures', 'is_active', 'last_boot' | ||
| ] %} | ||
| {# Support custom fields for generic assets #} | ||
| {% set custom_fields = custom_fields|default({}) %} | ||
| {# Set real field older using 'field_order' provided and any fields missing added to the end of the order array #} | ||
| {% set field_order = field_order|default(default_field_order) %} | ||
| {% set field_order = field_order|merge(array_diff(default_field_order, field_order)) %} | ||
| {# Add missing custom fields to end of the order array #} | ||
| {% set custom_fields_keys = custom_fields|keys %} | ||
| {% set field_order = field_order|merge(array_diff(custom_fields_keys, field_order)) %} | ||
| {# Remove fields that don't apply #} | ||
| {% set field_order = field_order|filter((f) => f starts with '_' or item.isField(f)) %} | ||
|
|
||
| {# Exclude specified fields #} | ||
| {% set fields_excluded = fields_excluded|default([]) %} | ||
| {% set field_order = field_order|filter((f) => f not in fields_excluded) %} | ||
|
|
There was a problem hiding this comment.
That seems like a lot of computation inside a view.
Maybe it could be computed by a testable method on PHP's side ?
There was a problem hiding this comment.
There isn't really a lot of logic here and it makes sense to handle it all at this level. At best, this would be replaced by a call to a PHP method, but that doesn't seem strictly needed.
|
A quick functional review for today's checkout on my side:
|
Didn't realize that was required. This PR was originally just to reorder fields in the UI. I didn't see a spec anywhere for anything extra like making core fields mandatory/readonly/full width/etc. Also all this extra stuff is just in the web UI display. Nothing enforces it on the server side so the API doesn't respect it.
Doubt it is useful
Not intended to even have this option. Core fields have no structure so I faked it by reusing the custom field stuff.
We kind of do. You just need to know that select2 controls look like a regular text field when they allow multiple options.
Not supposed to be an option.
You cannot hide fields using this UI currently. Look at the generic_show_form template and you see how this is all handled. It takes a custom defined order if one is provided and then appends the missing fields based on the default order. So, if we add new core fields that don't exist in the field order config for a custom asset, they will still appear in the form but be at the end of the form. I did add handling for a The original spec was basically just:
|
cconard96
force-pushed
the
feature/custom_asset_field_order
branch
from
039a6dc
to
6c0c6a2
Compare
cconard96
force-pushed
the
feature/custom_asset_field_order
branch
from
6c0c6a2
to
27d3488
Compare
|
When hiding everything except "name", a few fields remain on a custom asset: "UUID" and "update source". |
I can't recreate. I also cannot recreate the situation where all fields can be removed. AFAIK it would be impossible from the web UI because all |
Taking over from #16975
Currently focusing on reordering default fields until the custom fields PR is merged, and then the Fields tab UI will be adjusted and support for the custom fields added.