Custom asset fields #17462

cconard96 · 2024-07-07T02:40:46Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-

Still very early WIP, but has at least basic functionality as a proof of concept and allows starting to test performance.

Supported field types (mainly based on Fields plugin types):

Fields will be allowed to be defined as being required or readonly.

UI/UX Notes:

The internal name for the custom fields are currently automatically generated in the web form based on the default label. If the field type is date or datetime, a date_ is prefixed to the internal name to match how fields are named in GLPI itself.
No support currently for showing custom fields in a new tab like the Fields plugin. All custom fields show at the end of the asset form.

Technical features:

Automatic search option generation. The ID is currently 4500 + the ID for the field definition
When new fields are defined, it is intended that nothing is added to the custom_fields_json column for existing items of that asset type. The default value will be transparently used in those cases until the asset item is saved again.
Search options fill in default values when individual values are missing

install/migrations/update_10.0.x_to_11.0.0/assets.php

orthagh

I will not review the UI/UX.

In a next PR, we should implements reordering/removal of all fields of an asset definition, including basic fields. I started a few weeks ago a pr (#16975) for that. I don't have the time right now to finish it but in a very quick summary:

all fields manipulation should be in a single "Fields" tab
we can reorder any existing field (basic and custom)
we can remove any fields (same)
we can add basic fields (if they have been removed) and new custom fields. The UX here will be not that easy to make

Just one thing, btw, the first displayed input is the system name, it's complicated to understant as the field, although set as read-only is not identifiable as non editable (a global issue in GLPI main we should address) but also is deducted from label input.
I suggest to display the label field first, and make system name just for information (in a HtmlField rather Input in read only)

install/mysql/glpi-empty.sql

cconard96 · 2024-07-10T12:25:16Z

Just one thing, btw, the first displayed input is the system name, it's complicated to understant as the field, although set as read-only is not identifiable as non editable (a global issue in GLPI main we should address) but also is deducted from label input. I suggest to display the label field first, and make system name just for information (in a HtmlField rather Input in read only)

I'm overriding the form_fields block rather than using more_fields block, so I control where the field goes. Would it be desired to give users the ability to set this field themselves? Right now it is just auto-generated for convenience. There shouldn't be any issue with users changing the field name except that it would break compatibility of API integrations. Everything added by this PR refers to the fields by ID in the DB.

orthagh · 2024-07-10T13:30:01Z

Would it be desired to give users the ability to set this field themselves

Not sure it adds something useful, let's keep the information visible (displayed in second), just avoid the field input display

orthagh

Things I noted while testing on a fresh instance:

"Custom Field" tab should be placed 3rd (after "Capacities") and later renamed "Fields"
"Uncaught Exception Error: Call to undefined function Glpi\Asset_() in /var/www/glpi/src/Glpi/Asset/CustomField.php at line 252" when going to the "Custom fields" tab for the first time
name field should be labelled "System name"
The full width toggle is cool, but I think it's more related to what he discussed about (all) fields reordering/removing/adding
the following could be done later in another PR, but UX for the fields properties could be maybe shared, at least partially, with form questions (ping @AdrienClairembault) . Especially the options setup for dropdowns is better in the current form implementation. We may also have icons for field types (also done in forms questions), date fields also have a current time toggle.

cconard96 · 2024-07-12T11:00:43Z

* "Custom Field" tab should be placed 3rd (after "Capacities") and later renamed "Fields"

Done

* "Uncaught Exception Error: Call to undefined function Glpi\Asset_() in /var/www/glpi/src/Glpi/Asset/CustomField.php at line 252" when going to the "Custom fields" tab for the first time

I can't recreate the issue and the referenced line number doesn't line up to anything that makes sense.

* name field should be labelled "System name"

Done

* The full width toggle is cool, but I think it's more related to what he discussed about (all) fields reordering/removing/adding

That was the plan. I used it to make the richtext field usable while testing the creation/functionality of each field type though.

* the following could be done later in another PR, but UX for the fields properties could be maybe shared, at least partially, with form questions (ping @AdrienClairembault).

I'm not so sure about using anything directly from that.

Especially the options setup for dropdowns is better in the current form implementation.

Its just how select2 natively supports custom options.

We may also have icons for field types (also done in forms questions), date fields also have a current time toggle.

For "current time" toggle, it would require changing how default values are handled and requiring a mass update of every existing asset of the type to set the value if it isn't already. Right now, the default value is used as a fallback until the asset form is saved again or the value is specifically set in the API. Again, it wasn't really in the spec and also doesn't seem to have existed in the plugin.

cconard96 · 2024-07-12T11:02:17Z

Unrelated to custom fields but on the topic of the Forms feature, the UI isn't working properly in any dark themes. I guess some elements are using hard-coded colors instead of variables from the framework.

cedric-anne

IMHO, we should not try to implement custom dropdown in this PR. Indeed, we have to discuss about the best solution first. For instance, people will probably want to translate each dropdown value label. Storing it in a JSON field and adding the UI to handle translations may require some work and would be less flexible than for instance, implementing generic dropdowns support.

cedric-anne

I did not reviewed everything yet. I will do it later in the week.

ajax/asset/customfield.php

front/asset/customfield.form.php

install/mysql/glpi-empty.sql

install/migrations/update_10.0.x_to_11.0.0/assets.php

templates/pages/assets/customfield.html.twig

install/migrations/update_10.0.x_to_11.0.0/assets.php

src/Glpi/Asset/CustomField.php

src/Glpi/Asset/AssetDefinition.php

src/Glpi/Asset/Asset.php

cedric-anne · 2024-07-31T10:40:34Z

I suashed commits, rebased, and fix some issues.

I removed the rich text fields as it requires more work (the default value should be editable in a rich text editor, files and documents should be processed, ...). It could be done in a later PR, to be able to finalize the current PR ASAP.

I will continue the review this afternoon.

@cconard96 Could you check the failing E2E test? It is related to flatpicker inputs that does not seems to be targetable by their label.

cconard96 · 2024-07-31T10:42:58Z

It is related to flatpicker inputs that does not seems to be targetable by their label.

Probably. See note in #17323. "Input labels broken when using altInput option".

Some changes were made, others will be done in a separated PR.

AdrienClairembault · 2024-08-01T07:28:40Z

tests/cypress/e2e/Asset/custom_fields.cy.js

+        cy.findByLabelText("Active").select('1', { force: true });
+        cy.findByRole('button', {name: "Add"}).click();
+
+        cy.get('div.toast-container .toast-body a').click();


Can probably use findByRole('alert') and/or findByRole('link') here to not be dependent on the classes.

AdrienClairembault · 2024-08-01T07:30:28Z

tests/cypress/e2e/Asset/custom_fields.cy.js

+        cy.findByRole('button', {name: 'Add a new field'}).parents('.tab-pane').should('have.class', 'active').first().within(() => {
+            cy.findByRole('button', {name: 'Add a new field'}).click();
+            cy.findByLabelText('Label').type('Test Text');
+            cy.findByLabelText('Type').select('Text', { force: true });


Can use getSelectByLabel+ setDropdownValue to avoid using force: true (same for other instances).

AdrienClairembault · 2024-08-01T07:31:12Z

tests/cypress/e2e/Asset/custom_fields.cy.js

+        });
+
+        cy.findByRole('tab', {name: 'Profiles'}).click();
+        cy.get('input[type="checkbox"][id^="cb_checkall_table"]').check({ force: true });


Can use findByRole('checkbox') here

AdrienClairembault · 2024-08-01T07:32:35Z

tests/cypress/e2e/Asset/custom_fields.cy.js

+        cy.getDropdownByLabelText('Test YesNo').selectDropdownValue('Yes');
+
+        // This is the placeholder field
+        cy.get('.form-field .field-container:not(:has(*))').should('have.length', 1);


Is there a query method that could be used here instead of a css selector ?

The placeholder corresponds to an almost empty element. No ID, no name, no label, just whitespaces.

But as it is not yet possible to reorder fields, maybe this field type should be removed. Indeed, reordering fields and changing the form layout should be done on all fields, not just custom fields, and we should probably only store it in the layout config itself, see #16975 .

AdrienClairembault

I think there is a big issue with the lack of polymorphism regarding fields types.
We have condition on specific types everywhere + many match and switch depending on types.

This will cause a lot a maintainability issue in the future, we really need to apply some kind of strategy pattern with a common interface.

ajax/asset/customfield.php

front/asset/customfield.form.php

cedric-anne · 2024-08-01T08:28:44Z

I think there is a big issue with the lack of polymorphism regarding fields types.
We have condition on specific types everywhere + many match and switch depending on types.

Indeed, I think the fields types should each have an own class to be able to give the ability to the plugins to register new fields types.
I add it to the backlog, we could refactor this later.

cedric-anne

There are still some bugs.

The options/default values seems to be loaded correctly on the field definition update form, but they are not loaded anyomre on the field definition creation form.
It result in errors like:

Undefined array key "multiple" in /var/www/glpi/src/Glpi/Asset/CustomFieldOption/AbstractOption.php at line 65
Undefined array key "min" in /var/www/glpi/src/Glpi/Asset/CustomFieldOption/AbstractOption.php at line 65
Undefined array key "max" in /var/www/glpi/src/Glpi/Asset/CustomFieldOption/AbstractOption.php at line 65
Undefined array key "step" in /var/www/glpi/src/Glpi/Asset/CustomFieldOption/AbstractOption.php at line 65

When a field is defined as mandatory and/or readonly, these options are also applied to the default value field. They should not.
The default value field for a Number type applies the current min/max/step values. When these are updated, it may produce an unexepected behaviour:
For instance, here I changed the max from 25 to 100 and tried to change the default value to 50:
The itemtype of a Dropdown field is not configurable (there is no field displayed for that).
The Text field seems to be similar to the String type, I guess it is an error. In the fields plugin, there was 3 fields types for a text: Text (single line) (a basic input), Text (multiple lines) (a textarea) and Rich Text (a TinyMCE editor). We may implement the Rich Text type later, as it may not be simple as the ther fields, but I think that we should implement the 2 others right now.
Deleting a field definition does not seems to work. I get a blank page.
When I submit the asset form containaing a field of each type, I get the following SQL error:

  *** MySQL query warnings:
  SQL: SELECT `glpi_computers`.*, '' AS `transname`, '' AS `transcomment` FROM `glpi_computers` WHERE `glpi_computers`.`id` = '{\"2\": \"0\", \"3\": \"abcdef\", \"4\": \"2024-09-18\", \"5\": \"0\", \"6\": \"blabla2\", \"7\": 0, \"8\": \"25\", \"9\": \"2024-09-23 08:00:10\", \"10\": \"http://babla7/**/\"}'
  Warnings: 
1292: Truncated incorrect DOUBLE value: '{"2": "0", "3": "abcdef", "4": "2024-09-18", "5": "0", "6": "blabla2", "7": 0, "8": "25", "9": "2024-09-23 08:00:10", "10": "htt'
  Backtrace :
  src/DBmysqlIterator.php:120                        
  src/DBmysql.php:1090                               DBmysqlIterator->execute()
  src/Dropdown.php:574                               DBmysql->request()
  src/Log.php:187                                    Dropdown::getDropdownName()
  src/CommonDBTM.php:683                             Log::constructHistory()
  src/CommonDBTM.php:1675                            CommonDBTM->updateInDB()
  front/asset/asset.form.php:81                      CommonDBTM->update()
  ...Glpi/Controller/LegacyFileLoadController.php:58 require()
  vendor/symfony/http-kernel/HttpKernel.php:101      Glpi\Controller\LegacyFileLoadController->Glpi\Controller\{closure}()
  ...ymfony/http-foundation/StreamedResponse.php:106 Symfony\Component\HttpKernel\HttpKernel::Symfony\Component\HttpKernel\{closure}()
  vendor/symfony/http-foundation/Response.php:423    Symfony\Component\HttpFoundation\StreamedResponse->sendContent()
  public/index.php:58                                Symfony\Component\HttpFoundation\Response->send()

This is related to the Dropdown field:

Other fields types are not logging anything in the Historical tab when they are updated.

Also, this is not a bug, but there is currently no way to disable the min/max/step on a Number field. I do not know if it is a problem, but I guess some people will complain about that. These options were not available, and therefore not mandatory in the fields plugin. People could probably use max=999999999 min=-999999999999 step=0.0001 as a workaround.

cconard96 · 2024-09-12T10:59:56Z

1. The options/default values seems to be loaded correctly on the field definition update form, but they are not loaded anyomre on the field definition creation form.

Should be fixed.

2. When a field is defined as mandatory and/or readonly, these options are also applied to the default value field. They should not.

I added a param to options to define if they apply to the default value field (true by default) and removed the special handling done previously just for the "full_width" option.

3. The default value field for a `Number` type applies the current min/max/step values. When these are updated, it may produce an unexepected behaviour:

This has been fixed.

4. The `itemtype` of a `Dropdown` field is not configurable (there is no field displayed for that).

Fixed. Check was using the old "dropdown" key instead of the class name.

5. The `Text` field seems to be similar to the `String` type, I guess it is an error. In the `fields` plugin, there was 3 fields types for a text: `Text (single line)` (a basic input), `Text (multiple lines)` (a textarea) and `Rich Text` (a TinyMCE editor). We may implement the `Rich Text` type later, as it may not be simple as the ther fields, but I think that we should implement the 2 others right now.

String - Plain-text only and limited to 255 characters
Text - Multiline/Long text. Rich text was, and I guess will be when it gets re-added, an option on this field type.

6. Deleting a field definition does not seems to work. I get a blank page.

Fixed

7. When I submit the asset form containaing a field of each type, I get the following SQL error:

I don't get this SQL error

8. Other fields types are not logging anything in the `Historical` tab when they are updated.

Not yet addressed. Also not sure why it says a dropdown was updated when the values haven't changed.

Also, this is not a bug, but there is currently no way to disable the min/max/step on a Number field. I do not know if it is a problem, but I guess some people will complain about that. These options were not available, and therefore not mandatory in the fields plugin. People could probably use max=999999999 min=-999999999999 step=0.0001 as a workaround.

Workaround added by supporting a default value for options.

cedric-anne · 2024-09-12T13:04:27Z

7. When I submit the asset form containaing a field of each type, I get the following SQL error:

I don't get this SQL error

Do you have the Historical capacity enabled on your custom asset definition ?

cedric-anne

The custom field update form does not load anymore:

cconard96 · 2024-09-13T11:10:12Z

The custom field update form does not load anymore

Test again on latest commit with a new asset definition/field definition to ensure the right ID is used in the AJAX request and the field definition is using the new "type" values.

cconard96 · 2024-09-13T11:13:36Z

7. When I submit the asset form containaing a field of each type, I get the following SQL error:
I don't get this SQL error
Do you have the Historical capacity enabled on your custom asset definition ?

Yes

cedric-anne · 2024-09-16T07:49:42Z

The custom field update form does not load anymore

Test again on latest commit with a new asset definition/field definition to ensure the right ID is used in the AJAX request and the field definition is using the new "type" values.

I still have this issue:

Payload is:

orthagh · 2024-09-17T14:44:23Z

A quick functional review:

when adding a new field, the "add a new field" button disappears (it's ok), but you can't cancel the addition without reloading the page

To avoid that, I suggest moving the form into a modal (same for the edit action), it will fix every behavior related to canceling the action
yes/no could be rendered as a slider or at least an option for a slider could be appreciated
The display order of the fields is strange, but I guess we will fix that later in the dedicated task of reordering all fields (including default ones)
full-width option -> We need to do something about that, this is a global issue for this option in GLPI. It makes fields not aligned and it's ugly.

I didn't encounter anything blocking regarding usage, no bugs, and no errors in logs. Apart from the above-mentioned UX issues, everything seems robust

cedric-anne

Tests are failing.

cconard96 · 2024-09-18T19:38:56Z

* when adding a new field, the "add a new field" button disappears (it's ok), but you can't cancel the addition without reloading the page
  > To avoid that, I suggest moving the form into a modal (same for the edit action), it will fix every behavior related to canceling the action

Yeah, this is more a global issue with the inline forms/sublist combo tabs. If the add/edit is inline when the form isn't too complex, it is good UX as opposed to using a modal. With the UI work I did on Rules, the Add button is always visible. I've removed the code in the new template I started in this PR to standardize "viewsubitem" form display that had hidden the "Add" button after clicking it.

* yes/no could be rendered as a slider or at least an option for a slider could be appreciated

Done

* full-width option -> We need to do something about that, this is a global issue for this option in GLPI. It makes fields not aligned and it's ugly.

Yeah, I've noticed and there are a ton of 'hacks' in templates that have full width fields to fix label/input sizes. Probably good to do in a separate PR.

AdrienClairembault · 2024-09-19T07:23:33Z

I can add multiple values for a dropdown where "multiple" is not enabled:

AdrienClairembault · 2024-09-19T07:25:27Z

Toggling the "mandatory" or "read only" option on a dropdown with a default value triggers a fatal error:

AdrienClairembault · 2024-09-19T07:27:51Z

On a dropfield field, I can add an invalid empty value:

If I then switch the type, I have a fatal error:

PS: to reproduce you need to have 0 monitor on your GLPI

AdrienClairembault · 2024-09-19T07:30:38Z

Dropdown with readonly + multiple values rendering seems broken:

AdrienClairembault · 2024-09-19T07:41:39Z

The mandatory "red dot" is only added for dropdown when previewing fields.
Same for readonly, the input is only made readonly for dropdowns, anything else can be edited.

AdrienClairembault · 2024-09-19T07:48:40Z

What is the purpose of the "itemtype" column ?

I thought it was for dropdowns but it doesn't show the itemtype I've used for my dropdown.

AdrienClairembault · 2024-09-19T07:53:35Z

I can't search on dates and datetimes:

AdrienClairembault · 2024-09-19T07:54:23Z

I can't search on numbers:

AdrienClairembault · 2024-09-19T07:58:54Z

I think it would be very important to add tests to confirm and fix each issues reported by testers here.
Indeed, with a complex feature like we have here, fixing something is always likely to break something else.
Having strong tests would make us more confident that everything is working as expected.

cedric-anne

Please add tests (with Cypress or PHPUnit) to verify scenarios where bugs were found during code reviews.

cedric-anne · 2024-09-19T08:03:29Z

2 other minor issues found:

It is not possible to remove the value of a Date field. When saving the form, the previous value is still present and no history entry is added.
When updating a multiple dropdown value, the history entry contains only one of the selected values.

orthagh · 2024-09-19T08:07:05Z

A statement about the last comments regarding tests.
I strongly agree, that each detected issue (some are regressions from early versions) should be tested (E2E or unit).
We need to stabilize this pr, and without tests, we will not suceed

cconard96 self-assigned this Jul 7, 2024

cconard96 marked this pull request as draft July 7, 2024 02:45

cconard96 added enhancement wip labels Jul 7, 2024

cedric-anne reviewed Jul 8, 2024

View reviewed changes

install/migrations/update_10.0.x_to_11.0.0/assets.php Outdated Show resolved Hide resolved

orthagh requested changes Jul 10, 2024

View reviewed changes

install/mysql/glpi-empty.sql Outdated Show resolved Hide resolved

cconard96 added Need unit tests and removed wip labels Jul 11, 2024

cconard96 marked this pull request as ready for review July 12, 2024 02:36

orthagh previously requested changes Jul 12, 2024

View reviewed changes

cedric-anne reviewed Jul 15, 2024

View reviewed changes

cconard96 force-pushed the feature/custom_asset_fields branch from a4ffe22 to 947786f Compare July 15, 2024 09:14

cedric-anne requested changes Jul 15, 2024

View reviewed changes

cconard96 force-pushed the feature/custom_asset_fields branch 2 times, most recently from 122f3b4 to e5c1f96 Compare July 17, 2024 09:58

cconard96 removed the Need unit tests label Jul 18, 2024

cedric-anne force-pushed the feature/custom_asset_fields branch 2 times, most recently from b51c50d to db8eb77 Compare July 31, 2024 10:38

cedric-anne mentioned this pull request Jul 31, 2024

Fix dropdown fields label when name contains [] chars #17616

Merged

cedric-anne self-requested a review July 31, 2024 14:09

AdrienClairembault reviewed Aug 1, 2024

View reviewed changes

AdrienClairembault requested changes Aug 1, 2024

View reviewed changes

ajax/asset/customfield.php Outdated Show resolved Hide resolved

front/asset/customfield.form.php Outdated Show resolved Hide resolved

cedric-anne requested changes Sep 12, 2024

View reviewed changes

cconard96 force-pushed the feature/custom_asset_fields branch from c055747 to a32441d Compare September 15, 2024 17:04

cconard96 requested a review from cedric-anne September 16, 2024 12:00

cedric-anne requested changes Sep 18, 2024

View reviewed changes

cconard96 force-pushed the feature/custom_asset_fields branch from fba2628 to 0e02679 Compare September 18, 2024 10:43

cedric-anne requested review from orthagh and cedric-anne September 19, 2024 06:59

cedric-anne requested changes Sep 19, 2024

View reviewed changes

Custom asset fields

d02f518

cconard96 force-pushed the feature/custom_asset_fields branch from 893b04b to d02f518 Compare September 20, 2024 18:27

test fixes

f479951

Custom asset fields #17462

Are you sure you want to change the base?

Custom asset fields #17462

Conversation

cconard96 commented Jul 7, 2024 • edited Loading

orthagh left a comment

Choose a reason for hiding this comment

cconard96 commented Jul 10, 2024

orthagh commented Jul 10, 2024

orthagh left a comment

Choose a reason for hiding this comment

cconard96 commented Jul 12, 2024

cconard96 commented Jul 12, 2024

cedric-anne left a comment

Choose a reason for hiding this comment

cedric-anne left a comment

Choose a reason for hiding this comment

cedric-anne commented Jul 31, 2024

cconard96 commented Jul 31, 2024

AdrienClairembault Aug 1, 2024

Choose a reason for hiding this comment

AdrienClairembault Aug 1, 2024

Choose a reason for hiding this comment

AdrienClairembault Aug 1, 2024

Choose a reason for hiding this comment

AdrienClairembault Aug 1, 2024

Choose a reason for hiding this comment

cedric-anne Aug 1, 2024

Choose a reason for hiding this comment

AdrienClairembault left a comment

Choose a reason for hiding this comment

cedric-anne commented Aug 1, 2024

cedric-anne left a comment

Choose a reason for hiding this comment

cconard96 commented Sep 12, 2024

cedric-anne commented Sep 12, 2024

cedric-anne left a comment

Choose a reason for hiding this comment

cconard96 commented Sep 13, 2024

cconard96 commented Sep 13, 2024

cedric-anne commented Sep 16, 2024

orthagh commented Sep 17, 2024

cedric-anne left a comment

Choose a reason for hiding this comment

cconard96 commented Sep 18, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

AdrienClairembault commented Sep 19, 2024

cedric-anne left a comment

Choose a reason for hiding this comment

cedric-anne commented Sep 19, 2024

orthagh commented Sep 19, 2024

cconard96 commented Jul 7, 2024 •

edited

Loading