Inconsistent use of EnumerationField

[MatteoPiovanelli-Laser]

ref: #8298 (@GiuseppeMusso-Laser @HermesSbicego-Laser @sebastienros )

I think there is an issue in how we are using/editing the values in EnumerationField.

The possible values that a user can select are configured in EnumerationFieldSettings.Options: this property is a string that contains all the possible options for the EnumerationField, one per line. For example:

foo
bar
qwe
foobar

Then, depending on the selection of EnumerationFieldSettings.ListMode, the editor displays differently. See

Orchard/src/Orchard.Web/Modules/Orchard.Fields/Views/EditorTemplates/Fields/Enumeration.Edit.cshtml

Line 9 in ca86ec9

@switch (settings.ListMode) {

The result of this is that if ListMode is Dropdown or RadioButton the driver will set the Value property of the EnumerationField directly. On the other hand, if ListMode is ListBox or Checkbox the driver will set the SelectedValues property. Note that neither case adds a form element for the property it's not setting, which would then be bound to null.
As was pointed out in the PR linked above, this results in Value having different values depending on which is the configuration of the Field.
Supposing I want to select the value foo, with the first two settings I will have that the values saved in the record is foo, while with the other two settings the value in the record will be ;foo;.
If, from the second case, we changed ListMode to either Dropdown or RadioButton, we would not see the value of foo selected in the form, because the property Value has those semicolons. On save/update then, Value would be unset (assuming validation allows it).

In #8298 we propose to remove those surrounding semicolons; @sebastienros pointed out that having those allows the better identify the values in SQL queries.

I agree with @sebastienros that those semicolons help somewhat in contains SQL queries.

The "real" issue I see is that the EnumerationField currently has two incompatible ways it's being used. One (for example when ListMode is Dropdown or RadioButton) where Value is set and used directly; the other (for example when ListMode is ListBox or Checkbox) where we go through SelectedValues.
While the latter works with data as saved by the former, the reverse is not true.

Changing the setter method as was proposed in the PR would mitigate the issues we see when we have set SelectedValues and are trying to show an editor where ListMode is Dropdown or RadioButton, as long as we had only set a single value.
On the other hand, changing that setter method for SelectedValues would "break" existing queries that depend on the current formatting of the string.

Another possible mitigation would be having this code in the GET Editor method of the EnumerationFieldDriver:

  if (settings.ListMode == ListMode.Dropdown
    || settings.ListMode == ListMode.Radiobutton) {
    if (field.SelectedValues.Any()) {
      field.Value = field.SelectedValues.First();
     }
  }

(that basically reads as "if we should only have selected at most a value, and we have at least one, that is the value") as well as:

  if (settings.ListMode == ListMode.Dropdown
    || settings.ListMode == ListMode.Radiobutton) {
    field.SelectedValues = new string[] { field.Value };
  }

in the POST Editor method to ensure saving is also correct.
Note that the latter is not strictly required for the drivers, but it ensures that the Value in the records is saved consistently. However this latter snippet breaks a scenario where you had been using an EnumerationField with ListMode as Dropdown or RadioButton (so the Value was saved without semicolons) and had a query with an Equals rather than a Contains (because the EnumerationField was set to have a single value anyway).

[MatteoPiovanelli-Laser]

Something that would help would be to change the get and set methods for the properties of EnumerationField like this:

    public string Value {
            get { return Storage.Get<string>().Trim(new char[] { Separator }); }
            set {
                Storage.Set(string.IsNullOrEmpty(value)
                    ? string.Empty
                    // we add the Separator around the value here
                    : Separator + value.Trim(new char[] { Separator }) + Separator);
            }
        }

        public string[] SelectedValues {
            get {
                var value = Value;
                if(string.IsNullOrWhiteSpace(value)) {
                    return new string[0];
                }

                return value.Split(new [] { Separator }, StringSplitOptions.RemoveEmptyEntries);
            }

            set {
                if (value == null || value.Length == 0) {
                    Value = String.Empty;
                }
                else {
                    // we don't need to add the Separator around the string here anymore
                    Value = string.Join(Separator.ToString(), value);
                }
            }
        }

With this we would always be reading/writing the records consistenly.

That still leaves the issue with trying to open the Edit view with ListMode as Dropdown or RadioButton while having selected more than one option.

[sebastienros]

Do you changes keep the ; in the database?

[MatteoPiovanelli-Laser]

I'm positive it should, but I haven't had a chance to implement them and try.

[BenedekFarkas]

Fixed by #8789.