Custom Asset Field Reordering

[cconard96]

• [ ] I have read the CONTRIBUTING document.
• [ ] I have performed a self-review of my code.
• [ ] I have added tests that prove my fix is effective or that my feature works.
• [ ] This change requires a documentation update.

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.

[orthagh]

Now #17462 has been merged, here are a few notes on that feature as it's the continuation;

Technically

• fields attributes (mandatory, full width, etc) should be expanded to native fields
• so we may store these informations in fields_display
• I suggest so to remove these attributes from glpi_assets_customfields

Regarding UX/UI

• we may have an edit icon for editing field attributes
• native and custom fields should be set/displayed in the same grid (mainly to let people reorder them)
• I STRONGLY suggest using modals in the current UI (for addition and edition). People may add a lot of custom fields and the need to scroll breaks the usage.
• We shouldn't have a "Save" button globally. Maybe it's just a display bug, if not, all actions should auto-save.
• "fake inputs" on the grid should display the default value if any
• The addition process (and primary button) should be common for native and custom fields, maybe a toggle at the top lets you choose the proper form.

That's all for the moment, I may have other minor suggestions later (after seeing a first iteration)

[cconard96]

A checklist based on previous review comment:

• [x] Allow changing field options for core fields. Since core fields aren't really defined well, the options will be stored in the fields_display column for core fields. For custom fields, it is easier to keep storing the options the way they already are.
• [x] Edit icon to change field options, change custom field properties, and delete custom fields
• [x] All field previews reflect the correct type (string, number, dropdown) regardless of if they are core or custom.
• [x] Ability to delete custom field definitions in new UI
• [x] Reflect full width option in UI
• [x] Reflect mandatory option in UI. Need to manually add the mandatory indicator to the preview when needed.
• [x] Reflect readonly option in UI. Since all previews are disabled, need some other way to indicate a readonly field.
• [x] Show default value in UI
• [x] Process to add fields to the order preview is the same for core and custom fields

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.

[cconard96]

Tests need updated, but I think this is ready for a review of the initial implementation.

[orthagh]

A quick functional review for today's checkout on my side:

• we can't set a default value for classic fields
• The same for the "disabled" attribute but, for this one, I'm not sure it could be useful
• The dropdowns in classic fields (models, manufacturers, etc) can be tagged as "multiple values", but it works only for groups_id/groups_id_tech.
• We lack an indication a custom dropdown has been set as multiple (like done for other attributes as "mandatory", "read-only", etc)
• There is a strange bug also with this attribute, when I set it on groups_id, save, set on groups_id tech, save, the first fields lose the attribute in the editor. Anyway, I don't think there is a need to have this toggle on classic fields. The group fields will have the behavior, when displaying an asset, no matter what is set up in the editor.
• Could you explain the top helper phrase "Fields not added below will be appended at the end of the form rather than be hidden". I'm not sure to understand what it does.
• I'm not satisfied with the current UX for adding classic fields and custom ones, but I don't have any suggestions (yet) I'll think about it.

[cconard96]

* we can't set a default value for classic fields

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.

* The same for the "disabled" attribute but, for this one, I'm not sure it could be useful

Doubt it is useful

* The dropdowns in classic fields (models, manufacturers, etc) can be tagged as "multiple values", but it works only for groups_id/groups_id_tech.

Not intended to even have this option. Core fields have no structure so I faked it by reusing the custom field stuff.

* We lack an indication a custom dropdown has been set as multiple (like done for other attributes as "mandatory", "read-only", etc)

We kind of do. You just need to know that select2 controls look like a regular text field when they allow multiple options.

* There is a strange bug also with this attribute, when I set it on groups_id, save, set on groups_id tech, save, the first fields lose the attribute in the editor. **Anyway, I don't think there is a need to have this toggle on classic fields**. The group fields will have the behavior, when displaying an asset, no matter what is set up in the editor.

Not supposed to be an option.

* Could you explain the top helper phrase "Fields not added below will be appended at the end of the form rather than be hidden". I'm not sure to understand what it does.

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 fields_excluded twig parameter to exclude fields completely from the UI but it wasn't hooked up to this new UI. I missed that the spec did actually specify this feature.

The original spec was basically just:

• Can reorder fields in UI
• Can hide fields
• Core fields cannot be changed at all

