Do not remove selection when clicking refresh fields

[thomasneirynck]

for #8300.

Instead of clearing out the combobox, fill it in with default option.

[tylersmalley]

What are your thoughts on simply updating the updateFieldList() function as to not introduce another conditional path?

  function updateFieldList(results) {
    index.fetchFieldsError = results.fetchFieldsError;
    index.dateFields = results.dateFields;

    if (results.dateFields.length && index.timeField) {
      const matchingField = results.dateFields.find(field => field.name === index.timeField.name);

      index.timeField = matchingField ? matchingField : results.dateFields[0];
    }
  }

[thomasneirynck]

@tylersmalley

mostly because of this:

• the response for the fields come in asynchronously: the refresh clears outs index.timeField before fetching the data (https://github.com/thomasneirynck/kibana/blob/fix/8300/src/core_plugins/kibana/public/management/sections/indices/_create.js#L211). So in the above, index.timeField is always empty.
• that conditional would not put in a default selection if there was no previous selection (so it wouldn't put in the first element. by doing so, you show pressing the button has an immediate effect).

There's also the fact that by moving the conditional higher up, the autocomplete is tied to actually clicking the "refresh fields" button. Otherwise, it would also occur on index-changes, where I'm not sure if its necessary.

[cjcenizal]

1 minor nit: I haven't generally seen function bodies with whitespace padding in the codebase, and I know the AirBnB style-guide complains about it by default. It's up to you, but it might be a little more consistent/future-proof to remove the padding.

[cjcenizal]

Also, I think we should tweak the names here to reinforce the intent of seeding the date field. As it is, it isn't clear that this function is specific to date fields unless you read its implementation and get to line 265.

I think we can make the logic a little clearer by renaming matchingField to something which reflects its role as a flag, because we don't really want to set it as the timeField value, we want to set the value passed into the function.

  function updateFieldListAndSetTimeField(results, timeField) {
    updateFieldList(results);

    if (!results.dateFields.length) {
      return;
    }

    const defaultTimeField = results.dateFields[0];
    const isTimeFieldAvailable = results.dateFields.find(field => field.name === timeField.name);
    index.timeField = isTimeFieldAvailable ? timeField : defaultTimeField;
  }

Lastly, how about combining this with updateFieldList and using timeField as an optional argument? Then you won't need a condition inside of refreshFieldList:

  function updateFieldList(results, timeField = undefined) {
    index.fetchFieldsError = results.fetchFieldsError;
    index.dateFields = results.dateFields;

    if (!timeField) {
      return;
    }

    if (!results.dateFields.length) {
      return;
    }

    const defaultTimeField = results.dateFields[0];
    const isTimeFieldAvailable = results.dateFields.find(field => field.name === timeField.name);
    index.timeField = isTimeFieldAvailable ? timeField : defaultTimeField;
  }

[cjcenizal]

Awesome work! Just had a couple small suggestions about code clarity.

[thomasneirynck]

@cjcenizal thanks for the tips. I made your changes to improve readability.

• I did keep the two named functions, I'm not a fan of function overloading (it doesn't improve clarity imho, and it's bad habit, especially in JS where a compiler can't deduplicate the function). I also think we don't need that code path, when we are changing indices.
• Also, the angular creates new timefield instances each refresh, so I don't want to carry the old one in the new state. hence the small deviation from your suggestion at the end.

@tylersmalley can you both do a quick check again? thx!

[tylersmalley]

Fair enough, LGTM.

[cjcenizal]

seedField seems a little deceptive, since we're always passing in a reference to a timeField. So I think that naming it timeField will make the code a little easier to follow.

[cjcenizal]

Nice, I just had a couple questions... sorry for being so nit picky!

[cjcenizal]

LGTM if you just want to merge already. :)