Merge pull request #1606 from concord-consortium/188104685-unify-v2-i…

…mport

unify handling of v2 documents

v3/src/components/app.tsx

@@ -4,7 +4,7 @@ import { CodapDndContext } from "../lib/dnd-kit/codap-dnd-context"
 import { Container } from "./container/container"
 import { ToolShelf } from "./tool-shelf/tool-shelf"
 import { kCodapAppElementId } from "./constants"
-import { importV2Document } from "../v2/import-v2-document"
+import { ICodapV2DocumentJson } from "../v2/codap-v2-types"
 import { MenuBar, kMenuBarElementId } from "./menu-bar/menu-bar"
 import { useCloudFileManager } from "../lib/use-cloud-file-manager"
 import { Logger } from "../lib/logger"
@@ -57,7 +57,7 @@ export const App = observer(function App() {
       sharedData?.dataSet.completeSnapshot()
     }, [])
 
-  const handleImportV3Document = useCallback((document: IDocumentModelSnapshot) => {
+  const handleImportDocument = useCallback((document: IDocumentModelSnapshot | ICodapV2DocumentJson) => {
     appState.setDocument(document)
   }, [])
 
@@ -69,8 +69,7 @@ export const App = observer(function App() {
   useDropHandler({
     selector: `#${kCodapAppElementId}`,
     onImportDataSet: handleImportDataSet,
-    onImportV2Document: importV2Document,
-    onImportV3Document: handleImportV3Document,
+    onImportDocument: handleImportDocument,
     onHandleUrlDrop: handleUrlDrop
   })
 

v3/src/hooks/use-drop-handler.ts

@@ -3,23 +3,16 @@ import { IDataSet } from "../models/data/data-set"
 import { IDocumentModelSnapshot } from "../models/document/document"
 import { convertParsedCsvToDataSet, CsvParseResult, importCsvFile } from "../utilities/csv-import"
 import { safeJsonParse } from "../utilities/js-utils"
-import { CodapV2Document } from "../v2/codap-v2-document"
 import { ICodapV2DocumentJson } from "../v2/codap-v2-types"
 
-function importCodapV2Document(file: File | null, onComplete: (document: CodapV2Document) => void) {
+function importCodapDocument(
+  file: File | null,
+  onComplete: (document: IDocumentModelSnapshot | ICodapV2DocumentJson) => void
+) {
   const reader = new FileReader()
   reader.onload = () => {
-    const result = reader.result && safeJsonParse<ICodapV2DocumentJson>(reader.result as string)
-    const document = result && new CodapV2Document(result)
-    document && onComplete(document)
-  }
-  file && reader.readAsText(file)
-}
-
-function importCodapV3Document(file: File | null, onComplete: (document: IDocumentModelSnapshot) => void) {
-  const reader = new FileReader()
-  reader.onload = () => {
-    const document = reader.result && safeJsonParse<IDocumentModelSnapshot>(reader.result as string)
+    const document = reader.result &&
+      safeJsonParse<IDocumentModelSnapshot | ICodapV2DocumentJson>(reader.result as string)
     document && onComplete(document)
   }
   file && reader.readAsText(file)
@@ -28,12 +21,11 @@ function importCodapV3Document(file: File | null, onComplete: (document: IDocume
 export interface IDropHandler {
   selector: string
   onImportDataSet?: (data: IDataSet) => void
-  onImportV2Document?: (document: CodapV2Document) => void
-  onImportV3Document?: (document: IDocumentModelSnapshot) => void
+  onImportDocument?: (document: IDocumentModelSnapshot | ICodapV2DocumentJson) => void
   onHandleUrlDrop?: (url: string) => void
 }
 export const useDropHandler = ({
-  selector, onImportDataSet, onImportV2Document, onImportV3Document, onHandleUrlDrop
+  selector, onImportDataSet, onImportDocument, onHandleUrlDrop
 }: IDropHandler) => {
   const eltRef = useRef<HTMLElement | null>(null)
 
@@ -47,12 +39,8 @@ export const useDropHandler = ({
 
     function dropHandler(event: DragEvent) {
 
-      function onCompleteCodapV2Import(document: CodapV2Document) {
-        onImportV2Document?.(document)
-      }
-
-      function onCompleteCodapV3Import(document: IDocumentModelSnapshot) {
-        onImportV3Document?.(document)
+      function onCompleteCodapImport(document: IDocumentModelSnapshot | ICodapV2DocumentJson) {
+        onImportDocument?.(document)
       }
 
       function onCompleteCsvImport(results: CsvParseResult, aFile: any) {
@@ -73,10 +61,8 @@ export const useDropHandler = ({
             const extension = nameParts?.length ? nameParts[nameParts.length - 1] : ""
             switch (extension) {
               case "codap":
-                importCodapV2Document(file, onCompleteCodapV2Import)
-                break
               case "codap3":
-                importCodapV3Document(file, onCompleteCodapV3Import)
+                importCodapDocument(file, onCompleteCodapImport)
                 break
               case "csv":
                 importCsvFile(file, onCompleteCsvImport)
@@ -116,7 +102,7 @@ export const useDropHandler = ({
       eltRef.current?.removeEventListener('dragover', dragOverHandler)
       eltRef.current?.removeEventListener('drop', dropHandler)
     }
-  }, [onHandleUrlDrop, onImportDataSet, onImportV2Document, onImportV3Document, selector])
+  }, [onHandleUrlDrop, onImportDataSet, onImportDocument, selector])
 
   // return element to which listeners were attached; useful for tests
   return eltRef.current

v3/src/lib/handle-cfm-event.test.ts

@@ -21,7 +21,7 @@ describe("handleCFMEvent", () => {
     updateDocumentSpy.mockRestore()
   })
 
-  it("handles the `connected` message", () => {
+  it("handles the `connected` message", async () => {
     const mockCfmClient = {
       openUrlFile: jest.fn(),
       setProviderOptions: jest.fn(),
@@ -33,7 +33,7 @@ describe("handleCFMEvent", () => {
     const cfmEvent = {
       type: "connected"
     } as CloudFileManagerClientEvent
-    handleCFMEvent(mockCfmClientArg, cfmEvent)
+    await handleCFMEvent(mockCfmClientArg, cfmEvent)
     expect(mockCfmClient.openUrlFile).not.toHaveBeenCalled()
     expect(mockCfmClient.setProviderOptions).toHaveBeenCalledTimes(1)
     const [providerNameArg, providerOptionsArg] = mockCfmClient.setProviderOptions.mock.calls[0]
@@ -46,41 +46,39 @@ describe("handleCFMEvent", () => {
     expect(menuBarInfoArg).toBe(`v${providerOptionsArg.appVersion} (${providerOptionsArg.appBuildNum})`)
 
     urlParamsModule.urlParams.url = "https://concord.org/example.json"
-    handleCFMEvent(mockCfmClientArg, cfmEvent)
+    await handleCFMEvent(mockCfmClientArg, cfmEvent)
     expect(mockCfmClient.openUrlFile).toHaveBeenCalledTimes(1)
     expect(mockCfmClient.setProviderOptions).toHaveBeenCalledTimes(2)
     expect(mockCfmClient._ui.setMenuBarInfo).toHaveBeenCalledTimes(2)
   })
 
-  it("handles the `getContent` message", done => {
+  it("handles the `getContent` message", async () => {
     const mockCfmClient = {} as CloudFileManagerClient
     const mockCfmEvent = {
       type: "getContent",
       callback: jest.fn()
     }
     const mockCfmEventArg = mockCfmEvent as unknown as CloudFileManagerClientEvent
-    handleCFMEvent(mockCfmClient, mockCfmEventArg)
-    setTimeout(() => {
-      const contentArg = mockCfmEvent.callback.mock.calls[0][0]
-      expect(isCodapDocument(contentArg)).toBe(true)
-      done()
-    })
+    await handleCFMEvent(mockCfmClient, mockCfmEventArg)
+
+    const contentArg = mockCfmEvent.callback.mock.calls[0][0]
+    expect(isCodapDocument(contentArg)).toBe(true)
   })
 
-  it("handles the willOpenFile message", () => {
+  it("handles the willOpenFile message", async () => {
     const mockCfmClient = {} as CloudFileManagerClient
     const mockCfmEvent = {
       type: "willOpenFile",
       callback: jest.fn()
     }
     const spy = jest.spyOn(urlParamsModule, "removeDevUrlParams")
     const mockCfmEventArg = mockCfmEvent as unknown as CloudFileManagerClientEvent
-    handleCFMEvent(mockCfmClient, mockCfmEventArg)
+    await handleCFMEvent(mockCfmClient, mockCfmEventArg)
     expect(spy).toHaveBeenCalledTimes(1)
     spy.mockRestore()
   })
 
-  it("handles the `openedFile` message with a v2 document", () => {
+  it("handles the `openedFile` message with a v2 document", async () => {
     const mockCfmClient = {} as CloudFileManagerClient
     const mockV2Document: ICodapV2DocumentJson = {
       appName: "DG",
@@ -94,12 +92,12 @@ describe("handleCFMEvent", () => {
       }
     } as CloudFileManagerClientEvent
     const spy = jest.spyOn(ImportV2Document, "importV2Document")
-    handleCFMEvent(mockCfmClient, cfmEvent)
+    await handleCFMEvent(mockCfmClient, cfmEvent)
     expect(ImportV2Document.importV2Document).toHaveBeenCalledTimes(1)
     spy.mockRestore()
   })
 
-  it("handles the `openedFile` message with a v3 document", () => {
+  it("handles the `openedFile` message with a v3 document", async () => {
     const mockCfmClient = {} as CloudFileManagerClient
     const v3Document = createCodapDocument()
     const cfmEvent = {
@@ -109,7 +107,7 @@ describe("handleCFMEvent", () => {
       }
     } as CloudFileManagerClientEvent
     const spy = jest.spyOn(appState, "setDocument")
-    handleCFMEvent(mockCfmClient, cfmEvent)
+    await handleCFMEvent(mockCfmClient, cfmEvent)
     expect(spy).toHaveBeenCalledTimes(1)
     spy.mockRestore()
   })

v3/src/lib/handle-cfm-event.ts

@@ -1,9 +1,6 @@
 import { CloudFileManagerClient, CloudFileManagerClientEvent } from "@concord-consortium/cloud-file-manager"
 import { appState } from "../models/app-state"
 import { removeDevUrlParams, urlParams } from "../utilities/url-params"
-import { isCodapV2Document } from "../v2/codap-v2-types"
-import { CodapV2Document } from "../v2/codap-v2-document"
-import { importV2Document } from "../v2/import-v2-document"
 import { wrapCfmCallback } from "./cfm-utils"
 
 import build from "../../build_number.json"
@@ -36,10 +33,10 @@ export function handleCFMEvent(cfmClient: CloudFileManagerClient, event: CloudFi
     // case "closedFile":
     //   break
     case "getContent": {
-      appState.getDocumentSnapshot().then(content => {
+      // return the promise so tests can make sure it is complete
+      return appState.getDocumentSnapshot().then(content => {
         event.callback(content)
       })
-      break
     }
     case "willOpenFile":
       removeDevUrlParams()
@@ -49,14 +46,8 @@ export function handleCFMEvent(cfmClient: CloudFileManagerClient, event: CloudFi
     case "openedFile": {
       const content = event.data.content
       const metadata = event.data.metadata
-      if (isCodapV2Document(content)) {
-        const v2Document = new CodapV2Document(content, metadata)
-        importV2Document(v2Document)
-      }
-      else {
-        appState.setDocument(content, metadata)
-      }
-      break
+      // return the promise so tests can make sure it is complete
+      return appState.setDocument(content, metadata)
     }
     case "savedFile": {
       const { content } = event.data

v3/src/lib/use-cloud-file-manager.ts

@@ -1,12 +1,11 @@
 import { CFMAppOptions, CloudFileManager, CloudFileManagerClientEvent } from "@concord-consortium/cloud-file-manager"
-import { getSnapshot } from "mobx-state-tree"
 import { useEffect, useRef } from "react"
 import { Root, createRoot } from "react-dom/client"
 import { useMemo } from "use-memo-one"
 import { clientConnect, createCloudFileManager, renderRoot } from "./cfm-utils"
 import { handleCFMEvent } from "./handle-cfm-event"
 import { appState } from "../models/app-state"
-import { createCodapDocument, isCodapDocument } from "../models/codap/create-codap-document"
+import { isCodapDocument } from "../models/codap/create-codap-document"
 import { gLocale } from "../utilities/translation/locale"
 import { t } from "../utilities/translation/translate"
 import { removeDevUrlParams, urlParams } from "../utilities/url-params"
@@ -114,7 +113,7 @@ function getMenuConfig(cfm: CloudFileManager) {
       action() {
         cfm.client.closeFileDialog(function() {
           removeDevUrlParams()
-          appState.setDocument(getSnapshot(createCodapDocument()))
+          appState.setDocument({type: "CODAP"})
         })
       }
     },

v3/src/models/app-state.ts

@@ -7,7 +7,7 @@
   generally be MobX-observable.
  */
 import { cloneDeep } from "lodash"
-import { action, computed, makeObservable, observable, reaction } from "mobx"
+import { action, computed, makeObservable, observable, reaction, flow } from "mobx"
 import { getSnapshot } from "mobx-state-tree"
 import { CloudFileManager } from "@concord-consortium/cloud-file-manager"
 import { createCodapDocument } from "./codap/create-codap-document"
@@ -20,9 +20,14 @@ import { Logger } from "../lib/logger"
 import { t } from "../utilities/translation/translate"
 import { DEBUG_DOCUMENT } from "../lib/debug"
 import { TreeManagerType } from "./history/tree-manager"
+import { ICodapV2DocumentJson, isCodapV2Document } from "../v2/codap-v2-types"
+import { CodapV2Document } from "../v2/codap-v2-document"
+import { importV2Document } from "../v2/import-v2-document"
 
 type AppMode = "normal" | "performance"
 
+type ISerializedDocumentModel = IDocumentModelSnapshot & {revisionId?: string}
+
 class AppState {
   @observable
   private currentDocument: IDocumentModel
@@ -84,13 +89,29 @@ class AppState {
     this.cfm = cfm
   }
 
-  @action
-  setDocument(snap: IDocumentModelSnapshot & {revisionId?: string}, metadata?: Record<string, any>) {
+  @flow
+  *setDocument(
+    snap: ISerializedDocumentModel | ICodapV2DocumentJson,
+    metadata?: Record<string, any>
+  ) {
     // stop monitoring changes for undo/redo on the existing document
     this.disableDocumentMonitoring()
 
+    let content: ISerializedDocumentModel
+    if (isCodapV2Document(snap)) {
+      const v2Document = new CodapV2Document(snap, metadata)
+      const v3Document = importV2Document(v2Document)
+
+      // We serialize the v3 document to enforce the idea that the conversion process
+      // needs to result in a basic javascript object. This prevents the import process
+      // from accidentally setting up something in the v3 document that doesn't serialize.
+      content = yield serializeDocument(v3Document, doc => getSnapshot(doc))
+    } else {
+      content = snap
+    }
+
     try {
-      const document = createCodapDocument(snap)
+      const document = createCodapDocument(content)
       if (document) {
         this.currentDocument = document
         if (DEBUG_DOCUMENT) {
@@ -106,11 +127,11 @@ class AppState {
         }
         const docTitle = this.currentDocument.getDocumentTitle()
         this.currentDocument.setTitle(docTitle || t("DG.Document.defaultDocumentName"))
-        if (snap.revisionId && this.treeManager) {
+        if (content.revisionId && this.treeManager) {
           // Restore the revisionId from the stored document
           // This will allow us to consistently compare the local document
           // to the stored document.
-          this.treeManager.setRevisionId(snap.revisionId)
+          this.treeManager.setRevisionId(content.revisionId)
         }
 
         // monitor document changes for undo/redo

v3/src/v2/import-v2-document.ts

@@ -1,16 +1,13 @@
-import { getSnapshot } from "mobx-state-tree"
-import { appState } from "../models/app-state"
 import { createCodapDocument } from "../models/codap/create-codap-document"
 import { gDataBroker } from "../models/data/data-broker"
-import { serializeDocument } from "../models/document/serialize-document"
 import { getTileComponentInfo } from "../models/tiles/tile-component-info"
 import { getSharedModelManager } from "../models/tiles/tile-environment"
 import { ITileModel, ITileModelSnapshotIn } from "../models/tiles/tile-model"
 import { CodapV2Document } from "./codap-v2-document"
 import { importV2Component } from "./codap-v2-tile-importers"
 import { IFreeTileInRowOptions, isFreeTileRow } from "../models/document/free-tile-row"
 
-export async function importV2Document(v2Document: CodapV2Document) {
+export function importV2Document(v2Document: CodapV2Document) {
   const v3Document = createCodapDocument(undefined, { layout: "free" })
   const sharedModelManager = getSharedModelManager(v3Document)
   sharedModelManager && gDataBroker.setSharedModelManager(sharedModelManager)
@@ -61,8 +58,5 @@ export async function importV2Document(v2Document: CodapV2Document) {
     row.setMaxZIndex(maxZIndex)
   }
 
-  // retrieve document snapshot
-  const docSnapshot = await serializeDocument(v3Document, doc => getSnapshot(doc))
-  // use document snapshot
-  appState.setDocument(docSnapshot)
+  return v3Document
 }