From 42d79e8def54234e353d3f50f47cc9f0ff50e408 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Fri, 13 Sep 2024 09:23:38 -0400 Subject: [PATCH 1/3] reenable cypress test and fix some sort work issues Changes to the test: - need to comment on exemplar docs twice to make doc show up in the correct strategy sort group - deleting strategies is not supported, the test confirms the wrong behavior so we remember to fix it when we fix the behavior Other changes: - the requesting of the investigation by ordinal was wrong and was causing console warnings, fixing this also has fixed part of the issue with titles PT-188227517 - removed the firestoreTagDocumentMap, it was not actually being populated so was not really used in getTagsWithDocs. - handle the case where users comment on exemplar documents. Previously this was causing duplicate items in the sort work groups. --- .../teacher_sort_work_view_spec.js | 23 +++++++--- .../thumbnail/simple-document-item.tsx | 5 +-- src/hooks/document-comment-hooks.ts | 5 +++ src/models/stores/document-group.ts | 3 +- src/models/stores/sorted-documents.ts | 45 ++++++++++++------- src/utilities/sort-document-utils.ts | 19 +++----- 6 files changed, 58 insertions(+), 42 deletions(-) diff --git a/cypress/e2e/functional/teacher_tests/teacher_sort_work_view_spec.js b/cypress/e2e/functional/teacher_tests/teacher_sort_work_view_spec.js index f2c6146c2..1637bf6e0 100644 --- a/cypress/e2e/functional/teacher_tests/teacher_sort_work_view_spec.js +++ b/cypress/e2e/functional/teacher_tests/teacher_sort_work_view_spec.js @@ -175,8 +175,7 @@ describe('SortWorkView Tests', () => { }); - // TODO: Reinstate the tests below when all metadata documents have the new fields and are being updated in real time. - it.skip("should open Sort Work tab and test sorting by group", () => { + it("should open Sort Work tab and test sorting by group", () => { const students = ["student:1", "student:2", "student:3", "student:4"]; const studentProblemDocs = [ @@ -300,15 +299,20 @@ describe('SortWorkView Tests', () => { sortWork.getSortWorkItem().contains(exemplarDocs[0]).click(); chatPanel.getChatPanelToggle().click(); chatPanel.addCommentTagAndVerify("Diverging Designs"); + // FIXME: at the moment it is necessary to comment the document twice. + // Search for "exemplar" in document-comment-hooks.ts for an explanation. + cy.wait(100); + chatPanel.addCommentTagAndVerify("Diverging Designs"); cy.log("check that exemplar document is displayed in new tag"); chatPanel.getChatCloseButton().click(); cy.openTopTab('sort-work'); // at the moment this is required to refresh the sort - sortWork.getPrimarySortByMenu().click(); - sortWork.getPrimarySortByNameOption().click(); - sortWork.getPrimarySortByMenu().click(); - sortWork.getPrimarySortByTagOption().click(); + sortWork.getShowForMenu().click(); + sortWork.getShowForInvestigationOption().click(); + sortWork.getShowForMenu().click(); + sortWork.getShowForProblemOption().click(); + cy.get('.section-header-arrow').click({multiple: true}); // Open the sections sortWork.checkDocumentInGroup("Diverging Designs", exemplarDocs[0]); @@ -326,7 +330,12 @@ describe('SortWorkView Tests', () => { sortWork.getPrimarySortByTagOption().click(); cy.get('.section-header-arrow').click({multiple: true}); // Open the sections sortWork.checkDocumentInGroup("Unit Rate", exemplarDocs[0]); - sortWork.checkGroupIsEmpty("Diverging Designs"); + + // FIXME: We haven't implemented support for deleting comments + // what should be true: + // sortWork.checkGroupIsEmpty("Diverging Designs"); + // what currently happens + sortWork.checkDocumentInGroup("Diverging Designs", exemplarDocs[0]); cy.log("run CLUE as a student:1 and join group 6"); runClueAsStudent(students[0], 6); diff --git a/src/components/thumbnail/simple-document-item.tsx b/src/components/thumbnail/simple-document-item.tsx index ba48cbaa6..fe0820ea4 100644 --- a/src/components/thumbnail/simple-document-item.tsx +++ b/src/components/thumbnail/simple-document-item.tsx @@ -16,12 +16,11 @@ export const SimpleDocumentItem = ({ document, investigationOrdinal, onSelectDoc const { documents, class: classStore, unit, user } = useStores(); const { uid } = document; const userName = classStore.getUserById(uid)?.displayName; - const investigations = unit.investigations; // 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 = investigations[Number(investigationOrdinal)]; - const problem = investigation?.problems[Number(problemOrdinal) - 1]; + 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 isPrivate = !isDocumentAccessibleToUser(document, user, documents); diff --git a/src/hooks/document-comment-hooks.ts b/src/hooks/document-comment-hooks.ts index 41b470603..5de83972f 100644 --- a/src/hooks/document-comment-hooks.ts +++ b/src/hooks/document-comment-hooks.ts @@ -139,6 +139,11 @@ export const usePostDocumentComment = (options?: PostDocumentCommentUseMutationO // FIXME: provide access to remoteContext here so we can update strategies on remote // documents. Alternatively move this into a firebase function instead of doing this // in the client. + // FIXME: in the case of exemplar documents, the metadata document won't exist + // until this mutation happens. That probably means metadataQuery.get() below + // will run before the document has been created so it will return an empty array of + // docs. This is another reason the firebase function approach to keep the strategies + // updated is a better way to go const metadataQuery = firestore.collection("documents") .where("key", "==", documentKey) .where("context_id", "==", context.classHash); diff --git a/src/models/stores/document-group.ts b/src/models/stores/document-group.ts index 24db168fd..8bb92658a 100644 --- a/src/models/stores/document-group.ts +++ b/src/models/stores/document-group.ts @@ -45,7 +45,6 @@ export class DocumentGroup { stores: ISortedDocumentsStores; label: string; documents: IDocumentMetadata[]; - firestoreTagDocumentMap = new Map>(); icon?: FC>; constructor(props: IDocumentGroup) { @@ -98,7 +97,7 @@ export class DocumentGroup { get byStrategy(): DocumentGroup[] { const commentTags = this.stores.appConfig.commentTags; - const tagsWithDocs = getTagsWithDocs(this.documents, commentTags, this.firestoreTagDocumentMap); + const tagsWithDocs = getTagsWithDocs(this.documents, commentTags); const sortedDocsArr: DocumentGroup[] = []; Object.entries(tagsWithDocs).forEach((tagKeyAndValObj) => { diff --git a/src/models/stores/sorted-documents.ts b/src/models/stores/sorted-documents.ts index 1318b1b8d..6b7cbefd4 100644 --- a/src/models/stores/sorted-documents.ts +++ b/src/models/stores/sorted-documents.ts @@ -43,7 +43,6 @@ export interface ISortedDocumentsStores { export class SortedDocuments { stores: ISortedDocumentsStores; - firestoreTagDocumentMap = new Map>(); firestoreMetadataDocs: IObservableArray = observable.array([]); constructor(stores: ISortedDocumentsStores) { @@ -113,7 +112,7 @@ export class SortedDocuments { get byStrategy(): DocumentGroup[] { const commentTags = this.commentTags; - const tagsWithDocs = getTagsWithDocs(this.firestoreMetadataDocs, commentTags, this.firestoreTagDocumentMap); + const tagsWithDocs = getTagsWithDocs(this.firestoreMetadataDocs, commentTags); const sortedDocsArr: DocumentGroup[] = []; Object.entries(tagsWithDocs).forEach((tagKeyAndValObj) => { @@ -208,20 +207,34 @@ export class SortedDocuments { tools.push("Sparrow"); } - const metadata: IDocumentMetadata = { - uid: doc.uid, - type: doc.type, - key: doc.key, - createdAt: doc.createdAt, - title: doc.title, - properties: undefined, - tools, - strategies: exemplarStrategy ? [exemplarStrategy] : [], - investigation: doc.investigation, - problem: doc.problem, - unit: doc.unit - }; - docsArray.push(metadata); + const authoredStrategies = exemplarStrategy ? [exemplarStrategy] : []; + + const existingMetadataDoc = docsArray.find(metadataDoc => doc.key === metadataDoc.key); + if (existingMetadataDoc) { + // This will happen if a user comments on a exemplar + // That will create a metadata document in Firestore. + // So in this case we want to update this existing metadata document so we don't + // create a duplicate one + const userStrategies = existingMetadataDoc.strategies || []; + existingMetadataDoc.tools = tools; + existingMetadataDoc.strategies = [...new Set([...authoredStrategies, ...userStrategies])]; + } else { + const metadata: IDocumentMetadata = { + uid: doc.uid, + type: doc.type, + key: doc.key, + createdAt: doc.createdAt, + title: doc.title, + properties: undefined, + tools, + strategies: exemplarStrategy ? [exemplarStrategy] : [], + investigation: doc.investigation, + problem: doc.problem, + unit: doc.unit + }; + docsArray.push(metadata); + } + }); runInAction(() => { diff --git a/src/utilities/sort-document-utils.ts b/src/utilities/sort-document-utils.ts index 60175323b..c742a23ec 100644 --- a/src/utilities/sort-document-utils.ts +++ b/src/utilities/sort-document-utils.ts @@ -71,8 +71,10 @@ export const sortNameSectionLabels = (docMapKeys: string[]) => { }); }; -export const getTagsWithDocs = (documents: IDocumentMetadata[], commentTags: Record|undefined, - firestoreTagDocumentMap: Map>) => { +export const getTagsWithDocs = ( + documents: IDocumentMetadata[], + commentTags: Record|undefined, +) => { const tagsWithDocs: Record = {}; if (commentTags) { for (const key of Object.keys(commentTags)) { @@ -93,18 +95,7 @@ export const getTagsWithDocs = (documents: IDocumentMetadata[], commentTags: Rec // in store to find "Documents with no comments" then place those doc keys to "Not Tagged" const uniqueDocKeysWithTags = new Set(); - // grouping documents based on firestore comment tags - firestoreTagDocumentMap.forEach((docKeysSet, tag) => { - const docKeysArray = Array.from(docKeysSet); // Convert the Set to an array - if (tagsWithDocs[tag]) { - docKeysSet.forEach((docKey: string) =>{ - uniqueDocKeysWithTags.add(docKey); - }); - tagsWithDocs[tag].docKeysFoundWithTag = docKeysArray; - } - }); - - // adding in (exemplar) documents with authored tags + // Sort documents into their groups documents.forEach(doc => { doc.strategies?.forEach(strategy => { if (tagsWithDocs[strategy]) { From 073a763ca9276e0b7d0d0a7331eb902978623b1b Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Fri, 13 Sep 2024 09:59:28 -0400 Subject: [PATCH 2/3] decrease Cypress timeout when running locally --- cypress/config/cypress.dev.json | 4 ---- cypress/config/cypress.local.json | 3 ++- cypress/run_cypress_test.sh | 19 ------------------- package.json | 1 - 4 files changed, 2 insertions(+), 25 deletions(-) delete mode 100644 cypress/config/cypress.dev.json delete mode 100755 cypress/run_cypress_test.sh diff --git a/cypress/config/cypress.dev.json b/cypress/config/cypress.dev.json deleted file mode 100644 index 2aafc67eb..000000000 --- a/cypress/config/cypress.dev.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "baseUrl": "http://localhost:8080/" -} - diff --git a/cypress/config/cypress.local.json b/cypress/config/cypress.local.json index d36314102..00ea18a12 100644 --- a/cypress/config/cypress.local.json +++ b/cypress/config/cypress.local.json @@ -1,4 +1,5 @@ { "baseUrl": "http://localhost:8080/", - "hideXHRInCommandLog": true + "hideXHRInCommandLog": true, + "defaultCommandTimeout": 5000 } diff --git a/cypress/run_cypress_test.sh b/cypress/run_cypress_test.sh deleted file mode 100755 index 47fe7341a..000000000 --- a/cypress/run_cypress_test.sh +++ /dev/null @@ -1,19 +0,0 @@ -#!/bin/bash - -declare -a BRANCH_ARRAY=("master" "production" "dataflow" "dataflow_production") - -npm run start & -wait-on http://localhost:8080 -echo "start TRAVIS_BRANCH=$TRAVIS_BRANCH" -if [[ "$TRAVIS_COMMIT_MESSAGE" == *"[dev-build]"* ]]; then - echo "dev-build" - npm run test:cypress:smoke -elif !(echo $BRANCH_ARRAY | grep -q $TRAVIS_BRANCH); then - echo "elif TRAVIS_BRANCH=$TRAVIS_BRANCH" - npm run test:cypress:branch -else - echo "else TRAVIS_BRANCH=$TRAVIS_BRANCH" - npm run test:cypress:ci -fi - - \ No newline at end of file diff --git a/package.json b/package.json index 72a0dd5a2..040e8f201 100644 --- a/package.json +++ b/package.json @@ -97,7 +97,6 @@ "test:coverage:watch": "jest --coverage --watchAll", "test:coverage:cypress:open": "cypress open --env coverage=true", "test:cypress": "cypress run --env testEnv=local", - "test:cypress:ci": "cypress run --env testEnv=local --record", "test:cypress:open": "cypress open --env testEnv=local", "test:cypress:open:disable-gpu": "cross-env ELECTRON_EXTRA_LAUNCH_ARGS=--disable-gpu cypress open --env testEnv=local", "test:cypress:smoke": "cypress run --spec 'cypress/e2e/smoke/single_student_canvas_test.js' --env testEnv=local", From e757aaad080be4f9273030012d7cea12ae6bb155 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Fri, 13 Sep 2024 11:08:04 -0400 Subject: [PATCH 3/3] include fixes that Ethan found in a closed PR --- src/components/document/sorted-section.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/document/sorted-section.tsx b/src/components/document/sorted-section.tsx index 5361b6b93..5b9f15839 100644 --- a/src/components/document/sorted-section.tsx +++ b/src/components/document/sorted-section.tsx @@ -23,7 +23,7 @@ interface IProps { secondarySort: SecondarySortType; } -export const SortedSection: React.FC = observer(function SortedDocuments(props: IProps) { +export const SortedSection: React.FC = observer(function SortedSection(props: IProps) { const { docFilter, documentGroup, idx, secondarySort } = props; const { persistentUI, sortedDocuments } = useStores(); const [showDocuments, setShowDocuments] = useState(false); @@ -51,7 +51,7 @@ export const SortedSection: React.FC = observer(function SortedDocuments const renderUngroupedDocument = (doc: IDocumentMetadata) => { const fullDocument = getDocument(doc.key); - if (!fullDocument) return
; + if (!fullDocument) return
; return