[orthagh]

When hiding everything except "name", a few fields remain on a custom asset: "UUID" and "update source". Note, that it's not particularly useful from a functional pov, but you can't remove everything. It fails with the following error:

[2024-10-21 08:57:30] glpiphplog.CRITICAL:   *** Uncaught Exception TypeError: array_merge(): Argument #1 must be of type array, null given in .../ajax/asset/assetdefinition.php at line 92

[cconard96]

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 fields_display fields would be removed and therefore nothing is sent to the server. The missing field isn't treated as a deletion.

[orthagh]

A few things to discuss or fix. Sorry for the long wall of text, this took me more time than I tough.

Removing all fields

I can't recreate. I also cannot recreate the situation where all fields can be removed.

Below is a recording of my instance. I created a new type to be sure. Note I may have a broken state on my database, I'll check later. With the last commit, I don't have any more errors, but the save is not taken into account.

Remaining fields

Anyway, I kept only the field "name". When I create an instance of this new asset definition, there are still a few fields displayed that are not managed: "is active", "UUID", "Update source"

Edit modal

Attributes of a native field are not retrieved in the edit modal. Ex I checked all properties here (without saving globally) reopening the edit modal, fields are not checked:

Purge of a custom fields

The field remains in the grid after being purged permanently. A refresh fixes the display issue.

Strange behavior of dropdown

When adding a native field (previously hidden), the selected option in the " field " dropdown isn't removed. When you click the "Add" button a second time, it doesn't change anything in the "Preview" section, but the field is finally removed from the dropdown.

Strange behavior of drag&drop

I finally found an issue with Drag&drop behavior I didn't understand previously but bothering me: When selecting an item and starting to drag it, the placeholder appears, but the initial item remains in DOM. This results in double space usage in the grid and doesn't reflect the final result when you drop the item.

Drag&drop icon alignment:

align vertically the icon (maybe for this recurring one, we could find a more global fix)

Adding a custom field

A new added custom field should directly be added to the grid. We shouldn't have to add it manually.

custom field modal

• add a title
• remove horizontal lines
• remove the grey footer

discussing UX

Below issues/suggestions, I don't have a strong opinion, and it's mainly open to discussion.

The main issue is the distinction for the final user between native and custom fields. If you are not familiar with the concept, this is very hard to understand what is going on.

Maybe we can add a helper explaining the concept at the top.

Regarding design, I always found the controls for adding a field, placed on top, don't permit a fluid understanding. 1st suggestion: integrating one unified button into the grid of sortable fields:

alternative: separate the add form of the grid with an hr

Another suggestion: rename the labels to identify clearly the action

• "insert" (an existing field)
• "Create a custom field"

In the same idea, all the people I presented the page didn't identify well the "Hide" action and what it achieves (returning the field in the list of available)

[cconard96]

• [x] Allow removing all fields
• [x] Fix issue with inventory-related fields showing when they aren't in the field display
• [x] Fix field options when reopening edit modal
• [x] Field dropdown selection not cleared after field added
• [x] Remove fields from view when purged from modal form
• [x] Fix original element when dragging and dropping still being shown which throws off alignment of the remaining fields
• [x] Vertically align sort handle with the field labels
• [x] Custom fields should be added directly to the view after creation
• [x] Custom field modal needs title and extra UI parts (hr and footer) should be removed

Not previously requested:

• [x] Convert to Vue for cleaner implementation and potentially faster E2E testing using Component Tests (component tests not done yet)

[cconard96]

All points addressed except the dragged field still being in the UI. I haven't found a way to target the original element in CSS without also targeting the drag image element.

[orthagh]

I'll try to make a test in 1h or 2

[orthagh]

