fix: Adding and removing annotations (#11811)

* Fix creating new annotation layer and refactor AnnotationLayer component * Refactor checking overrides keys in AnnotationLayer * Fix adding and editing annotations. Refactor AnnotationLayer submit and remove functions * Remove unused parent prop from AnnotationLayer * Fix toggling annotation layer popover * Revert extracting annotation props to nested object * Clean up AnnotationLayer component * Remove unnecessary import * Use original name for identifying annotations in add/remove actions * Change props order and rename removeAnnotation function * Compare annotations by reference instead of name
2026-04-19 08:04:53 +00:00 · 2020-11-28 09:59:16 +01:00
parent f121107bbe
commit 43c69d52a4
2 changed files with 78 additions and 62 deletions
--- a/superset-frontend/src/explore/components/controls/AnnotationLayer.jsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayer.jsx
@@ -120,8 +120,8 @@ export default class AnnotationLayer extends React.PureComponent {
      intervalEndColumn,
    } = props;

-    const overridesKeys = Object.keys(overrides);
-    if (overridesKeys.includes('since') || overridesKeys.includes('until')) {
+    // Only allow override whole time_range
+    if ('since' in overrides || 'until' in overrides) {
      overrides.time_range = null;
      delete overrides.since;
      delete overrides.until;
@@ -130,7 +130,6 @@ export default class AnnotationLayer extends React.PureComponent {
    this.state = {
      // base
      name,
-      oldName: !this.props.name ? null : name,
      annotationType,
      sourceType,
      value,
@@ -149,10 +148,9 @@ export default class AnnotationLayer extends React.PureComponent {
      showMarkers,
      hideLine,
      // refData
-      isNew: !this.props.name,
+      isNew: !name,
      isLoadingOptions: true,
      valueOptions: [],
-      validationErrors: {},
    };
    this.submitAnnotation = this.submitAnnotation.bind(this);
    this.deleteAnnotation = this.deleteAnnotation.bind(this);
@@ -237,7 +235,6 @@ export default class AnnotationLayer extends React.PureComponent {
    this.setState({
      annotationType,
      sourceType: null,
-      validationErrors: {},
      value: null,
    });
  }
@@ -246,12 +243,7 @@ export default class AnnotationLayer extends React.PureComponent {
    const { sourceType: prevSourceType } = this.state;

    if (prevSourceType !== sourceType) {
-      this.setState({
-        sourceType,
-        isLoadingOptions: true,
-        validationErrors: {},
-        value: null,
-      });
+      this.setState({ sourceType, value: null, isLoadingOptions: true });
    }
  }

@@ -267,7 +259,7 @@ export default class AnnotationLayer extends React.PureComponent {
  }

  fetchOptions(annotationType, sourceType, isLoadingOptions) {
-    if (isLoadingOptions === true) {
+    if (isLoadingOptions) {
      if (sourceType === ANNOTATION_SOURCE_TYPES.NATIVE) {
        SupersetClient.get({
          endpoint: '/annotationlayermodelview/api/read?',
@@ -310,28 +302,43 @@ export default class AnnotationLayer extends React.PureComponent {
  }

  deleteAnnotation() {
+    this.props.removeAnnotationLayer();
    this.props.close();
-    if (!this.state.isNew) {
-      this.props.removeAnnotationLayer(this.state);
-    }
  }

  applyAnnotation() {
-    if (this.state.name.length) {
-      const annotation = {};
-      Object.keys(this.state).forEach(k => {
-        if (this.state[k] !== null) {
-          annotation[k] = this.state[k];
+    if (this.isValidForm()) {
+      const annotationFields = [
+        'name',
+        'annotationType',
+        'sourceType',
+        'color',
+        'opacity',
+        'style',
+        'width',
+        'showMarkers',
+        'hideLine',
+        'value',
+        'overrides',
+        'show',
+        'titleColumn',
+        'descriptionColumns',
+        'timeColumn',
+        'intervalEndColumn',
+      ];
+      const newAnnotation = {};
+      annotationFields.forEach(field => {
+        if (this.state[field] !== null) {
+          newAnnotation[field] = this.state[field];
        }
      });
-      delete annotation.isNew;
-      delete annotation.valueOptions;
-      delete annotation.isLoadingOptions;
-      delete annotation.validationErrors;
-      annotation.color =
-        annotation.color === AUTOMATIC_COLOR ? null : annotation.color;
-      this.props.addAnnotationLayer(annotation);
-      this.setState(prevState => ({ isNew: false, oldName: prevState.name }));
+
+      if (newAnnotation.color === AUTOMATIC_COLOR) {
+        newAnnotation.color = null;
+      }
+
+      this.props.addAnnotationLayer(newAnnotation);
+      this.setState({ isNew: false });
    }
  }

@@ -363,7 +370,7 @@ export default class AnnotationLayer extends React.PureComponent {
        label = 'Annotation Layer';
        description = 'Select the Annotation Layer you would like to use.';
      } else {
-        label = label = t('Chart');
+        label = t('Chart');
        description = `Use a pre defined Superset Chart as a source for annotations and overlays.
        your chart must be one of these visualization types:
        [${this.getSupportedSourceTypes(annotationType)
@@ -502,7 +509,7 @@ export default class AnnotationLayer extends React.PureComponent {
                label="Override time range"
                description={`This controls whether the "time_range" field from the current
                  view should be passed down to the chart containing the annotation data.`}
-                value={!!Object.keys(overrides).find(x => x === 'time_range')}
+                value={'time_range' in overrides}
                onChange={v => {
                  delete overrides.time_range;
                  if (v) {
@@ -520,9 +527,7 @@ export default class AnnotationLayer extends React.PureComponent {
                label="Override time grain"
                description={`This controls whether the time grain field from the current
                  view should be passed down to the chart containing the annotation data.`}
-                value={
-                  !!Object.keys(overrides).find(x => x === 'time_grain_sqla')
-                }
+                value={'time_grain_sqla' in overrides}
                onChange={v => {
                  delete overrides.time_grain_sqla;
                  delete overrides.granularity;
@@ -710,7 +715,7 @@ export default class AnnotationLayer extends React.PureComponent {
                value={annotationType}
                onChange={this.handleAnnotationType}
              />
-              {!!supportedSourceTypes.length && (
+              {supportedSourceTypes.length > 0 && (
                <SelectControl
                  hovered
                  description="Choose the source of your annotations"
@@ -728,9 +733,15 @@ export default class AnnotationLayer extends React.PureComponent {
          {this.renderDisplayConfiguration()}
        </div>
        <div style={{ display: 'flex', justifyContent: 'space-between' }}>
-          <Button buttonSize="sm" onClick={this.deleteAnnotation}>
-            {!isNew ? t('Remove') : t('Cancel')}
-          </Button>
+          {isNew ? (
+            <Button buttonSize="sm" onClick={() => this.props.close()}>
+              {t('Cancel')}
+            </Button>
+          ) : (
+            <Button buttonSize="sm" onClick={this.deleteAnnotation}>
+              {t('Remove')}
+            </Button>
+          )}
          <div>
            <Button
              buttonSize="sm"
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl.jsx
@@ -58,7 +58,7 @@ const defaultProps = {
 class AnnotationLayerControl extends React.PureComponent {
  constructor(props) {
    super(props);
-    this.state = { popoverVisible: false };
+    this.state = { popoverVisible: {}, addedAnnotationIndex: null };
    this.addAnnotationLayer = this.addAnnotationLayer.bind(this);
    this.removeAnnotationLayer = this.removeAnnotationLayer.bind(this);
    this.handleVisibleChange = this.handleVisibleChange.bind(this);
@@ -83,48 +83,50 @@ class AnnotationLayerControl extends React.PureComponent {
    }
  }

-  addAnnotationLayer(annotationLayer) {
-    const annotation = annotationLayer;
-    let annotations = this.props.value.slice();
-    const i = annotations.findIndex(
-      x => x.name === (annotation.oldName || annotation.name),
-    );
-    delete annotation.oldName;
-    if (i > -1) {
-      annotations[i] = annotation;
+  addAnnotationLayer(originalAnnotation, newAnnotation) {
+    let annotations = this.props.value;
+    if (annotations.includes(originalAnnotation)) {
+      annotations = annotations.map(anno =>
+        anno === originalAnnotation ? newAnnotation : anno,
+      );
    } else {
-      annotations = annotations.concat(annotation);
+      annotations = [...annotations, newAnnotation];
+      this.setState({ addedAnnotationIndex: annotations.length - 1 });
    }
-    this.props.refreshAnnotationData(annotation);
+
+    this.props.refreshAnnotationData(newAnnotation);
    this.props.onChange(annotations);
  }

  handleVisibleChange(visible, popoverKey) {
    this.setState(prevState => ({
-      popoverVisible: { ...prevState, [popoverKey]: visible },
+      popoverVisible: { ...prevState.popoverVisible, [popoverKey]: visible },
    }));
  }

  removeAnnotationLayer(annotation) {
-    const annotations = this.props.value
-      .slice()
-      .filter(x => x.name !== annotation.oldName);
+    const annotations = this.props.value.filter(anno => anno !== annotation);
    this.props.onChange(annotations);
  }

-  renderPopover(parent, popoverKey, annotation, error) {
-    const id = !annotation ? '_new' : annotation.name;
+  renderPopover(popoverKey, annotation, error) {
+    const id = annotation?.name || '_new';
+
    return (
      <div id={`annotation-pop-${id}`} data-test="popover-content">
        <AnnotationLayer
          {...annotation}
-          parent={this.refs[parent]}
          error={error}
          colorScheme={this.props.colorScheme}
          vizType={this.props.vizType}
-          addAnnotationLayer={this.addAnnotationLayer}
-          removeAnnotationLayer={this.removeAnnotationLayer}
-          close={() => this.handleVisibleChange(false, popoverKey)}
+          addAnnotationLayer={newAnnotation =>
+            this.addAnnotationLayer(annotation, newAnnotation)
+          }
+          removeAnnotationLayer={() => this.removeAnnotationLayer(annotation)}
+          close={() => {
+            this.handleVisibleChange(false, popoverKey);
+            this.setState({ addedAnnotationIndex: null });
+          }}
        />
      </div>
    );
@@ -153,6 +155,9 @@ class AnnotationLayerControl extends React.PureComponent {
  }

  render() {
+    const { addedAnnotationIndex } = this.state;
+    const addedAnnotation = this.props.value[addedAnnotationIndex];
+
    const annotations = this.props.value.map((anno, i) => (
      <Popover
        key={i}
@@ -160,7 +165,6 @@ class AnnotationLayerControl extends React.PureComponent {
        placement="right"
        title={t('Edit Annotation Layer')}
        content={this.renderPopover(
-          `overlay-${i}`,
          i,
          anno,
          this.props.annotationError[anno.name],
@@ -183,9 +187,10 @@ class AnnotationLayerControl extends React.PureComponent {
          <Popover
            trigger="click"
            placement="right"
-            content={this.renderPopover('overlay-new', addLayerPopoverKey)}
+            content={this.renderPopover(addLayerPopoverKey, addedAnnotation)}
            title={t('Add Annotation Layer')}
            visible={this.state.popoverVisible[addLayerPopoverKey]}
+            destroyTooltipOnHide
            onVisibleChange={visible =>
              this.handleVisibleChange(visible, addLayerPopoverKey)
            }