From 2e8fc26fa603628e8a21773a98308a5319cf1ab1 Mon Sep 17 00:00:00 2001 From: Liz Date: Sat, 11 Oct 2025 14:54:17 -0400 Subject: [PATCH 01/10] refactored MarksPanel child components into their own file, converted Checkbox and Rubric Criterion Input into hook-based function components --- .../Result/checkbox_criterion_input.jsx | 97 +++++ .../Result/flexible_criterion_input.jsx | 208 +++++++++ .../Components/Result/marks_panel.jsx | 412 +----------------- .../Result/rubric_criterion_input.jsx | 103 +++++ 4 files changed, 411 insertions(+), 409 deletions(-) create mode 100644 app/javascript/Components/Result/checkbox_criterion_input.jsx create mode 100644 app/javascript/Components/Result/flexible_criterion_input.jsx create mode 100644 app/javascript/Components/Result/rubric_criterion_input.jsx diff --git a/app/javascript/Components/Result/checkbox_criterion_input.jsx b/app/javascript/Components/Result/checkbox_criterion_input.jsx new file mode 100644 index 0000000000..94da4edfe5 --- /dev/null +++ b/app/javascript/Components/Result/checkbox_criterion_input.jsx @@ -0,0 +1,97 @@ +import React from "react"; +import PropTypes from "prop-types"; + +import safe_marked from "../../common/safe_marked"; + +export default function CheckboxCriterionInput({ + description, + destroyMark, + expanded, + id, + mark, + max_mark, + oldMark, + released_to_students, + toggleExpanded, + unassigned, + updateMark, + name, + bonus, +}) { + const unassignedClass = unassigned ? "unassigned" : ""; + const expandedClass = expanded ? "expanded" : "collapsed"; + + return ( +
  • +
    +
    +
    + {name} + {bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} + {!released_to_students && !unassigned && mark !== null && ( + destroyMark(e, id)} style={{float: "right"}}> + {I18n.t("helpers.submit.delete", { + model: I18n.t("activerecord.models.mark.one"), + })} + + )} +
    +
    + {!released_to_students && ( + + + + + )} + + {mark === null ? "-" : mark} +  /  + {max_mark} + +
    + {oldMark !== undefined && oldMark.mark !== undefined && ( +
    {`(${I18n.t("results.remark.old_mark")}: ${ + oldMark.mark + })`}
    + )} +
    +
    +
    • + ); +} + +CheckboxCriterionInput.propTypes = { + description: PropTypes.string.isRequired, + destroyMark: PropTypes.func.isRequired, + expanded: PropTypes.bool.isRequired, + id: PropTypes.number.isRequired, + mark: PropTypes.number, + max_mark: PropTypes.number.isRequired, + oldMark: PropTypes.object, + released_to_students: PropTypes.bool.isRequired, + toggleExpanded: PropTypes.func.isRequired, + unassigned: PropTypes.bool.isRequired, + updateMark: PropTypes.func.isRequired, +}; diff --git a/app/javascript/Components/Result/flexible_criterion_input.jsx b/app/javascript/Components/Result/flexible_criterion_input.jsx new file mode 100644 index 0000000000..fa5025fe06 --- /dev/null +++ b/app/javascript/Components/Result/flexible_criterion_input.jsx @@ -0,0 +1,208 @@ +import React from "react"; +import PropTypes from "prop-types"; + +import safe_marked from "../../common/safe_marked"; + +export class FlexibleCriterionInput extends React.Component { + constructor(props) { + super(props); + this.state = { + rawText: this.props.mark === null ? "" : String(this.props.mark), + invalid: false, + }; + this.typing_timer = undefined; + } + + listDeductions = () => { + let label = I18n.t("annotations.list_deductions"); + let deductiveAnnotations = this.props.annotations.filter(a => { + return !!a.deduction && a.criterion_id === this.props.id && !a.is_remark; + }); + + if (deductiveAnnotations.length === 0) { + return ""; + } + + let hyperlinkedDeductions = deductiveAnnotations.map((a, index) => { + let full_path = a.path ? a.path + "/" + a.filename : a.filename; + return ( + + + this.props.findDeductiveAnnotation( + full_path, + a.submission_file_id, + a.line_start, + a.id + ) + } + href="#" + className={"red-text"} + > + {"-" + a.deduction} + + {index !== deductiveAnnotations.length - 1 ? ", " : ""} + + ); + }); + + if (this.props.override) { + label = "(" + I18n.t("results.overridden_deductions") + ") " + label; + } + + return ( +
    + {label} + {hyperlinkedDeductions} +
    + ); + }; + + deleteManualMarkLink = () => { + if (!this.props.released_to_students && !this.props.unassigned) { + if ( + this.props.annotations.some(a => !!a.deduction && a.criterion_id === this.props.id) && + this.props.override + ) { + return ( + this.props.revertToAutomaticDeductions(this.props.id)} + style={{float: "right"}} + > + {I18n.t("results.cancel_override")} + + ); + } else if (this.props.mark !== null && this.props.override) { + return ( + this.props.destroyMark(e, this.props.id)} + style={{float: "right"}} + > + {I18n.t("helpers.submit.delete", { + model: I18n.t("activerecord.models.mark.one"), + })} + + ); + } + } + return ""; + }; + + renderOldMark = () => { + if (this.props.oldMark === undefined || this.props.oldMark.mark === undefined) { + return null; + } + let label = String(this.props.oldMark.mark); + + if (this.props.oldMark.override) { + label = `(${I18n.t("results.overridden_deductions")}) ${label}`; + } + + return
    {`(${I18n.t("results.remark.old_mark")}: ${label})`}
    ; + }; + + handleChange = event => { + if (this.typing_timer) { + clearTimeout(this.typing_timer); + } + + const mark = parseFloat(event.target.value); + if (event.target.value !== "" && isNaN(mark)) { + this.setState({rawText: event.target.value, invalid: true}); + } else if (mark === this.props.mark) { + // This can happen if the user types a decimal point at the end of the input. + this.setState({rawText: event.target.value, invalid: false}); + } else if (mark > this.props.max_mark) { + this.setState({rawText: event.target.value, invalid: true}); + } else { + this.setState({rawText: event.target.value, invalid: false}); + + this.typing_timer = setTimeout(() => { + this.props.updateMark(this.props.id, isNaN(mark) ? null : mark); + }, 300); + } + }; + + componentDidUpdate(oldProps) { + if (oldProps.mark !== this.props.mark) { + this.setState({ + rawText: this.props.mark === null ? "" : String(this.props.mark), + invalid: false, + }); + } + } + + render() { + const unassignedClass = this.props.unassigned ? "unassigned" : ""; + const expandedClass = this.props.expanded ? "expanded" : "collapsed"; + + let markElement; + if (this.props.released_to_students) { + // Student view + markElement = this.props.mark; + } else { + markElement = ( + + ); + } + + return ( +
  • +
    +
    +
    + {this.props.name} + {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} + {this.deleteManualMarkLink()} +
    +
    + + {markElement} +  /  + {this.props.max_mark} + + {this.listDeductions()} + {this.renderOldMark()} +
    +
    • + ); + } +} + +FlexibleCriterionInput.propTypes = { + annotations: PropTypes.arrayOf(PropTypes.object).isRequired, + bonus: PropTypes.bool, + description: PropTypes.string.isRequired, + destroyMark: PropTypes.func.isRequired, + expanded: PropTypes.bool.isRequired, + findDeductiveAnnotation: PropTypes.func.isRequired, + id: PropTypes.number.isRequired, + mark: PropTypes.number, + max_mark: PropTypes.number.isRequired, + oldMark: PropTypes.object, + override: PropTypes.bool, + released_to_students: PropTypes.bool.isRequired, + revertToAutomaticDeductions: PropTypes.func.isRequired, + toggleExpanded: PropTypes.func.isRequired, + unassigned: PropTypes.bool.isRequired, + updateMark: PropTypes.func.isRequired, +}; diff --git a/app/javascript/Components/Result/marks_panel.jsx b/app/javascript/Components/Result/marks_panel.jsx index b483f973e0..e5e9ebe002 100644 --- a/app/javascript/Components/Result/marks_panel.jsx +++ b/app/javascript/Components/Result/marks_panel.jsx @@ -1,7 +1,8 @@ import React from "react"; -import PropTypes from "prop-types"; -import safe_marked from "../../common/safe_marked"; +import CheckboxCriterionInput from "./checkbox_criterion_input"; +import {FlexibleCriterionInput} from "./flexible_criterion_input"; +import RubricCriterionInput from "./rubric_criterion_input"; export class MarksPanel extends React.Component { static defaultProps = { @@ -141,410 +142,3 @@ export class MarksPanel extends React.Component { ); } } - -export class CheckboxCriterionInput extends React.Component { - constructor(props) { - super(props); - } - - render() { - const unassignedClass = this.props.unassigned ? "unassigned" : ""; - const expandedClass = this.props.expanded ? "expanded" : "collapsed"; - return ( -
  • -
    -
    -
    - {this.props.name} - {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} - {!this.props.released_to_students && - !this.props.unassigned && - this.props.mark !== null && ( - this.props.destroyMark(e, this.props.id)} - style={{float: "right"}} - > - {I18n.t("helpers.submit.delete", { - model: I18n.t("activerecord.models.mark.one"), - })} - - )} -
    -
    - {!this.props.released_to_students && ( - - - - - )} - - {this.props.mark === null ? "-" : this.props.mark} -  /  - {this.props.max_mark} - -
    - {this.props.oldMark !== undefined && this.props.oldMark.mark !== undefined && ( -
    {`(${I18n.t("results.remark.old_mark")}: ${ - this.props.oldMark.mark - })`}
    - )} -
    -
    -
    • - ); - } -} - -CheckboxCriterionInput.propTypes = { - description: PropTypes.string.isRequired, - destroyMark: PropTypes.func.isRequired, - expanded: PropTypes.bool.isRequired, - id: PropTypes.number.isRequired, - mark: PropTypes.number, - max_mark: PropTypes.number.isRequired, - oldMark: PropTypes.object, - released_to_students: PropTypes.bool.isRequired, - toggleExpanded: PropTypes.func.isRequired, - unassigned: PropTypes.bool.isRequired, - updateMark: PropTypes.func.isRequired, -}; - -export class FlexibleCriterionInput extends React.Component { - constructor(props) { - super(props); - this.state = { - rawText: this.props.mark === null ? "" : String(this.props.mark), - invalid: false, - }; - this.typing_timer = undefined; - } - - listDeductions = () => { - let label = I18n.t("annotations.list_deductions"); - let deductiveAnnotations = this.props.annotations.filter(a => { - return !!a.deduction && a.criterion_id === this.props.id && !a.is_remark; - }); - - if (deductiveAnnotations.length === 0) { - return ""; - } - - let hyperlinkedDeductions = deductiveAnnotations.map((a, index) => { - let full_path = a.path ? a.path + "/" + a.filename : a.filename; - return ( - - - this.props.findDeductiveAnnotation( - full_path, - a.submission_file_id, - a.line_start, - a.id - ) - } - href="#" - className={"red-text"} - > - {"-" + a.deduction} - - {index !== deductiveAnnotations.length - 1 ? ", " : ""} - - ); - }); - - if (this.props.override) { - label = "(" + I18n.t("results.overridden_deductions") + ") " + label; - } - - return ( -
    - {label} - {hyperlinkedDeductions} -
    - ); - }; - - deleteManualMarkLink = () => { - if (!this.props.released_to_students && !this.props.unassigned) { - if ( - this.props.annotations.some(a => !!a.deduction && a.criterion_id === this.props.id) && - this.props.override - ) { - return ( - this.props.revertToAutomaticDeductions(this.props.id)} - style={{float: "right"}} - > - {I18n.t("results.cancel_override")} - - ); - } else if (this.props.mark !== null && this.props.override) { - return ( - this.props.destroyMark(e, this.props.id)} - style={{float: "right"}} - > - {I18n.t("helpers.submit.delete", { - model: I18n.t("activerecord.models.mark.one"), - })} - - ); - } - } - return ""; - }; - - renderOldMark = () => { - if (this.props.oldMark === undefined || this.props.oldMark.mark === undefined) { - return null; - } - let label = String(this.props.oldMark.mark); - - if (this.props.oldMark.override) { - label = `(${I18n.t("results.overridden_deductions")}) ${label}`; - } - - return
    {`(${I18n.t("results.remark.old_mark")}: ${label})`}
    ; - }; - - handleChange = event => { - if (this.typing_timer) { - clearTimeout(this.typing_timer); - } - - const mark = parseFloat(event.target.value); - if (event.target.value !== "" && isNaN(mark)) { - this.setState({rawText: event.target.value, invalid: true}); - } else if (mark === this.props.mark) { - // This can happen if the user types a decimal point at the end of the input. - this.setState({rawText: event.target.value, invalid: false}); - } else if (mark > this.props.max_mark) { - this.setState({rawText: event.target.value, invalid: true}); - } else { - this.setState({rawText: event.target.value, invalid: false}); - - this.typing_timer = setTimeout(() => { - this.props.updateMark(this.props.id, isNaN(mark) ? null : mark); - }, 300); - } - }; - - componentDidUpdate(oldProps) { - if (oldProps.mark !== this.props.mark) { - this.setState({ - rawText: this.props.mark === null ? "" : String(this.props.mark), - invalid: false, - }); - } - } - - render() { - const unassignedClass = this.props.unassigned ? "unassigned" : ""; - const expandedClass = this.props.expanded ? "expanded" : "collapsed"; - - let markElement; - if (this.props.released_to_students) { - // Student view - markElement = this.props.mark; - } else { - markElement = ( - - ); - } - - return ( -
  • -
    -
    -
    - {this.props.name} - {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} - {this.deleteManualMarkLink()} -
    -
    - - {markElement} -  /  - {this.props.max_mark} - - {this.listDeductions()} - {this.renderOldMark()} -
    -
    • - ); - } -} - -FlexibleCriterionInput.propTypes = { - annotations: PropTypes.arrayOf(PropTypes.object).isRequired, - bonus: PropTypes.bool, - description: PropTypes.string.isRequired, - destroyMark: PropTypes.func.isRequired, - expanded: PropTypes.bool.isRequired, - findDeductiveAnnotation: PropTypes.func.isRequired, - id: PropTypes.number.isRequired, - mark: PropTypes.number, - max_mark: PropTypes.number.isRequired, - oldMark: PropTypes.object, - override: PropTypes.bool, - released_to_students: PropTypes.bool.isRequired, - revertToAutomaticDeductions: PropTypes.func.isRequired, - toggleExpanded: PropTypes.func.isRequired, - unassigned: PropTypes.bool.isRequired, - updateMark: PropTypes.func.isRequired, -}; - -export class RubricCriterionInput extends React.Component { - constructor(props) { - super(props); - } - - // The parameter `level` is the level object selected - handleChange = level => { - this.props.updateMark(this.props.id, level.mark); - }; - - // The parameter `level` is the level object selected - renderRubricLevel = level => { - const levelMark = level.mark.toFixed(2); - let selectedClass = ""; - let oldMarkClass = ""; - if ( - this.props.mark !== undefined && - this.props.mark !== null && - levelMark === this.props.mark.toFixed(2) - ) { - selectedClass = "selected"; - } - if ( - this.props.oldMark !== undefined && - this.props.oldMark.mark !== undefined && - levelMark === this.props.oldMark.mark.toFixed(2) - ) { - oldMarkClass = "old-mark"; - } - - return ( - this.handleChange(level)} - key={`${this.props.id}-${levelMark}`} - className={`rubric-level ${selectedClass} ${oldMarkClass}`} - > - - {level.name} - - - - {levelMark} -  /  - {this.props.max_mark} - - - ); - }; - - render() { - const levels = this.props.levels.map(this.renderRubricLevel); - const expandedClass = this.props.expanded ? "expanded" : "collapsed"; - const unassignedClass = this.props.unassigned ? "unassigned" : ""; - return ( -
  • -
    -
    -
    - {this.props.name} - {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} - {!this.props.released_to_students && - !this.props.unassigned && - this.props.mark !== null && ( - this.props.destroyMark(e, this.props.id)} - style={{float: "right"}} - > - {I18n.t("helpers.submit.delete", { - model: I18n.t("activerecord.models.mark.one"), - })} - - )} -
    - - {levels} -
    -
    -
    • - ); - } -} - -RubricCriterionInput.propTypes = { - bonus: PropTypes.bool, - destroyMark: PropTypes.func.isRequired, - expanded: PropTypes.bool.isRequired, - id: PropTypes.number.isRequired, - levels: PropTypes.array, - mark: PropTypes.number, - max_mark: PropTypes.number, - oldMark: PropTypes.object, - released_to_students: PropTypes.bool.isRequired, - toggleExpanded: PropTypes.func.isRequired, - unassigned: PropTypes.bool.isRequired, - updateMark: PropTypes.func.isRequired, -}; diff --git a/app/javascript/Components/Result/rubric_criterion_input.jsx b/app/javascript/Components/Result/rubric_criterion_input.jsx new file mode 100644 index 0000000000..f27fec68c9 --- /dev/null +++ b/app/javascript/Components/Result/rubric_criterion_input.jsx @@ -0,0 +1,103 @@ +import React from "react"; +import PropTypes from "prop-types"; + +import safe_marked from "../../common/safe_marked"; +export default function RubricCriterionInput({ + bonus, + destroyMark, + expanded, + id, + levels, + mark, + max_mark, + oldMark, + released_to_students, + toggleExpanded, + unassigned, + updateMark, + name, +}) { + // The parameter `level` is the level object selected + const handleChange = level => { + updateMark(id, level.mark); + }; + + // The parameter `level` is the level object selected + const renderRubricLevel = level => { + const levelMark = level.mark.toFixed(2); + let selectedClass = ""; + let oldMarkClass = ""; + if (mark !== undefined && mark !== null && levelMark === mark.toFixed(2)) { + selectedClass = "selected"; + } + if ( + oldMark !== undefined && + oldMark.mark !== undefined && + levelMark === oldMark.mark.toFixed(2) + ) { + oldMarkClass = "old-mark"; + } + + return ( + handleChange(level)} + key={`${id}-${levelMark}`} + className={`rubric-level ${selectedClass} ${oldMarkClass}`} + > + + {level.name} + + + + {levelMark} +  /  + {max_mark} + + + ); + }; + + const rubricLevels = levels.map(renderRubricLevel); + const expandedClass = expanded ? "expanded" : "collapsed"; + const unassignedClass = unassigned ? "unassigned" : ""; + + return ( +
  • +
    +
    +
    + {name} + {bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} + {!released_to_students && !unassigned && mark !== null && ( + destroyMark(e, id)} style={{float: "right"}}> + {I18n.t("helpers.submit.delete", { + model: I18n.t("activerecord.models.mark.one"), + })} + + )} +
    + + {rubricLevels} +
    +
    +
    • + ); +} + +RubricCriterionInput.propTypes = { + bonus: PropTypes.bool, + destroyMark: PropTypes.func.isRequired, + expanded: PropTypes.bool.isRequired, + id: PropTypes.number.isRequired, + levels: PropTypes.array, + mark: PropTypes.number, + max_mark: PropTypes.number, + oldMark: PropTypes.object, + released_to_students: PropTypes.bool.isRequired, + toggleExpanded: PropTypes.func.isRequired, + unassigned: PropTypes.bool.isRequired, + updateMark: PropTypes.func.isRequired, +}; From 2af92c50f2f6c391002b9accc77d184f4b024188 Mon Sep 17 00:00:00 2001 From: Liz Date: Sat, 11 Oct 2025 14:59:55 -0400 Subject: [PATCH 02/10] fixed import of child components in tests for Marks Panel --- .../Components/__tests__/marks_panel.test.jsx | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/javascript/Components/__tests__/marks_panel.test.jsx b/app/javascript/Components/__tests__/marks_panel.test.jsx index 631c9d89db..af6067cd57 100644 --- a/app/javascript/Components/__tests__/marks_panel.test.jsx +++ b/app/javascript/Components/__tests__/marks_panel.test.jsx @@ -1,12 +1,11 @@ import {render, screen} from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import { - MarksPanel, - CheckboxCriterionInput, - FlexibleCriterionInput, - RubricCriterionInput, -} from "../Result/marks_panel"; +import {MarksPanel} from "../Result/marks_panel"; + +import CheckboxCriterionInput from "../Result/checkbox_criterion_input"; +import {FlexibleCriterionInput} from "../Result/flexible_criterion_input"; +import RubricCriterionInput from "../Result/rubric_criterion_input"; const convertToKebabCase = { CheckboxCriterion: "checkbox_criterion", From 10b677a2e770505d176841196c7e47ac202de40a Mon Sep 17 00:00:00 2001 From: Liz Date: Sat, 11 Oct 2025 15:00:20 -0400 Subject: [PATCH 03/10] updated changelog --- Changelog.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Changelog.md b/Changelog.md index 39710ae520..59f2e68f54 100644 --- a/Changelog.md +++ b/Changelog.md @@ -28,6 +28,7 @@ - Fixed Rack deprecation warnings by updating HTTP status code symbols (#7675) - Refactored "Reuse Groups" functionality to use React modal and relocated button to action box row (#7688) - Updated pre-commit `rubocop-rails` version to 2.33.4 (#7691) +- Refactored MarksPanel child components and converted CheckboxCriterionInput and RubricCriterionInput into hook-based function components ## [v2.8.1] From fd7cb6eed1d8ffe2bbf6fc82027f7344c2394cfa Mon Sep 17 00:00:00 2001 From: Liz Date: Sat, 11 Oct 2025 16:12:49 -0400 Subject: [PATCH 04/10] updated marks panel tests to check if child components still properly rendered --- .../Components/__tests__/marks_panel.test.jsx | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/javascript/Components/__tests__/marks_panel.test.jsx b/app/javascript/Components/__tests__/marks_panel.test.jsx index af6067cd57..774d3689ff 100644 --- a/app/javascript/Components/__tests__/marks_panel.test.jsx +++ b/app/javascript/Components/__tests__/marks_panel.test.jsx @@ -152,6 +152,10 @@ describe("CheckboxCriterionInput", () => { expect(screen.queryByText(`(${I18n.t("results.remark.old_mark")}: 1)`)).toBeTruthy(); }); + + it("renders CheckboxCriterionInput", () => { + render(); + }); }); describe("FlexibleCriterionInput", () => { @@ -354,6 +358,10 @@ describe("FlexibleCriterionInput", () => { render(); expect(screen.queryAllByRole("textbox")).toEqual([]); }); + + it("renders FlexibleCriterionInput", () => { + render(); + }); }); describe("RubricCriterionInput", () => { @@ -469,4 +477,8 @@ describe("RubricCriterionInput", () => { expect(screen.queryAllByRole("link")).toEqual([]); }); + + it("renders RubricCriterionInput", () => { + render(); + }); }); From 114bb50667dbfdd448a1a1abaaaf876ee60d5b4d Mon Sep 17 00:00:00 2001 From: Liz Date: Fri, 17 Oct 2025 22:52:27 -0400 Subject: [PATCH 05/10] refactored flexible criterion input to be hook-based function component --- .../Result/flexible_criterion_input.jsx | 280 +++++++++++------- .../Components/Result/marks_panel.jsx | 2 +- .../Components/__tests__/marks_panel.test.jsx | 8 +- 3 files changed, 180 insertions(+), 110 deletions(-) diff --git a/app/javascript/Components/Result/flexible_criterion_input.jsx b/app/javascript/Components/Result/flexible_criterion_input.jsx index fa5025fe06..6efc773412 100644 --- a/app/javascript/Components/Result/flexible_criterion_input.jsx +++ b/app/javascript/Components/Result/flexible_criterion_input.jsx @@ -1,22 +1,35 @@ -import React from "react"; +import React, {useState, useEffect, useRef} from "react"; import PropTypes from "prop-types"; import safe_marked from "../../common/safe_marked"; -export class FlexibleCriterionInput extends React.Component { - constructor(props) { - super(props); - this.state = { - rawText: this.props.mark === null ? "" : String(this.props.mark), - invalid: false, - }; - this.typing_timer = undefined; - } +export default function FlexibleCriterionInput({ + annotations, + bonus, + description, + destroyMark, + expanded, + findDeductiveAnnotation, + id, + mark, + max_mark, + oldMark, + override, + released_to_students, + revertToAutomaticDeductions, + toggleExpanded, + unassigned, + updateMark, + name, +}) { + const [rawText, setRawText] = useState(mark === null ? "" : String(mark)); + const [invalid, setInvalid] = useState(false); + const typing_timer = useRef(undefined); - listDeductions = () => { + const listDeductions = () => { let label = I18n.t("annotations.list_deductions"); - let deductiveAnnotations = this.props.annotations.filter(a => { - return !!a.deduction && a.criterion_id === this.props.id && !a.is_remark; + let deductiveAnnotations = annotations.filter(a => { + return !!a.deduction && a.criterion_id === id && !a.is_remark; }); if (deductiveAnnotations.length === 0) { @@ -29,12 +42,7 @@ export class FlexibleCriterionInput extends React.Component { - this.props.findDeductiveAnnotation( - full_path, - a.submission_file_id, - a.line_start, - a.id - ) + findDeductiveAnnotation(full_path, a.submission_file_id, a.line_start, a.id) } href="#" className={"red-text"} @@ -46,7 +54,7 @@ export class FlexibleCriterionInput extends React.Component { ); }); - if (this.props.override) { + if (override) { label = "(" + I18n.t("results.overridden_deductions") + ") " + label; } @@ -58,29 +66,22 @@ export class FlexibleCriterionInput extends React.Component { ); }; - deleteManualMarkLink = () => { - if (!this.props.released_to_students && !this.props.unassigned) { - if ( - this.props.annotations.some(a => !!a.deduction && a.criterion_id === this.props.id) && - this.props.override - ) { + const deleteManualMarkLink = () => { + if (!released_to_students && !unassigned) { + if (annotations.some(a => !!a.deduction && a.criterion_id === id) && override) { return ( this.props.revertToAutomaticDeductions(this.props.id)} + onClick={_ => revertToAutomaticDeductions(id)} style={{float: "right"}} > {I18n.t("results.cancel_override")} ); - } else if (this.props.mark !== null && this.props.override) { + } else if (mark !== null && override) { return ( - this.props.destroyMark(e, this.props.id)} - style={{float: "right"}} - > + destroyMark(e, id)} style={{float: "right"}}> {I18n.t("helpers.submit.delete", { model: I18n.t("activerecord.models.mark.one"), })} @@ -91,102 +92,169 @@ export class FlexibleCriterionInput extends React.Component { return ""; }; - renderOldMark = () => { - if (this.props.oldMark === undefined || this.props.oldMark.mark === undefined) { + const renderOldMark = () => { + if (oldMark === undefined || oldMark.mark === undefined) { return null; } - let label = String(this.props.oldMark.mark); + let label = String(oldMark.mark); - if (this.props.oldMark.override) { + if (oldMark.override) { label = `(${I18n.t("results.overridden_deductions")}) ${label}`; } return
    {`(${I18n.t("results.remark.old_mark")}: ${label})`}
    ; }; - handleChange = event => { - if (this.typing_timer) { - clearTimeout(this.typing_timer); + const handleChange = event => { + if (typing_timer.current) { + clearTimeout(typing_timer.current); } - const mark = parseFloat(event.target.value); - if (event.target.value !== "" && isNaN(mark)) { - this.setState({rawText: event.target.value, invalid: true}); - } else if (mark === this.props.mark) { + const inputMark = parseFloat(event.target.value); + if (event.target.value !== "" && isNaN(inputMark)) { + setRawText(event.target.value); + setInvalid(true); + } else if (inputMark === mark) { // This can happen if the user types a decimal point at the end of the input. - this.setState({rawText: event.target.value, invalid: false}); - } else if (mark > this.props.max_mark) { - this.setState({rawText: event.target.value, invalid: true}); + setRawText(event.target.value); + setInvalid(false); + } else if (inputMark > max_mark) { + setRawText(event.target.value); + setInvalid(true); } else { - this.setState({rawText: event.target.value, invalid: false}); + setRawText(event.target.value); + setInvalid(false); - this.typing_timer = setTimeout(() => { - this.props.updateMark(this.props.id, isNaN(mark) ? null : mark); + typing_timer.current = setTimeout(() => { + updateMark(id, isNaN(inputMark) ? null : inputMark); }, 300); } }; - componentDidUpdate(oldProps) { - if (oldProps.mark !== this.props.mark) { - this.setState({ - rawText: this.props.mark === null ? "" : String(this.props.mark), - invalid: false, - }); - } - } + useEffect(() => { + setRawText(mark === null ? "" : String(mark)); + setInvalid(false); + }, [mark]); - render() { - const unassignedClass = this.props.unassigned ? "unassigned" : ""; - const expandedClass = this.props.expanded ? "expanded" : "collapsed"; + const unassignedClass = unassigned ? "unassigned" : ""; + const expandedClass = expanded ? "expanded" : "collapsed"; - let markElement; - if (this.props.released_to_students) { - // Student view - markElement = this.props.mark; - } else { - markElement = ( - - ); - } - - return ( -
  • -
    -
    -
    - {this.props.name} - {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} - {this.deleteManualMarkLink()} -
    -
    - - {markElement} -  /  - {this.props.max_mark} - - {this.listDeductions()} - {this.renderOldMark()} -
    -
    • + let markElement; + if (released_to_students) { + // Student view + markElement = mark; + } else { + markElement = ( + ); } + + return ( +
  • +
    +
    +
    + {name} + {bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} + {deleteManualMarkLink()} +
    +
    + + {markElement} +  /  + {max_mark} + + {listDeductions()} + {renderOldMark()} +
    +
    • + ); } +// export class FlexibleCriterionInput extends React.Component { +// constructor(props) { +// super(props); +// this.state = { +// rawText: this.props.mark === null ? "" : String(this.props.mark), +// invalid: false, +// }; +// this.typing_timer = undefined; +// } +// +// +// componentDidUpdate(oldProps) { +// if (oldProps.mark !== this.props.mark) { +// this.setState({ +// rawText: this.props.mark === null ? "" : String(this.props.mark), +// invalid: false, +// }); +// } +// } +// +// render() { +// // const unassignedClass = this.props.unassigned ? "unassigned" : ""; +// // const expandedClass = this.props.expanded ? "expanded" : "collapsed"; +// // +// // let markElement; +// // if (this.props.released_to_students) { +// // // Student view +// // markElement = this.props.mark; +// // } else { +// // markElement = ( +// // +// // ); +// // } +// +// // return ( +// //
  • +// //
    +// //
    +// //
    +// // {this.props.name} +// // {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} +// // {this.deleteManualMarkLink()} +// //
    +// //
    +// // +// // {markElement} +// //  /  +// // {this.props.max_mark} +// // +// // {this.listDeductions()} +// // {this.renderOldMark()} +// //
    +// //
    • +// // ); +// } +// } FlexibleCriterionInput.propTypes = { annotations: PropTypes.arrayOf(PropTypes.object).isRequired, diff --git a/app/javascript/Components/Result/marks_panel.jsx b/app/javascript/Components/Result/marks_panel.jsx index e5e9ebe002..43ae79ba74 100644 --- a/app/javascript/Components/Result/marks_panel.jsx +++ b/app/javascript/Components/Result/marks_panel.jsx @@ -1,7 +1,7 @@ import React from "react"; import CheckboxCriterionInput from "./checkbox_criterion_input"; -import {FlexibleCriterionInput} from "./flexible_criterion_input"; +import FlexibleCriterionInput from "./flexible_criterion_input"; import RubricCriterionInput from "./rubric_criterion_input"; export class MarksPanel extends React.Component { diff --git a/app/javascript/Components/__tests__/marks_panel.test.jsx b/app/javascript/Components/__tests__/marks_panel.test.jsx index 774d3689ff..6dd82471a7 100644 --- a/app/javascript/Components/__tests__/marks_panel.test.jsx +++ b/app/javascript/Components/__tests__/marks_panel.test.jsx @@ -1,10 +1,10 @@ -import {render, screen} from "@testing-library/react"; +import {render, screen, waitFor} from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import {MarksPanel} from "../Result/marks_panel"; import CheckboxCriterionInput from "../Result/checkbox_criterion_input"; -import {FlexibleCriterionInput} from "../Result/flexible_criterion_input"; +import FlexibleCriterionInput from "../Result/flexible_criterion_input"; import RubricCriterionInput from "../Result/rubric_criterion_input"; const convertToKebabCase = { @@ -306,7 +306,9 @@ describe("FlexibleCriterionInput", () => { await userEvent.clear(input); await userEvent.type(input, "Hi Prof Liu"); expect(input.value).toEqual("Hi Prof Liu"); - expect(input.classList.contains("invalid")).toBeTruthy(); + await waitFor(() => { + expect(input.classList.contains("invalid")).toBeTruthy(); + }); }); it("should set the mark as valid if it has a decimal", async () => { From 62b6867c6b1e6d45812ebc184a40c62bc7ff8eaf Mon Sep 17 00:00:00 2001 From: Liz Date: Fri, 17 Oct 2025 22:56:36 -0400 Subject: [PATCH 06/10] updated changelog --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 5cfa1af3e9..535f7d28d0 100644 --- a/Changelog.md +++ b/Changelog.md @@ -22,7 +22,7 @@ - Fixed Rack deprecation warnings by updating HTTP status code symbols (#7675) - Refactored "Reuse Groups" functionality to use React modal and relocated button to action box row (#7688) - Updated pre-commit `rubocop-rails` version to 2.33.4 (#7691) -- Refactored MarksPanel child components and converted CheckboxCriterionInput and RubricCriterionInput into hook-based function components +- Refactored MarksPanel child components and converted the components into hook-based function components ## [v2.8.2] From 166c109993d9b667aee06410c95ed78dfef878c1 Mon Sep 17 00:00:00 2001 From: Liz Date: Fri, 17 Oct 2025 22:57:48 -0400 Subject: [PATCH 07/10] cleaned up code --- .../Result/flexible_criterion_input.jsx | 72 ------------------- 1 file changed, 72 deletions(-) diff --git a/app/javascript/Components/Result/flexible_criterion_input.jsx b/app/javascript/Components/Result/flexible_criterion_input.jsx index 6efc773412..8a39e72fd4 100644 --- a/app/javascript/Components/Result/flexible_criterion_input.jsx +++ b/app/javascript/Components/Result/flexible_criterion_input.jsx @@ -183,78 +183,6 @@ export default function FlexibleCriterionInput({ ); } -// export class FlexibleCriterionInput extends React.Component { -// constructor(props) { -// super(props); -// this.state = { -// rawText: this.props.mark === null ? "" : String(this.props.mark), -// invalid: false, -// }; -// this.typing_timer = undefined; -// } -// -// -// componentDidUpdate(oldProps) { -// if (oldProps.mark !== this.props.mark) { -// this.setState({ -// rawText: this.props.mark === null ? "" : String(this.props.mark), -// invalid: false, -// }); -// } -// } -// -// render() { -// // const unassignedClass = this.props.unassigned ? "unassigned" : ""; -// // const expandedClass = this.props.expanded ? "expanded" : "collapsed"; -// // -// // let markElement; -// // if (this.props.released_to_students) { -// // // Student view -// // markElement = this.props.mark; -// // } else { -// // markElement = ( -// // -// // ); -// // } -// -// // return ( -// //
  • -// //
    -// //
    -// //
    -// // {this.props.name} -// // {this.props.bonus && ` (${I18n.t("activerecord.attributes.criterion.bonus")})`} -// // {this.deleteManualMarkLink()} -// //
    -// //
    -// // -// // {markElement} -// //  /  -// // {this.props.max_mark} -// // -// // {this.listDeductions()} -// // {this.renderOldMark()} -// //
    -// //
    • -// // ); -// } -// } FlexibleCriterionInput.propTypes = { annotations: PropTypes.arrayOf(PropTypes.object).isRequired, From 3e3a94ab685077f5606cbc328a19cabc55edae8c Mon Sep 17 00:00:00 2001 From: Liz Date: Sun, 19 Oct 2025 18:37:08 -0400 Subject: [PATCH 08/10] added tests for checkbox, flexible, and rubric criterion input components --- .../Components/__tests__/marks_panel.test.jsx | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/app/javascript/Components/__tests__/marks_panel.test.jsx b/app/javascript/Components/__tests__/marks_panel.test.jsx index 6dd82471a7..f902f5e40d 100644 --- a/app/javascript/Components/__tests__/marks_panel.test.jsx +++ b/app/javascript/Components/__tests__/marks_panel.test.jsx @@ -153,8 +153,23 @@ describe("CheckboxCriterionInput", () => { expect(screen.queryByText(`(${I18n.t("results.remark.old_mark")}: 1)`)).toBeTruthy(); }); - it("renders CheckboxCriterionInput", () => { + it("renders CheckboxCriterionInput with radio buttons", () => { render(); + + // Check at least 1 criterion label renders + expect(screen.getAllByText(/criterion/i).length).toBeGreaterThan(0); + + // Check at least 1 radio button pair render (yes/no) + const radios = screen.getAllByRole("radio"); + expect(radios.length).toBeGreaterThanOrEqual(2); + }); + + it("shows Delete Mark link when mark exists and not released", () => { + render(); + + // Check at least one delete link renders + const deleteLinks = screen.getAllByText(/delete mark/i); + expect(deleteLinks.length).toBeGreaterThan(0); }); }); @@ -361,8 +376,27 @@ describe("FlexibleCriterionInput", () => { expect(screen.queryAllByRole("textbox")).toEqual([]); }); - it("renders FlexibleCriterionInput", () => { + it("renders FlexibleCriterionInput with input field", () => { render(); + + // Check input box for marks renders + const input = screen.getByRole("textbox"); + expect(input).toBeInTheDocument(); + }); + + it("updates input value when mark prop changes", () => { + const {rerender} = render(); + + const input = screen.getByRole("textbox"); + expect(input.value).toBe("2"); + + // Re-render with new mark + rerender(); + expect(input.value).toBe("5"); + + // Re-render with null mark (should clear it) + rerender(); + expect(input.value).toBe(""); }); }); @@ -480,7 +514,18 @@ describe("RubricCriterionInput", () => { expect(screen.queryAllByRole("link")).toEqual([]); }); - it("renders RubricCriterionInput", () => { + it("renders RubricCriterionInput with rubric levels", () => { render(); + + // Check criterion label renders + expect(screen.getByText(/criterion/i)).toBeInTheDocument(); + + // Check rubric levels render + expect(screen.getByText(/level 1/i)).toBeInTheDocument(); + expect(screen.getByText(/level 2/i)).toBeInTheDocument(); + + // Check rubric description renders + const descriptions = screen.getAllByText(/description/i); + expect(descriptions).toHaveLength(2); }); }); From 2a5ee59c082b9dd9c00b0c2a77ba23fdead7b016 Mon Sep 17 00:00:00 2001 From: Liz Date: Tue, 21 Oct 2025 22:34:42 -0400 Subject: [PATCH 09/10] added blank line before function definition --- app/javascript/Components/Result/rubric_criterion_input.jsx | 1 + 1 file changed, 1 insertion(+) diff --git a/app/javascript/Components/Result/rubric_criterion_input.jsx b/app/javascript/Components/Result/rubric_criterion_input.jsx index f27fec68c9..f29cf6275a 100644 --- a/app/javascript/Components/Result/rubric_criterion_input.jsx +++ b/app/javascript/Components/Result/rubric_criterion_input.jsx @@ -2,6 +2,7 @@ import React from "react"; import PropTypes from "prop-types"; import safe_marked from "../../common/safe_marked"; + export default function RubricCriterionInput({ bonus, destroyMark, From 10811b9fd7b276829ae32cf7f8db4adea86df164 Mon Sep 17 00:00:00 2001 From: Liz Date: Tue, 21 Oct 2025 22:38:39 -0400 Subject: [PATCH 10/10] ordered props alphabetically --- app/javascript/Components/Result/checkbox_criterion_input.jsx | 4 ++-- app/javascript/Components/Result/flexible_criterion_input.jsx | 2 +- app/javascript/Components/Result/rubric_criterion_input.jsx | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/javascript/Components/Result/checkbox_criterion_input.jsx b/app/javascript/Components/Result/checkbox_criterion_input.jsx index 94da4edfe5..ab9fdb24ec 100644 --- a/app/javascript/Components/Result/checkbox_criterion_input.jsx +++ b/app/javascript/Components/Result/checkbox_criterion_input.jsx @@ -4,19 +4,19 @@ import PropTypes from "prop-types"; import safe_marked from "../../common/safe_marked"; export default function CheckboxCriterionInput({ + bonus, description, destroyMark, expanded, id, mark, max_mark, + name, oldMark, released_to_students, toggleExpanded, unassigned, updateMark, - name, - bonus, }) { const unassignedClass = unassigned ? "unassigned" : ""; const expandedClass = expanded ? "expanded" : "collapsed"; diff --git a/app/javascript/Components/Result/flexible_criterion_input.jsx b/app/javascript/Components/Result/flexible_criterion_input.jsx index 8a39e72fd4..5cea9729ae 100644 --- a/app/javascript/Components/Result/flexible_criterion_input.jsx +++ b/app/javascript/Components/Result/flexible_criterion_input.jsx @@ -13,6 +13,7 @@ export default function FlexibleCriterionInput({ id, mark, max_mark, + name, oldMark, override, released_to_students, @@ -20,7 +21,6 @@ export default function FlexibleCriterionInput({ toggleExpanded, unassigned, updateMark, - name, }) { const [rawText, setRawText] = useState(mark === null ? "" : String(mark)); const [invalid, setInvalid] = useState(false); diff --git a/app/javascript/Components/Result/rubric_criterion_input.jsx b/app/javascript/Components/Result/rubric_criterion_input.jsx index f29cf6275a..5c8c07f67a 100644 --- a/app/javascript/Components/Result/rubric_criterion_input.jsx +++ b/app/javascript/Components/Result/rubric_criterion_input.jsx @@ -11,12 +11,12 @@ export default function RubricCriterionInput({ levels, mark, max_mark, + name, oldMark, released_to_students, toggleExpanded, unassigned, updateMark, - name, }) { // The parameter `level` is the level object selected const handleChange = level => {