glpi icon indicating copy to clipboard operation
glpi copied to clipboard

feat(forms): Convert default values when question type change

Open ccailly opened this issue 1 year ago • 7 comments

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

Added a mechanism for converting default values when changing question types.

ccailly avatar May 06 '24 09:05 ccailly

I am not sure if this should be done on the server. IMO, it would be better on the client as we don't really need server input on this and it slow the process.

The onQuestionTypeChange could return a short js script instead.

What do you think @cedric-anne ?

AdrienClairembault avatar May 07 '24 13:05 AdrienClairembault

I am not sure if this should be done on the server. IMO, it would be better on the client as we don't really need server input on this and it slow the process.

The onQuestionTypeChange could return a short js script instead.

What do you think @cedric-anne ?

I am not sure that this kind of logic is even required. Maybe it can automatically be done by using strict input types. For instance:

  • type="email" when an email address is expected;
  • type="number" when an integer/float is expected;
  • type="text" when a small text is expected;
  • textarea when a longer text is expected.

Switching from on type to another will probably natively invalidate/discard the value if it is not compatible.

cedric-anne avatar May 13 '24 08:05 cedric-anne

I am not sure that this kind of logic is even required. Maybe it can automatically be done by using strict input types. For instance:

* `type="email"` when an email address is expected;

* `type="number"` when an integer/float is expected;

* `type="text"` when a small text is expected;

* `textarea` when a longer text is expected.

Switching from on type to another will probably natively invalidate/discard the value if it is not compatible.

This logic follows on from @orthagh's request in the PR, which integrates actors https://github.com/glpi-project/glpi/pull/16791#pullrequestreview-1986624405. This logic could also be relevant when switching between checkbox and radio questions.

In this PR, a first example has been made with short answers, which seem a little less relevant than actors or selectables.

ccailly avatar May 13 '24 08:05 ccailly

Having a unique PHP endpoint is probably easier to handle. Indeed, I am afraid that some cases may not be handled on JS side only, and would require to create specific ajax endpoints. It is preferable to have a unique ajax endpoint that forwards to a PHP logic normalized by an interface instead of having multiple specific ajax endpoints added later in GLPI or in plugins.

cedric-anne avatar May 13 '24 08:05 cedric-anne

I am still not sold on the backend part. Right now, every action run directly on the front end without any delay. Adding a backend call here will cause a delay on one of the most common action.

Each question could define two javascript method:

1) extractValue Return the computed value of the question. For a simple text question, it can be the raw text. For a checkbox question, it can be an array of the available options. For complex ajax-backed dropdown like the actor question type, it could return null as the value is too complex to be shared with other questions.

2) initializeFromValue Initialize the new type from the given value (which would be retrieved by extractValue). For a simple text question, we accept any string values. For a checkbox question, we accept any array of strings. For a question too complex to be initialized from something else we don't accept anything.

I think this way it can be handled with 0 backend interference. The goal is to support a few type changes between similar and compatible types (most importantly, the switch from checkbox <-> dropdown <-> radio as it would be painful for the user to re-type all options), not ALL possibles types changes.

What do you think @cedric-anne ?

AdrienClairembault avatar May 23 '24 08:05 AdrienClairembault

OK to do it on JS side, using JS callbacks defined by the QuestionType PHP classes, and defined by the interface.

A way to do it could be to have a public function getFormEditorJsOptions(): string method, that would return, for instance

return <<<JS
    {
        "extractDefaultValue": function (input_name) { return null; },
        "convertDefaultValue": function (input_name, value) { return value; },
        ...
    }
JS;

and it would be registered on form initialization this way

{% for question_type in question types %}
    glpi_form_editor_controller.registerQuestionTypeOptions('{{ get_class(question_type)|e('js') }}, {{ question_type.getFormEditorJsOptions() }})
{% endfor %}

It is probably the most flexible way to permit to each question type to register some specific behaviours on Js side, but it would require to be as much documented as possible.

cedric-anne avatar May 23 '24 08:05 cedric-anne

OK to do it on JS side, using JS callbacks defined by the QuestionType PHP classes, and defined by the interface.

A way to do it could be to have a public function getFormEditorJsOptions(): string method, that would return, for instance

return <<<JS
    {
        "extractDefaultValue": function (input_name) { return null; },
        "convertDefaultValue": function (input_name, value) { return value; },
        ...
    }
JS;

and it would be registered on form initialization this way

{% for question_type in question types %}
    glpi_form_editor_controller.registerQuestionTypeOptions('{{ get_class(question_type)|e('js') }}, {{ question_type.getFormEditorJsOptions() }})
{% endfor %}

It is probably the most flexible way to permit to each question type to register some specific behaviours on Js side, but it would require to be as much documented as possible.

For GLPI native question types, the JS options may probably be placed in some dedicated JS files and the PHP code may be something like that (to be tested):

public function getFormEditorJsOptions(): string
{
    return 'await import("/path/to/question/defaults.js");';
}

cedric-anne avatar May 23 '24 08:05 cedric-anne

Will need a rebase for #17483

AdrienClairembault avatar Jul 10 '24 13:07 AdrienClairembault