diff --git a/src/stories/hubbles_law/database.ts b/src/stories/hubbles_law/database.ts index f149d2a..8051e6d 100644 --- a/src/stories/hubbles_law/database.ts +++ b/src/stories/hubbles_law/database.ts @@ -7,6 +7,7 @@ import { HubbleStudentData } from "./models/hubble_student_data"; import { HubbleClassData } from "./models/hubble_class_data"; import { IgnoreStudent } from "../../models/ignore_student"; import { logger } from "../../logger"; +import { HubbleClassMergeGroup } from "./models/hubble_class_merge_group"; const galaxyAttributes = ["id", "ra", "decl", "z", "type", "name", "element"]; @@ -207,7 +208,9 @@ export async function getStudentHubbleMeasurements(studentID: number): Promise { +async function getHubbleMeasurementsForStudentClass(studentID: number, classID: number, excludeWithNull: boolean = false): Promise { + + const classIDs = await getMergedIDsForClass(classID); const studentWhereConditions: WhereOptions = []; const classDataStudentIDs = await getClassDataIDsForStudent(studentID); @@ -317,6 +320,31 @@ async function getClassIDsForSyncClass(classID: number): Promise { return classIDs; } +export async function getMergedIDsForClass(classID: number): Promise { + // TODO: Currently this uses two queries: + // The first to get the merge group (if there is one) + // Then a second to get all of the classes in the merge group + // Maybe we can just write some SQL to make these one query? + const mergeGroup = await HubbleClassMergeGroup.findOne({ + where: { + class_id: classID + } + }); + if (mergeGroup === null) { + return [classID]; + } + + const mergeEntries = await HubbleClassMergeGroup.findAll({ + where: { + group_id: mergeGroup.group_id, + merge_order: { + [Op.lte] : mergeGroup.merge_order, + } + } + }); + return mergeEntries.map(entry => entry.class_id); +} + export async function getClassDataIDsForStudent(studentID: number): Promise { const state = (await StoryState.findOne({ where: { student_id: studentID }, @@ -329,69 +357,38 @@ export async function getClassDataIDsForStudent(studentID: number): Promise { - const classIDs = await getClassIDsForSyncClass(classID); - return getHubbleMeasurementsForStudentClasses(studentID, classIDs, excludeWithNull); -} - -async function getHubbleMeasurementsForAsyncStudent(studentID: number, classID: number | null, excludeWithNull: boolean = false): Promise { - const classIDs = await getClassIDsForAsyncStudent(studentID, classID); - return getHubbleMeasurementsForStudentClasses(studentID, classIDs, excludeWithNull); -} - export async function getClassMeasurements(studentID: number, - classID: number | null, + classID: number, lastChecked: number | null = null, excludeWithNull: boolean = false, ): Promise { - const cls = classID !== null ? await findClassById(classID) : null; - const asyncClass = cls?.asynchronous ?? true; - let data: HubbleMeasurement[] | null; - if (classID === null || asyncClass) { - data = await getHubbleMeasurementsForAsyncStudent(studentID, classID, excludeWithNull); - } else { - data = await getHubbleMeasurementsForSyncStudent(studentID, classID, excludeWithNull); - } - if (data != null && lastChecked != null) { + let data = await getHubbleMeasurementsForStudentClass(studentID, classID, excludeWithNull); + if (data.length > 0 && lastChecked != null) { const lastModified = Math.max(...data.map(meas => meas.last_modified.getTime())); if (lastModified <= lastChecked) { - data = null; + data = []; } } - return data ?? []; + return data; } // The advantage of this over the function above is that it saves bandwidth, // since we aren't sending the data itself. // This is intended to be used with cases where we need to frequently check the number of measurements export async function getClassMeasurementCount(studentID: number, - classID: number | null, + classID: number, excludeWithNull: boolean = false, ): Promise { - const cls = classID !== null ? await findClassById(classID) : null; - const asyncClass = cls?.asynchronous ?? true; - let data: HubbleMeasurement[] | null; - if (classID === null || asyncClass) { - data = await getHubbleMeasurementsForAsyncStudent(studentID, classID, excludeWithNull); - } else { - data = await getHubbleMeasurementsForSyncStudent(studentID, classID, excludeWithNull); - } + const data = await getClassMeasurements(studentID, classID, null, excludeWithNull); return data?.length ?? 0; } // Similar to the function above, this is intended for cases where we need to frequently check // how many students have completed their measurements, such as the beginning of Stage 4 in the Hubble story export async function getStudentsWithCompleteMeasurementsCount(studentID: number, - classID: number | null, + classID: number, ): Promise { - const cls = classID !== null ? await findClassById(classID) : null; - const asyncClass = cls?.asynchronous ?? true; - let data: HubbleMeasurement[] | null; - if (classID === null || asyncClass) { - data = await getHubbleMeasurementsForAsyncStudent(studentID, classID, true); - } else { - data = await getHubbleMeasurementsForSyncStudent(studentID, classID, true); - } + const data = await getHubbleMeasurementsForStudentClass(studentID, classID, true); const counts: Record = {}; data?.forEach(measurement => { if (measurement.student_id in counts) { @@ -761,60 +758,74 @@ export async function getMergeDataForClass(classID: number): Promise { - const size = await classSize(classID); - - // Running into the limits of the ORM a bit here - // Maybe there's a clever way to write this? - // But straight SQL gets the job done - return database.query( - `SELECT * FROM (SELECT - id, - test, - (SELECT - COUNT(*) - FROM - StudentsClasses - WHERE - StudentsClasses.class_id = id) AS size +type GroupData = { + unique_gid: string; + is_group: boolean; + merged_count: number; +}; +export async function findClassForMerge(database: Sequelize, classID: number): Promise { + // The SQL is complicated enough here; doing this with the ORM + // will probably be unreadable + const result = await database.query( + ` + SELECT + id, + COUNT(*) as group_count, + IFNULL(group_id, UUID()) AS unique_gid, + (group_id IS NOT NULL) AS is_group, + MAX(merge_order) as merged_count FROM - Classes) q - WHERE - (size >= ${sizeThreshold - size} AND test = 0) - `, { type: QueryTypes.SELECT }) as Promise; -} - -// Try and merge the class with the given ID with another class such that the total size is above the threshold -// We say "try" because if a client doesn't know that the merge has already occurred, we may get -// multiple such requests from different student clients. -// If a merge has already been created, we don't make another one - we just return the existing one, with a -// message that indicates that this was the case. -export interface MergeAttemptData { - mergeData: SyncMergedHubbleClasses | null; - message: string; -} -export async function tryToMergeClass(db: Sequelize, classID: number): Promise { - const cls = await findClassById(classID); - if (cls === null) { - return { mergeData: null, message: "Invalid class ID!" }; + Classes + LEFT OUTER JOIN + (SELECT * FROM HubbleClassMergeGroups ORDER BY merge_order DESC) G ON Classes.id = G.class_id + INNER JOIN + ( + SELECT * FROM StudentsClasses GROUP BY class_id + HAVING COUNT(student_id) >= 12 + ) C + ON Classes.id = C.class_id + WHERE id != ${classID} + GROUP BY unique_gid + ORDER BY is_group ASC, group_count ASC, merged_count DESC + LIMIT 1; + `, + { type: QueryTypes.SELECT } + ) as (Class & GroupData)[]; + return result[0]; +} + +async function nextMergeGroupID(): Promise { + const max = (await HubbleClassMergeGroup.findAll({ + attributes: [ + [Sequelize.fn("MAX", Sequelize.col("group_id")), "group_id"] + ] + })) as (HubbleClassMergeGroup & { group_id: number })[]; + return (max[0].group_id + 1) as number; +} + +export async function addClassToMergeGroup(classID: number): Promise { + + // Sanity check + const existingGroup = await HubbleClassMergeGroup.findOne({ where: { class_id: classID } }); + if (existingGroup !== null) { + return existingGroup.group_id; } - let mergeData = await getMergeDataForClass(classID); - if (mergeData !== null) { - return { mergeData, message: "Class already merged" }; + const database = Class.sequelize; + if (database === undefined) { + return null; } - const eligibleClasses = await eligibleClassesForMerge(db, classID); - if (eligibleClasses.length > 0) { - const index = Math.floor(Math.random() * eligibleClasses.length); - const classToMerge = eligibleClasses[index]; - mergeData = await SyncMergedHubbleClasses.create({ class_id: classID, merged_class_id: classToMerge.id }); - if (mergeData === null) { - return { mergeData, message: "Error creating merge!" }; - } - return { mergeData, message: "New merge created" }; + const clsToMerge = await findClassForMerge(database, classID); + let mergeGroup; + if (clsToMerge.is_group) { + mergeGroup = await HubbleClassMergeGroup.create({ class_id: classID, group_id: Number(clsToMerge.unique_gid), merge_order: clsToMerge.merged_count + 1 }); + } else { + const newGroupID = await nextMergeGroupID(); + await HubbleClassMergeGroup.create({ class_id: clsToMerge.id, group_id: newGroupID, merge_order: 1 }); + mergeGroup = await HubbleClassMergeGroup.create({ class_id: classID, group_id: newGroupID, merge_order: 2 }); } - return { mergeData: null, message: "No eligible classes to merge" }; + return mergeGroup.group_id; } diff --git a/src/stories/hubbles_law/models/hubble_class_merge_group.ts b/src/stories/hubbles_law/models/hubble_class_merge_group.ts new file mode 100644 index 0000000..b2c65fa --- /dev/null +++ b/src/stories/hubbles_law/models/hubble_class_merge_group.ts @@ -0,0 +1,42 @@ +import { Sequelize, DataTypes, Model, InferAttributes, InferCreationAttributes } from "sequelize"; +import { Class } from "../../../models"; + +export class HubbleClassMergeGroup extends Model, InferCreationAttributes> { + declare group_id: number; + declare class_id: number; + declare merge_order: number; +} + +export function initializeHubbleClassMergeGroupModel(sequelize: Sequelize) { + HubbleClassMergeGroup.init({ + group_id: { + type: DataTypes.INTEGER.UNSIGNED, + allowNull: false, + primaryKey: true, + }, + class_id: { + type: DataTypes.INTEGER.UNSIGNED, + allowNull: false, + unique: true, + primaryKey: true, + references: { + model: Class, + key: "id", + } + }, + merge_order: { + type: DataTypes.INTEGER.UNSIGNED, + allowNull: false, + } + }, { + sequelize, + indexes: [ + { + fields: ["group_id"], + }, + { + fields: ["class_id"], + }, + ] + }); +} diff --git a/src/stories/hubbles_law/models/index.ts b/src/stories/hubbles_law/models/index.ts index f6bd4a0..976c691 100644 --- a/src/stories/hubbles_law/models/index.ts +++ b/src/stories/hubbles_law/models/index.ts @@ -6,6 +6,7 @@ import { SyncMergedHubbleClasses, initializeSyncMergedHubbleClassesModel } from import { Sequelize } from "sequelize"; import { initializeHubbleStudentDataModel } from "./hubble_student_data"; import { initializeHubbleClassDataModel } from "./hubble_class_data"; +import { initializeHubbleClassMergeGroupModel } from "./hubble_class_merge_group"; export { Galaxy, @@ -23,4 +24,5 @@ export function initializeModels(db: Sequelize) { initializeSyncMergedHubbleClassesModel(db); initializeHubbleStudentDataModel(db); initializeHubbleClassDataModel(db); + initializeHubbleClassMergeGroupModel(db); } diff --git a/src/stories/hubbles_law/router.ts b/src/stories/hubbles_law/router.ts index 108b20c..1443a62 100644 --- a/src/stories/hubbles_law/router.ts +++ b/src/stories/hubbles_law/router.ts @@ -1,3 +1,6 @@ +import * as S from "@effect/schema/Schema"; +import * as Either from "effect/Either"; + import { Galaxy } from "./models/galaxy"; import { @@ -32,9 +35,10 @@ import { getGalaxyById, removeSampleHubbleMeasurement, getAllNthSampleHubbleMeasurements, - tryToMergeClass, getClassMeasurementCount, - getStudentsWithCompleteMeasurementsCount + getStudentsWithCompleteMeasurementsCount, + getMergedIDsForClass, + addClassToMergeGroup } from "./database"; import { @@ -44,8 +48,8 @@ import { import { Express, Router } from "express"; import { Sequelize } from "sequelize"; -import { findClassById, findStudentById } from "../../database"; -import { SyncMergedHubbleClasses, initializeModels } from "./models"; +import { classForStudentStory, findClassById, findStudentById } from "../../database"; +import { initializeModels } from "./models"; import { setUpHubbleAssociations } from "./associations"; export const router = Router(); @@ -345,8 +349,10 @@ router.get(["/class-measurements/:studentID/:classID", "/stage-3-data/:studentID classID = 159; } - const invalidStudent = (await findStudentById(studentID)) === null; - const invalidClass = (await findClassById(classID)) === null; + const student = await findStudentById(studentID); + const invalidStudent = student === null; + const cls = await findClassById(classID); + const invalidClass = cls === null; if (invalidStudent || invalidClass) { const invalidItems = []; if (invalidStudent) { invalidItems.push("student"); } @@ -358,7 +364,7 @@ router.get(["/class-measurements/:studentID/:classID", "/stage-3-data/:studentID return; } - const measurements = await getClassMeasurements(studentID, classID, lastChecked, completeOnly); + const measurements = await getClassMeasurements(student.id, cls.id, lastChecked, completeOnly); res.status(200).json({ student_id: studentID, class_id: classID, @@ -372,12 +378,20 @@ router.get(["/class-measurements/:studentID", "stage-3-measurements/:studentID"] const isValidStudent = (await findStudentById(studentID)) !== null; if (!isValidStudent) { res.status(404).json({ - message: "Invalid student ID" + message: "Invalid student ID", }); return; } - const measurements = await getClassMeasurements(studentID, null); + const cls = await classForStudentStory(studentID, "hubbles_law"); + if (cls === null) { + res.status(404).json({ + message: `Student ${studentID} is not in a class signed up for the Hubble's Law story`, + }); + return; + } + + const measurements = await getClassMeasurements(studentID, cls.id); res.status(200).json({ student_id: studentID, class_id: null, @@ -385,51 +399,72 @@ router.get(["/class-measurements/:studentID", "stage-3-measurements/:studentID"] }); }); -router.get("/all-data", async (req, res) => { - const minimal = (req.query?.minimal as string)?.toLowerCase() === "true"; - const beforeMs: number = parseInt(req.query.before as string); - const before = isNaN(beforeMs) ? null : new Date(beforeMs); - const [measurements, studentData, classData] = - await Promise.all([ - getAllHubbleMeasurements(before, minimal), - getAllHubbleStudentData(before, minimal), - getAllHubbleClassData(before, minimal) - ]); +router.get("/merged-classes/:classID", async (req, res) => { + const classID = Number(req.params.classID); + const cls = await findClassById(classID); + if (cls === null) { + res.status(404).json({ + message: `No class found with ID ${classID}`, + }); + return; + } + const classIDs = await getMergedIDsForClass(classID); res.json({ - measurements, - studentData, - classData + merged_class_ids: classIDs, }); }); -router.put("/sync-merged-class/:classID", async(req, res) => { - const classID = parseInt(req.params.classID); - if (isNaN(classID)) { - res.statusCode = 400; - res.json({ - error: "Class ID must be a number" +const MergeClassInfo = S.struct({ + class_id: S.number.pipe(S.int()), +}); +router.put("/merge-class", async (req, res) => { + const body = req.body; + const maybe = S.decodeUnknownEither(MergeClassInfo)(body); + + if (Either.isLeft(maybe)) { + res.status(400).json({ + message: `Expected class ID to be an integer, got ${body.class_id}`, }); return; } - const database = SyncMergedHubbleClasses.sequelize; - if (database === undefined) { - res.status(500).json({ - error: "Error connecting to database", + + const data = maybe.right; + const cls = await findClassById(data.class_id); + if (cls === null) { + res.status(404).json({ + message: `No class found with ID ${data.class_id}`, }); return; } - const data = await tryToMergeClass(database, classID); - if (data.mergeData === null) { - res.statusCode = 404; - res.json({ - error: data.message + + const groupID = await addClassToMergeGroup(data.class_id); + if (groupID === null) { + res.status(500).json({ + message: `There was an error while adding class ${data.class_id} to a merge group`, }); return; } res.json({ - merge_info: data.mergeData, - message: data.message + class_id: data.class_id, + group_id: groupID, + }); +}); + +router.get("/all-data", async (req, res) => { + const minimal = (req.query?.minimal as string)?.toLowerCase() === "true"; + const beforeMs: number = parseInt(req.query.before as string); + const before = isNaN(beforeMs) ? null : new Date(beforeMs); + const [measurements, studentData, classData] = + await Promise.all([ + getAllHubbleMeasurements(before, minimal), + getAllHubbleStudentData(before, minimal), + getAllHubbleClassData(before, minimal) + ]); + res.json({ + measurements, + studentData, + classData }); }); diff --git a/src/stories/hubbles_law/sql/create_merge_groups_table.sql b/src/stories/hubbles_law/sql/create_merge_groups_table.sql new file mode 100644 index 0000000..f5b3256 --- /dev/null +++ b/src/stories/hubbles_law/sql/create_merge_groups_table.sql @@ -0,0 +1,13 @@ +CREATE TABLE HubbleClassMergeGroups( + group_id int(11) UNSIGNED NOT NULL, + class_id int(11) UNSIGNED NOT NULL UNIQUE, + merge_order int(11) UNSIGNED NOT NULL, + + PRIMARY KEY(group_id, class_id), + INDEX(group_id), + INDEX(class_id), + FOREIGN KEY(class_id) + REFERENCES Classes(id) + ON UPDATE CASCADE + ON DELETE CASCADE +) ENGINE=InnoDB AUTO_INCREMENT=0 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci PACK_KEYS=0;