From 8b470153561d249ab8ba0b76a29292abe9590e4a Mon Sep 17 00:00:00 2001 From: Sam Date: Sun, 14 Nov 2021 17:45:56 -0500 Subject: [PATCH] Requirement builder with the new data format --- .../Modals/NewCourse/NewCourseModal.vue | 16 +- .../Requirements/IncompleteSelfCheck.vue | 26 ++- src/components/Semester/Semester.vue | 14 +- src/global-firestore-data/index.ts | 6 +- .../override-fulfillment-choices.ts | 36 ++++ .../selectable-requirement-choices.ts | 69 -------- src/global-firestore-data/semesters.ts | 22 ++- .../requirement-graph-builder.test.ts | 163 +++++------------- .../admin/requirement-graph-admin-utils.ts | 17 +- .../requirement-frontend-computation.ts | 4 +- .../requirement-frontend-utils.ts | 40 +++-- ...equirement-graph-builder-from-user-data.ts | 156 +++-------------- src/requirements/requirement-graph-builder.ts | 83 ++++----- src/store.ts | 3 +- src/user-data.d.ts | 15 -- 15 files changed, 237 insertions(+), 433 deletions(-) create mode 100644 src/global-firestore-data/override-fulfillment-choices.ts delete mode 100644 src/global-firestore-data/selectable-requirement-choices.ts diff --git a/src/components/Modals/NewCourse/NewCourseModal.vue b/src/components/Modals/NewCourse/NewCourseModal.vue index 0265a45a8..09a6b2fa2 100644 --- a/src/components/Modals/NewCourse/NewCourseModal.vue +++ b/src/components/Modals/NewCourse/NewCourseModal.vue @@ -54,8 +54,8 @@ export default defineComponent({ components: { CourseSelector, TeleportModal, SelectedRequirementEditor }, emits: { 'close-course-modal': () => true, - 'add-course': (course: CornellCourseRosterCourse, requirementID: string) => - typeof course === 'object' && typeof requirementID === 'string', + 'add-course': (course: CornellCourseRosterCourse, choice: FirestoreCourseOptInOptOutChoices) => + typeof course === 'object' && typeof choice === 'object', }, data() { return { @@ -98,7 +98,7 @@ export default defineComponent({ selectedCourse, store.state.groupedRequirementFulfillmentReport, store.state.toggleableRequirementChoices, - /* deprecated AppOverriddenFulfillmentChoices */ {} + store.state.overriddenFulfillmentChoices ); const requirementsThatAllowDoubleCounting: string[] = []; @@ -137,7 +137,15 @@ export default defineComponent({ }, addCourse() { if (this.selectedCourse == null) return; - this.$emit('add-course', this.selectedCourse, this.selectedRequirementID); + this.$emit('add-course', this.selectedCourse, { + optOut: this.relatedRequirements + .filter(it => it.id !== this.selectedRequirementID) + .map(it => it.id), + acknowledgedCheckerWarningOptIn: this.selfCheckRequirements + .filter(it => it.id !== this.selectedRequirementID) + .map(it => it.id), + arbitraryOptIn: {}, + }); this.closeCurrentModal(); }, onSelectedChange(selected: string) { diff --git a/src/components/Requirements/IncompleteSelfCheck.vue b/src/components/Requirements/IncompleteSelfCheck.vue index d8046d625..2d094f4c8 100644 --- a/src/components/Requirements/IncompleteSelfCheck.vue +++ b/src/components/Requirements/IncompleteSelfCheck.vue @@ -41,7 +41,7 @@ import store from '@/store'; import { cornellCourseRosterCourseToFirebaseSemesterCourseWithGlobalData, addCourseToSemester, - addCourseToSelectableRequirements, + updateRequirementChoice, } from '@/global-firestore-data'; import { canFulfillChecker } from '@/requirements/requirement-frontend-utils'; @@ -150,12 +150,32 @@ export default defineComponent({ }, addExistingCourse(option: string) { this.showDropdown = false; - addCourseToSelectableRequirements(this.selfCheckCourses[option].uniqueID, this.subReqId); + updateRequirementChoice(this.selfCheckCourses[option].uniqueID, choice => ({ + ...choice, + // Since we edit from a self-check requirement, + // we know it must be `acknowledgedCheckerWarningOptIn`. + acknowledgedCheckerWarningOptIn: Array.from( + new Set([...choice.acknowledgedCheckerWarningOptIn, this.subReqId]) + ), + })); }, addNewCourse(course: CornellCourseRosterCourse, season: FirestoreSemesterSeason, year: number) { this.showDropdown = false; const newCourse = cornellCourseRosterCourseToFirebaseSemesterCourseWithGlobalData(course); - addCourseToSemester(year, season, newCourse, this.subReqId, this.$gtag); + addCourseToSemester( + year, + season, + newCourse, + // Since the course is new, we know the old choice does not exist. + () => ({ + arbitraryOptIn: {}, + // Since we edit from a self-check requirement, + // we know it must be `acknowledgedCheckerWarningOptIn`. + acknowledgedCheckerWarningOptIn: [this.subReqId], + optOut: [], + }), + this.$gtag + ); }, openCourseModal() { this.isCourseModalOpen = true; diff --git a/src/components/Semester/Semester.vue b/src/components/Semester/Semester.vue index 6db605a83..0e2ca0522 100644 --- a/src/components/Semester/Semester.vue +++ b/src/components/Semester/Semester.vue @@ -139,7 +139,7 @@ import { addCourseToSemester, deleteCourseFromSemester, deleteAllCoursesFromSemester, - addCourseToSelectableRequirements, + updateRequirementChoice, } from '@/global-firestore-data'; import { updateSubjectColorData } from '@/store'; @@ -240,7 +240,12 @@ export default defineComponent({ }) ); newCourses.forEach(({ uniqueID, requirementID }) => - addCourseToSelectableRequirements(uniqueID, requirementID) + // Since it's dragged from a requirement without checker warning, + // we edit the normal opt-out field. + updateRequirementChoice(uniqueID, choice => ({ + ...choice, + optOut: choice.optOut.filter(it => it !== requirementID), + })) ); }, }, @@ -328,9 +333,10 @@ export default defineComponent({ closeConfirmationModal() { this.isConfirmationOpen = false; }, - addCourse(data: CornellCourseRosterCourse, requirementID: string) { + addCourse(data: CornellCourseRosterCourse, choice: FirestoreCourseOptInOptOutChoices) { const newCourse = cornellCourseRosterCourseToFirebaseSemesterCourseWithGlobalData(data); - addCourseToSemester(this.year, this.season, newCourse, requirementID, this.$gtag); + // Since the course is new, we know the old choice does not exist. + addCourseToSemester(this.year, this.season, newCourse, () => choice, this.$gtag); const courseCode = `${data.subject} ${data.catalogNbr}`; this.openConfirmationModal(`Added ${courseCode} to ${this.season} ${this.year}`); diff --git a/src/global-firestore-data/index.ts b/src/global-firestore-data/index.ts index d8a4025eb..f1d380c41 100644 --- a/src/global-firestore-data/index.ts +++ b/src/global-firestore-data/index.ts @@ -26,6 +26,6 @@ export { } from './semesters'; export { default as chooseToggleableRequirementOption } from './toggleable-requirement-choices'; export { - addCourseToSelectableRequirements, - deleteCourseFromSelectableRequirements, -} from './selectable-requirement-choices'; + updateRequirementChoice, + deleteCourseFromRequirementChoices, +} from './override-fulfillment-choices'; diff --git a/src/global-firestore-data/override-fulfillment-choices.ts b/src/global-firestore-data/override-fulfillment-choices.ts new file mode 100644 index 000000000..04467b4e2 --- /dev/null +++ b/src/global-firestore-data/override-fulfillment-choices.ts @@ -0,0 +1,36 @@ +import { overriddenFulfillmentChoicesCollection } from '../firebase-frontend-config'; +import store from '../store'; + +export const updateRequirementChoice = ( + courseUniqueID: string | number, + choiceUpdater: (choice: FirestoreCourseOptInOptOutChoices) => FirestoreCourseOptInOptOutChoices +): void => { + overriddenFulfillmentChoicesCollection.doc(store.state.currentFirebaseUser.email).set({ + ...store.state.overriddenFulfillmentChoices, + [courseUniqueID]: choiceUpdater( + store.state.overriddenFulfillmentChoices[courseUniqueID] || { + arbitraryOptIn: {}, + acknowledgedCheckerWarningOptIn: [], + optOut: [], + } + ), + }); +}; + +export const deleteCourseFromRequirementChoices = (courseUniqueID: string | number): void => + deleteCoursesFromRequirementChoices([courseUniqueID]); + +export const deleteCoursesFromRequirementChoices = ( + courseUniqueIds: readonly (string | number)[] +): void => { + const courseUniqueIdStrings = new Set(courseUniqueIds.map(uniqueId => uniqueId.toString())); + overriddenFulfillmentChoicesCollection + .doc(store.state.currentFirebaseUser.email) + .set( + Object.fromEntries( + Object.entries(store.state.overriddenFulfillmentChoices).filter( + ([k]) => !courseUniqueIdStrings.has(k) + ) + ) + ); +}; diff --git a/src/global-firestore-data/selectable-requirement-choices.ts b/src/global-firestore-data/selectable-requirement-choices.ts deleted file mode 100644 index c28d7d37b..000000000 --- a/src/global-firestore-data/selectable-requirement-choices.ts +++ /dev/null @@ -1,69 +0,0 @@ -import { getCourseCodesArray } from '../requirements/requirement-frontend-computation'; -import { getFirestoreCourseOptInOptOutChoicesBuilder } from '../requirements/requirement-graph-builder-from-user-data'; -import { - db, - selectableRequirementChoicesCollection, - overriddenFulfillmentChoicesCollection, -} from '../firebase-frontend-config'; -import store from '../store'; - -const chooseSelectableRequirementOption = ( - selectableRequirementChoices: AppSelectableRequirementChoices -): void => { - const courses = getCourseCodesArray(store.state.semesters, store.state.onboardingData); - const newFormatChoicesBuilder = getFirestoreCourseOptInOptOutChoicesBuilder( - courses, - store.state.onboardingData, - store.state.toggleableRequirementChoices, - selectableRequirementChoices, - /* deprecated AppOverriddenFulfillmentChoices */ {} - ); - const overriddenFulfillmentChoices = Object.fromEntries( - courses.map(course => [course.uniqueId, newFormatChoicesBuilder(course)]) - ); - - const batch = db.batch(); - batch.set( - selectableRequirementChoicesCollection.doc(store.state.currentFirebaseUser.email), - selectableRequirementChoices - ); - batch.set( - overriddenFulfillmentChoicesCollection.doc(store.state.currentFirebaseUser.email), - overriddenFulfillmentChoices - ); - batch.commit(); -}; - -export const addCourseToSelectableRequirements = ( - courseUniqueID: string | number, - requirementID: string | undefined -): void => { - // Even when there is no change, we set the old data anyways, - // so it can trigger a save of user choice in new format. - chooseSelectableRequirementOption( - requirementID - ? { - ...store.state.selectableRequirementChoices, - [courseUniqueID]: requirementID, - } - : store.state.selectableRequirementChoices - ); -}; - -export const deleteCourseFromSelectableRequirements = (courseUniqueID: string | number): void => { - deleteCoursesFromSelectableRequirements([courseUniqueID]); -}; - -export const deleteCoursesFromSelectableRequirements = ( - courseUniqueIds: (string | number)[] -): void => { - const courseUniqueIdStrings = new Set(courseUniqueIds.map(uniqueId => uniqueId.toString())); - chooseSelectableRequirementOption( - Object.assign( - {}, - ...Object.entries(store.state.selectableRequirementChoices) - .filter(([k]) => !courseUniqueIdStrings.has(k)) - .map(([k, v]) => ({ [k]: v })) - ) - ); -}; diff --git a/src/global-firestore-data/semesters.ts b/src/global-firestore-data/semesters.ts index 36f2a66bd..9755ae7b8 100644 --- a/src/global-firestore-data/semesters.ts +++ b/src/global-firestore-data/semesters.ts @@ -4,10 +4,10 @@ import { GTag, GTagEvent } from '../gtag'; import { sortedSemesters } from '../utilities'; import { - addCourseToSelectableRequirements, - deleteCourseFromSelectableRequirements, - deleteCoursesFromSelectableRequirements, -} from './selectable-requirement-choices'; + updateRequirementChoice, + deleteCourseFromRequirementChoices, + deleteCoursesFromRequirementChoices, +} from './override-fulfillment-choices'; export const editSemesters = ( updater: (oldSemesters: readonly FirestoreSemester[]) => readonly FirestoreSemester[] @@ -81,7 +81,7 @@ export const deleteSemester = ( GTagEvent(gtag, 'delete-semester'); const semester = store.state.semesters.find(sem => semesterEquals(sem, year, season)); if (semester) { - deleteCoursesFromSelectableRequirements(semester.courses.map(course => course.uniqueID)); + deleteCoursesFromRequirementChoices(semester.courses.map(course => course.uniqueID)); editSemesters(oldSemesters => oldSemesters.filter(sem => !semesterEquals(sem, year, season))); } }; @@ -90,7 +90,7 @@ export const addCourseToSemester = ( year: number, season: FirestoreSemesterSeason, newCourse: FirestoreSemesterCourse, - requirementID?: string, + choiceUpdater: (choice: FirestoreCourseOptInOptOutChoices) => FirestoreCourseOptInOptOutChoices, gtag?: GTag ): void => { GTagEvent(gtag, 'add-course'); @@ -106,9 +106,7 @@ export const addCourseToSemester = ( if (semesterFound) return newSemestersWithCourse; return [...oldSemesters, createSemester(year, season, [newCourse])]; }); - if (requirementID) { - addCourseToSelectableRequirements(newCourse.uniqueID, requirementID); - } + updateRequirementChoice(newCourse.uniqueID, choiceUpdater); }; export const deleteCourseFromSemester = ( @@ -120,7 +118,7 @@ export const deleteCourseFromSemester = ( GTagEvent(gtag, 'delete-course'); const semester = store.state.semesters.find(sem => semesterEquals(sem, year, season)); if (semester) { - deleteCourseFromSelectableRequirements(courseUniqueID); + deleteCourseFromRequirementChoices(courseUniqueID); editSemesters(oldSemesters => oldSemesters.map(sem => ({ ...sem, @@ -140,7 +138,7 @@ export const deleteAllCoursesFromSemester = ( GTagEvent(gtag, 'delete-semester-courses'); const semester = store.state.semesters.find(sem => semesterEquals(sem, year, season)); if (semester) { - deleteCoursesFromSelectableRequirements(semester.courses.map(course => course.uniqueID)); + deleteCoursesFromRequirementChoices(semester.courses.map(course => course.uniqueID)); editSemesters(oldSemesters => oldSemesters.map(sem => ({ ...sem, @@ -160,7 +158,7 @@ export const deleteCourseFromSemesters = (courseUniqueID: number, gtag?: GTag): return { ...semester, courses: coursesWithoutDeleted }; }) ); - deleteCourseFromSelectableRequirements(courseUniqueID); + deleteCourseFromRequirementChoices(courseUniqueID); }; // exposed for testing diff --git a/src/requirements/__test__/requirement-graph-builder.test.ts b/src/requirements/__test__/requirement-graph-builder.test.ts index 865034196..db2952b6e 100644 --- a/src/requirements/__test__/requirement-graph-builder.test.ts +++ b/src/requirements/__test__/requirement-graph-builder.test.ts @@ -22,18 +22,13 @@ const getAllCoursesThatCanPotentiallySatisfyRequirement = ( }; it('buildRequirementFulfillmentGraph phase 1 test 1', () => { - const graph = buildRequirementFulfillmentGraph( - { - requirements, - userCourses: [CS3410, CS3420, MATH4710], - userChoiceOnFulfillmentStrategy: {}, - userChoiceOnDoubleCountingElimination: {}, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, - }, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); + const graph = buildRequirementFulfillmentGraph({ + requirements, + userCourses: [CS3410, CS3420, MATH4710], + userChoiceOnFulfillmentStrategy: {}, + userChoiceOnRequirementOverrides: {}, + getAllCoursesThatCanPotentiallySatisfyRequirement, + }); expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410, CS3420]); expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([MATH4710]); @@ -43,18 +38,13 @@ it('buildRequirementFulfillmentGraph phase 1 test 1', () => { // This test ensures that we are actually using userCourses and drop any courses from pre-computed // course list that are not in userCourses. it('buildRequirementFulfillmentGraph phase 1 test 2', () => { - const graph = buildRequirementFulfillmentGraph( - { - requirements, - userCourses: [CS3410, MATH4710], - userChoiceOnFulfillmentStrategy: {}, - userChoiceOnDoubleCountingElimination: {}, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, - }, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); + const graph = buildRequirementFulfillmentGraph({ + requirements, + userCourses: [CS3410, MATH4710], + userChoiceOnFulfillmentStrategy: {}, + userChoiceOnRequirementOverrides: {}, + getAllCoursesThatCanPotentiallySatisfyRequirement, + }); expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410]); expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([MATH4710]); @@ -63,18 +53,13 @@ it('buildRequirementFulfillmentGraph phase 1 test 2', () => { // Following two tests test how we are removing edges depending on user choices on fulfillment strategy. it('buildRequirementFulfillmentGraph phase 2-1 test', () => { - const graph = buildRequirementFulfillmentGraph( - { - requirements, - userCourses: [CS3410, CS3420, MATH4710], - userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, - userChoiceOnDoubleCountingElimination: {}, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, - }, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); + const graph = buildRequirementFulfillmentGraph({ + requirements, + userCourses: [CS3410, CS3420, MATH4710], + userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, + userChoiceOnRequirementOverrides: {}, + getAllCoursesThatCanPotentiallySatisfyRequirement, + }); // In this case, 3420 is removed since user chooses strategy 1. expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410]); @@ -83,18 +68,13 @@ it('buildRequirementFulfillmentGraph phase 2-1 test', () => { }); it('buildRequirementFulfillmentGraph phase 2-2 test', () => { - const graph = buildRequirementFulfillmentGraph( - { - requirements, - userCourses: [CS3410, CS3420, MATH4710], - userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3420.courseId] }, - userChoiceOnDoubleCountingElimination: {}, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, - }, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); + const graph = buildRequirementFulfillmentGraph({ + requirements, + userCourses: [CS3410, CS3420, MATH4710], + userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3420.courseId] }, + userChoiceOnRequirementOverrides: {}, + getAllCoursesThatCanPotentiallySatisfyRequirement, + }); // In this case, 3410 is removed since user chooses strategy 2. expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3420]); @@ -104,18 +84,15 @@ it('buildRequirementFulfillmentGraph phase 2-2 test', () => { // The following two tests test that we will remove edges incompatible with user supplied choices. it('buildRequirementFulfillmentGraph phase 3 test 1', () => { - const graph = buildRequirementFulfillmentGraph( - { - requirements, - userCourses: [CS3410, CS3420, MATH4710], - userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, - userChoiceOnDoubleCountingElimination: { [MATH4710.uniqueId]: 'Probability' }, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, + const graph = buildRequirementFulfillmentGraph({ + requirements, + userCourses: [CS3410, CS3420, MATH4710], + userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, + userChoiceOnRequirementOverrides: { + [MATH4710.uniqueId]: { optIn: [], optOut: ['Elective'] }, }, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); + getAllCoursesThatCanPotentiallySatisfyRequirement, + }); expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410]); expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([MATH4710]); @@ -123,81 +100,35 @@ it('buildRequirementFulfillmentGraph phase 3 test 1', () => { }); it('buildRequirementFulfillmentGraph phase 3 test 2', () => { - const graph = buildRequirementFulfillmentGraph( - { - requirements, - userCourses: [CS3410, CS3420, MATH4710], - userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, - userChoiceOnDoubleCountingElimination: { [MATH4710.uniqueId]: 'Elective' }, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, - }, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); - - expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410]); - expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([]); - expect(graph.getConnectedCoursesFromRequirement('Elective')).toEqual([CS3410, CS3420, MATH4710]); -}); - -// The following test ensures that we will remove edges when user makes no choice on courses. -it('buildRequirementFulfillmentGraph phase 3 test 3', () => { const graph = buildRequirementFulfillmentGraph({ requirements, userCourses: [CS3410, CS3420, MATH4710], userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, - userChoiceOnDoubleCountingElimination: { [MATH4710.uniqueId]: 'Probability' }, - userChoiceOnRequirementOverrides: {}, + userChoiceOnRequirementOverrides: { + [MATH4710.uniqueId]: { optIn: [], optOut: ['Probability'] }, + }, getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: () => false, }); - expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([]); - expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([MATH4710]); - expect(graph.getConnectedCoursesFromRequirement('Elective')).toEqual([]); + expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410]); + expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([]); + expect(graph.getConnectedCoursesFromRequirement('Elective')).toEqual([CS3410, CS3420, MATH4710]); }); // The following test runs on a fully specified graph algorithm on complete user choices -it('buildRequirementFulfillmentGraph phase 3 test 4', () => { +it('buildRequirementFulfillmentGraph phase 3 test 3', () => { const graph = buildRequirementFulfillmentGraph({ requirements, userCourses: [CS3410, CS3420, MATH4710], userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, - userChoiceOnDoubleCountingElimination: { - [CS3410.uniqueId]: 'CS3410/CS3420', - [CS3420.uniqueId]: 'Elective', - [MATH4710.uniqueId]: 'Elective', + userChoiceOnRequirementOverrides: { + [CS3410.uniqueId]: { optIn: [], optOut: ['Elective'] }, + [MATH4710.uniqueId]: { optIn: [], optOut: ['Elective'] }, }, - userChoiceOnRequirementOverrides: {}, getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: r => r === 'Probability', }); expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([CS3410]); expect(graph.getConnectedCoursesFromRequirement('Probability')).toEqual([MATH4710]); - expect(graph.getConnectedCoursesFromRequirement('Elective')).toEqual([CS3420, MATH4710]); -}); - -// Normally, we will remove all edges when there is no user choice associated with a unique ID. -// If we do the same for AP/IB/swim courses, then all these courses will never be able to fulfill -// anything. -// The following test ensures that we don't regress again. -it('AP/IB/swim test course edge is not removed in step 3', () => { - // Test this for a lot of different unique ID less than - for (let uniqueId = -1; uniqueId >= -10; uniqueId -= 1) { - const graph = buildRequirementFulfillmentGraph({ - requirements, - userCourses: [{ courseId: CS3410.courseId, uniqueId }], // mock an AP/IB course - userChoiceOnFulfillmentStrategy: { 'CS3410/CS3420': [CS3410.courseId] }, - userChoiceOnDoubleCountingElimination: {}, - userChoiceOnRequirementOverrides: {}, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting: r => r === 'Probability', - }); - - expect(graph.getConnectedCoursesFromRequirement('CS3410/CS3420')).toEqual([ - { courseId: CS3410.courseId, uniqueId }, - ]); - } + expect(graph.getConnectedCoursesFromRequirement('Elective')).toEqual([CS3420]); }); diff --git a/src/requirements/admin/requirement-graph-admin-utils.ts b/src/requirements/admin/requirement-graph-admin-utils.ts index 1e4e35210..b37490a55 100644 --- a/src/requirements/admin/requirement-graph-admin-utils.ts +++ b/src/requirements/admin/requirement-graph-admin-utils.ts @@ -3,7 +3,7 @@ import { checkNotNull } from '../../utilities'; import { semestersCollection, toggleableRequirementChoicesCollection, - selectableRequirementChoicesCollection, + overriddenFulfillmentChoicesCollection, onboardingDataCollection, } from '../../firebase-admin-config'; import { getCourseCodesArray } from '../requirement-frontend-computation'; @@ -14,7 +14,7 @@ interface UserDataOnAdmin { readonly courses: readonly CourseTaken[]; readonly onboardingData: AppOnboardingData; readonly toggleableRequirementChoices: Readonly>; - readonly selectableRequirementChoices: Readonly>; + readonly overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices; } interface UserRequirementDataOnAdmin extends UserDataOnAdmin { @@ -26,7 +26,7 @@ export async function getUserDataOnAdmin(userEmail: string): Promise it.data() || {}), - selectableRequirementChoicesCollection + overriddenFulfillmentChoicesCollection .doc(userEmail) .get() .then(it => it.data() || {}), @@ -49,7 +49,7 @@ export async function getUserDataOnAdmin(userEmail: string): Promise>; readonly requirementFulfillmentGraph: RequirementFulfillmentGraph; @@ -198,7 +197,6 @@ export default function computeGroupedRequirementFulfillmentReports( coursesTaken, onboardingData, toggleableRequirementChoices, - selectableRequirementChoices, overriddenFulfillmentChoices ); diff --git a/src/requirements/requirement-frontend-utils.ts b/src/requirements/requirement-frontend-utils.ts index 8adb040ad..bf163392a 100644 --- a/src/requirements/requirement-frontend-utils.ts +++ b/src/requirements/requirement-frontend-utils.ts @@ -177,6 +177,7 @@ export function getUserRequirements({ */ type MatchedRequirementFulfillmentSpecificationBase = { readonly fulfilledBy: 'courses' | 'credits'; + readonly hasRequirementCheckerWarning: boolean; readonly eligibleCourses: readonly (readonly number[])[]; readonly perSlotMinCount: readonly number[]; readonly slotNames: readonly string[]; @@ -226,6 +227,7 @@ export function getMatchedRequirementFulfillmentSpecification( name, { fulfilledBy: subRequirement.fulfilledBy, + hasRequirementCheckerWarning: false, eligibleCourses: subRequirement.courses, perSlotMinCount: subRequirement.perSlotMinCount, slotNames, @@ -235,12 +237,14 @@ export function getMatchedRequirementFulfillmentSpecification( }) ); + const hasRequirementCheckerWarning = requirement.checkerWarning != null; switch (requirement.fulfilledBy) { case 'self-check': return null; case 'courses': return { fulfilledBy: requirement.fulfilledBy, + hasRequirementCheckerWarning, eligibleCourses: requirement.courses, additionalRequirements: convertAdditionalRequirements(requirement.additionalRequirements), perSlotMinCount: requirement.perSlotMinCount, @@ -250,6 +254,7 @@ export function getMatchedRequirementFulfillmentSpecification( case 'credits': return { fulfilledBy: requirement.fulfilledBy, + hasRequirementCheckerWarning, eligibleCourses: requirement.courses, additionalRequirements: convertAdditionalRequirements(requirement.additionalRequirements), perSlotMinCount: requirement.perSlotMinCount, @@ -264,6 +269,7 @@ export function getMatchedRequirementFulfillmentSpecification( ]; return { fulfilledBy: option.counting, + hasRequirementCheckerWarning, eligibleCourses: option.courses, perSlotMinCount: option.perSlotMinCount, slotNames: option.counting === 'courses' ? option.slotNames : [], @@ -278,10 +284,11 @@ export function getMatchedRequirementFulfillmentSpecification( const computeFulfillmentStatistics = ( requirementName: string, coursesTaken: readonly CourseTaken[], - overriddenFulfillmentChoices: AppOverriddenFulfillmentChoices, + overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices, disallowTransferCredit: boolean, { fulfilledBy, + hasRequirementCheckerWarning, eligibleCourses, perSlotMinCount, slotNames, @@ -291,19 +298,33 @@ const computeFulfillmentStatistics = ( const coursesThatFulfilledSubRequirements: CourseTaken[][] = eligibleCourses.map(() => []); const subRequirementProgress: number[] = eligibleCourses.map(() => 0); coursesTaken.forEach(courseTaken => { - const overrideOptions = overriddenFulfillmentChoices[courseTaken.uniqueId]; - const optInSlotNames = (overrideOptions && overrideOptions.optIn[requirementName]) || null; - const optOutSlotNames = (overrideOptions && overrideOptions.optOut[requirementName]) || null; + const overrideOptions = overriddenFulfillmentChoices[courseTaken.uniqueId] || { + arbitraryOptIn: [], + acknowledgedCheckerWarningOptIn: [], + optOut: [], + }; + // If a requirement has checker warning, do not match it the course unless it's acknoledged. + if ( + overrideOptions.optOut.includes(requirementName) || + (hasRequirementCheckerWarning && + !overrideOptions.acknowledgedCheckerWarningOptIn.includes(requirementName)) + ) { + return; + } + const arbitraryOptInSlotNames = new Set(overrideOptions.arbitraryOptIn[requirementName] || []); // block AP/IB equivalent courses if disallowTransferCredit - if (!(disallowTransferCredit && courseIsAPIB(courseTaken)) || optInSlotNames) { + if ( + !(disallowTransferCredit && courseIsAPIB(courseTaken)) || + arbitraryOptInSlotNames.size > 0 + ) { for ( let subRequirementIndex = 0; subRequirementIndex < eligibleCourses.length; subRequirementIndex += 1 ) { const slotName = fulfilledBy === 'courses' ? slotNames[subRequirementIndex] : 'Course'; - if (optInSlotNames && (fulfilledBy === 'credits' || optInSlotNames.has(slotName))) { + if (arbitraryOptInSlotNames.has(slotName)) { // the user wants to use this course to override this sub-requirement coursesThatFulfilledSubRequirements[subRequirementIndex].push(courseTaken); subRequirementProgress[subRequirementIndex] += @@ -311,8 +332,7 @@ const computeFulfillmentStatistics = ( // don't break, in case the user wants to override more sub-requirements with the same course } else if ( eligibleCourses[subRequirementIndex].includes(courseTaken.courseId) && - subRequirementProgress[subRequirementIndex] < perSlotMinCount[subRequirementIndex] && - !(optOutSlotNames && (fulfilledBy === 'credits' || optOutSlotNames.has(slotName))) + subRequirementProgress[subRequirementIndex] < perSlotMinCount[subRequirementIndex] ) { // this course is eligible to fulfill this sub-requirement, and the user did not opt out coursesThatFulfilledSubRequirements[subRequirementIndex].push(courseTaken); @@ -357,7 +377,7 @@ export function computeFulfillmentCoursesAndStatistics( requirement: RequirementWithIDSourceType, coursesTaken: readonly CourseTaken[], toggleableRequirementChoices: AppToggleableRequirementChoices, - overriddenFulfillmentChoices: AppOverriddenFulfillmentChoices + overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices ): RequirementFulfillmentStatisticsWithCoursesWithAdditionalRequirements { const spec = getMatchedRequirementFulfillmentSpecification( requirement, @@ -402,7 +422,7 @@ export function getRelatedUnfulfilledRequirements( }: CornellCourseRosterCourse, groupedRequirements: readonly GroupedRequirementFulfillmentReport[], toggleableRequirementChoices: AppToggleableRequirementChoices, - overriddenFulfillmentChoices: AppOverriddenFulfillmentChoices + overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices ): { readonly directlyRelatedRequirements: readonly RequirementWithIDSourceType[]; readonly selfCheckRequirements: readonly RequirementWithIDSourceType[]; diff --git a/src/requirements/requirement-graph-builder-from-user-data.ts b/src/requirements/requirement-graph-builder-from-user-data.ts index 3d134c8bb..b8f8f73b8 100644 --- a/src/requirements/requirement-graph-builder-from-user-data.ts +++ b/src/requirements/requirement-graph-builder-from-user-data.ts @@ -1,5 +1,5 @@ import { CREDITS_COURSE_ID } from './data/constants'; -import { courseIsAPIB, getUserRequirements } from './requirement-frontend-utils'; +import { getUserRequirements } from './requirement-frontend-utils'; import RequirementFulfillmentGraph from './requirement-graph'; import buildRequirementFulfillmentGraph, { BuildRequirementFulfillmentGraphParameters, @@ -32,10 +32,7 @@ export default function buildRequirementFulfillmentGraphFromUserData( coursesTaken: readonly CourseTaken[], onboardingData: AppOnboardingData, toggleableRequirementChoices: AppToggleableRequirementChoices, - selectableRequirementChoices: AppSelectableRequirementChoices, - overriddenFulfillmentChoices: AppOverriddenFulfillmentChoices, - /** A flag for data migration. Prod code should never use this */ - keepCoursesWithoutDoubleCountingEliminationChoice = false + overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices ): { readonly userRequirements: readonly RequirementWithIDSourceType[]; readonly userRequirementsMap: Readonly>; @@ -62,30 +59,32 @@ export default function buildRequirementFulfillmentGraphFromUserData( }) .filter((it): it is [string, number[]] => it != null) ), - userChoiceOnDoubleCountingElimination: Object.fromEntries( - Object.entries(selectableRequirementChoices).map(([uniqueIDString, requirementID]) => { - const uniqueId = Number.isNaN(uniqueIDString) - ? uniqueIDString - : parseInt(uniqueIDString, 10); - return [uniqueId, requirementID] as const; + userChoiceOnRequirementOverrides: Object.fromEntries( + coursesTaken.flatMap(course => { + const uniqueId = course.uniqueId.toString(); + const choice = overriddenFulfillmentChoices[uniqueId]; + if (choice == null) return []; + return [ + [ + uniqueId, + { + optIn: Array.from( + new Set([ + ...Object.keys(choice.arbitraryOptIn), + ...choice.acknowledgedCheckerWarningOptIn, + ]) + ), + optOut: choice.optOut, + }, + ], + ]; }) ), - userChoiceOnRequirementOverrides: { - ...Object.fromEntries( - coursesTaken - .map(course => { - const uniqueId = course.uniqueId.toString(); - if (!(uniqueId in overriddenFulfillmentChoices)) return null; - const overriddenFulfillments = new Set( - Object.keys(overriddenFulfillmentChoices[uniqueId].optIn) - ); - return [uniqueId, overriddenFulfillments]; - }) - .filter((it): it is [string, Set] => it != null) - ), - }, getAllCoursesThatCanPotentiallySatisfyRequirement: requirementID => { const requirement = userRequirementsMap[requirementID]; + // When a requirement has checker warning, we do not add those edges in phase 1. + // All edges will be explictly opt-in only from stage 3. + if (requirement.checkerWarning != null) return []; let eligibleCoursesList: readonly (readonly number[])[]; switch (requirement.fulfilledBy) { case 'self-check': @@ -104,115 +103,10 @@ export default function buildRequirementFulfillmentGraphFromUserData( } return eligibleCoursesList.flat(); }, - allowDoubleCounting: requirementID => - userRequirementsMap[requirementID].allowCourseDoubleCounting || false, }; const requirementFulfillmentGraph = buildRequirementFulfillmentGraph( - requirementGraphBuilderParameters, - keepCoursesWithoutDoubleCountingEliminationChoice + requirementGraphBuilderParameters ); return { userRequirements, userRequirementsMap, requirementFulfillmentGraph }; } - -export type FirestoreCourseOptInOptOutChoicesBuilder = ( - c: CourseTaken -) => FirestoreCourseOptInOptOutChoices; - -/** - * @returns a function that given a course, return a `FirestoreCourseOptInOptOutChoices`. - * - * Frontend code can use the returned function to compute the equivalent opt-out data, - * and the data migration code can use the returned function to compute opt-out choice for all courses. - * This function should be deleted once we fully finish the migration. - */ -export function getFirestoreCourseOptInOptOutChoicesBuilder( - coursesTaken: readonly CourseTaken[], - onboardingData: AppOnboardingData, - toggleableRequirementChoices: AppToggleableRequirementChoices, - selectableRequirementChoices: AppSelectableRequirementChoices, - overriddenFulfillmentChoices: AppOverriddenFulfillmentChoices -): FirestoreCourseOptInOptOutChoicesBuilder { - // In this graph, each course is connected to all requirements that course can be used to satisfy. - const { - requirementFulfillmentGraph: graphWithoutDoubleCountingAccounted, - userRequirementsMap, - } = buildRequirementFulfillmentGraphFromUserData( - coursesTaken, - onboardingData, - toggleableRequirementChoices, - {}, // Provide no double counting choices, so all the edges will be kept - overriddenFulfillmentChoices, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ true - ); - - // In this graph, each course is only connected to the selected requirement and the requirements - // that allow double counting. - const graphWithDoubleCountingAccounted = buildRequirementFulfillmentGraphFromUserData( - coursesTaken, - onboardingData, - toggleableRequirementChoices, - selectableRequirementChoices, - overriddenFulfillmentChoices, - /* keepCoursesWithoutDoubleCountingEliminationChoice */ false - ).requirementFulfillmentGraph; - - /** - * The table below summerizes the type of all possible requirement-course edges before - * double-counting elimination, and where they will go in the new format. - * - * ------------------------------------------------------------------------------------------------- - * | Requirement Type | edge exists after double-counting elim | where is it in the new format | - * | ---------------- | -------------------------------------- | --------------------------------- | - * | selected | True | implicit (connected by default) | - * | (no warning) | | | - * | | | | - * | selected | True | `acknowledgedCheckerWarningOptIn` | - * | (has warning) | | | - * | | | | - * | allow double | True | implicit (connected by default) | - * | counting | | | - * | | | | - * | not connected | False | `optOut` | - * | (no warning) | | | - * | | | | - * | not connected | False | implicit (unconnected by default) | - * | (has warning) | | | - */ - return function builder(course) { - const requirementsWithDoubleCountingRemoved = graphWithDoubleCountingAccounted.getConnectedRequirementsFromCourse( - course - ); - const allRelevantRequirements = graphWithoutDoubleCountingAccounted.getConnectedRequirementsFromCourse( - course - ); - /** - * complementary == All unconnected requirement after double counting elimination - * - * It's true that - * ``` - * union( - * selectableRequirementChoices[course], - * requirements that allow double counting and the given course can be used to satisfy, - * complementary, - * ) == all requirements that course can be used to satisfy - * ``` - */ - const complementary = new Set(allRelevantRequirements); - requirementsWithDoubleCountingRemoved.forEach(r => complementary.delete(r)); - - // We only need to explicitly opt-out of requirements without checker warnings, since requirement - // with checker warnings need to be explicitly opt-in. - const optOut = Array.from(complementary).filter( - it => userRequirementsMap[it].checkerWarning == null - ); - // Find requirements with checker warnings that needs to be explictly opt-in. - const acknowledgedCheckerWarningOptIn = courseIsAPIB(course) - ? [] - : requirementsWithDoubleCountingRemoved.filter( - it => userRequirementsMap[it].checkerWarning != null - ); - - return { optOut, arbitraryOptIn: {}, acknowledgedCheckerWarningOptIn }; - }; -} diff --git a/src/requirements/requirement-graph-builder.ts b/src/requirements/requirement-graph-builder.ts index 3a9c3a0bb..908f79bb7 100644 --- a/src/requirements/requirement-graph-builder.ts +++ b/src/requirements/requirement-graph-builder.ts @@ -24,22 +24,21 @@ export type BuildRequirementFulfillmentGraphParameters< */ readonly userChoiceOnFulfillmentStrategy: Readonly>; /** - * The mapping from course's unique ID to requirement. - * It describes how the user decides how a course is used to satisfy requirements. - * Suppose a course with unique ID c can be used to satisfy both r1 and r2, but the - * requirements do not allow double counting, then a mapping `c => r1` means that we should keep the - * (r1, c) edge in the graph and drop the (r2, c) edge. - */ - readonly userChoiceOnDoubleCountingElimination: Readonly>; - /** - * The mapping from course's unique ID to requirement override (opt-in) options. + * The mapping from course's unique ID to requirement override opt-in and opt-out options. * It describes how the user wants to use a course to override requirements. - * This handles AP/IB overrides, as well as general overrides. + * This handles AP/IB overrides, as well as general overrides. This can encode double counting + * elimination as opt-out. * The granularity of optIn/optOut being slot-specific requires the actual * slot computation to be handled in the frontend computation. When building the - * graph, only optIn choices are relevant (to add the extra edges). + * graph, only optIn/optOut choices are relevant (to add the extra edges). */ - readonly userChoiceOnRequirementOverrides: Readonly>>; + readonly userChoiceOnRequirementOverrides: { + readonly [uniqueId: string]: { + // Including both acknowledged and arbitrary opt-in. + readonly optIn: readonly Requirement[]; + readonly optOut: readonly Requirement[]; + }; + }; /** * Naively give a list of courses ID that can satisfy a requirement. Most of the time this function * should just return the pre-computed eligible course id list. For requirements have multiple @@ -48,29 +47,21 @@ export type BuildRequirementFulfillmentGraphParameters< readonly getAllCoursesThatCanPotentiallySatisfyRequirement: ( requirement: Requirement ) => readonly number[]; - /** - * Report whether a requirement allows a course connected to it to also be used to fulfill some - * other requirement. - */ - readonly allowDoubleCounting: (requirement: Requirement) => boolean; }; const buildRequirementFulfillmentGraph = < Requirement extends string, Course extends CourseForRequirementGraph ->( - { - requirements, - userCourses, - userChoiceOnFulfillmentStrategy, - userChoiceOnDoubleCountingElimination, - userChoiceOnRequirementOverrides, - getAllCoursesThatCanPotentiallySatisfyRequirement, - allowDoubleCounting, - }: BuildRequirementFulfillmentGraphParameters, - /** A flag for testing. Prod code should never use this */ - keepCoursesWithoutDoubleCountingEliminationChoice = false -): RequirementFulfillmentGraph => { +>({ + requirements, + userCourses, + userChoiceOnFulfillmentStrategy, + userChoiceOnRequirementOverrides, + getAllCoursesThatCanPotentiallySatisfyRequirement, +}: BuildRequirementFulfillmentGraphParameters): RequirementFulfillmentGraph< + Requirement, + Course +> => { const graph = new RequirementFulfillmentGraph(); const userCourseCourseIDToCourseMap = new Map(); userCourses.forEach(course => { @@ -110,34 +101,22 @@ const buildRequirementFulfillmentGraph = < } ); - // Phase 3: Respect user's choices on double-counted courses. - userCourses.forEach(({ uniqueId }) => { + // Phase 3: Respect user's choices on opt-in/opt-out. + userCourses.forEach(course => { + const { uniqueId } = course; // typeof uniqueId === 'string' means it's AP/IB equivalent course. // uniqueId < 0 means it's swim test. // User never gets to make a choice about these courses, so it will never appear in the choices. // Therefore, removing those edges will nullify all these credits. if (typeof uniqueId === 'string' || uniqueId < 0) return; - const chosenRequirement = userChoiceOnDoubleCountingElimination[uniqueId]; - graph.getConnectedRequirementsFromCourse({ uniqueId }).forEach(connectedRequirement => { - if (allowDoubleCounting(connectedRequirement)) return; - // keepCoursesWithoutDoubleCountingEliminationChoice is used to avoid removing edges when - // the user choice is null on the course. It is used to test phase-1 and phase-2 output - // independently. - if (keepCoursesWithoutDoubleCountingEliminationChoice && chosenRequirement == null) return; - if (connectedRequirement !== chosenRequirement) { - graph.removeEdge(connectedRequirement, { uniqueId }); - } + const userChoiceOnOptInOptOutCourse = userChoiceOnRequirementOverrides[uniqueId]; + if (userChoiceOnOptInOptOutCourse == null) return; + userChoiceOnOptInOptOutCourse.optIn.forEach(optedInRequirement => { + graph.addEdge(optedInRequirement, course); + }); + userChoiceOnOptInOptOutCourse.optOut.forEach(optedInRequirement => { + graph.removeEdge(optedInRequirement, course); }); - }); - - // Phase 4: Respect user's choices on overrides (optIn only). - userCourses.forEach(course => { - const { uniqueId } = course; - if (uniqueId in userChoiceOnRequirementOverrides) { - userChoiceOnRequirementOverrides[uniqueId].forEach(requirement => { - graph.addEdge(requirement, course); - }); - } }); // Phase MAX_INT: PROFIT! diff --git a/src/store.ts b/src/store.ts index 73e994063..a65e80f9d 100644 --- a/src/store.ts +++ b/src/store.ts @@ -279,8 +279,7 @@ const autoRecomputeDerivedData = (): (() => void) => state.semesters, state.onboardingData, state.toggleableRequirementChoices, - state.selectableRequirementChoices, - /* deprecated AppOverriddenFulfillmentChoices */ {} + state.overriddenFulfillmentChoices ) ); } diff --git a/src/user-data.d.ts b/src/user-data.d.ts index b47f78c53..66230450e 100644 --- a/src/user-data.d.ts +++ b/src/user-data.d.ts @@ -160,18 +160,3 @@ type AppToggleableRequirementChoices = Readonly>; /** Map from course's unique ID to requirement ID */ type AppSelectableRequirementChoices = Readonly>; - -/** - * @deprecated replaced by `FirestoreOverriddenFulfillmentChoices` - * - * Map from course's unique ID to override options. - */ -type AppOverriddenFulfillmentChoices = Readonly< - Record< - string, - { - readonly optIn: Record>; - readonly optOut: Record>; - } - > ->;