diff --git a/CHANGELOG.md b/CHANGELOG.md index 001375d6b..08d6ac6c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ - Fixed course node dimensions and centering in Generate graph to match main graph - Fixed the positions of the Theory of Computing and Artificial Intelligence labels - Fixed MAT237 prerequisite parsing by enabling `lParen` and `rParen` in `WebParsing.ReqParser` to handle square brackets +- Fix hybrid node text parsing not properly accounting for all types of logical prerequisite strings ### 🔧 Internal changes diff --git a/js/components/graph/Graph.js b/js/components/graph/Graph.js index e3e48e9ad..2b8cc82bc 100644 --- a/js/components/graph/Graph.js +++ b/js/components/graph/Graph.js @@ -945,17 +945,29 @@ export class Graph extends React.Component { */ updateNode = (nodeId, recursive) => { let newState - if (this.arePrereqsSatisfiedNode(nodeId)) { - if (this.isSelected(nodeId) || this.state.hybridsJSON[nodeId]) { + if (this.state.hybridsJSON[nodeId]) { + // For a hybrid node, set the state to 'active' or 'inactive' depending on if its text is satisfied + // as a prerequisite string + if (this.arePrereqsSatisfiedHybrid(nodeId)) { newState = "active" } else { - newState = "takeable" + newState = "inactive" } } else { - if (this.isSelected(nodeId) && !this.state.hybridsJSON[nodeId]) { - newState = "overridden" + // For a regular course node, set the state to 'active', 'takeable', 'inactive', or 'overridden' + // depending on whether the user has selected the course and whether the course's prereqs are met + if (this.arePrereqsSatisfiedNode(nodeId)) { + if (this.isSelected(nodeId)) { + newState = "active" + } else { + newState = "takeable" + } } else { - newState = "inactive" + if (this.isSelected(nodeId)) { + newState = "overridden" + } else { + newState = "inactive" + } } } @@ -1278,6 +1290,45 @@ export class Graph extends React.Component { return parents.every(isAllTrue) } + /** + * Checks whether a hybrid node's prereq string is satisfied + * @return {boolean} + */ + arePrereqsSatisfiedHybrid = nodeId => { + // Concatenate prereq string + let hybridNode = this.state.hybridsJSON[nodeId] + let hybridText = hybridNode.text.map(textTag => textTag.text).join("") + + // Parse prereq string into a nested list alternating between AND and OR conditions + let prereqList = parseAnd(hybridText) + + // Recursively check if each prerequisite condition is satisfied within the selected courses + let nodesList = Object.values(this.state.nodesJSON) + const isSelectedCourse = course => { + let prereqNode = findRelationship(course, nodesList) + if (prereqNode !== undefined) { + return this.isSelectedNode(prereqNode.id_) + } else { + return false + } + } + const andSatisfied = andList => { + if (typeof andList === "string") { + return isSelectedCourse(andList) + } else { + return andList.every(orSatisfied) + } + } + const orSatisfied = orList => { + if (typeof orList === "string") { + return isSelectedCourse(orList) + } else { + return orList.some(andSatisfied) + } + } + return andSatisfied(prereqList) + } + /** * Renders a group of Bools * @param {JSON} boolsJSON @@ -1761,19 +1812,21 @@ export { ZOOM_INCREMENT, KEYBOARD_PANNING_INCREMENT } */ export function populateHybridRelatives(hybridNode, nodesJSON, parents, childrenObj) { // parse prereqs based on text - let hybridText = "" - hybridNode.text.forEach(textTag => (hybridText += textTag.text)) + let hybridText = hybridNode.text.map(textTag => textTag.text).join("") const nodeParents = [] - // First search for entire string (see Stats graph) + // First search for a node that matches the entire string (see Stats graph) let prereqNode = findRelationship(hybridText, nodesJSON) if (prereqNode !== undefined) { nodeParents.push(prereqNode.id_) childrenObj[prereqNode.id_].push(hybridNode.id_) - } else { - // Parse text first - const prereqs = parseAnd(hybridText)[0] - prereqs.forEach(course => { - if (typeof course === "string") { + } + // Otherwise, parse the hybrid node's text as a prerequisite string of multiple courses, + // and add a parent-child connection for each involved course + else { + let prereqs = parseAnd(hybridText) + if (typeof prereqs === "object") { + prereqs = prereqs.flat(Infinity) + prereqs.forEach(course => { prereqNode = findRelationship(course, nodesJSON) if (prereqNode !== undefined) { nodeParents.push(prereqNode.id_) @@ -1781,22 +1834,10 @@ export function populateHybridRelatives(hybridNode, nodesJSON, parents, children } else { console.error("Could not find prereq for ", hybridText) } - } else if (typeof course === "object") { - const orPrereq = [] - course.forEach(c => { - const prereqNode = findRelationship(c, nodesJSON) - if (prereqNode !== undefined) { - orPrereq.push(prereqNode.id_) - childrenObj[prereqNode.id_].push(hybridNode.id_) - } else { - console.error("Could not find prereq for ", hybridText) - } - }) - if (orPrereq.length > 0) { - nodeParents.push(orPrereq) - } - } - }) + }) + } else { + console.error("Could not find prereq for ", hybridText) + } } parents[hybridNode.id_] = nodeParents } diff --git a/js/components/graph/__tests__/populateHybridRelatives.test.js b/js/components/graph/__tests__/populateHybridRelatives.test.js index f218f0598..29d7cfbfb 100644 --- a/js/components/graph/__tests__/populateHybridRelatives.test.js +++ b/js/components/graph/__tests__/populateHybridRelatives.test.js @@ -147,7 +147,7 @@ describe("populateHybridRelatives", () => { id_: "mat135136137157calc1", text: [ { - text: "MAT(135,136)/137/157", + text: "(MAT135,136)/MAT137/157", }, { text: "Calc1", @@ -175,7 +175,7 @@ describe("populateHybridRelatives", () => { csc111: [], csc165: [], mat135136137157calc1: [], - h62: [["csc111", "csc165", "mat135136137157calc1"]], + h62: ["csc111", "csc165", "mat135136137157calc1"], } const expectedChildren = { csc111: ["h62"], @@ -193,7 +193,7 @@ describe("populateHybridRelatives", () => { csc111: [], csc165: [], mat135136137157calc1: [], - h62: [["csc111", "csc165"]], + h62: ["csc111", "csc165"], } const expectedChildren = { csc111: ["h62"], diff --git a/js/util/parse.test.js b/js/util/parse.test.js index ce6114624..c9ec39e6a 100644 --- a/js/util/parse.test.js +++ b/js/util/parse.test.js @@ -1,125 +1,159 @@ // Tests for parsing algorithms from js/util/util.js -import { parseAnd, parseOr, parseCourse } from "./util" +import { parseAnd, parseOr, splitPrereqString, removeOuterParens } from "./util" describe("parseAnd", () => { test("parseAnd correctly parses courses when a comma separates two couress", () => { const input = "CSC111, STA247" const actual = parseAnd(input) - const expected = [["CSC111", "STA247"], ""] + const expected = ["CSC111", "STA247"] + expect(actual).toEqual(expected) + }) + test("parseAnd correctly accounts for shorthand course code expansion", () => { + const input = "CSC110,111" + const actual = parseAnd(input) + const expected = ["CSC110", "CSC111"] + expect(actual).toEqual(expected) + }) + test("parseAnd correctly filters out grade requirements", () => { + const input = "CSC110 (70%),111 (77%)" + const actual = parseAnd(input) + const expected = ["CSC110", "CSC111"] + expect(actual).toEqual(expected) + }) + test("parseAnd correctly returns parsed course when a string of a single course is wrapped in parentheses", () => { + const input = "(CSC110)" + const actual = parseAnd(input) + const expected = "CSC110" + expect(actual).toEqual(expected) + }) + test("parseAnd correctly returns parsed course when the string only contains one course", () => { + const input = "CSC110" + const actual = parseAnd(input) + const expected = "CSC110" expect(actual).toEqual(expected) }) test("parseAnd correctly parses courses separated by both comma and slash", () => { const input = "CSC111, MAT135/136/137" const actual = parseAnd(input) - const expected = [["CSC111", ["MAT135", "MAT136", "MAT137"]], ""] + const expected = ["CSC111", ["MAT135", "MAT136", "MAT137"]] expect(actual).toEqual(expected) }) - - test("parseAnd correctly parses courses separated by both ; and slash", () => { + test("parseAnd correctly parses courses separated by ; and slash together", () => { const input = "CSC111/; MAT135/136/137" const actual = parseAnd(input) - const expected = [["CSC111", ["MAT135", "MAT136", "MAT137"]], ""] + const expected = [["CSC111"], ["MAT135", "MAT136", "MAT137"]] expect(actual).toEqual(expected) }) test("parseAnd correctly parses courses separated by multiple commas and slash and ;", () => { const input = "CSC111, STA247, Calc1/; MAT135/136, CSC145/CSC165/; CSC108/199" const actual = parseAnd(input) const expected = [ - [ "CSC111", "STA247", - "CALC1", + ["Calc1"], ["MAT135", "MAT136"], ["CSC145", "CSC165"], ["CSC108", "CSC199"], - ], - "", ] expect(actual).toEqual(expected) }) }) describe("parseOr", () => { - test("parseOr correctly calls returns all parsed courses separated by /", () => { + test("parseOr correctly parses courses separated by /", () => { + const input = "CSC111/CSC165/MAT149" + const actual = parseOr(input) + const expected = ["CSC111", "CSC165", "MAT149"] + expect(actual).toEqual(expected) + }) + test("parseOr correctly accounts for shorthand course code expansion", () => { const input = "CSC111/207/209/258" const actual = parseOr(input) - const expected = [["CSC111", "CSC207", "CSC209", "CSC258"], ""] + const expected = ["CSC111", "CSC207", "CSC209", "CSC258"] + expect(actual).toEqual(expected) + }) + test("parseOr correctly filters out grade requirements", () => { + const input = "MAT137(73%) / MAT157(67%)" + const actual = parseOr(input) + const expected = ["MAT137", "MAT157"] expect(actual).toEqual(expected) }) test("parseOr correctly returns parsed course when a string of a single course is wrapped in parentheses", () => { const input = "(CSC207)" const actual = parseOr(input) - const expected = ["CSC207)", ""] + const expected = "CSC207" expect(actual).toEqual(expected) }) test("parseOr correctly returns parsed course when the string only contains one course", () => { const input = "CSC207" const actual = parseOr(input) - const expected = ["CSC207", ""] + const expected = "CSC207" expect(actual).toEqual(expected) }) - test("parseOr correctly returns parsed course when a comma separates two courses and breaks after parsing the first course", () => { + test("parseOr correctly returns a nested conjunction when only a comma split is to be made", () => { const input = "CSC207,209" const actual = parseOr(input) - const expected = ["CSC207", ",209"] + const expected = [["CSC207", "CSC209"]] expect(actual).toEqual(expected) }) test("parseOr correctly returns parsed course when there is empty space between some of the courses", () => { - const input = "csc311/ Calc1/ 301" + const input = "CSC311/ 301/ Calc1" const actual = parseOr(input) - const expected = [["CSC311", "CALC1", "CSC301"], ""] + const expected = ["CSC311", "CSC301", "Calc1"] expect(actual).toEqual(expected) }) - test("parseOr correctly returns parsed course when the last two courses are separated by , or ;", () => { - const input1 = "csc301/317/,Calc1" - const actual1 = parseOr(input1) - const expected1 = [["CSC301", "CSC317"], ",Calc1"] - expect(actual1).toEqual(expected1) - - const input2 = "csc301/317/;Calc1" - const actual2 = parseOr(input2) - const expected2 = [["CSC301", "CSC317"], ";Calc1"] - expect(actual2).toEqual(expected2) - }) }) -describe("parseCourse", () => { - test("parseCourse correctly returns an array with a string starting with a prefix", () => { - const input = "CSC111/207/209/258" - const actual = parseCourse(input, "CSC") - const expected = ["CSC111", "/207/209/258"] +describe("removeOuterParens", () => { + test("removeOuterParens correctly strips a set of enclosing parentheses around a string", () => { + const input = "(CSC111/207/209/258)" + const actual = removeOuterParens(input) + const expected = "CSC111/207/209/258" expect(actual).toEqual(expected) }) - test("parseCourse correctly returns an array with a string starting without a prefix", () => { - const input = "207/209/258" - const actual = parseCourse(input, "CSC") - const expected = ["CSC207", "/209/258"] + test("removeOuterParens does not strip extra nested parentheses inside a string", () => { + const input = "(CSC111/207/209/258, (MAT149/159), CSC300)" + const actual = removeOuterParens(input) + const expected = "CSC111/207/209/258, (MAT149/159), CSC300" expect(actual).toEqual(expected) }) - test("parseCourse correctly returns an array with a string containing one course number", () => { - const input = "207" - const actual = parseCourse(input, "CSC") - const expected = ["CSC207", ""] + test("removeOuterParens does not strip parentheses enclosing only part of a string", () => { + const input = "(MAT235, MAT236)/MAT237/MAT257" + const actual = removeOuterParens(input) + const expected = "(MAT235, MAT236)/MAT237/MAT257" expect(actual).toEqual(expected) }) - - test("parseCourse correctly returns an array with a string containing one course", () => { - const input = "CSC209" - const actual = parseCourse(input, "CSC") - const expected = ["CSC209", ""] + test("removeOuterParens does not strip disjoint sets of parentheses enclosing a string", () => { + const input = "(MAT235, MAT236)/(MAT237/MAT257)" + const actual = removeOuterParens(input) + const expected = "(MAT235, MAT236)/(MAT237/MAT257)" expect(actual).toEqual(expected) }) +}) - test("parseCourse correctly returns an array with a string with comma as separator", () => { - const input = "CSC207,209,236" - const actual = parseCourse(input, "CSC") - const expected = ["CSC207", ",209,236"] +describe("splitPrereqString", () => { + test("splitPrereqString correctly splits courses by a separator", () => { + const input = "CSC110,CSC111" + const actual = splitPrereqString(input, ",") + const expected = ["CSC110", "CSC111"] + expect(actual).toEqual(expected) + }) + test("splitPrereqString correctly filters out spaces when performing a split", () => { + const input = "CSC110/ CSC111/ CSC207" + const actual = splitPrereqString(input, "/") + const expected = ["CSC110", "CSC111", "CSC207"] + expect(actual).toEqual(expected) + }) + test("splitPrereqString correctly filters out enclosing parentheses when performing a split", () => { + const input = "CSC111/207/209/258, (MAT149/159), CSC300" + const actual = splitPrereqString(input, ",") + const expected = ["CSC111/207/209/258", "MAT149/159", "CSC300"] expect(actual).toEqual(expected) }) - test("parseCourse returns an array containing two empty strings when the input s is empty", () => { - const input = "" - const actual = parseCourse(input, "CSC") - const expected = ["", ""] + test("splitPrereqString does not perform a split inside a parenthesis layer", () => { + const input = "(MAT235, MAT236)/MAT237/MAT257, CSC111" + const actual = splitPrereqString(input, ",") + const expected = ["(MAT235,MAT236)/MAT237/MAT257", "CSC111"] expect(actual).toEqual(expected) }) }) diff --git a/js/util/util.js b/js/util/util.js index e3e905f49..be6500816 100644 --- a/js/util/util.js +++ b/js/util/util.js @@ -1,99 +1,138 @@ -/** Helper function for parsing hybrid node's text. - * - * @param {string} s a combination of course codes - * @returns {Array} an array containing parsed course codes +/** + * Parse a logical prerequisite string as a conjunction of disjunctions. + * @param {string} s the prerequisite string + * @returns a nested list of courses as an AND of ORs, or the course itself if no splitting is made */ export function parseAnd(s) { - "use strict" - let curr = s - const andList = [] - while (curr.length > 0) { - if (curr.charAt(0) === "," || curr.charAt(0) === ";" || curr.charAt(0) === " ") { - curr = curr.substr(1) - } else { - const result = parseOr(curr) - if (curr === result[1]) { - console.error("Parsing failed for " + s + " with curr = " + curr) - break - } else { - curr = result[1] - andList.push(result[0]) - } + // Base case: return the course if no splitting is to be made. + if (!s.includes(",") && !s.includes(";") && !s.includes("/")) { + return removeOuterParens(s) + } + // Otherwise, recurse and parse each conjunctive as a disjunction. + const andList = splitPrereqString(removeOuterParens(s.replaceAll(";", ",")), ",") + let splitList = [] + for (const str of andList) { + if (str.length > 0) { + splitList.push(parseOr(str)) } } - return [andList, curr] + + // Modify the returned list to account for shorthand course codes and remove grade requirements + parseSplitList(splitList) + return splitList } /** - * Helper function for parsing hybrid node's text. - * Calls parseCourse to parse courses separated by '/'. - * If the resulting parsed list only has one course, return the course - * itself as the first element as the return array. - * If there is a course separated by ',', stop and return the remaining - * courses as the second element in the return array. - * - * @param {string} s a combination of course codes - * @returns {Array} an array containing parsed course codes + * Parse a logical prerequisite string as a disjunction of conjunctions. + * @param {string} s the prerequisite string + * @returns a nested list of courses as an OR of ANDs, or the course itself if no splitting is made */ export function parseOr(s) { - "use strict" - let curr = s - let orList = [] - let tmp - let result - let coursePrefix - while (curr.length > 0 && curr.charAt(0) !== "," && curr.charAt(0) !== ";") { - if (curr.charAt(0) === "(") { - tmp = curr.substr(1, curr.indexOf(")")) - if (coursePrefix === undefined && tmp.length >= 6) { - coursePrefix = tmp.substr(0, 3).toUpperCase() - } - - result = parseCourse(tmp, coursePrefix) - orList.push(result[0]) - curr = curr.substr(curr.indexOf(")") + 1) - } else if (curr.charAt(0) === " " || curr.charAt(0) === "/") { - curr = curr.substr(1) - } else { - if (coursePrefix === undefined && curr.length >= 6) { - coursePrefix = curr.substr(0, 3).toUpperCase() - } - result = parseCourse(curr, coursePrefix) - if (curr === result[1]) { - console.error("Parsing failed for " + s + " with curr = " + curr) - break - } - curr = result[1] - orList.push(result[0]) + // Base case: return the course if no splitting is to be made. + if (!s.includes(",") && !s.includes(";") && !s.includes("/")) { + return removeOuterParens(s) + } + // Otherwise, recurse and parse each conjunctive as a disjunction. + const orList = splitPrereqString(removeOuterParens(s), "/") + let splitList = [] + for (const str of orList) { + if (str.length > 0) { + splitList.push(parseAnd(str)) } } - // If only one course was parsed, return that course. - if (orList.length === 1) { - orList = orList[0] + + // Modify the returned list to account for shorthand course codes and remove grade requirements + parseSplitList(splitList) + return splitList +} + +/** + * Helper function to split a prerequisite string by its 'and' or 'or' separator, and + * strip the result of top-level outer parentheses and spaces. + * @param {string} s the prerequisite string + * @param {string} separator the separator to split by (',' for and, '/' for or) + * @returns the resulting list of conjunctives/disjunctives + */ +export function splitPrereqString(s, separator) { + let splitList = [] + let currIndex = 0 + let curr = "" + let parenLayer = 0 // Depth of nested parentheses + while (currIndex < s.length) { + // If a parenthesis is encountered, update parenLayer + if (s.charAt(currIndex) === "(") { + parenLayer += 1 + } else if (s.charAt(currIndex) === ")") { + parenLayer -= 1 + } + + if (s.charAt(currIndex) === separator && parenLayer === 0) { + // If the separator is encountered and we aren't inside parentheses, split on it and reset curr + splitList.push(removeOuterParens(curr)) + curr = "" + } else if (s.charAt(currIndex) !== " ") { + // Add all other non-space characters to curr + curr += s.charAt(currIndex) + } + currIndex += 1 } + splitList.push(removeOuterParens(curr)) - return [orList, curr] + return splitList } /** - * Helper function for parsing hybrid node's text - * - * @param {string} s a combination of courses - * @param {string} prefix prefix of the current course - * @returns {Array} an array containing parsed courses + * Helper function to strip a string entirely contained within a pair of parentheses. */ -export function parseCourse(s, prefix) { - "use strict" +export function removeOuterParens(s) { + if (s.length < 2 || s.charAt(0) !== "(" || s.charAt(s.length - 1) !== ")") { + return s + } - const start = s.search(/[,/]/) - if (start === 3) { - return [prefix + s.substr(0, start), s.substr(start)] - } else if (start > 0) { - return [s.substr(0, start).toUpperCase(), s.substr(start)] + let parenLayer = 1 // Depth of nested parentheses + // Iterate through the string outside its opening '(' and closing ')'. + // If we reach a nest depth of 0 prior to the end of the string, it isn't contained in parentheses. + for (let i = 1; i <= s.length - 2; i++) { + if (s.charAt(i) === "(") { + parenLayer += 1 + } + if (s.charAt(i) === ")") { + parenLayer -= 1 + } + if (parenLayer === 0) { + return s + } } - if (s.length === 3) { - return [prefix + s, ""] + return s.substr(1, s.length - 2) +} + +/** + * Helper function to expand shorthand course codes (e.g. "MAT237/257") in-place from a list of course + * strings, and remove any grade requirement strings (e.g. "MAT137 (73%)") + * @param splitList the nested array to modify, representing a partially parsed prerequisite string + * @returns {void} + */ +export function parseSplitList(splitList) { + let currPrefix = "" + for (let i = 0; i < splitList.length; i++) { + if (typeof splitList[i] === "object") { + currPrefix = "" + } else if (typeof splitList[i] === "string") { + // Filter out a grade requirement from the current course string + let matchResult = splitList[i].match(/^(.+)\(.*%\)$/) + if (matchResult !== null) { + splitList[i] = matchResult[1] + } + + // Update currPrefix if the current course string contains a prefix + if (splitList[i].match(/^[A-Z]{3}/g)) { + currPrefix = splitList[i].substr(0, 3) + } + // Append currPrefix if the current course string is missing a prefix + else if (splitList[i].match(/^[0-9]{3}$/g)) { + splitList[i] = currPrefix + splitList[i] + } + } } - return [s, ""] }