Merge pull request #1653 from concord-consortium/180073119-fix-4-up-m…

…onitoring Fix 4-up monitoring
concord-consortium · Mar 31, 2023 · 91cc02c · 91cc02c
2 parents 3c0d8d1 + 8cec24f
commit 91cc02c
Show file tree

Hide file tree

Showing 12 changed files with 77 additions and 32 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
@@ -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
@@ -25,7 +25,7 @@ interface IProps extends IBaseProps {
 export const ClueAppHeaderComponent: React.FC<IProps> = 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
@@ -160,7 +160,7 @@ export class AppComponent extends BaseComponent<IProps, IState> {
   // time.
 
   public render() {
-    const {appConfig, user, ui, db, groups} = this.stores;
+    const {appConfig, user, ui, db} = this.stores;
 
     if (ui.showDemoCreator) {
       return this.renderApp(<DemoCreatorComponent />);
@@ -184,7 +184,7 @@ export class AppComponent extends BaseComponent<IProps, IState> {
     }
 
     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
@@ -276,7 +276,7 @@ export class DocumentComponent extends BaseComponent<IProps, IState> {
 
     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-docs-content-listener.ts b/src/lib/db-listeners/db-docs-content-listener.ts
@@ -21,6 +21,8 @@ class DocumentMonitor {
     this.ref.on("value", this.snapshotHandler);
   }
 
+  // The arrow function is required here so the function will be
+  // bound to `this` when used by `ref.on(...)`
   private snapshotHandler = (snapshot: firebase.database.DataSnapshot) => {
     if (snapshot?.val()) {
       const updatedDoc: DBDocument = snapshot.val();
@@ -53,21 +55,14 @@ export class DBDocumentsContentListener 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) => {
         // Besides collecting the documents to monitor, we also update the group
         // 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)) {
@@ -76,7 +71,7 @@ export class DBDocumentsContentListener extends BaseListener {
 
         // teachers monitor all problem documents
         // students only monitor documents in their group to save bandwidth
-        if (user.isTeacher || document.groupId === user.latestGroupId) {
+        if (user.isTeacher || document.groupId === user.currentGroupId) {
           documentsToMonitor.push(document);
         }
       });

diff --git a/src/lib/db-listeners/db-groups-listener.ts b/src/lib/db-listeners/db-groups-listener.ts
@@ -25,7 +25,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) {
@@ -100,7 +100,10 @@ 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);
+
+      user.setCurrentGroupId(this.db.stores.groups.groupIdForUser(user.id));
+
     }
   };
 }
diff --git a/src/lib/db.ts b/src/lib/db.ts
@@ -436,8 +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();
-        const userGroup = groups.groupForUser(user.id)!;
-        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/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/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
@@ -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 || {};
@@ -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<string, GroupModelType> = {};
+      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) {

diff --git 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,13 +51,15 @@ describe("user model", () => {
         name: "Test User",
         className: "Test Class",
         latestGroupId: "1",
+        currentGroupId: "2",
         id: "2",
     });
     expect(user.authenticated).toBe(true);
     expect(user.type).toBe("student");
     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
@@ -36,7 +36,13 @@ 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. 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),
     networks: types.array(types.string),
@@ -69,6 +75,9 @@ export const UserModel = types
     setLatestGroupId(latestGroupId?: string) {
       self.latestGroupId = latestGroupId;
     },
+    setCurrentGroupId(currentGroupId?: string) {
+      self.currentGroupId = currentGroupId;
+    },
     setId(id: string) {
       self.id = id;
     },