simplifying logic for showing extend trial and adjusting tests

[bmcconaghy]

Closes #19741

This change modifies the logic for showing the extend trial pane in license management to the following:
License is not platinum or is expired AND trial has already been used.

[jen-huang]

Code LGTM

[cjcenizal]

I have a few suggestions. If we can address my feedback about the snapshots then I think I would be able to verify the logic we're testing more easily.

[cjcenizal]

Can we use early exiting here to untangle these conditions?

export const shouldShowRequestTrialExtension = state => {
  if (state.trialStatus.canStartTrial) {
    return false;
  }

  const { type } = getLicense(state);

  return type !== 'platinum' || isExpired(state);
}

[cjcenizal]

Can we rephrase this to "is not active" to keep the terminology consistent?

[cjcenizal]

Likewise can we change this to "is active"?

[cjcenizal]

These snapshot are tough to compare against one another and to see the changes in a diff because they're on one line each. Can we either a) switch to using Enzyme to generate the snapshot or b) use data-test-subj selectors to assert the presence of this component? I'm strongly in favor of the latter if we don't expect the UI to meaningfully change from snapshot to snapshot.

[bmcconaghy]

I'm using enzyme to generate the snapshot. I'm using .html to strip out a lot of junk from the snapshot and that is what is squashing down to a single line.
Someone opened an issue about this but it got closed: enzymejs/enzyme#649

[bmcconaghy]

Also this seems like something outside of this PR.

[cjcenizal]

Oh, I see. BTW there's a helper in EUI which you can use to create a snapshot from a mounted component (example, corresponding snapshot). The snapshot will have whitespace preserved.

[cjcenizal]

This is probably outside the scope of this PR, but I was having a hard time tracing the relationships between these functions and seeing what kind of data they're interested in because they all accept a state object. I think we could make this code more transparent by changing them to accept more granular arguments, e.g. license, licenseType, uploadStatus.

[bmcconaghy]

they are all selectors and thus operate on entire state object.

[cjcenizal]

You're right, though I still think there's plenty of room for improving the readability of this code while preserving its identify as a collection of selectors. For example, getExpirationDate and isImminentExpiration are only used internally, so they don't need to be coupled to the state object:

export const getExpirationDate = license => {
  const { type, expiryDateInMillis } = license;
  //basic licenses do not expire
  if (type === 'basic') {
    return null;
  }

  if (expirationMillis) {
    return moment.tz(expirationMillis, moment.tz.guess());
  } else {
    return null;
  }
};

export const isImminentExpiration = license => {
  const now = new Date();
  const expirationDate = getExpirationDate(license);
  return (
    expirationDate &&
    expirationDate.isAfter(now) &&
    expirationDate.diff(now, 'days') <= WARNING_THRESHOLD_IN_DAYS
  );
};

[cjcenizal]

TypeScript will also help a lot here.

[bmcconaghy]

Seems like the same effect could be achieved with decomposition in those methods:
export const isImminentExpiration = ({ license }) => { ...

At any rate, this is all beyond the scope of this simple logic shift PR I think.

[cjcenizal]

LGTM

[elasticmachine]

💚 Build Succeeded

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

[elasticmachine]

💚 Build Succeeded

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