fix(creator): Prevent creating during creator pending (#643)

* fix(creator): Prevent creating during creator pending

* fix(creator): Address comments

Author: Mingze

src/drawing/DrawingAnnotations.tsx

@@ -57,7 +57,7 @@ const DrawingAnnotations = (props: Props): JSX.Element => {
                 onSelect={handleAnnotationActive}
             />
 
-            {isCreating && hasDrawnPathGroups && (
+            {hasDrawnPathGroups && (
                 <DrawingSVG ref={setStagedRootEl} className="ba-DrawingAnnotations-target">
                     {/* Group element to capture the bounding box around the drawn path groups */}
                     <g data-ba-reference-id={uuidRef.current}>

src/drawing/DrawingAnnotationsContainer.tsx

@@ -9,9 +9,11 @@ import {
 import { AnnotationDrawing, PathGroup } from '../@types';
 import {
     AppState,
+    CreatorStatus,
     getActiveAnnotationId,
     getAnnotationMode,
     getAnnotationsForLocation,
+    getCreatorStatus,
     isFeatureEnabled,
     Mode,
     setActiveAnnotationIdAction,
@@ -32,7 +34,10 @@ export const mapStateToProps = (state: AppState, { location }: { location: numbe
         activeAnnotationId: getActiveAnnotationId(state),
         annotations: getAnnotationsForLocation(state, location).filter(isDrawing),
         drawnPathGroups: getDrawingDrawnPathGroupsForLocation(state, location),
-        isCreating: isFeatureEnabled(state, 'drawingCreate') && getAnnotationMode(state) === Mode.DRAWING,
+        isCreating:
+            isFeatureEnabled(state, 'drawingCreate') &&
+            getAnnotationMode(state) === Mode.DRAWING &&
+            getCreatorStatus(state) !== CreatorStatus.pending,
     };
 };
 

src/drawing/__tests__/DrawingAnnotations-test.tsx

@@ -91,11 +91,10 @@ describe('DrawingAnnotations', () => {
         });
 
         test('should render staged drawing if present and allowed', () => {
-            const wrapper = getWrapper({ isCreating: true, drawnPathGroups: pathGroups });
+            const wrapper = getWrapper({ drawnPathGroups: pathGroups });
 
             expect(wrapper.find(DrawingList).exists()).toBe(true);
             expect(wrapper.find(DrawingSVG).exists()).toBe(true);
-            expect(wrapper.find(DrawingCreator).exists()).toBe(true);
         });
     });
 });

src/drawing/__tests__/DrawingAnnotationsContainer-test.tsx

@@ -4,7 +4,7 @@ import { Store } from 'redux';
 import { mount, ReactWrapper } from 'enzyme';
 import DrawingAnnotations from '../DrawingAnnotations';
 import DrawingAnnotationsContainer, { Props } from '../DrawingAnnotationsContainer';
-import { createStore } from '../../store';
+import { createStore, CreatorStatus, Mode } from '../../store';
 
 jest.mock('../../common/withProviders');
 
@@ -33,5 +33,25 @@ describe('DrawingAnnotationsContainer', () => {
                 isCreating: false,
             });
         });
+
+        test.each`
+            mode            | status                   | isCreating
+            ${Mode.NONE}    | ${CreatorStatus.staged}  | ${false}
+            ${Mode.DRAWING} | ${CreatorStatus.staged}  | ${true}
+            ${Mode.DRAWING} | ${CreatorStatus.pending} | ${false}
+            ${Mode.REGION}  | ${CreatorStatus.staged}  | ${false}
+        `(
+            'should pass down isCreating as $isCreating if mode is $mode and status is $status',
+            ({ mode, isCreating, status }) => {
+                const store = createStore({
+                    common: { mode },
+                    creator: { status },
+                    options: { features: { drawingCreate: true } },
+                });
+                const wrapper = getWrapper({ store });
+
+                expect(wrapper.find(DrawingAnnotations).prop('isCreating')).toEqual(isCreating);
+            },
+        );
     });
 });

src/region/RegionCreation.tsx

