Skip to content

Commit

Permalink
Fix requirements double counting (#638)
Browse files Browse the repository at this point in the history 
* fix bugs

* remove console log

* format

* fix arg order

* fix add modal opt out

* refactor dummy uniqueid

* refactor to getRelatedUnfulfilledRequirements

* move opt out filter to opt out function

* automatically fulfilled requirements

* add comment

* opt out of all requirements possibly in constraint violation
  • Loading branch information
@benjamin-shen
benjamin-shen authored Jan 17, 2022
1 parent d2ec4a9 commit cfcacf6
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 61 deletions.
66 changes: 33 additions & 33 deletions src/components/Modals/NewCourse/NewCourseModal.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,16 @@
data-cyId="newCourse-dropdown"
/>
<div v-else class="selected-course" data-cyId="newCourse-selectedCourse">
{{ selectedCourse.subject }} {{ selectedCourse.catalogNbr }}: {{ selectedCourse.titleLong }}
{{ selectedCourse.subject }} {{ selectedCourse.catalogNbr }}:
{{ selectedCourse.titleLong }}
</div>
<div v-if="selectedCourse != null">
<!-- if a course is selected -->
<selected-requirement-editor
:key="courseSelectorKey"
:editMode="editMode"
:selectedRequirementID="selectedRequirementID"
:requirementsThatAllowDoubleCounting="requirementsThatAllowDoubleCounting"
:automaticallyFulfilledRequirements="automaticallyFulfilledRequirements"
:relatedRequirements="relatedRequirements"
:potentialRequirements="selfCheckRequirements"
@on-selected-change="onSelectedChange"
Expand All @@ -48,7 +49,10 @@ import TeleportModal from '@/components/Modals/TeleportModal.vue';
import CourseSelector from '@/components/Modals/NewCourse/CourseSelector.vue';
import store from '@/store';
import { getRelatedUnfulfilledRequirements } from '@/requirements/requirement-frontend-utils';
import {
getRelatedRequirementIdsForCourseOptOut,
getRelatedUnfulfilledRequirements,
} from '@/requirements/requirement-frontend-utils';
export default defineComponent({
components: { CourseSelector, TeleportModal, SelectedRequirementEditor },
Expand All @@ -61,7 +65,7 @@ export default defineComponent({
return {
selectedCourse: null as CornellCourseRosterCourse | null,
selectedRequirementID: '',
requirementsThatAllowDoubleCounting: [] as readonly string[],
automaticallyFulfilledRequirements: [] as readonly string[],
// relatedRequirements : the requirements that don't allow double counting
relatedRequirements: [] as readonly RequirementWithIDSourceType[],
selfCheckRequirements: [] as readonly RequirementWithIDSourceType[],
Expand Down Expand Up @@ -89,38 +93,31 @@ export default defineComponent({
},
getReqsRelatedToCourse(selectedCourse: CornellCourseRosterCourse) {
const {
directlyRelatedRequirements,
relatedRequirements,
selfCheckRequirements,
automaticallyFulfilledRequirements,
} = getRelatedUnfulfilledRequirements(
selectedCourse,
store.state.groupedRequirementFulfillmentReport,
store.state.toggleableRequirementChoices,
store.state.overriddenFulfillmentChoices
store.state.overriddenFulfillmentChoices,
store.state.userRequirementsMap
);
const automaticallyFulfilledRequirementIds = new Set(
automaticallyFulfilledRequirements.map(({ id }) => id)
);
const requirementsThatAllowDoubleCounting: string[] = [];
const relatedRequirements: RequirementWithIDSourceType[] = [];
directlyRelatedRequirements.forEach(it => {
if (it.allowCourseDoubleCounting) {
requirementsThatAllowDoubleCounting.push(it.name);
} else {
relatedRequirements.push(it);
}
});
const selfCheckRequirementsThatDoesNotAllowDoubleCounting: RequirementWithIDSourceType[] = [];
selfCheckRequirements.forEach(it => {
if (it.allowCourseDoubleCounting) {
requirementsThatAllowDoubleCounting.push(it.name);
} else {
selfCheckRequirementsThatDoesNotAllowDoubleCounting.push(it);
}
});
this.requirementsThatAllowDoubleCounting = requirementsThatAllowDoubleCounting;
this.relatedRequirements = relatedRequirements;
this.selfCheckRequirements = selfCheckRequirementsThatDoesNotAllowDoubleCounting;
if (relatedRequirements.length > 0) {
this.selectedRequirementID = relatedRequirements[0].id;
this.automaticallyFulfilledRequirements = automaticallyFulfilledRequirements.map(
({ name }) => name
);
this.relatedRequirements = relatedRequirements.filter(
req => !automaticallyFulfilledRequirementIds.has(req.id)
);
this.selfCheckRequirements = selfCheckRequirements.filter(
req => !automaticallyFulfilledRequirementIds.has(req.id)
);
if (this.relatedRequirements.length > 0) {
this.selectedRequirementID = this.relatedRequirements[0].id;
} else {
this.selectedRequirementID = '';
}
Expand All @@ -135,10 +132,13 @@ export default defineComponent({
addCourse() {
if (this.selectedCourse == null) return;
this.$emit('add-course', this.selectedCourse, {
// Only exclude the selected requirement from opt-out.
optOut: this.relatedRequirements
.filter(it => it.id !== this.selectedRequirementID)
.map(it => it.id),
optOut: getRelatedRequirementIdsForCourseOptOut(
this.selectedCourse.crseId,
this.selectedRequirementID,
store.state.groupedRequirementFulfillmentReport,
store.state.toggleableRequirementChoices,
store.state.userRequirementsMap
),
// Only include the selected requirement from opt-in.
acknowledgedCheckerWarningOptIn: this.selfCheckRequirements
.filter(it => it.id === this.selectedRequirementID)
Expand Down
10 changes: 5 additions & 5 deletions src/components/Modals/NewCourse/SelectedRequirementEditor.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
<div>
<div
v-if="
editMode ? requirementsThatAllowDoubleCounting.length > 0 : chosenRequirementText.length > 0
editMode ? automaticallyFulfilledRequirements.length > 0 : chosenRequirementText.length > 0
"
>
<div class="newCourse-title">
This class automatically fulfills the following requirement(s):
</div>
<div class="newCourse-requirements-container">
<div class="newCourse-requirements" data-cyId="newCourse-requirements">
{{ editMode ? requirementsThatAllowDoubleCounting.join(', ') : chosenRequirementText }}
{{ editMode ? automaticallyFulfilledRequirements.join(', ') : chosenRequirementText }}
</div>
</div>
</div>
Expand Down Expand Up @@ -82,7 +82,7 @@ export default defineComponent({
props: {
editMode: { type: Boolean, required: true },
selectedRequirementID: { type: String, required: true },
requirementsThatAllowDoubleCounting: {
automaticallyFulfilledRequirements: {
type: Array as PropType<readonly string[]>,
required: true,
},
Expand All @@ -104,12 +104,12 @@ export default defineComponent({
computed: {
chosenRequirementText(): string {
if (this.selectedRequirementID === '') {
return this.requirementsThatAllowDoubleCounting.join(', ');
return this.automaticallyFulfilledRequirements.join(', ');
}
const chosenRequirementNames = [...this.relatedRequirements, ...this.potentialRequirements]
.filter(it => it.id === this.selectedRequirementID)
.map(it => it.name);
return [...this.requirementsThatAllowDoubleCounting, ...chosenRequirementNames].join(', ');
return [...this.automaticallyFulfilledRequirements, ...chosenRequirementNames].join(', ');
},
// nonAutoRequirements = relatedRequirements + potentialRequirements
// All the requirements that are not automatically associated with the course
Expand Down
21 changes: 14 additions & 7 deletions src/components/Requirements/IncompleteSelfCheck.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import {
} from '@/global-firestore-data';
import {
canFulfillChecker,
getAllEligibleRelatedRequirementIds,
getRelatedRequirementIdsForCourseOptOut,
} from '@/requirements/requirement-frontend-utils';
import NewSelfCheckCourseModal from '@/components/Modals/NewCourse/NewSelfCheckCourseModal.vue';
Expand Down Expand Up @@ -101,6 +101,8 @@ export default defineComponent({
return;
}
/* TODO @bshen fix .allowCourseDoubleCounting flag
we should allow the constraint violation to be broken, i.e. don't return early */
const currentRequirementAllowDoubleCounting =
store.state.userRequirementsMap[this.subReqCourseId]?.allowCourseDoubleCounting;
const allOtherRequirementsAllowDoubleCounting = store.state.safeRequirementFulfillmentGraph
Expand Down Expand Up @@ -139,18 +141,21 @@ export default defineComponent({
},
addExistingCourse(option: string) {
this.showDropdown = false;
updateRequirementChoice(this.selfCheckCourses[option].uniqueID, choice => ({
const { uniqueID, crseId } = this.selfCheckCourses[option];
updateRequirementChoice(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])
),
// Keep existing behavior of keeping it connected to at most one requirement.
optOut: getAllEligibleRelatedRequirementIds(
this.selfCheckCourses[option].crseId,
optOut: getRelatedRequirementIdsForCourseOptOut(
crseId,
this.subReqId,
store.state.groupedRequirementFulfillmentReport,
store.state.toggleableRequirementChoices
store.state.toggleableRequirementChoices,
store.state.userRequirementsMap
),
}));
},
Expand All @@ -170,10 +175,12 @@ export default defineComponent({
// We also need to opt-out of all requirements without warnings,
// because the user intention is clear that we only want to bind
// the course to this specific requirement.
optOut: getAllEligibleRelatedRequirementIds(
optOut: getRelatedRequirementIdsForCourseOptOut(
newCourse.crseId,
this.subReqId,
store.state.groupedRequirementFulfillmentReport,
store.state.toggleableRequirementChoices
store.state.toggleableRequirementChoices,
store.state.userRequirementsMap
),
}),
this.$gtag
Expand Down
10 changes: 6 additions & 4 deletions src/components/Semester/Semester.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ import {
updateRequirementChoices,
} from '@/global-firestore-data';
import store, { updateSubjectColorData } from '@/store';
import { getAllEligibleRelatedRequirementIds } from '@/requirements/requirement-frontend-utils';
import { getRelatedRequirementIdsForCourseOptOut } from '@/requirements/requirement-frontend-utils';
type ComponentRef = { $el: HTMLDivElement };
Expand Down Expand Up @@ -280,11 +280,13 @@ export default defineComponent({
// the `optOut` field.
// Below, we find all the requirements it can possibly match,
// and only remove the requirementID since that's the one we should keep.
const optOut = getAllEligibleRelatedRequirementIds(
const optOut = getRelatedRequirementIdsForCourseOptOut(
crseId,
requirementID,
store.state.groupedRequirementFulfillmentReport,
store.state.toggleableRequirementChoices
).filter(it => it !== requirementID);
store.state.toggleableRequirementChoices,
store.state.userRequirementsMap
);
choices[uniqueID] = { ...choice, optOut };
});
return choices;
Expand Down
64 changes: 52 additions & 12 deletions src/requirements/requirement-frontend-utils.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import requirementJson from './typed-requirement-json';
import specialized from './specialize';
import { examCourseIds } from './requirement-exam-mapping';
import { getConstraintViolationsForSingleCourse } from './requirement-constraints-utils';

/**
* A collection of helper functions
Expand Down Expand Up @@ -444,10 +445,17 @@ export function computeFulfillmentCoursesAndStatistics(
};
}

export function getAllEligibleRelatedRequirementIds(
/**
* Find related requirement ids to opt out of. This maintains the invariant that
* there are no constraint violations when adding a course to the semester.
* It should be deprecated after the new add modal is implemented.
*/
export function getRelatedRequirementIdsForCourseOptOut(
courseId: number,
associatedRequirementId: string,
groupedRequirements: readonly GroupedRequirementFulfillmentReport[],
toggleableRequirementChoices: AppToggleableRequirementChoices
toggleableRequirementChoices: AppToggleableRequirementChoices,
userRequirementsMap: Readonly<Record<string, RequirementWithIDSourceType>>
): readonly string[] {
const requirements = groupedRequirements
.flatMap(it => it.reqs)
Expand All @@ -458,12 +466,29 @@ export function getAllEligibleRelatedRequirementIds(
);
if (spec == null) return [];
const allEligibleCourses = spec.eligibleCourses.flat();
if (allEligibleCourses.includes(courseId) && requirement.checkerWarning == null) {
if (
requirement.id === associatedRequirementId ||
(allEligibleCourses.includes(courseId) && requirement.checkerWarning == null)
) {
return [requirement.id];
}
return [];
});
return requirements;
// only return the requirements that are in a constraint violation
const uniqueId = -1; // dummy unique id
const { requirementsThatDoNotAllowDoubleCounting } = getConstraintViolationsForSingleCourse(
{ uniqueId },
requirements,
(reqA, reqB) =>
allowCourseDoubleCountingBetweenRequirements(
userRequirementsMap[reqA],
userRequirementsMap[reqB]
)
);
// order does not need to be preserved
return Array.from(requirementsThatDoNotAllowDoubleCounting).filter(
it => it !== associatedRequirementId
);
}

export function getRelatedUnfulfilledRequirements(
Expand All @@ -475,13 +500,15 @@ export function getRelatedUnfulfilledRequirements(
}: CornellCourseRosterCourse,
groupedRequirements: readonly GroupedRequirementFulfillmentReport[],
toggleableRequirementChoices: AppToggleableRequirementChoices,
overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices
overriddenFulfillmentChoices: FirestoreOverriddenFulfillmentChoices,
userRequirementsMap: Readonly<Record<string, RequirementWithIDSourceType>>
): {
readonly directlyRelatedRequirements: readonly RequirementWithIDSourceType[];
readonly relatedRequirements: readonly RequirementWithIDSourceType[];
readonly selfCheckRequirements: readonly RequirementWithIDSourceType[];
readonly automaticallyFulfilledRequirements: readonly RequirementWithIDSourceType[];
} {
const code = `${subject} ${catalogNbr}`;
const directlyRelatedRequirements: RequirementWithIDSourceType[] = [];
const relatedRequirements: RequirementWithIDSourceType[] = [];
const selfCheckRequirements: RequirementWithIDSourceType[] = [];
for (let i = 0; i < groupedRequirements.length; i += 1) {
const subreqs = groupedRequirements[i].reqs.filter(
Expand All @@ -501,9 +528,6 @@ export function getRelatedUnfulfilledRequirements(
toggleableRequirementChoices
);
// potential self-check requirements
if (requirementSpec == null && !subRequirement.allowCourseDoubleCounting) {
selfCheckRequirements.push(subRequirement);
}
if (requirementSpec != null) {
const allEligibleCourses = requirementSpec.eligibleCourses.flat();
if (allEligibleCourses.includes(courseId)) {
Expand All @@ -524,14 +548,30 @@ export function getRelatedUnfulfilledRequirements(
);
if (fulfillmentStatisticsWithNewCourse.minCountFulfilled > existingMinCountFulfilled) {
if (subRequirement.checkerWarning == null) {
directlyRelatedRequirements.push(subRequirement);
relatedRequirements.push(subRequirement);
} else {
selfCheckRequirements.push(subRequirement);
}
}
}
} else {
selfCheckRequirements.push(subRequirement);
}
}
}
return { directlyRelatedRequirements, selfCheckRequirements };
const allRequirements = [...relatedRequirements, ...selfCheckRequirements];
const { requirementsThatDoNotAllowDoubleCounting } = getConstraintViolationsForSingleCourse(
{ uniqueId: -1 },
allRequirements.map(({ id }) => id),
(reqA, reqB) =>
allowCourseDoubleCountingBetweenRequirements(
userRequirementsMap[reqA],
userRequirementsMap[reqB]
)
);
const automaticallyFulfilledRequirements = relatedRequirements.filter(
({ id }) => !requirementsThatDoNotAllowDoubleCounting.has(id)
);

return { relatedRequirements, selfCheckRequirements, automaticallyFulfilledRequirements };
}

0 comments on commit cfcacf6

Please sign in to comment.