diff --git a/src/lib/db-listeners/db-groups-listener.ts b/src/lib/db-listeners/db-groups-listener.ts index d43374472..367b88ef2 100644 --- a/src/lib/db-listeners/db-groups-listener.ts +++ b/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 = {}; 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(); diff --git a/src/models/stores/groups.ts b/src/models/stores/groups.ts index 233f9d7fd..d70cb3809 100644 --- a/src/models/stores/groups.ts +++ b/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[] = []; + + 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 = {}; Object.entries(groups).forEach(([groupId, group]) => { - const groupUserSnapshots: SnapshotIn[] = []; - - 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; +