[indexPatterns] remove support for time patterns - #12158

Can we remove isNoMatchingIndicesError from lib/errors.js?

The function was used more in the past, but I didn't remove it because it's used to ensure that convertEsIndexNotFoundError() is producing the right errors. Open to other solutions there and removing this if you have ideas. I would just much rather export isNoMatchingIndicesError than the ERR_NO_MATCHING_INDICES constant.

Sorry, can you point me to where it's used? I grepped src for "isNoMatchingIndicesError" and didn't see any hits.

Yep, it's not in src unfortunately

I don't think we should tightly couple application logic (whether things should be displayed) with view logic (how things are displayed). In the vein, I think it makes sense to uncouple this module from notify and kbnUrl. Then the caller would have the option of surfacing the message via notifications, inside of an InfoPanel, or in some other part of the UI, depending on what makes the most sense in the given context.

From there, we probably don't need to dictate the precise notification message. The caller should be responsible for determining how to communicate information to the user, again based on the context. At that point, we're left with what I see as the main concern of this module, which is determining whether the message should be displayed at all, based on our flag and persistence rules. This will simplify the code and the unit tests significantly.

So I'd rename this module to be is_unsupported_time_pattern and have it return true or false. What do you think?

I definitely agree about separating concerns, but indexPattern.isUnsupportedTimePattern() already exists and is responsible for determining if this logic should execute as well as if we should show the warning on the edit page.

This module used to be implemented in the caller, and the caller was responsible for determining if the unsupported index pattern should trigger a warning, but in an effort to make it more testable I pulled it out into a module that is purely responsible for determining if we should call notify.warning() or not.

All that said, I'm not fond of how much stubbing is necessary to test this module and would be interested in other ways you think this could be broken up to make that less necessary. At the end of the day though, I'm pretty sure we are going to have to stub the browser storage if we're going to test how this or any other module interacts with session storage, and we're going to have to stub notify for the same reason... Open to other suggestions though.

Maybe we just need a different name. What do you think of my idea of making this module's single responsibility to be to check our flag and persistence rules? Then we'd end up with something like this:

import { BoundToConfigObjProvider } from 'ui/bound_to_config_obj';

export function IsUserAwareOfUnsupportedTimePatternProvider(Private, $injector) {
  const BoundToConfigObj = Private(BoundToConfigObjProvider);
  const sessionStorage = $injector.get('sessionStorage');

  const HISTORY_STORAGE_KEY = 'indexPatterns:warnAboutUnsupportedTimePatterns:history';
  const FLAGS = new BoundToConfigObj({
    enabled: '=indexPatterns:warnAboutUnsupportedTimePatterns'
  });

  return function isUserAwareOfUnsupportedTimePattern(indexPattern) {
    // The user's disabled the notification. They know about it.
    if (!FLAGS.enabled) {
      return true;
    }

    // We've already told the user.
    const previousIds = sessionStorage.get(HISTORY_STORAGE_KEY) || [];
    if (previousIds.includes(indexPattern.id)) {
      return true;
    }

    // Let's store this for later, so we don't tell the user multiple times.
    sessionStorage.set(HISTORY_STORAGE_KEY, [...previousIds, indexPattern.id]);
    return false;
  };
}

And in index_pattern.js, we'd use it like this:

    if (indexPattern.isUnsupportedTimePattern()) {
      if (!isUserAwareOfUnsupportedTimePattern(indexPattern)) {
        notify.warning(
          'Support for time-intervals has been removed. ' +
          `View the ["${indexPattern.id}" index pattern in management](` +
          kbnUrl.getRouteHref(indexPattern, 'edit') +
          ') for more information.'
        );
      }
    }

Love the suggestion, thanks @cjcenizal