What do we need to finish this pr (and #18145 ) ?

[cconard96]

Translations PR needs tests added now that it should be functionally and technically complete.

This PR should clean the field display values when a custom field is deleted. Then, I assume it is finally ready for tests to be added.

[orthagh]

Sonarcloud report: remaining alert seems the same and should probably ignored.

The current failing e2e test is due to the modal losing focus on the input while Cypress is typing the label. A rough solution, waiting for the event shown/hidden to add an interceptable attribute by cypress:3

diff --git a/templates/components/form/viewsubitem.html.twig b/templates/components/form/viewsubitem.html.twig
index 40c2bdc79b..3f302044d6 100644
--- a/templates/components/form/viewsubitem.html.twig
+++ b/templates/components/form/viewsubitem.html.twig
@@ -56,6 +56,12 @@
 <script>
     function viewAddEditSubItem{{ rand }}(id) {
         const modal_el = $('#{{ subitem_container_id ~ '_modal' }}');
+        modal_el[0].addEventListener('shown.bs.modal', (event) => {
+            event.target.setAttribute('data-cy-ready', 'true')
+        });
+        modal_el[0].addEventListener('hidden.bs.modal', (event) => {
+            event.target.removeAttribute('data-cy-ready')
+        });
         $('#{{ subitem_container_id }}').load('/ajax/viewsubitem.php',{
             type: "{{ type|e('js') }}",
             parenttype: "{{ parenttype|e('js') }}",
diff --git a/tests/cypress/e2e/Asset/custom_fields.cy.js b/tests/cypress/e2e/Asset/custom_fields.cy.js
index ade0902bc2..b97e7d065a 100644
--- a/tests/cypress/e2e/Asset/custom_fields.cy.js
+++ b/tests/cypress/e2e/Asset/custom_fields.cy.js
@@ -125,7 +125,7 @@ describe("Custom Assets - Custom Fields", () => {
         function createField(label, type, options = new Map()) {
             cy.findByRole('button', {name: 'Create new field'}).click();
             cy.waitForNetworkIdle('/ajax/viewsubitem.php', 100);
-            cy.findByRole('button', {name: 'Create new field'}).siblings('.modal').should('be.visible').within(() => {
+            cy.findByRole('button', {name: 'Create new field'}).siblings('.modal[data-cy-ready=true]').should('be.visible').within(() => {
                 cy.findByLabelText('Label').type(label);
                 cy.findByLabelText('Type').select(type, {force: true});

They may be a better solution, but we should avoid adding a wait(...) for this kind of issue.

[cedric-anne]

E2E tests seems to fail.

[cconard96]

I cannot get the E2E test to fail outside of the CI

[cedric-anne]

I cannot get the E2E test to fail outside of the CI

CI is executed on a up-to-date branch (due to a Github option). It means that code is rebased on the latest version of the target branch before the execution of the test suite.

[orthagh]

I wrote my comments in the wrong pr (deleted in #17674) Here is:

After discussing a bit with @cedric-anne, this is due to the fact we enabled, long time ago, an option on github to rebase the pr before launching the tests suite.

The pr itself indeed doesn't fail locally, but after a rebase, yes.

I don't have a strong opinion about the option, but you can get the fail and fix it after a rebase. I'm not sure if we can keep the option as you must know the thing, and even after insisting to everyone about that, I think there still will be cases where we forget about it.

[trasher]

I don't have a strong opinion about the option,

Without that option, current PR may have been merged, and tests would have fail on main branch; where almost nobody sees it. IMHO, this option should be kept, to prevent fails on main branch as much as possible.

[AdrienClairembault]

Agreed.

[cconard96]

Don't see how it is related to this PR. Html::redirect is throwing an exception which ruins an AJAX call to front/asset/customfielddefinition.form.php by templates/components/form/viewsubitem.html.twig to submit form data using an AJAX call to avoid a page reload/redirect.

Seems to me that instead of not handling the redirect for AJAX requests, you should be suppressing it.

[cedric-anne]

Don't see how it is related to this PR. Html::redirect is throwing an exception which ruins an AJAX call to front/asset/customfielddefinition.form.php by templates/components/form/viewsubitem.html.twig to submit form data using an AJAX call to avoid a page reload/redirect.

Seems to me that instead of not handling the redirect for AJAX requests, you should be suppressing it.

#18448 should fix this issue. Could you validate it?

[cconard96]

Don't see how it is related to this PR. Html::redirect is throwing an exception which ruins an AJAX call to front/asset/customfielddefinition.form.php by templates/components/form/viewsubitem.html.twig to submit form data using an AJAX call to avoid a page reload/redirect. Seems to me that instead of not handling the redirect for AJAX requests, you should be suppressing it.

#18448 should fix this issue. Could you validate it?

Seems OK now

[orthagh]

Not sure the inclusion of #18448 was mandatory. For me the issue was on json decoding of translations.

Anyway, E2E tests are green now