Prevents canceling index pattern modal from throwing error

[tylersmalley]

We provide a modal on index pattern creation when a matching pattern exists. This allows the user to either cancel the modal window or edit the existing pattern.

Previously an exception was being bubbled up when they clicked cancel.

Fixes #12818

[cjcenizal]

Nice! I only found a couple minor problems.

Tangentially, I noticed that if you click the "Edit existing pattern" button, then there's a full refresh. I'm pretty sure this is an error, because we're still in the same app. I wonder if this is something to do with kbnUrl? I created #13489 to track this.

[cjcenizal]

Is there a reason to return a Promise rejection here or on line 372? If I'm following this logic correctly, we just want to tell the call site of this function "Hey, stop what you're doing, because you can't actually create an index pattern."

In which case, the Promise rejection has nothing to do with the confirm modal, and should probably happen outside of it:

          confirmModalPromise(confirmMessage, { confirmButtonText: 'Edit existing pattern' })
            .then(() => {
              kbnUrl.change('/management/kibana/indices/{{id}}', { id: duplicate.id });
            });

          // Abort index pattern creation.
          return Promise.reject();

[cjcenizal]

Unfortunately (and ironically?) the decision to use Promise rejections as the early exit mechanism means that we're once again dealing with error states. This rejection still causes an error state to be triggered, except the error itself is null, resulting in a TypeError in create_index_pattern.js:

The least invasive solution is to probably change that file on line 227, to check for the presence of an error before doing anything with it:

      if (err) {
        notify.fatal(err);
      }

[tylersmalley]

Thanks @cjcenizal, mind taking another look? A couple things:

• I changed kbnUrl.change to kbnUrl.redirect and it no longer refreshes. Not sure if that is the correct route, but it accomplishes the desired behavior.
• Instead of handling the cancellation as a promise, I am returning a boolean to indicate the presence of a duplicate.

[tylersmalley]

Most recent change should fix #13489

[cjcenizal]

Awesome!! This UX is so much better now.

This code LGTM but I have one suggestion that you might find worth implementing. Returning true twice in the promise handlers looks like a mistake, though it isn't.

Because the rejection/acceptance of this promise has no effect on the return value, perhaps it would be clearest to just move the return value out of it?

confirmModalPromise(confirmMessage, { confirmButtonText: 'Edit existing pattern' })
  .then(() => {
    kbnUrl.redirect('/management/kibana/indices/{{id}}', { id: duplicate.id });
  });

return true;

Or if you still want to respect the asynchronous nature of the modal, could we move it into a finally handler?

confirmModalPromise(confirmMessage, { confirmButtonText: 'Edit existing pattern' })
  .then(() => {
    kbnUrl.redirect('/management/kibana/indices/{{id}}', { id: duplicate.id });
  }).finally(() => {
    return true;
  });

[tylersmalley]

@cjcenizal the return needs to be asynchronous, as create respects the return value to determine if it's a duplicate and exits early. If we use finally it would not resolve until after then then in create resulting in the same issue.

[tylersmalley]

6.x/6.1: ff40688
6.0: f08a511
5.6: 47c3854