[CCR] Show remote cluster validation in CCR forms, and add edit follower index

[sebelga]

This PR adds the remote cluster validation inside the auto-follow pattern form and the follower index form. It also adds edit follower index functionality via adding an action in the context menu of the table list.

For now, ES does not provide an API to read the advanced settings of a follower index, but they are working on one and we'll be able to populate the fields with the values.

Remote cluster validation

The different use cases to tests are:

- Delete all remote clusters
- Create an auto-follow pattern ---> error and invite user to add a cluster
- Create a follower index --->  error and invite user to add a cluster

- Create a remote cluster, but put a wrong host (disconnected)
- Create an auto-follow pattern ---> error
- Create a follower index ---> error

- Create a remote cluster (connected)
- Create an auto-follow pattern with that cluster
- Delete the remote cluster
- Edit the previous auto-follow pattern ---> error

- Same as above with follower index

- Create a remote cluster (connected)
- Create an auto-follow pattern with that cluster
- Edit the remote cluster with wrong host (disconnected)
- Edit the previous auto-follow pattern ---> error

- Same as above with follower index.

Edit follower index

---

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

Pinging @elastic/es-ui

[cjcenizal]

Functionally this looks great! Tested locally and it works as expected. My comments consist of UX improvements and some code organization suggestions. I also found a couple places where text needs to be localized.

[cjcenizal]

It might make sense to call getRemoteClusterName here and pass in the remote cluster instead of these two arguments, since that's really what getEmptyAutoFollowPattern is concerned with.

const getEmptyAutoFollowPattern = (remoteCluster) => ({
  name: '',
  remoteCluster,
  leaderIndexPatterns: [],
  followIndexPatternPrefix: '',
  followIndexPatternSuffix: '',
});

/* ... */

const remoteCluster = getRemoteClusterName(this.props.remoteClusters, queryParams.cluster);
const autoFollowPattern = isNew
  ? getEmptyAutoFollowPattern(remoteCluster)
  : {
    ...this.props.autoFollowPattern,
  };

[cjcenizal]

This looks like a dupe from auto_follow_pattern_form.js. Can we dedupe this by extracting it into a service? Possibly along with getFirstConnectedCluster?

[cjcenizal]

Same suggestion here, about calling getRemoteClusterName and passing the cluster in here instead of making getEmptyFollowerIndex call it.

[cjcenizal]

This needs to be localized.

[cjcenizal]

I think this needs to be localized.

[sebelga]

Actually it does not as it is never shown on the screen 😊

[cjcenizal]

Is the message necessary then?

[cjcenizal]

We can simplify this by binding name to a local variable here, instead of requiring the consumer to provide it. This will also make it clearer where name is coming from. This idea can also be applied to similar logic inatuo_follow_pattern_form.js.

const renderRemoteClusterField = () => {
  const { remoteClusters, currentUrl } = this.props;

  const selectedRemoteCluster = followerIndex.remoteCluster ? followerIndex.remoteCluster : null;

  const errorMessages = {
    noClusterFound: (
      <FormattedMessage
        id="xpack.crossClusterReplication.followerIndexForm.emptyRemoteClustersCallOutDescription"
        defaultMessage="Follower indices replicate indices on remote clusters. You must add a remote cluster."
      />
    ),
    remoteClusterNotConnectedNotEditable: (
      <FormattedMessage
        id="xpack.crossClusterReplication.followerIndexForm.currentRemoteClusterNotConnectedCallOutDescription"
        defaultMessage="The remote cluster '{name}' is not connected.
        You need to connect it before editing the follower index."
        values={{ name: selectedRemoteCluster }}
      />
    ),
    remoteClusterDoesNotExist: (
      <FormattedMessage
        id="xpack.crossClusterReplication.followerIndexForm.currentRemoteClusterNotFoundCallOutDescription"
        defaultMessage="The remote cluster '{name}' was not found. It might have been removed.
        In order to edit the follower index, you need to add a remote cluster with the same name."
        values={{ name: selectedRemoteCluster }}
      />
    ),
  };

  return (
    <EuiDescribedFormGroup
      title={(
        <EuiTitle size="s">
          <h4>
            <FormattedMessage
              id="xpack.crossClusterReplication.followerIndexForm.sectionRemoteClusterTitle"
              defaultMessage="Remote cluster"
            />
          </h4>
        </EuiTitle>
      )}
      description={(
        <FormattedMessage
          id="xpack.crossClusterReplication.followerIndexForm.sectionRemoteClusterDescription"
          defaultMessage="The remote cluster to replicate your leader index from."
        />
      )}
      fullWidth
    >
      <RemoteClustersFormField
        selected={selectedRemoteCluster}
        remoteClusters={remoteClusters || []}
        currentUrl={currentUrl}
        isEditable={isNew}
        areErrorsVisible={areErrorsVisible}
        onChange={this.onClusterChange}
        onError={(error) => this.onFieldsErrorChange({ remoteCluster: error })}
        errorMessages={errorMessages}
      />
    </EuiDescribedFormGroup>
  );
};

[jen-huang]

