From 7d7ad854ee743452511970faa5ee440172567fd5 Mon Sep 17 00:00:00 2001 From: Wallace Breza Date: Tue, 12 Feb 2019 12:54:34 -0800 Subject: [PATCH] Update project when in-use connection changes AB#16921 (#564) * Update current project when connection changes * Removed redundant test in editor page * Updated toolbar to remove duplicate logic * Addressed PR feedback --- src/common/mockFactory.ts | 20 ++++---- .../pages/editorPage/editorPage.test.tsx | 42 ++++++++-------- .../pages/editorPage/editorPage.tsx | 18 +++---- .../reducers/currentProjectReducer.test.ts | 32 +++++++++++-- src/redux/reducers/currentProjectReducer.ts | 48 +++++++++---------- src/services/assetService.test.ts | 22 ++++++++- src/services/assetService.ts | 8 +++- 7 files changed, 118 insertions(+), 72 deletions(-) diff --git a/src/common/mockFactory.ts b/src/common/mockFactory.ts index 320a9b31..b02491e4 100644 --- a/src/common/mockFactory.ts +++ b/src/common/mockFactory.ts @@ -2,7 +2,7 @@ import shortid from "shortid"; import { AssetState, AssetType, IApplicationState, IAppSettings, IAsset, IAssetMetadata, IConnection, IExportFormat, IProject, ITag, StorageType, ISecurityToken, - EditorMode, IAppError, IProjectVideoSettings, AppError, ErrorCode, + EditorMode, IAppError, IProjectVideoSettings, ErrorCode, IPoint, IRegion, RegionType, IBoundingBox, } from "../models/applicationState"; import { ExportAssetState } from "../providers/export/exportProvider"; @@ -163,17 +163,17 @@ export default class MockFactory { /** * Creates array of fake IAsset - * @param count Number of assets to create - * @param videoFirst true if the first asset should be video; false otherwise + * @param count Number of assets to create (default: 10) + * @param startIndex The index that the assets should start at (default: 1) */ - public static createTestAssets(count: number = 10, videoFirst: boolean = true): IAsset[] { + public static createTestAssets(count: number = 10, startIndex: number = 1): IAsset[] { const assets: IAsset[] = []; - for (let i = 1; i <= count; i++) { - assets.push((i % 2 === 1) ? - videoFirst ? MockFactory.createVideoTestAsset(i.toString()) : - MockFactory.createTestAsset(i.toString()) : - !videoFirst ? MockFactory.createVideoTestAsset(i.toString()) : - MockFactory.createTestAsset(i.toString())); + for (let i = startIndex; i < (count + startIndex); i++) { + const newAsset = (i % 2 === 1) + ? MockFactory.createVideoTestAsset(i.toString()) + : MockFactory.createTestAsset(i.toString()); + + assets.push(newAsset); } return assets; diff --git a/src/react/components/pages/editorPage/editorPage.test.tsx b/src/react/components/pages/editorPage/editorPage.test.tsx index ba30b718..195a595f 100644 --- a/src/react/components/pages/editorPage/editorPage.test.tsx +++ b/src/react/components/pages/editorPage/editorPage.test.tsx @@ -1,5 +1,6 @@ -import { mount, ReactWrapper } from "enzyme"; import React from "react"; +import { mount, ReactWrapper } from "enzyme"; +import _ from "lodash"; import { Provider } from "react-redux"; import { BrowserRouter as Router } from "react-router-dom"; import { AnyAction, Store } from "redux"; @@ -125,8 +126,11 @@ describe("Editor Page Component", () => { expect(editorPage.prop("project")).toEqual(testProject); }); - it("Loads project assets when state changes", async () => { + it("Loads and merges project assets with asset provider assets when state changes", async () => { + const projectAssets = MockFactory.createTestAssets(10, 10); const testProject = MockFactory.createTestProject("TestProject"); + testProject.assets = _.keyBy(projectAssets, (asset) => asset.id); + const store = createStore(testProject, true); const props = MockFactory.editorPageProps(testProject.id); @@ -138,12 +142,12 @@ describe("Editor Page Component", () => { name: testProject.name, }; - const expectedAssetMetadtata: IAssetMetadata = getMockAssetMetadata(testAssets); - await MockFactory.flushUi(); + const expectedAssetMetadtata: IAssetMetadata = getMockAssetMetadata(projectAssets); + expect(editorPage.props().project).toEqual(expect.objectContaining(partialProject)); - expect(editorPage.state().assets.length).toEqual(testAssets.length); + expect(editorPage.state().assets.length).toEqual(projectAssets.length + testAssets.length); expect(editorPage.state().selectedAsset).toMatchObject(expectedAssetMetadtata); }); @@ -193,14 +197,7 @@ describe("Editor Page Component", () => { const props = MockFactory.editorPageProps(testProject.id); wrapper = createComponent(store, props); - - await MockFactory.waitForCondition(() => { - const editorPage = wrapper - .find(EditorPage) - .childAt(0); - - return !!editorPage.state().selectedAsset; - }); + await waitForSelectedAsset(wrapper); }); it("editor mode is changed correctly", async () => { @@ -292,14 +289,7 @@ describe("Editor Page Component", () => { }); const wrapper = createComponent(store, MockFactory.editorPageProps()); - - await MockFactory.waitForCondition(() => { - const editorPage = wrapper - .find(EditorPage) - .childAt(0); - - return !!editorPage.state().selectedAsset; - }); + await waitForSelectedAsset(wrapper); wrapper.update(); @@ -328,3 +318,13 @@ function createStore(project: IProject, setCurrentProject: boolean = false): Sto return createReduxStore(initialState); } + +async function waitForSelectedAsset(wrapper: ReactWrapper) { + await MockFactory.waitForCondition(() => { + const editorPage = wrapper + .find(EditorPage) + .childAt(0); + + return !!editorPage.state().selectedAsset; + }); +} diff --git a/src/react/components/pages/editorPage/editorPage.tsx b/src/react/components/pages/editorPage/editorPage.tsx index 8c17b396..63675ed4 100644 --- a/src/react/components/pages/editorPage/editorPage.tsx +++ b/src/react/components/pages/editorPage/editorPage.tsx @@ -271,9 +271,6 @@ export default class EditorPage extends React.Component asset.id) + .value(); this.setState({ - assets, + assets: allAssets, }, async () => { - if (assets.length > 0) { - await this.selectAsset(assets[0]); + if (allAssets.length > 0) { + await this.selectAsset(allAssets[0]); } this.loadingProjectAssets = false; }); diff --git a/src/redux/reducers/currentProjectReducer.test.ts b/src/redux/reducers/currentProjectReducer.test.ts index 1920f55f..ae64e652 100644 --- a/src/redux/reducers/currentProjectReducer.test.ts +++ b/src/redux/reducers/currentProjectReducer.test.ts @@ -11,6 +11,7 @@ import { saveAssetMetadataAction, } from "../actions/projectActions"; import { anyOtherAction } from "../actions/actionCreators"; +import { saveConnectionAction } from "../actions/connectionActions"; describe("Current Project Reducer", () => { it("Load project sets current project state", () => { @@ -39,14 +40,39 @@ describe("Current Project Reducer", () => { expect(result).toBeNull(); }); - it("Load Project Assets merges assets into current asset set", () => { + it("Updating connection not in use by current project performs noop", () => { + const currentProject = MockFactory.createTestProject("1"); + const state: IProject = currentProject; + const unrelatedConnection = MockFactory.createTestConnection("Unrelated Connection"); + const action = saveConnectionAction(unrelatedConnection); + const result = reducer(state, action); + expect(result).toEqual(currentProject); + }); + + it("Updating connection used by current project is updated in curren project", () => { + const currentProject = MockFactory.createTestProject("1"); + const state: IProject = currentProject; + + const updatedConnection = { ...currentProject.sourceConnection }; + updatedConnection.description += "updated"; + + const action = saveConnectionAction(updatedConnection); + const result = reducer(state, action); + + expect(result).toEqual({ + ...currentProject, + sourceConnection: updatedConnection, + targetConnection: updatedConnection, + }); + }); + + it("Load Project Assets does not merges assets into current asset set", () => { const state: IProject = MockFactory.createTestProject("TestProject"); const testAssets = MockFactory.createTestAssets(); const action = loadProjectAssetsAction(testAssets); const result = reducer(state, action); - expect(result).not.toBe(state); - expect(Object.keys(result.assets).length).toEqual(testAssets.length); + expect(result).toBe(state); }); it("Save Asset Metadata updates project asset state", () => { diff --git a/src/redux/reducers/currentProjectReducer.ts b/src/redux/reducers/currentProjectReducer.ts index 51dab603..3ffbf760 100644 --- a/src/redux/reducers/currentProjectReducer.ts +++ b/src/redux/reducers/currentProjectReducer.ts @@ -21,34 +21,32 @@ export const reducer = (state: IProject = null, action: AnyAction): IProject => return null; case ActionTypes.LOAD_PROJECT_SUCCESS: return { ...action.payload }; - case ActionTypes.LOAD_PROJECT_ASSETS_SUCCESS: - if (state) { - const currentAssets = { ...state.assets } || {}; - action.payload.forEach((asset) => { - if (!currentAssets[asset.id]) { - currentAssets[asset.id] = asset; - } - }); - - return { - ...state, - assets: currentAssets, - }; - } else { - return state; - } case ActionTypes.SAVE_ASSET_METADATA_SUCCESS: - if (state) { - const updatedAssets = { ...state.assets } || {}; - updatedAssets[action.payload.asset.id] = { ...action.payload.asset }; - - return { - ...state, - assets: updatedAssets, - }; - } else { + if (!state) { return state; } + + const updatedAssets = { ...state.assets } || {}; + updatedAssets[action.payload.asset.id] = { ...action.payload.asset }; + + return { + ...state, + assets: updatedAssets, + }; + case ActionTypes.SAVE_CONNECTION_SUCCESS: + if (!state) { + return state; + } + + return { + ...state, + sourceConnection: state.sourceConnection.id === action.payload.id + ? { ...action.payload } + : state.sourceConnection, + targetConnection: state.targetConnection.id === action.payload.id + ? { ...action.payload } + : state.targetConnection, + }; default: return state; } diff --git a/src/services/assetService.test.ts b/src/services/assetService.test.ts index d5bccbdc..6ac04690 100644 --- a/src/services/assetService.test.ts +++ b/src/services/assetService.test.ts @@ -1,5 +1,5 @@ import { AssetService } from "./assetService"; -import { AssetType, IAssetMetadata } from "../models/applicationState"; +import { AssetType, IAssetMetadata, AssetState } from "../models/applicationState"; import MockFactory from "../common/mockFactory"; import { AssetProviderFactory, IAssetProvider } from "../providers/storage/assetProviderFactory"; import { StorageProviderFactory, IStorageProvider } from "../providers/storage/storageProviderFactory"; @@ -118,7 +118,10 @@ describe("Asset Service", () => { it("Saves asset JSON to underlying storage provider", async () => { const assetMetadata: IAssetMetadata = { - asset: testAssets[0], + asset: { + ...testAssets[0], + state: AssetState.Tagged, + }, regions: [], }; @@ -131,6 +134,21 @@ describe("Asset Service", () => { expect(result).toBe(assetMetadata); }); + it("Does not save asset JSON to the storage provider if asset has not been tagged", async () => { + const assetMetadata: IAssetMetadata = { + asset: { + ...testAssets[0], + state: AssetState.Visited, + }, + regions: [], + }; + + const result = await assetService.save(assetMetadata); + + expect(storageProviderMock.writeText).not.toBeCalled(); + expect(result).toBe(assetMetadata); + }); + it("getAssets encodes local file path", async () => { const testAsset = MockFactory.createTestAsset(" 11"); testAssets.push(testAsset); diff --git a/src/services/assetService.ts b/src/services/assetService.ts index 38d0ca37..3e1f86f7 100644 --- a/src/services/assetService.ts +++ b/src/services/assetService.ts @@ -149,8 +149,12 @@ export class AssetService { public async save(metadata: IAssetMetadata): Promise { Guard.null(metadata); - const fileName = `${metadata.asset.id}${constants.assetMetadataFileExtension}`; - await this.storageProvider.writeText(fileName, JSON.stringify(metadata, null, 4)); + // Only save asset metadata if asset is in a tagged state + // Otherwise primary asset information is already persisted in the project file. + if (metadata.asset.state === AssetState.Tagged) { + const fileName = `${metadata.asset.id}${constants.assetMetadataFileExtension}`; + await this.storageProvider.writeText(fileName, JSON.stringify(metadata, null, 4)); + } return metadata; }