From 6ff3cfb9d02dee318074a50e8d35f89eb04e1790 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 30 Mar 2023 11:43:52 -0400 Subject: [PATCH 1/5] remove unused user id from group update function --- src/lib/db-listeners/db-groups-listener.ts | 4 ++-- src/models/stores/groups.test.ts | 4 ++-- src/models/stores/groups.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/db-listeners/db-groups-listener.ts b/src/lib/db-listeners/db-groups-listener.ts index f9d54a56be..9f19db4817 100644 --- a/src/lib/db-listeners/db-groups-listener.ts +++ b/src/lib/db-listeners/db-groups-listener.ts @@ -26,7 +26,7 @@ export class DBGroupsListener extends BaseListener { const dbGroups: DBOfferingGroupMap = snapshot.val() || {}; this.debugLogSnapshot("#start", snapshot); // Groups may be invalid at this point, but the listener will resolve it once connection times are set - groups.updateFromDB(user.id, dbGroups, this.db.stores.class); + groups.updateFromDB(dbGroups, this.db.stores.class); const group = groups.groupForUser(user.id); if (group) { @@ -101,7 +101,7 @@ export class DBGroupsListener extends BaseListener { } else { // otherwise set the groups - this.db.stores.groups.updateFromDB(user.id, groups, this.db.stores.class); + this.db.stores.groups.updateFromDB(groups, this.db.stores.class); // in teacher mode we listen to all documents and the document's group might change // if a student changes groups so we need to gather the updated group id for each diff --git a/src/models/stores/groups.test.ts b/src/models/stores/groups.test.ts index 612d962a9c..e8cd72762c 100644 --- a/src/models/stores/groups.test.ts +++ b/src/models/stores/groups.test.ts @@ -127,11 +127,11 @@ describe("Groups model", () => { } }); - groups.updateFromDB("1", dbGroupsWithoutUsers, clazz); + groups.updateFromDB(dbGroupsWithoutUsers, clazz); expect(groups.allGroups.length).toEqual(1); expect(groups.allGroups[0].users.length).toEqual(0); - groups.updateFromDB("1", dbGroupsWithUsers, clazz); + groups.updateFromDB(dbGroupsWithUsers, clazz); expect(groups.allGroups.length).toEqual(1); expect(groups.allGroups[0].users.length).toEqual(1); expect(groups.allGroups[0].users[0].id).toEqual("1"); diff --git a/src/models/stores/groups.ts b/src/models/stores/groups.ts index 91560365ba..c0bdc236df 100644 --- a/src/models/stores/groups.ts +++ b/src/models/stores/groups.ts @@ -35,7 +35,7 @@ export const GroupsModel = types acceptUnknownStudents: false }) .actions((self) => ({ - updateFromDB(uid: string, groups: DBOfferingGroupMap, clazz: ClassModelType) { + updateFromDB(groups: DBOfferingGroupMap, clazz: ClassModelType) { const allGroups = Object.keys(groups).map((groupId) => { const group = groups[groupId]; const groupUsers = group.users || {}; From 5110161ab78fde3956768e3fad9a69cba2562308 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 30 Mar 2023 16:47:05 -0400 Subject: [PATCH 2/5] add currentGroupId and use it for monitoring docs because it is hard to test this, I also tried to change all of the places that were finding the users current group to use the same approach. Hopefully that will turn up issues with the new currentGroupId more quickly. --- .../full/student_tests/group_chooser_spec.js | 22 ++++++++++++++----- src/clue/components/clue-app-header.tsx | 2 +- src/components/app.tsx | 2 +- src/components/document/document.tsx | 2 +- src/lib/db-listeners/db-groups-listener.ts | 4 +++- .../db-problem-documents-listener.ts | 4 +++- src/lib/db.ts | 4 +++- src/lib/logger.ts | 4 ++-- src/models/stores/user.test.ts | 16 +++++++++++++- src/models/stores/user.ts | 11 ++++++++++ 10 files changed, 57 insertions(+), 14 deletions(-) diff --git a/cypress/e2e/clue/full/student_tests/group_chooser_spec.js b/cypress/e2e/clue/full/student_tests/group_chooser_spec.js index 5f63d6e8ec..929c4dc795 100644 --- a/cypress/e2e/clue/full/student_tests/group_chooser_spec.js +++ b/cypress/e2e/clue/full/student_tests/group_chooser_spec.js @@ -20,9 +20,15 @@ describe('Test student join a group', function(){ cy.clearQAData('all'); }); - function setup(student, alreadyInGroup=false){ - cy.visit('?appMode=qa&fakeClass='+fakeClass+'&fakeUser=student:'+student+'&problem='+problem); - if (alreadyInGroup) { + const defaultSetupOptions = { + alreadyInGroup: false, + problem + }; + + function setup(student, opts={} ){ + const options = {...defaultSetupOptions, ...opts}; + cy.visit('?appMode=qa&fakeClass='+fakeClass+'&fakeUser=student:'+student+'&problem='+options.problem); + if (options.alreadyInGroup) { // This is looking for the version div in the header cy.waitForLoad(); } else { @@ -94,7 +100,7 @@ describe('Test student join a group', function(){ }); it('will verify cancel of leave group dialog',function(){ //have student leave first group and join second group - setup(student5, true); + setup(student5, {alreadyInGroup: true}); cy.get('.app .group > .name').contains('Group '+group1).click(); cy.get('#cancelButton').should('contain','No').click(); clueHeader.getGroupName().should('contain','Group '+group1); @@ -104,7 +110,7 @@ describe('Test student join a group', function(){ }); it('will verify a student can switch groups',function(){ //have student leave first group and join second group - setup(student5, true); + setup(student5, {alreadyInGroup: true}); cy.get('.app .group > .name').contains('Group '+group1).click(); cy.get("#okButton").should('contain','Yes').click(); cy.get('.groups > .group-list > .group').contains('Group '+group2).click(); @@ -121,4 +127,10 @@ describe('Test student join a group', function(){ header.getUserName().should('contain','Student '+student6); clueHeader.getGroupMembers().should('contain','S'+student1).and('contain','S'+student2).and('contain','S'+student4).and('contain','S'+student6); }); + it('Student will automatically join last group number in new problem', function(){ + setup(student1, {alreadyInGroup: true, problem: '2.3'}); + clueHeader.getGroupName().should('contain','Group '+group1); + header.getUserName().should('contain','Student '+student1); + clueHeader.getGroupMembers().should('contain','S'+student1).and('have.length', 1); + }); }); diff --git a/src/clue/components/clue-app-header.tsx b/src/clue/components/clue-app-header.tsx index b2e48e001d..33dc5dc88e 100644 --- a/src/clue/components/clue-app-header.tsx +++ b/src/clue/components/clue-app-header.tsx @@ -25,7 +25,7 @@ interface IProps extends IBaseProps { export const ClueAppHeaderComponent: React.FC = observer(function ClueAppHeaderComponent(props) { const { showGroup } = props; const { appConfig, appMode, appVersion, db, user, problem, groups, investigation, ui, unit } = useStores(); - const myGroup = showGroup ? groups.groupForUser(user.id) : undefined; + const myGroup = showGroup ? groups.getGroupById(user.currentGroupId) : undefined; const userTitle = appMode !== "authed" && appMode !== "demo" ? `Firebase UID: ${db.firebase.userId}` : undefined; diff --git a/src/components/app.tsx b/src/components/app.tsx index 972f29a016..5a826d18e1 100644 --- a/src/components/app.tsx +++ b/src/components/app.tsx @@ -184,7 +184,7 @@ export class AppComponent extends BaseComponent { } if (user.isStudent) { - if (!groups.groupForUser(user.id)) { + if (!user.currentGroupId) { if (appConfig.autoAssignStudentsToIndividualGroups || this.stores.isPreviewing) { // use userId as groupId db.joinGroup(user.id); diff --git a/src/components/document/document.tsx b/src/components/document/document.tsx index b812f0b7c1..7c69ec49a1 100644 --- a/src/components/document/document.tsx +++ b/src/components/document/document.tsx @@ -276,7 +276,7 @@ export class DocumentComponent extends BaseComponent { const stickyNotes = supports.getStickyNotesForUserProblem({ userId: user.id, - groupId: user.latestGroupId + groupId: user.currentGroupId }).filter((support) => support.supportType === SupportType.teacher) as TeacherSupportModelType[]; stickyNotes.sort((a, b) => b.authoredTime - a.authoredTime); diff --git a/src/lib/db-listeners/db-groups-listener.ts b/src/lib/db-listeners/db-groups-listener.ts index 9f19db4817..0b3bfb2b98 100644 --- a/src/lib/db-listeners/db-groups-listener.ts +++ b/src/lib/db-listeners/db-groups-listener.ts @@ -113,13 +113,15 @@ export class DBGroupsListener extends BaseListener { }); }); + user.setCurrentGroupId(userGroupIds[user.id]); + documents.byType(ProblemDocument).forEach((document) => { document.setGroupId(userGroupIds[document.uid]); // enable/disable monitoring of other students' documents when groups change if (user.isStudent && (document.uid !== user.id)) { // students only monitor documents in their group to save bandwidth - if (document.groupId === user.latestGroupId) { + if (document.groupId === user.currentGroupId) { // ensure the group document is monitored this.db.listeners.monitorDocument(document, Monitor.Remote); } diff --git a/src/lib/db-listeners/db-problem-documents-listener.ts b/src/lib/db-listeners/db-problem-documents-listener.ts index 2d96804d63..eaba942a5b 100644 --- a/src/lib/db-listeners/db-problem-documents-listener.ts +++ b/src/lib/db-listeners/db-problem-documents-listener.ts @@ -91,7 +91,9 @@ export class DBProblemDocumentsListener extends BaseListener { // both teachers and students listen to all problem documents // but only teachers listen to all content. students only listen // to content of users in their group to reduce network traffic - const userInGroup = groups.userInGroup(document.self.uid, currentUser.latestGroupId); + // TODO: maybe currentGroupId isn't always available here and that can cause + // a problem? + const userInGroup = groups.userInGroup(document.self.uid, currentUser.currentGroupId); // Local changes take precedence over remote changes const monitor = isOwnDocument ? Monitor.Local diff --git a/src/lib/db.ts b/src/lib/db.ts index d7778530fa..3fb7c34c22 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -442,7 +442,9 @@ export class DB { return new Promise<{document: DBDocument, metadata: DBPublicationDocumentMetadata}>((resolve, reject) => { this.createDocument({ type: ProblemPublication, content }).then(({document, metadata}) => { const publicationRef = this.firebase.ref(this.firebase.getProblemPublicationsPath(user)).push(); - const userGroup = groups.groupForUser(user.id)!; + // TODO: this is assuming there is always a group for this current user, that doesn't seem + // safe??? + const userGroup = groups.getGroupById(user.currentGroupId)!; const groupUserConnections: DBGroupUserConnections = userGroup && userGroup.users .filter(groupUser => groupUser.id !== user.id) .reduce((allUsers: DBGroupUserConnections, groupUser) => { diff --git a/src/lib/logger.ts b/src/lib/logger.ts index 56a39214c0..70044618b2 100644 --- a/src/lib/logger.ts +++ b/src/lib/logger.ts @@ -105,7 +105,7 @@ export class Logger { const { appConfig: { appName }, appMode, problemPath, ui: { activeGroupId, activeNavTab, navTabContentShown, problemWorkspace, teacherPanelKey }, - user: { activityUrl, classHash, id, isStudent, isTeacher, portal, type, latestGroupId, + user: { activityUrl, classHash, id, isStudent, isTeacher, portal, type, currentGroupId, loggingRemoteEndpoint, firebaseDisconnects, loggingDisconnects, networkStatusAlerts }} = this.stores; // only log disconnect counts if there have been any disconnections @@ -138,7 +138,7 @@ export class Logger { } if (isStudent) { - logMessage.group = latestGroupId; + logMessage.group = currentGroupId; logMessage.workspaceMode = problemWorkspace.mode; } if (isTeacher) { diff --git a/src/models/stores/user.test.ts b/src/models/stores/user.test.ts index f6e4bef3a4..5e88e1f972 100644 --- a/src/models/stores/user.test.ts +++ b/src/models/stores/user.test.ts @@ -40,6 +40,7 @@ describe("user model", () => { expect(user.name).toBe("Anonymous User"); expect(user.className).toBe(""); expect(user.latestGroupId).toBeUndefined(); + expect(user.currentGroupId).toBeUndefined(); expect(user.id).toBe("0"); }); @@ -50,6 +51,7 @@ describe("user model", () => { name: "Test User", className: "Test Class", latestGroupId: "1", + currentGroupId: "2", id: "2", }); expect(user.authenticated).toBe(true); @@ -57,6 +59,7 @@ describe("user model", () => { expect(user.name).toBe("Test User"); expect(user.className).toBe("Test Class"); expect(user.latestGroupId).toBe("1"); + expect(user.currentGroupId).toBe("2"); expect(user.id).toBe("2"); expect(user.initials).toBe("TU"); }); @@ -85,13 +88,20 @@ describe("user model", () => { expect(user.className).toBe(className); }); - it("can set a group", () => { + it("can set a latest group", () => { const user = UserModel.create(); const group = "1"; user.setLatestGroupId(group); expect(user.latestGroupId).toBe(group); }); + it("can set a current group", () => { + const user = UserModel.create(); + const group = "1"; + user.setCurrentGroupId(group); + expect(user.currentGroupId).toBe(group); + }); + it("can set an id", () => { const user = UserModel.create(); const id = "1"; @@ -121,6 +131,7 @@ describe("user model", () => { expect(user.name).toBe(authenticatedUser.fullName); expect(user.className).toBe(authenticatedUser.className); expect(user.latestGroupId).toBe(undefined); + expect(user.currentGroupId).toBe(undefined); expect(user.isStudent).toBe(true); expect(user.isTeacher).toBe(false); expect(user.isNetworkedTeacher).toBe(false); @@ -153,6 +164,7 @@ describe("user model", () => { expect(user.name).toBe(authenticatedUser.fullName); expect(user.className).toBe(authenticatedUser.className); expect(user.latestGroupId).toBeUndefined(); + expect(user.currentGroupId).toBeUndefined(); expect(user.isStudent).toBe(false); expect(user.isTeacher).toBe(true); expect(user.isNetworkedTeacher).toBe(false); @@ -188,6 +200,7 @@ describe("user model", () => { expect(user.name).toBe(authenticatedUser.fullName); expect(user.className).toBe(authenticatedUser.className); expect(user.latestGroupId).toBeUndefined(); + expect(user.currentGroupId).toBeUndefined(); expect(user.isStudent).toBe(false); expect(user.isTeacher).toBe(true); expect(user.isNetworkedTeacher).toBe(true); @@ -217,6 +230,7 @@ describe("user model", () => { expect(user.name).toBe(authenticatedUser.fullName); expect(user.className).toBe(authenticatedUser.className); expect(user.latestGroupId).toBeUndefined(); + expect(user.currentGroupId).toBeUndefined(); expect(user.isStudent).toBe(false); expect(user.isTeacher).toBe(true); expect(user.isNetworkedTeacher).toBe(false); diff --git a/src/models/stores/user.ts b/src/models/stores/user.ts index a539531662..05516ce703 100644 --- a/src/models/stores/user.ts +++ b/src/models/stores/user.ts @@ -36,7 +36,15 @@ export const UserModel = types className: "", classHash: "", offeringId: "", + // This is the last group the user joined. It is synced with the latestGroupId + // property in Firebase latestGroupId: types.maybe(types.string), + // This is the group of this user in the particular offering that is being + // run right now. There is a view which will return this value if set otherwise + // it returns latestGroupId. There might be a period when CLUE is being loaded + // where the currentGroupId is not set yet. This would happen if the user hasn't + // chosen a group yet. Or if groups haven't been loaded yet. + currentGroupId: types.maybe(types.string), portal: "", network: types.maybe(types.string), networks: types.array(types.string), @@ -69,6 +77,9 @@ export const UserModel = types setLatestGroupId(latestGroupId?: string) { self.latestGroupId = latestGroupId; }, + setCurrentGroupId(currentGroupId?: string) { + self.currentGroupId = currentGroupId; + }, setId(id: string) { self.id = id; }, From efbf859372af90633c17ddb881d8aa2fe10f0c42 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Thu, 30 Mar 2023 20:31:54 -0400 Subject: [PATCH 3/5] use an autorun to monitor the groups and documents This approach fixes the problem of some student's docs not being monitored in the 4-up view. However it results in a lot of churn, so it needs more work. --- src/lib/db-listeners/db-groups-listener.ts | 77 ++++++++++++++-------- src/models/stores/groups.ts | 20 ++++-- 2 files changed, 65 insertions(+), 32 deletions(-) diff --git a/src/lib/db-listeners/db-groups-listener.ts b/src/lib/db-listeners/db-groups-listener.ts index 0b3bfb2b98..b07ad98e2b 100644 --- a/src/lib/db-listeners/db-groups-listener.ts +++ b/src/lib/db-listeners/db-groups-listener.ts @@ -4,10 +4,12 @@ import { DBOfferingGroupMap } from "../db-types"; import { ProblemDocument } from "../../models/document/document-types"; import { map } from "lodash"; import { BaseListener } from "./base-listener"; +import { autorun } from "mobx"; export class DBGroupsListener extends BaseListener { private db: DB; private groupsRef: firebase.database.Reference | null = null; + private groupsMonitorDisposer: any; constructor(db: DB) { super("DBGroupsListener"); @@ -15,6 +17,50 @@ export class DBGroupsListener extends BaseListener { } public start() { + // TODO: we probably want to improve this so each new document + // doesn't trigger a bunch of monitor and unmonitor calls + // If we had the list of documents to monitor and we could get + // the list of documents that were currently monitored, then + // we could just rerun the code if the list to monitor changes + // and when it changes we could unmonitor everything not in the list + // that is a Problem document. + // + // Ideally we'd have a computed value which was the documents to + // monitor. We don't have a good place to put computed values like + // this, other than making a new MST Object. And this would just + // be for students without additional work so the documents + // are only monitored. To make it worse the documents start out + // monitored. + // + // This is important for this reason: + // https://firebase.google.com/docs/firestore/best-practices#realtime_updates + this.groupsMonitorDisposer = autorun(() => { + console.log("running groupsMonitor"); + const {user, groups, documents} = this.db.stores; + + // in teacher mode we listen to all documents and the document's group might change + // if a student changes groups so we need to gather the updated group id for each + // student and set it for the student's problem documents + documents.byType(ProblemDocument).forEach((document) => { + const groupId = groups.groupIdForUser(document.uid); + document.setGroupId(groupId); + + // enable/disable monitoring of other students' documents when groups change + if (user.isStudent && (document.uid !== user.id)) { + // students only monitor documents in their group to save bandwidth + if (document.groupId === user.currentGroupId) { + // ensure the group document is monitored + this.db.listeners.monitorDocument(document, Monitor.Remote); + } + else { + // ensure we don't monitor documents outside the group + this.db.listeners.unmonitorDocument(document, Monitor.Remote); + } + } + }); + + }); + return new Promise((resolve, reject) => { const {user, groups} = this.db.stores; const groupsRef = this.groupsRef = this.db.firebase.ref(this.db.firebase.getGroupsPath(user)); @@ -54,10 +100,11 @@ export class DBGroupsListener extends BaseListener { this.groupsRef.off("value", this.handleGroupsRef); this.groupsRef = null; } + this.groupsMonitorDisposer?.(); } private handleGroupsRef = (snapshot: firebase.database.DataSnapshot) => { - const {user, documents} = this.db.stores; + const {user} = this.db.stores; const groups: DBOfferingGroupMap = snapshot.val() || {}; const myGroupIds: string[] = []; const overSubscribedUserUpdates: any = {}; @@ -103,34 +150,8 @@ export class DBGroupsListener extends BaseListener { // otherwise set the groups this.db.stores.groups.updateFromDB(groups, this.db.stores.class); - // in teacher mode we listen to all documents and the document's group might change - // if a student changes groups so we need to gather the updated group id for each - // student and set it for the student's problem documents - const userGroupIds: any = {}; - this.db.stores.groups.allGroups.forEach((group) => { - group.users.forEach((groupUser) => { - userGroupIds[groupUser.id] = group.id; - }); - }); - - user.setCurrentGroupId(userGroupIds[user.id]); + user.setCurrentGroupId(this.db.stores.groups.groupIdForUser(user.id)); - documents.byType(ProblemDocument).forEach((document) => { - document.setGroupId(userGroupIds[document.uid]); - - // enable/disable monitoring of other students' documents when groups change - if (user.isStudent && (document.uid !== user.id)) { - // students only monitor documents in their group to save bandwidth - if (document.groupId === user.currentGroupId) { - // ensure the group document is monitored - this.db.listeners.monitorDocument(document, Monitor.Remote); - } - else { - // ensure we don't monitor documents outside the group - this.db.listeners.unmonitorDocument(document, Monitor.Remote); - } - } - }); } }; } diff --git a/src/models/stores/groups.ts b/src/models/stores/groups.ts index c0bdc236df..287d12a774 100644 --- a/src/models/stores/groups.ts +++ b/src/models/stores/groups.ts @@ -67,10 +67,22 @@ export const GroupsModel = types } })) .views((self) => ({ - groupForUser(uid: string) { - return self.allGroups.find((group) => { - return !!group.users.find((user) => user.id === uid); + get groupsByUser() { + const groupsByUser: Record = {}; + self.allGroups.forEach((group) => { + group.users.forEach((groupUser) => { + groupsByUser[groupUser.id] = group; + }); }); + return groupsByUser; + } + })) + .views((self) => ({ + groupForUser(uid: string) { + return self.groupsByUser[uid]; + }, + groupIdForUser(uid: string) { + return self.groupsByUser[uid]?.id; }, get groupVirtualDocuments() { return self.allGroups.map((group) => { @@ -79,7 +91,7 @@ export const GroupsModel = types }, getGroupById(id?: string) { return self.allGroups.find(group => group.id === id); - } + }, })) .views((self) => ({ userInGroup(uid: string, groupId?: string) { From 528171e2d160dbc50e2bd7d8b1003ef8af76f4e4 Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Fri, 31 Mar 2023 11:27:25 -0400 Subject: [PATCH 4/5] fixups after merging refactored monitors - use new groups.groupIdForUser - remove duplicate autorun from groups listener --- .../db-listeners/db-doc-content-listener.ts | 9 +--- src/lib/db-listeners/db-groups-listener.ts | 47 ------------------- 2 files changed, 1 insertion(+), 55 deletions(-) diff --git a/src/lib/db-listeners/db-doc-content-listener.ts b/src/lib/db-listeners/db-doc-content-listener.ts index 670bbf1227..1424702ad8 100644 --- a/src/lib/db-listeners/db-doc-content-listener.ts +++ b/src/lib/db-listeners/db-doc-content-listener.ts @@ -54,13 +54,6 @@ export class DBDocumentContentListener extends BaseListener { this.disposer = autorun(() => { const {documents, groups, user} = this.db.stores; - const userGroupIds: any = {}; - groups.allGroups.forEach((group) => { - group.users.forEach((groupUser) => { - userGroupIds[groupUser.id] = group.id; - }); - }); - const documentsToMonitor: DocumentModelType[] = []; documents.byType(ProblemDocument).forEach((document) => { @@ -68,7 +61,7 @@ export class DBDocumentContentListener extends BaseListener { // id of the document. A new document could be added with an out of date // group id. Or a user's group can change. In both cases the document // should be updated. - document.setGroupId(userGroupIds[document.uid]); + document.setGroupId(groups.groupIdForUser(document.uid)); // Users don't monitor their own documents if ((document.uid === user.id)) { diff --git a/src/lib/db-listeners/db-groups-listener.ts b/src/lib/db-listeners/db-groups-listener.ts index a7d6c0acf9..b65705118d 100644 --- a/src/lib/db-listeners/db-groups-listener.ts +++ b/src/lib/db-listeners/db-groups-listener.ts @@ -3,12 +3,10 @@ import { DB } from "../db"; import { DBOfferingGroupMap } from "../db-types"; import { map } from "lodash"; import { BaseListener } from "./base-listener"; -import { autorun } from "mobx"; export class DBGroupsListener extends BaseListener { private db: DB; private groupsRef: firebase.database.Reference | null = null; - private groupsMonitorDisposer: any; constructor(db: DB) { super("DBGroupsListener"); @@ -16,50 +14,6 @@ export class DBGroupsListener extends BaseListener { } public start() { - // TODO: we probably want to improve this so each new document - // doesn't trigger a bunch of monitor and unmonitor calls - // If we had the list of documents to monitor and we could get - // the list of documents that were currently monitored, then - // we could just rerun the code if the list to monitor changes - // and when it changes we could unmonitor everything not in the list - // that is a Problem document. - // - // Ideally we'd have a computed value which was the documents to - // monitor. We don't have a good place to put computed values like - // this, other than making a new MST Object. And this would just - // be for students without additional work so the documents - // are only monitored. To make it worse the documents start out - // monitored. - // - // This is important for this reason: - // https://firebase.google.com/docs/firestore/best-practices#realtime_updates - this.groupsMonitorDisposer = autorun(() => { - console.log("running groupsMonitor"); - const {user, groups, documents} = this.db.stores; - - // in teacher mode we listen to all documents and the document's group might change - // if a student changes groups so we need to gather the updated group id for each - // student and set it for the student's problem documents - documents.byType(ProblemDocument).forEach((document) => { - const groupId = groups.groupIdForUser(document.uid); - document.setGroupId(groupId); - - // enable/disable monitoring of other students' documents when groups change - if (user.isStudent && (document.uid !== user.id)) { - // students only monitor documents in their group to save bandwidth - if (document.groupId === user.currentGroupId) { - // ensure the group document is monitored - this.db.listeners.monitorDocument(document, Monitor.Remote); - } - else { - // ensure we don't monitor documents outside the group - this.db.listeners.unmonitorDocument(document, Monitor.Remote); - } - } - }); - - }); - return new Promise((resolve, reject) => { const {user, groups} = this.db.stores; const groupsRef = this.groupsRef = this.db.firebase.ref(this.db.firebase.getGroupsPath(user)); @@ -99,7 +53,6 @@ export class DBGroupsListener extends BaseListener { this.groupsRef.off("value", this.handleGroupsRef); this.groupsRef = null; } - this.groupsMonitorDisposer?.(); } private handleGroupsRef = (snapshot: firebase.database.DataSnapshot) => { From 8cec24f2ccb47d63083ecc82fdb131c7c0869c9f Mon Sep 17 00:00:00 2001 From: Scott Cytacki Date: Fri, 31 Mar 2023 14:25:30 -0400 Subject: [PATCH 5/5] address PR comments --- src/components/app.tsx | 2 +- src/lib/db.ts | 6 ++---- src/models/stores/user.ts | 6 ++---- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/components/app.tsx b/src/components/app.tsx index 5a826d18e1..c5c57bc191 100644 --- a/src/components/app.tsx +++ b/src/components/app.tsx @@ -160,7 +160,7 @@ export class AppComponent extends BaseComponent { // time. public render() { - const {appConfig, user, ui, db, groups} = this.stores; + const {appConfig, user, ui, db} = this.stores; if (ui.showDemoCreator) { return this.renderApp(); diff --git a/src/lib/db.ts b/src/lib/db.ts index 4a75c9ae91..91540acf3e 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -436,10 +436,8 @@ export class DB { return new Promise<{document: DBDocument, metadata: DBPublicationDocumentMetadata}>((resolve, reject) => { this.createDocument({ type: ProblemPublication, content }).then(({document, metadata}) => { const publicationRef = this.firebase.ref(this.firebase.getProblemPublicationsPath(user)).push(); - // TODO: this is assuming there is always a group for this current user, that doesn't seem - // safe??? - const userGroup = groups.getGroupById(user.currentGroupId)!; - const groupUserConnections: DBGroupUserConnections = userGroup && userGroup.users + const userGroup = groups.getGroupById(user.currentGroupId); + const groupUserConnections: DBGroupUserConnections | undefined = userGroup && userGroup.users .filter(groupUser => groupUser.id !== user.id) .reduce((allUsers: DBGroupUserConnections, groupUser) => { allUsers[groupUser.id] = groupUser.connected; diff --git a/src/models/stores/user.ts b/src/models/stores/user.ts index 05516ce703..d6daeaccaf 100644 --- a/src/models/stores/user.ts +++ b/src/models/stores/user.ts @@ -40,10 +40,8 @@ export const UserModel = types // property in Firebase latestGroupId: types.maybe(types.string), // This is the group of this user in the particular offering that is being - // run right now. There is a view which will return this value if set otherwise - // it returns latestGroupId. There might be a period when CLUE is being loaded - // where the currentGroupId is not set yet. This would happen if the user hasn't - // chosen a group yet. Or if groups haven't been loaded yet. + // run right now. This is the property you usually will want to use. + // latestGroupId could be referring to a group from a different assignment/offering currentGroupId: types.maybe(types.string), portal: "", network: types.maybe(types.string),