kick removed users out of groups first

src/lib/db-listeners/db-groups-listener.ts

@@ -1,5 +1,6 @@
+import { destroy } from "mobx-state-tree";
 import firebase from "firebase/app";
-import { map } from "lodash";
+import { getGroupSnapshot, GroupModel, GroupUserModelType, GroupUserState } from "../../models/stores/groups";
 import { DB } from "../db";
 import { DBOfferingGroupMap } from "../db-types";
 import { BaseListener } from "./base-listener";
@@ -56,43 +57,80 @@ export class DBGroupsListener extends BaseListener {
   }
 
   private handleGroupsRef = (snapshot: firebase.database.DataSnapshot) => {
-    const {user} = this.db.stores;
+    const { user, class: clazz } = this.db.stores;
     const groups: DBOfferingGroupMap = snapshot.val() || {};
     const myGroupIds: string[] = [];
-    const overSubscribedUserUpdates: any = {};
+    const overSubscribedUserUpdates: Record<string, null> = {};
 
     this.debugLogSnapshot("#handleGroupsRef", snapshot);
 
+    const markGroupUserForRemoval = (groupId: string, userToRemove: GroupUserModelType) => {
+      const userPath = this.db.firebase.getFullPath(
+        this.db.firebase.getGroupUserPath(user, groupId, userToRemove.id)
+      );
+      overSubscribedUserUpdates[userPath] = null;
+    };
+
     // ensure that the current user is not in more than 1 group and groups are not oversubscribed
     Object.keys(groups).forEach((groupId) => {
-      const rawUsers = groups[groupId].users || {};
-      // rawUsers can get interpreted as an array instead of a map if user IDs are small (e.g. in demo mode)
-      // So, make sure to filter out empty array spaces and convert number IDs to strings for standardization
-      const groupUsers = map(rawUsers, (groupUser, uid) => ({uid: `${uid}`, user: groupUser}))
-        .filter(groupUser => !!groupUser.user);
-      if (groupUsers.find(groupUser => groupUser.uid === user.id)) {
+      // Create a temporary instance of the MST Group so the same parsing logic is used both here
+      // and by updateGroupsFromDb
+      const groupSnapshot = getGroupSnapshot(groupId, groups[groupId]);
+      const tempGroup = GroupModel.create(groupSnapshot, {class: clazz});
+
+      if (tempGroup.users.find(groupUser => groupUser.id === user.id)){
         myGroupIds.push(groupId);
       }
-      if (groupUsers.length > 4) {
+
+      let subscribedUsers = tempGroup.users.slice();
+      if (subscribedUsers.length > 4) {
+        // If a user is removed from the class kick them out. We only do this when the group
+        // is over subscribed. Perhaps the user was only removed temporarily. As long as another
+        // user doesn't try to steal their place in a group, if they are re-added to the class
+        // the user will still be in the same group.
+        const remainingUsers: GroupUserModelType[] = [];
+        subscribedUsers.forEach(groupUser => {
+          if (groupUser.state === GroupUserState.Removed) {
+            markGroupUserForRemoval(groupId, groupUser);
+          } else {
+            remainingUsers.push(groupUser);
+          }
+        });
+        subscribedUsers = remainingUsers;
+      }
+
+      // Are we are still over 4?
+      if (subscribedUsers.length > 4) {
         // sort the users by connected timestamp and find the newest users to kick out
-        groupUsers.sort((a, b) => a.user.connectedTimestamp - b.user.connectedTimestamp);
-        for (let i = 4; i < groupUsers.length; i++) {
-          const userToRemove = groupUsers[i];
-          const userPath = this.db.firebase.getFullPath(
-            this.db.firebase.getGroupUserPath(user, groupId, userToRemove.uid)
-          );
-          overSubscribedUserUpdates[userPath] = null;
+        subscribedUsers.sort((a, b) => a.connectedTimestamp - b.connectedTimestamp);
+        for (let i = 4; i < subscribedUsers.length; i++) {
+          markGroupUserForRemoval(groupId, subscribedUsers[i]);
         }
       }
+      destroy(tempGroup);
     });
 
     // if there is a problem with the groups fix the problem in the next timeslice
     const numUpdates = Object.keys(overSubscribedUserUpdates).length;
     if ((numUpdates > 0) || (myGroupIds.length > 1)) {
       setTimeout(() => {
+        // FIXME: Note multiple CLUE windows might be doing this update at the same time. The
+        // connectedTimestamp values should be same in both windows. But the removed users
+        // are based on each individual CLUE window, so that might result in different
+        // lists. For example a CLUE window that started before the user was removed
+        // from the class will *not* see them as removed. And a CLUE window that started
+        // after the user was removed from the class *will* see them as removed.
+        // If the group is oversubscribed the first window will delete the most recent
+        // user. The second window will delete the removed user. So now two different users will
+        // be removed from the group.
         if (numUpdates > 0) {
           firebase.database().ref().update(overSubscribedUserUpdates);
         }
+        // FIXME: How do we know that we haven't already been removed from this group by
+        // the overSubscribedUserUpdates?
+        // What if we decide to remove another user from this group and then we
+        // leave the group too. That means the first user was kicked out for no
+        // reason.
         if (myGroupIds.length > 1) {
           this.db.leaveGroup();
         }
@@ -112,6 +150,20 @@ export class DBGroupsListener extends BaseListener {
     groups.updateFromDB(dbGroups);
     if (!groups.needToRefreshClass) return;
 
+    // FIXME: if a user has launched CLUE and then the teacher removes them from the class, they can
+    // keep joining new groups which will make them look like a new user because their "connected" time
+    // will be newer than last class info request. Each time they change groups this will immediately
+    // trigger a re-request of the class info here. This will reset last class info request time to be
+    // newer than the "connected" time of the user. However we have an offset of 1 second before the user
+    // is considered removed. So if the class timestamp is less than 1 second newer than the "connected"
+    // time of the user, the user will continue to show up as new. Unless the groups in the DB are
+    // updated, this removed user will continue to show up as new.
+    //
+    // For portal launches we can address this by locking CLUE if the current user is no longer in
+    // the class. The user could circumvent this in the browser console, but it should be good enough
+    // to prevent data loss and deter most people.
+    // We currently only do these class refreshes here when the groups change. So if a new student
+    // starts CLUE or a student changes groups then this rouge user would effectively be kicked out.
     const classInfo = await portal.getClassInfo();
     if (!classInfo) return;
     const timeOffset = await this.db.firebase.getServerTimeOffset();

src/models/stores/groups.ts

@@ -1,5 +1,5 @@
 import { types, getEnv, SnapshotIn, applySnapshot } from "mobx-state-tree";
-import { DBOfferingGroupMap } from "../../lib/db-types";
+import { DBOfferingGroup, DBOfferingGroupMap } from "../../lib/db-types";
 import { ClassModelType } from "./class";
 import { GroupVirtualDocument } from "../document/group-virtual-document";
 import { UserModelType } from "./user";
@@ -127,6 +127,26 @@ export const GroupModel = types
     }
   }));
 
+export function getGroupSnapshot(groupId: string, groupFromDB: DBOfferingGroup) {
+  const groupUserSnapshots: SnapshotIn<typeof GroupUserModel>[] = [];
+
+  const groupUsers = groupFromDB.users || {};
+  Object.keys(groupUsers).forEach((groupUserId) => {
+    const groupUser = groupUsers[groupUserId];
+    const {connectedTimestamp, disconnectedTimestamp} = groupUser;
+    // self may be undefined if the database was deleted while a tab remains open
+    // causing the disconnectedAt timestamp to be set at the groupUser level
+    if (groupUser.self) {
+      groupUserSnapshots.push({
+        id: groupUserId,
+        connectedTimestamp,
+        disconnectedTimestamp
+      });
+    }
+  });
+  return {id: groupId, users: groupUserSnapshots};
+}
+
 export const GroupsModel = types
   .model("Groups", {
     groupsMap: types.map(GroupModel),
@@ -191,23 +211,7 @@ export const GroupsModel = types
     updateFromDB(groups: DBOfferingGroupMap) {
       const groupsMapSnapshot: SnapshotIn<typeof self.groupsMap> = {};
       Object.entries(groups).forEach(([groupId, group]) => {
-        const groupUserSnapshots: SnapshotIn<typeof GroupUserModel>[] = [];
-
-        const groupUsers = group.users || {};
-        Object.keys(groupUsers).forEach((groupUserId) => {
-          const groupUser = groupUsers[groupUserId];
-          const {connectedTimestamp, disconnectedTimestamp} = groupUser;
-          // self may be undefined if the database was deleted while a tab remains open
-          // causing the disconnectedAt timestamp to be set at the groupUser level
-          if (groupUser.self) {
-            groupUserSnapshots.push({
-              id: groupUserId,
-              connectedTimestamp,
-              disconnectedTimestamp
-            });
-          }
-        });
-        groupsMapSnapshot[groupId] = {id: groupId, users: groupUserSnapshots};
+        groupsMapSnapshot[groupId] = getGroupSnapshot(groupId, group);
       });
       applySnapshot(self.groupsMap, groupsMapSnapshot);
     }
@@ -216,3 +220,4 @@ export const GroupsModel = types
 export type GroupUserModelType = typeof GroupUserModel.Type;
 export type GroupModelType = typeof GroupModel.Type;
 export type GroupsModelType = typeof GroupsModel.Type;
+