From 6712d99ef3cbe79d7fb5d9b50ada26d1561f30ae Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Wed, 25 Sep 2024 14:44:42 -0400 Subject: [PATCH 1/6] unify more the titles displayed for documents Specifically this unifies the way the simple document items (the boxes in the sorted documents view), and the thumbnail captions are computed. It also improves the titling when the unit or problem is not known or found for the current document. --- shared/shared.ts | 17 ++-- src/components/document/document-group.tsx | 9 +-- src/components/document/document.tsx | 5 +- .../document/sort-work-document-area.tsx | 24 +++--- src/components/document/sorted-section.tsx | 3 +- src/components/navigation/document-view.tsx | 16 ++-- .../thumbnail/simple-document-item.tsx | 24 +++--- src/hooks/use-document-caption.tsx | 9 +-- src/models/document/document-types.ts | 3 + src/models/document/document-utils.ts | 77 ++++++++++++++----- src/models/document/document.ts | 12 --- src/models/document/log-document-event.ts | 5 +- src/models/stores/sorted-documents.ts | 8 +- 13 files changed, 116 insertions(+), 96 deletions(-) diff --git a/shared/shared.ts b/shared/shared.ts index 91eb35bc33..20facf932f 100644 --- a/shared/shared.ts +++ b/shared/shared.ts @@ -106,20 +106,23 @@ export function getDocumentPath(userId: string, documentKey: string, network?: s return documentPath; } -export interface IDocumentMetadata { +export interface IDocumentMetadataBase { uid: string; type: string; key: string; + title?: string|null; + visibility?: string; + investigation?: string|null; + problem?: string|null; + unit?: string|null; +} + +export interface IDocumentMetadata extends IDocumentMetadataBase { createdAt?: number; - title?: string; - originDoc?: string; + originDoc?: string|null; properties?: Record; tools?: string[]; strategies?: string[]; - investigation?: string; - problem?: string; - unit?: string|null; - visibility?: string; } export function isDocumentMetadata(o: any): o is IDocumentMetadata { return !!o.uid && !!o.type && !!o.key; diff --git a/src/components/document/document-group.tsx b/src/components/document/document-group.tsx index 40cf6830ec..6c9cb1679d 100644 --- a/src/components/document/document-group.tsx +++ b/src/components/document/document-group.tsx @@ -1,9 +1,8 @@ import React, { useEffect, useRef, useState } from "react"; import { observer } from "mobx-react-lite"; -import { DocumentModelType } from "../../models/document/document"; import { SimpleDocumentItem } from "../thumbnail/simple-document-item"; -import { IDocumentMetadata } from "../../../shared/shared"; import { DocumentGroup } from "../../models/stores/document-group"; +import { IDocumentMetadataModel } from "../../models/stores/sorted-documents"; import ScrollArrowIcon from "../../assets/workspace-instance-scroll.svg"; @@ -12,7 +11,7 @@ import "./document-group.scss"; interface IProps { documentGroup: DocumentGroup; secondarySort: string; - onSelectDocument: (document: DocumentModelType | IDocumentMetadata) => void; + onSelectDocument: (document: IDocumentMetadataModel) => void; } export const DocumentGroupComponent = observer(function DocumentGroupComponent(props: IProps) { @@ -119,13 +118,11 @@ export const DocumentGroupComponent = observer(function DocumentGroupComponent(p } {visibleCount < docCount && renderScrollButton("left", leftArrowDisabled)}
- {documentGroup.documents?.map((doc: any) => { + {documentGroup.documents?.map((doc) => { return ( ); diff --git a/src/components/document/document.tsx b/src/components/document/document.tsx index a94c54bd78..e84360ee1a 100644 --- a/src/components/document/document.tsx +++ b/src/components/document/document.tsx @@ -11,6 +11,7 @@ import { logDocumentEvent, logDocumentViewEvent } from "../../models/document/lo import { IToolbarModel } from "../../models/stores/problem-configuration"; import { SupportType, TeacherSupportModelType, AudienceEnum } from "../../models/stores/supports"; import { WorkspaceModelType } from "../../models/stores/workspace"; +import { getDocumentTitleWithTimestamp } from "../../models/document/document-utils"; import { ENavTab } from "../../models/view/nav-tabs"; import { IconButton } from "../utilities/icon-button"; import ToggleControl from "../utilities/toggle-control"; @@ -384,7 +385,9 @@ export class DocumentComponent extends BaseComponent { { !hideButtons && }
:
- + { !hideButtons && }
} diff --git a/src/components/document/sort-work-document-area.tsx b/src/components/document/sort-work-document-area.tsx index 512af6a36a..50944ccd63 100644 --- a/src/components/document/sort-work-document-area.tsx +++ b/src/components/document/sort-work-document-area.tsx @@ -1,8 +1,7 @@ import React, { useState } from "react"; import classNames from "classnames"; import { observer } from "mobx-react"; -import { useAppConfig, useProblemStore, - usePersistentUIStore, useUserStore, useClassStore, useUIStore, useStores } from "../../hooks/use-stores"; +import { useStores } from "../../hooks/use-stores"; import { DocumentModelType } from "../../models/document/document"; import { EditableDocumentContent } from "./editable-document-content"; import { getDocumentDisplayTitle } from "../../models/document/document-utils"; @@ -24,13 +23,8 @@ interface IProps { export const SortWorkDocumentArea: React.FC = observer(function SortWorkDocumentArea(props: IProps) { const { openDocumentsGroup } = props; - const store = useStores(); - const ui = useUIStore(); - const persistentUI = usePersistentUIStore(); - const user = useUserStore(); - const classStore = useClassStore(); - const problemStore = useProblemStore(); - const appConfigStore = useAppConfig(); + const {appConfig, class: classStore, documents, networkDocuments, + persistentUI, sortedDocuments, ui, unit, user} = useStores(); const tabState = persistentUI.tabs.get(ENavTab.kSortWork); const openDocumentKey = tabState?.openSubTab && tabState?.openDocuments.get(tabState.openSubTab); const showScroller = persistentUI.showDocumentScroller; @@ -43,24 +37,24 @@ export const SortWorkDocumentArea: React.FC = observer(function SortWork } const getOpenDocument = () => { - const openDoc = store.documents.getDocument(openDocumentKey) || - store.networkDocuments.getDocument(openDocumentKey); + const openDoc = documents.getDocument(openDocumentKey) || + networkDocuments.getDocument(openDocumentKey); if (openDoc) { return openDoc; } // Calling `fetchFullDocument` will update the `documents` store with the full document, // triggering a re-render of this component since it's an observer. - store.sortedDocuments.fetchFullDocument(openDocumentKey); + sortedDocuments.fetchFullDocument(openDocumentKey); }; const openDocument = getOpenDocument(); - const isVisible = openDocument?.isAccessibleToUser(user, store.documents); - const showPlayback = user.type && appConfigStore.enableHistoryRoles.includes(user.type); + const isVisible = openDocument?.isAccessibleToUser(user, documents); + const showPlayback = user.type && appConfig.enableHistoryRoles.includes(user.type); const showExemplarShare = user.type === "teacher" && openDocument && isExemplarType(openDocument.type); const getDisplayTitle = (document: DocumentModelType) => { const documentOwner = classStore.users.get(document.uid); - const documentTitle = getDocumentDisplayTitle(document, appConfigStore, problemStore, store.unit.code); + const documentTitle = getDocumentDisplayTitle(unit, document, appConfig); return {owner: documentOwner ? documentOwner.fullName : "", title: documentTitle}; }; const displayTitle = openDocument && getDisplayTitle(openDocument); diff --git a/src/components/document/sorted-section.tsx b/src/components/document/sorted-section.tsx index ee3017bd79..24d99214e1 100644 --- a/src/components/document/sorted-section.tsx +++ b/src/components/document/sorted-section.tsx @@ -5,7 +5,6 @@ import classNames from "classnames"; import { DocumentModelType } from "../../models/document/document"; import { useStores } from "../../hooks/use-stores"; import { DocFilterType, SecondarySortType } from "../../models/stores/ui-types"; -import { IDocumentMetadata } from "../../../shared/shared"; import { DocumentGroup } from "../../models/stores/document-group"; import { DocumentGroupComponent } from "./document-group"; import { logDocumentViewEvent } from "../../models/document/log-document-event"; @@ -53,7 +52,7 @@ export const SortedSection: React.FC = observer(function SortedSection(p : { primaryLabel: documentGroup.label, primaryType: documentGroup.sortType, secondaryLabel: label, secondaryType: sortType }; - return async (document: DocumentModelType | IDocumentMetadata) => { + return async (document: DocumentModelType | IDocumentMetadataModel) => { const openSubTab = JSON.stringify(openSubTabMetadata); persistentUI.openSubTabDocument(ENavTab.kSortWork, openSubTab, document.key); logDocumentViewEvent(document); diff --git a/src/components/navigation/document-view.tsx b/src/components/navigation/document-view.tsx index 09e953d530..5af8fc6db2 100644 --- a/src/components/navigation/document-view.tsx +++ b/src/components/navigation/document-view.tsx @@ -2,8 +2,8 @@ import React from "react"; import { observer } from "mobx-react"; import { useQueryClient } from "react-query"; import classNames from "classnames"; -import { useAppConfig, useLocalDocuments, useProblemStore, useStores, - usePersistentUIStore, useUserStore, useClassStore, useUIStore } from "../../hooks/use-stores"; +import { useAppConfig, useLocalDocuments, useStores, + usePersistentUIStore } from "../../hooks/use-stores"; import { useUserContext } from "../../hooks/use-user-context"; import { ISubTabSpec, NavTabModelType, kBookmarksTabTitle } from "../../models/view/nav-tabs"; import { DocumentType } from "../../models/document/document-types"; @@ -202,18 +202,14 @@ interface IDocumentAreaProps { } const DocumentArea = ({openDocument, subTab, tab, sectionClass, isSecondaryDocument, - hasSecondaryDocument, hideLeftFlipper, hideRightFlipper, onChangeDocument}: IDocumentAreaProps) => { - const ui = useUIStore(); - const persistentUI = usePersistentUIStore(); - const user = useUserStore(); - const appConfig = useAppConfig(); - const classStore = useClassStore(); - const problemStore = useProblemStore(); + hasSecondaryDocument, hideLeftFlipper, hideRightFlipper, onChangeDocument}: IDocumentAreaProps +) => { + const {appConfig, class: classStore, persistentUI, ui, unit, user} = useStores(); const showPlayback = user.type && !openDocument?.isPublished ? appConfig.enableHistoryRoles.includes(user.type) : false; const getDisplayTitle = (document: DocumentModelType) => { const documentOwner = classStore.users.get(document.uid); - const documentTitle = getDocumentDisplayTitle(document, appConfig, problemStore); + const documentTitle = getDocumentDisplayTitle(unit, document, appConfig); return {owner: documentOwner ? documentOwner.fullName : "", title: documentTitle}; }; const displayTitle = getDisplayTitle(openDocument); diff --git a/src/components/thumbnail/simple-document-item.tsx b/src/components/thumbnail/simple-document-item.tsx index 6a3afb0ea7..5e499c9c38 100644 --- a/src/components/thumbnail/simple-document-item.tsx +++ b/src/components/thumbnail/simple-document-item.tsx @@ -1,30 +1,24 @@ import { observer } from "mobx-react"; import React from "react"; -import { IDocumentMetadata } from "../../../shared/shared"; +import { IDocumentMetadataModel } from "../../models/stores/sorted-documents"; import { useStores } from "../../hooks/use-stores"; -import { isDocumentAccessibleToUser } from "../../models/document/document-utils"; +import { getDocumentDisplayTitle, isDocumentAccessibleToUser } from "../../models/document/document-utils"; import "./simple-document-item.scss"; interface IProps { - document: IDocumentMetadata; - investigationOrdinal: string; - problemOrdinal: string; - onSelectDocument: (document: IDocumentMetadata) => void; + document: IDocumentMetadataModel; + onSelectDocument: (document: IDocumentMetadataModel) => void; } export const SimpleDocumentItem = observer(function SimpleDocumentItem( - { document, investigationOrdinal, onSelectDocument, problemOrdinal }: IProps + { document, onSelectDocument }: IProps ) { - const { documents, class: classStore, unit, user } = useStores(); + const { appConfig, documents, class: classStore, unit, user } = useStores(); const { uid } = document; const userName = classStore.getUserById(uid)?.displayName; - // TODO: Make it so we don't have to convert investigationOrdinal and problemOrdinal to numbers here? We do so - // because the values originate as strings. Changing their types to numbers in the model would make this unnecessary, - // but doing that causes errors elsewhere when trying to load documents that aren't associated with a problem. - const investigation = unit.getInvestigation(Number(investigationOrdinal)); - const problem = investigation?.getProblem(Number(problemOrdinal)); - const title = document.title ? `${userName}: ${document.title}` : `${userName}: ${problem?.title ?? "unknown title"}`; + const title = getDocumentDisplayTitle(unit, document, appConfig); + const titleWithUser = `${userName}: ${title}`; const isPrivate = !isDocumentAccessibleToUser(document, user, documents); const handleClick = () => { @@ -35,7 +29,7 @@ export const SimpleDocumentItem = observer(function SimpleDocumentItem(
diff --git a/src/hooks/use-document-caption.tsx b/src/hooks/use-document-caption.tsx index d7e9552031..7d73c26dec 100644 --- a/src/hooks/use-document-caption.tsx +++ b/src/hooks/use-document-caption.tsx @@ -1,14 +1,11 @@ import { useFirestoreTeacher } from "./firestore-hooks"; -import { useAppConfig, useClassStore, useProblemStore, useUserStore } from "./use-stores"; import { DocumentModelType } from "../models/document/document"; import { ExemplarDocument, isPublishedType, isUnpublishedType } from "../models/document/document-types"; import { getDocumentDisplayTitle } from "../models/document/document-utils"; +import { useStores } from "./use-stores"; export function useDocumentCaption(document: DocumentModelType, isStudentWorkspaceDoc?: boolean) { - const appConfig = useAppConfig(); - const problem = useProblemStore(); - const classStore = useClassStore(); - const user = useUserStore(); + const {appConfig, class: classStore, unit, user} = useStores(); const { type, uid } = document; const pubVersion = document.pubVersion; const teacher = useFirestoreTeacher(uid, user.network || ""); @@ -29,6 +26,6 @@ export function useDocumentCaption(document: DocumentModelType, isStudentWorkspa : isPublishedType(type) && pubVersion ? ` v${pubVersion}` : ""; - const title = getDocumentDisplayTitle(document, appConfig, problem); + const title = getDocumentDisplayTitle(unit, document, appConfig); return `${namePrefix}${title}${dateSuffix}`; } diff --git a/src/models/document/document-types.ts b/src/models/document/document-types.ts index 7ea9a33aea..7a90da467a 100644 --- a/src/models/document/document-types.ts +++ b/src/models/document/document-types.ts @@ -25,6 +25,9 @@ export function isPersonalType(type: string) { export function isLearningLogType(type: string) { return [LearningLogDocument, LearningLogPublication].indexOf(type) >= 0; } +export function isSupportType(type: string) { + return type === SupportPublication; +} export function isExemplarType(type: string) { return type === ExemplarDocument; } diff --git a/src/models/document/document-utils.ts b/src/models/document/document-utils.ts index 0712c82db8..f4a0584133 100644 --- a/src/models/document/document-utils.ts +++ b/src/models/document/document-utils.ts @@ -1,31 +1,70 @@ import { getParent } from "mobx-state-tree"; -import { IDocumentMetadata } from "../../../shared/shared"; -import { ProblemModelType } from "../curriculum/problem"; +import { upperFirst } from "lodash"; +import { IDocumentMetadataBase } from "../../../shared/shared"; import { SectionModelType } from "../curriculum/section"; -import { getSectionPath } from "../curriculum/unit"; +import { getSectionPath, UnitModelType } from "../curriculum/unit"; import { AppConfigModelType } from "../stores/app-config-model"; import { UserModelType } from "../stores/user"; import { DocumentModelType, IExemplarVisibilityProvider } from "./document"; import { DocumentContentModelType } from "./document-content"; -import { isExemplarType, isPlanningType, isProblemType, isPublishedType } from "./document-types"; +import { isExemplarType, isPlanningType, isProblemType, isPublishedType, isSupportType } from "./document-types"; +import { IDocumentMetadataModel } from "../stores/sorted-documents"; +import { getLocalTimeStamp } from "../../utilities/time"; + +function getProblemFromDoc(unit: UnitModelType, document: DocumentModelType | IDocumentMetadataModel) { + if (unit.code !== document.unit) { + return undefined; + } + const investigation = unit.getInvestigation(Number(document.investigation)); + const problem = investigation?.getProblem(Number(document.problem)); + return problem; +} + +function getDocumentTileFromProblem(currentUnit: UnitModelType, document: DocumentModelType | IDocumentMetadataModel) { + const {type, unit, investigation, problem} = document; + const problemModel = getProblemFromDoc(currentUnit, document); + if (problemModel) { + if (isPlanningType(type)) { + return `${problemModel.title}: Planning`; + } + return problemModel.title; + } + + const upperType = upperFirst(document.type); + if (!unit) { + return `${upperType} doc without unit`; + } + return `${upperType} doc from ${unit}-${investigation}.${problem}`; +} + +export function getDocumentTitleWithTimestamp( + document: DocumentModelType | IDocumentMetadataModel, + appConfig: AppConfigModelType +) { + const timeStampPropName = appConfig.docTimeStampPropertyName || undefined; + const timeStampProp = timeStampPropName && document.getProperty(timeStampPropName); + const timeStamp = timeStampProp + ? parseFloat(timeStampProp) + : undefined; + const timeStampStr = timeStamp ? getLocalTimeStamp(timeStamp) : undefined; + return timeStampStr + ? `${document.title} (${timeStampStr})` + : document.title; +} export function getDocumentDisplayTitle( - document: DocumentModelType, appConfig: AppConfigModelType, problem?: ProblemModelType, - unit?: string + unit: UnitModelType, + document: DocumentModelType | IDocumentMetadataModel, + appConfig: AppConfigModelType ) { const { type } = document; - const documentProblemOrdinal = `${document.investigation}.${document.problem}`; - const problemTitle = !(document.problem || document.investigation || document.unit) || - (documentProblemOrdinal === String(problem?.ordinal) && unit === document?.unit) - ? problem?.title || "Unknown Problem" - : "Unknown Problem"; - return document.isSupport - ? document.getProperty("caption") || "Support" - : isProblemType(type) - ? problemTitle - : isPlanningType(type) - ? `${problem?.title || "Unkown"}: Planning` - : document.getDisplayTitle(appConfig); + if (isSupportType(type)) { + return document.getProperty("caption") || "Support"; + } else if (isProblemType(type) || isPlanningType(type)) { + return getDocumentTileFromProblem(unit, document); + } else { + return getDocumentTitleWithTimestamp(document, appConfig); + } } /** @@ -48,7 +87,7 @@ export function getDocumentIdentifier(document?: DocumentContentModelType) { } export const isDocumentAccessibleToUser = ( - doc: IDocumentMetadata, user: UserModelType, documentStore: IExemplarVisibilityProvider + doc: IDocumentMetadataBase, user: UserModelType, documentStore: IExemplarVisibilityProvider ) => { const ownDocument = doc.uid === user.id; const isShared = doc.visibility === "public"; diff --git a/src/models/document/document.ts b/src/models/document/document.ts index 28b39b3abf..c5f5e367f3 100644 --- a/src/models/document/document.ts +++ b/src/models/document/document.ts @@ -15,7 +15,6 @@ import { } from "../../../shared/shared"; import { getFirebaseFunction } from "../../hooks/use-firebase-function"; import { IDocumentProperties } from "../../lib/db-types"; -import { getLocalTimeStamp } from "../../utilities/time"; import { safeJsonParse } from "../../utilities/js-utils"; import { Tree } from "../history/tree"; import { TreeMonitor } from "../history/tree-monitor"; @@ -131,17 +130,6 @@ export const DocumentModel = Tree.named("Document") }); return appConfig.getDocumentLabel(docStr, count, lowerCase); }, - getDisplayTitle(appConfig: AppConfigModelType) { - const timeStampPropName = appConfig.docTimeStampPropertyName || undefined; - const timeStampProp = timeStampPropName && self.getProperty(timeStampPropName); - const timeStamp = timeStampProp - ? parseFloat(timeStampProp) - : undefined; - const timeStampStr = timeStamp ? getLocalTimeStamp(timeStamp) : undefined; - return timeStampStr - ? `${self.title} (${timeStampStr})` - : self.title; - }, getDisplayId(appConfig: AppConfigModelType) { const { docDisplayIdPropertyName } = appConfig; if (!docDisplayIdPropertyName) return undefined; diff --git a/src/models/document/log-document-event.ts b/src/models/document/log-document-event.ts index c0a1404e55..ecf30aad4f 100644 --- a/src/models/document/log-document-event.ts +++ b/src/models/document/log-document-event.ts @@ -1,6 +1,7 @@ import { IDocumentMetadata } from "../../../shared/shared"; import { Logger } from "../../lib/logger"; import { LogEventMethod, LogEventName } from "../../lib/logger-types"; +import { IDocumentMetadataModel } from "../stores/sorted-documents"; import { UserModelType } from "../stores/user"; import { DocumentModelType } from "./document"; import { ExemplarDocument } from "./document-types"; @@ -11,7 +12,7 @@ interface ITeacherNetworkInfo { } export interface IDocumentLogEvent extends Record { - document: DocumentModelType | IDocumentMetadata; + document: DocumentModelType | IDocumentMetadata | IDocumentMetadataModel; } export function isDocumentLogEvent(params: Record): params is IDocumentLogEvent { @@ -59,7 +60,7 @@ export function logDocumentEvent(event: LogEventName, _params: IDocumentLogEvent * Convenience function to log the appropriate type of VIEW_SHOW_*_DOCUMENT event for the document. * @param document */ -export function logDocumentViewEvent(document: DocumentModelType | IDocumentMetadata) { +export function logDocumentViewEvent(document: DocumentModelType | IDocumentMetadata | IDocumentMetadataModel) { const isRemote = "isRemote" in document ? document.isRemote : undefined; const event = document.type === ExemplarDocument diff --git a/src/models/stores/sorted-documents.ts b/src/models/stores/sorted-documents.ts index 324986e2df..8d92d4750c 100644 --- a/src/models/stores/sorted-documents.ts +++ b/src/models/stores/sorted-documents.ts @@ -64,7 +64,13 @@ export const DocumentMetadataModel = types.model("DocumentMetadata", { problem: types.maybeNull(types.string), unit: types.maybeNull(types.string), visibility: types.maybe(types.string) -}); +}) +.views((self) => ({ + getProperty(key: string) { + return self.properties.get(key); + }, +})); + export interface IDocumentMetadataModel extends Instance {} export const MetadataDocMapModel = types.map(DocumentMetadataModel); From 9f8b6d29ae19f3ee5f02b9af4e18e2c2868c6eb9 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Wed, 25 Sep 2024 21:35:50 -0400 Subject: [PATCH 2/6] make sure documents have problem info This removes a duplicate creation of the metadata documents from the tree-manager. This duplication could create metadata documents for problem docs that didn't have problem, investigation, and unit info. It also add ths project, investigation, and unit info to the DocumentModels for problem, planning, and published document models which are created by the db listeners. This info is used by the title code now so it has to be set on the document models all of the time now. --- src/lib/db.ts | 34 +++++++++++++++++++----------- src/models/history/tree-manager.ts | 33 +++++++++++++++++++---------- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/lib/db.ts b/src/lib/db.ts index f24ff93fb9..cf5cf03bd0 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -426,6 +426,12 @@ export class DB { if (!docSnapshot.exists) { const { classHash, self, version, ...cleanedMetadata } = metadata as DBDocumentMetadata & { classHash: string }; + + let problemInfo: {unit:string|null, investigation?: string, problem?: string} = {unit: null}; + if ("offeringId" in metadata && metadata.offeringId != null) { + problemInfo = this.currentProblemInfo; + } + const firestoreMetadata: IDocumentMetadata & { contextId: string } = { ...cleanedMetadata, // The validateCommentableDocument firebase function currently deployed to production is out of date. @@ -434,17 +440,8 @@ export class DB { key: documentKey, properties: {}, uid: userContext.uid, - unit: null + ...problemInfo }; - if ("offeringId" in metadata && metadata.offeringId != null) { - const { investigation, problem, unit } = this.stores; - const investigationOrdinal = String(investigation.ordinal); - const problemOrdinal = String(problem.ordinal); - const unitCode = unit.code; - firestoreMetadata.investigation = investigationOrdinal; - firestoreMetadata.problem = problemOrdinal; - firestoreMetadata.unit = unitCode; - } const validateCommentableDocument = getFirebaseFunction("validateCommentableDocument_v1"); // FIXME-HISTORY: rename this function to validateFirestoreDocumentMetadata_v1 @@ -452,6 +449,15 @@ export class DB { } } + private get currentProblemInfo() { + const { investigation, problem, unit } = this.stores; + return { + investigation: String(investigation.ordinal), + problem: String(problem.ordinal), + unit: unit.code + }; + } + public async createDocument(params: { type: DBDocumentType, content?: string, title?: string }) { const { type, content, title } = params; const { user } = this.stores; @@ -795,12 +801,14 @@ export class DB { metadata: DBOfferingUserProblemDocument) { const {documentKey} = metadata; const group = this.stores.groups.groupForUser(userId); + const problemInfo = this.currentProblemInfo; return this.openDocument({ type, userId, groupId: group?.id, documentKey, - visibility: metadata.visibility + visibility: metadata.visibility, + ...problemInfo }); } @@ -836,6 +844,7 @@ export class DB { return allUsers; }, {} as DBGroupUserConnections); + const problemInfo = this.currentProblemInfo; return this.openDocument({ documentKey, type: "publication", @@ -843,7 +852,8 @@ export class DB { groupId, visibility: "public", groupUserConnections: groupUserConnectionsMap, - pubVersion + pubVersion, + ...problemInfo }); } diff --git a/src/models/history/tree-manager.ts b/src/models/history/tree-manager.ts index ea0c8ee9f7..35afe046c0 100644 --- a/src/models/history/tree-manager.ts +++ b/src/models/history/tree-manager.ts @@ -8,8 +8,7 @@ import { IUndoManager, UndoStore } from "./undo-store"; import { TreePatchRecord, HistoryEntry, TreePatchRecordSnapshot, HistoryOperation } from "./history"; import { DEBUG_HISTORY } from "../../lib/debug"; -import { getFirebaseFunction } from "../../hooks/use-firebase-function"; -import { getDocumentPath, ICommentableDocumentParams, IDocumentMetadata } from "../../../shared/shared"; +import { getDocumentPath, IDocumentMetadata } from "../../../shared/shared"; import { Firestore } from "../../lib/firestore"; import { UserModelType } from "../stores/user"; import { UserContextProvider } from "../stores/user-context-provider"; @@ -625,16 +624,28 @@ async function prepareFirestoreHistoryInfo( const documentPath = getDocumentPath(userContext.uid, mainDocument.key, userContext.network); const documentRef = firestore.doc(documentPath); - const docSnapshot = await documentRef.get(); - // create a document if necessary - if (!docSnapshot.exists) { - const validateCommentableDocument = - getFirebaseFunction("validateCommentableDocument_v1"); - - // FIXME-HISTORY: rename this function to validateFirestoreDocumentMetadata_v1 - await validateCommentableDocument({context: userContext, document: mainDocument.metadata}); - } + // The metadata documents are created by DB#createDocument however it does not wait for the metadata + // document to be created. So we might end up here before the metadata document has been created. + await new Promise((resolve, reject) => { + let timeoutId: NodeJS.Timeout | undefined = undefined; + const disposer = documentRef.onSnapshot(doc => { + if (doc.exists) { + resolve(); + if (timeoutId) { + disposer(); + clearTimeout(timeoutId); + } + } + }); + timeoutId = setTimeout(() => { + // If there isn't a firestore metadata document in 2 seconds then give up + disposer(); + console.warn("Could not find metadata document to attach history to", documentPath); + resolve(); + // TODO: how should we handle this error? + }, 2000); + }); const lastHistoryEntry = await getLastHistoryEntry(firestore, documentPath); From 0766c578139bc97d143407e1897c7b1b49f7eef4 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Wed, 25 Sep 2024 23:45:56 -0400 Subject: [PATCH 3/6] fix typo --- src/models/document/document-utils.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/document/document-utils.ts b/src/models/document/document-utils.ts index f4a0584133..064db89db9 100644 --- a/src/models/document/document-utils.ts +++ b/src/models/document/document-utils.ts @@ -20,7 +20,7 @@ function getProblemFromDoc(unit: UnitModelType, document: DocumentModelType | ID return problem; } -function getDocumentTileFromProblem(currentUnit: UnitModelType, document: DocumentModelType | IDocumentMetadataModel) { +function getDocumentTitleFromProblem(currentUnit: UnitModelType, document: DocumentModelType | IDocumentMetadataModel) { const {type, unit, investigation, problem} = document; const problemModel = getProblemFromDoc(currentUnit, document); if (problemModel) { @@ -61,7 +61,7 @@ export function getDocumentDisplayTitle( if (isSupportType(type)) { return document.getProperty("caption") || "Support"; } else if (isProblemType(type) || isPlanningType(type)) { - return getDocumentTileFromProblem(unit, document); + return getDocumentTitleFromProblem(unit, document); } else { return getDocumentTitleWithTimestamp(document, appConfig); } From 20fe7bb3b11000471508ef0db076fb0e0b9c7971 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 26 Sep 2024 10:50:53 -0400 Subject: [PATCH 4/6] describe what happens if a timeout happens --- src/models/history/tree-manager.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/models/history/tree-manager.ts b/src/models/history/tree-manager.ts index 35afe046c0..3fed92c55d 100644 --- a/src/models/history/tree-manager.ts +++ b/src/models/history/tree-manager.ts @@ -639,12 +639,20 @@ async function prepareFirestoreHistoryInfo( } }); timeoutId = setTimeout(() => { - // If there isn't a firestore metadata document in 2 seconds then give up + // If there isn't a firestore metadata document in 5 seconds then give up disposer(); console.warn("Could not find metadata document to attach history to", documentPath); - resolve(); - // TODO: how should we handle this error? - }, 2000); + // If there is an error here the history will not be saved for the duration + // of this CLUE session. + // This happens because the rejection will bubble up to completeHistoryEntry. + // That does not handle errors from this promise. The "then" function will + // not be called. The error should be printed as an unhandled promise error. + // The next time a history entry is "completed" this rejected promise + // will be "then'd" again which will again not run its function. + // TODO: consider updating this to create the metadata document itself by + // calling createFirestoreMetadataDocument. + reject(`Could not find metadata document to attach history to ${documentPath}`); + }, 5000); }); const lastHistoryEntry = await getLastHistoryEntry(firestore, documentPath); From 8aaffe3f9450616961d51b4c26684cf4dd3e247c Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 26 Sep 2024 16:19:37 -0400 Subject: [PATCH 5/6] add some tests --- src/models/document/document-utils.test.ts | 179 +++++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 src/models/document/document-utils.test.ts diff --git a/src/models/document/document-utils.test.ts b/src/models/document/document-utils.test.ts new file mode 100644 index 0000000000..03d2b0e499 --- /dev/null +++ b/src/models/document/document-utils.test.ts @@ -0,0 +1,179 @@ +import { UnitModel } from "../curriculum/unit"; +import { AppConfigModel } from "../stores/app-config-model"; +import { DocumentMetadataModel } from "../stores/sorted-documents"; +import { PersonalDocument, ProblemDocument, SupportPublication } from "./document-types"; +import { getDocumentDisplayTitle } from "./document-utils"; +import { unitConfigDefaults } from "../../test-fixtures/sample-unit-configurations"; + +describe("document utils", () => { + describe("getDocumentDisplayTitle", () => { + describe("support documents", () => { + test("without caption", () => { + const metadata = DocumentMetadataModel.create({ + type: SupportPublication, + uid: "123", + key: "123", + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create(); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Support"); + }); + test("with caption", () => { + const metadata = DocumentMetadataModel.create({ + type: SupportPublication, + uid: "123", + key: "123", + properties: { + caption: "Test Title" + } + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create(); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Test Title"); + }); + }); + + describe("personal documents", () => { + test("without title", () => { + const metadata = DocumentMetadataModel.create({ + type: PersonalDocument, + uid: "123", + key: "123", + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe(null); + }); + + test("with a title", () => { + const metadata = DocumentMetadataModel.create({ + type: PersonalDocument, + uid: "123", + key: "123", + title: "Test Title" + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Test Title"); + }); + + // NOTE: the default appConfig does not configure a timestamp property + // and none of the production units set this property. + // So really this timestamp feature is dead code in production + test("with a title and configured timestamp property", () => { + const metadata = DocumentMetadataModel.create({ + type: PersonalDocument, + uid: "123", + key: "123", + title: "Test Title", + properties: { + timeStamp: "193899600000" + } + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Test Title (23FEB76-00:00:00)"); + }); + }); + + describe("problem documents", () => { + test("without a unit", () => { + const metadata = DocumentMetadataModel.create({ + type: ProblemDocument, + uid: "123", + key: "123", + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Problem doc without unit"); + }); + test("from another unit", () => { + const metadata = DocumentMetadataModel.create({ + type: ProblemDocument, + uid: "123", + key: "123", + unit: "other", + investigation: "1", + problem: "1" + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Problem doc from other-1.1"); + }); + test("from the same unit but for a problem that doesn't exist", () => { + const metadata = DocumentMetadataModel.create({ + type: ProblemDocument, + uid: "123", + key: "123", + unit: "test", + investigation: "1", + problem: "1" + }); + const unit = UnitModel.create({ + code: "test", + title: "test" + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Problem doc from test-1.1"); + }); + test("from the same unit", () => { + const metadata = DocumentMetadataModel.create({ + type: ProblemDocument, + uid: "123", + key: "123", + unit: "test", + investigation: "1", + problem: "1" + }); + const unit = UnitModel.create({ + code: "test", + title: "Test Unit", + investigations: [ + { + ordinal: 1, + title: "Test Investigation", + problems: [ + { + ordinal: 1, + title: "Test Problem" + } + ] + } + ] + }); + const appConfig = AppConfigModel.create({config: unitConfigDefaults}); + const title = getDocumentDisplayTitle(unit, metadata, appConfig); + expect(title).toBe("Test Problem"); + }); + }); + }); +}); From f4a95b52d6a3ddef47408658b31054095cc09b9a Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 26 Sep 2024 16:45:46 -0400 Subject: [PATCH 6/6] make test immune to timezone --- src/models/document/document-utils.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/document/document-utils.test.ts b/src/models/document/document-utils.test.ts index 03d2b0e499..9b68870435 100644 --- a/src/models/document/document-utils.test.ts +++ b/src/models/document/document-utils.test.ts @@ -92,7 +92,7 @@ describe("document utils", () => { }); const appConfig = AppConfigModel.create({config: unitConfigDefaults}); const title = getDocumentDisplayTitle(unit, metadata, appConfig); - expect(title).toBe("Test Title (23FEB76-00:00:00)"); + expect(title).toMatch(/Test Title \(23FEB76-..:..:..\)/); }); });