after pulling out name into the callout titles instead of description, this is no longer needed 😄

[cjcenizal]

Not really relevant to this PR, but I do find this pattern of declaring local renderSomething() functions unusual. Everywhere else I've seen them defined as component instance methods.

[sebelga]

It helps by grouping functions by behavior. Instead of heaving loads of functions declared at the component level. I like to toggle all first level methods when I open a component I never worked on and see the moving parts. If renderForm() needs 5 functions to make it easier to reason about, I don't really need that information when I first open a component file.

[cjcenizal]

We could define hasClusters on this line since this variable is only used by this branch.

[sebelga]

This is my code style 😊 I read better in if statements what if being compared. Even if that means declaring a variable for a 1-time use.

[cjcenizal]

++ definitely! My suggestion was just to move the variable inside of the first condition, because when it's outside of the condition it misleadingly appears like the condition depends upon it.

[cjcenizal]

I think we can improve the UI by rendering the error as part of the dropdown. In the screenshot below I've made these changes:

1. Put the callout below the dropdown, but above the "Add" button. I think this hierarchy makes sense because the callout pertains to the selected remote cluster, but not to the button.
2. Simplified the callout copy and moved some body text into the title.
3. Changed the callout button to "Edit remote cluster". I think it makes more sense to prompt the user to edit the selected remote cluster than to view all remote clusters in this case.
4. Change the "Add" button text to "Add new remote cluster", to make it clearer that you're adding a new remote cluster, and not somehow adding the one selected in the dropdown.
5. Make the button flush left to align it with the dropdown and callout, and added an icon.

[cjcenizal]

Instead of prompting the user to view the remote clusters, wouldn't it make more sense to prompt them to add a new one? I think we could also simplify the copy by moving some of the body text into the title.

[sebelga]

Maybe the user does not understand why suddenly the cluster does not exist anymore. This is why I thought to leave him the choice to view his clusters.

[cjcenizal]

Gotcha -- but how would the user use the list of existing clusters to figure out why a cluster no longer exists?

[jen-huang]

@cjcenizal I've addressed most of your feedback!

The remote cluster error callout is now displayed between dropdown and Add cluster link. For clusters that are not in a connected state, a link to Edit cluster is provided:

Previously, unconnected clusters were not available to select in the dropdown (unless it happens to be the first one in the dropdown). With this change, I think it is safe to allow the user to select them since it gives them a convenient link to edit the cluster. The edit cluster form was also adjusted to accept a redirect value, mimicking the existing behavior of the add cluster form. Both form's Cancel option now takes redirect into account as well.

The issue reported #28874 is now fixed as well, since the new edit cluster flow exposed it pretty badly. I also did some misc. cleanup - unused code, making forms equal width, etc.

[snide]

Checked over the sass releated bits. LGTM

[jen-huang]

@cjcenizal This is ready for another pass with the edit functionality! For editing, advanced settings will always be shown and there is no toggle button:

I have also addressed most of your feedback from #28600.

[cjcenizal]

Found a couple lingering console.log calls, had a couple other comments, and I did find one UX issue which we could address separately if you want.

When the remote cluster doesn't exist, it's really tricky to spot the error field because only the label is read. What do you think of rendering the select, but in a disabled state, with either no option selected or a placeholder option that says "No remote cluster selected"?

[cjcenizal]

Leftover console.log.

[cjcenizal]

Another leftover.

[cjcenizal]

I deleted this file in the advanced settings PR, so it shouldn't be needed any more.

[cjcenizal]

This should also not be needed.

[cjcenizal]

Another leftover.

[cjcenizal]

What's this used for? If you user updates the URL to be a different ID? If so, could we add a comment to explain the intention?

[jen-huang]

I believe this is to fetch follower index data if we don't already have it in the store, i.e. during a page reload on the edit screen. I've updated the comment here and also in the equivalent method of auto follow pattern edit

[sebelga]

Yes this is when we directly access the page without going first through the list of followers

[cjcenizal]

Maybe this status is something we should calculate during deserialization? That way our logic for defining the index status lives in one place.

[jen-huang]

this will be removed once the new list api is integrated! (that will give us a separate status field natively)

[jen-huang]

Thanks for the re-review @cjcenizal! I like the suggestion of having a disabled select for no remote clusters. It also simplifies the rendering logic. Updated UI:

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[jen-huang]

@cjcenizal I implemented the changes we discussed. I had to change EuiSuperSelect to EuiSelect in order to present the red underline. EuiSuperSelect doesn't appear to know about isInvalid 😦

Note that the disabled state for editing remote cluster also doesn't have red underline styling, but I changed the callouts in those case to red as well to represent a fatal state.

No remote clusters:

Editable unconnected cluster:

Not found cluster:

Uneditable unconnected cluster:

[cjcenizal]

👏 Nice!!!! That looks so good. Great work identifying all of the various states this crazy field can get into... I can't believe there are so many. I filed an EUI issue for the missing isInvalid state on the super select (elastic/eui#1460).

[cjcenizal]

👌

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[jen-huang]

retest

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request