@@ -55,22 +55,23 @@ export default class RegionCreation extends React.PureComponent<Props, State> {
 
     render(): JSX.Element | null {
         const { isCreating, isDiscoverabilityEnabled, isRotated, staged } = this.props;
-        const canCreate = isCreating && !isRotated;
 
-        if (!canCreate) {
+        if (isRotated) {
             return null;
         }
 
         return (
             <>
-                <RegionCreator
-                    className={classNames('ba-RegionCreation-creator', {
-                        'is-discoverability-enabled': isDiscoverabilityEnabled,
-                    })}
-                    onAbort={this.handleAbort}
-                    onStart={this.handleStart}
-                    onStop={this.handleStop}
-                />
+                {isCreating && (
+                    <RegionCreator
+                        className={classNames('ba-RegionCreation-creator', {
+                            'is-discoverability-enabled': isDiscoverabilityEnabled,
+                        })}
+                        onAbort={this.handleAbort}
+                        onStart={this.handleStart}
+                        onStop={this.handleStop}
+                    />
+                )}
 
                 {staged && (
                     <div className="ba-RegionCreation-target">

src/region/RegionCreationContainer.tsx

@@ -2,8 +2,10 @@ import { connect } from 'react-redux';
 import {
     AppState,
     CreatorItemRegion,
+    CreatorStatus,
     getAnnotationMode,
     getCreatorStagedForLocation,
+    getCreatorStatus,
     getRotation,
     isCreatorStagedRegion,
     isFeatureEnabled,
@@ -27,7 +29,7 @@ export const mapStateToProps = (state: AppState, { location }: { location: numbe
     const staged = getCreatorStagedForLocation(state, location);
 
     return {
-        isCreating: getAnnotationMode(state) === Mode.REGION,
+        isCreating: getAnnotationMode(state) === Mode.REGION && getCreatorStatus(state) !== CreatorStatus.pending,
         isDiscoverabilityEnabled: isFeatureEnabled(state, 'discoverability'),
         isRotated: !!getRotation(state),
         staged: isCreatorStagedRegion(staged) ? staged : null,

src/region/__tests__/RegionCreation-test.tsx

@@ -99,15 +99,13 @@ describe('RegionCreation', () => {
         test('should render a RegionAnnotation if one has been drawn', () => {
             const shape = getRect();
             const wrapper = getWrapper({
-                isCreating: true,
                 staged: {
                     location: 1,
                     message: null,
                     shape,
                 },
             });
 
-            expect(wrapper.exists(RegionCreator)).toBe(true);
             expect(wrapper.find(RegionRect).prop('shape')).toEqual(shape);
         });
 
@@ -122,7 +120,9 @@ describe('RegionCreation', () => {
 
         test('should not render creation components if file is rotated', () => {
             const wrapper = getWrapper({
-                isRotated: false,
+                isCreating: true,
+                isRotated: true,
+                staged: {},
             });
 
             expect(wrapper.exists(RegionCreator)).toBe(false);

src/region/__tests__/RegionCreationContainer-test.tsx

@@ -3,7 +3,7 @@ import { IntlShape } from 'react-intl';
 import { mount, ReactWrapper } from 'enzyme';
 import RegionCreation from '../RegionCreation';
 import RegionCreationContainer, { Props } from '../RegionCreationContainer';
-import { createStore } from '../../store';
+import { createStore, CreatorStatus, Mode } from '../../store';
 
 jest.mock('../../common/withProviders');
 jest.mock('../RegionCreation');
@@ -33,6 +33,22 @@ describe('RegionCreationContainer', () => {
             });
         });
 
+        test.each`
+            mode              | status                   | isCreating
+            ${Mode.NONE}      | ${CreatorStatus.staged}  | ${false}
+            ${Mode.HIGHLIGHT} | ${CreatorStatus.staged}  | ${false}
+            ${Mode.REGION}    | ${CreatorStatus.staged}  | ${true}
+            ${Mode.REGION}    | ${CreatorStatus.pending} | ${false}
+        `(
+            'should pass down isCreating as $isCreating if mode is $mode and status is $status',
+            ({ mode, isCreating, status }) => {
+                const store = createStore({ common: { mode }, creator: { status } });
+                const wrapper = getWrapper({ store });
+
+                expect(wrapper.find(RegionCreation).prop('isCreating')).toEqual(isCreating);
+            },
+        );
+
         test.each`
             rotation     | isRotated
             ${null}      | ${false}