feat(questions): display staff only comments on assessment view #8204
Conversation
- separate question show view (used in rubric playground) into its own view
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to display staff-only comments for questions on the assessment view page, making internal notes visible to course staff when reviewing assessment questions.
Changes:
- Added
staffOnlyCommentsfield to question type definitions - Rendered staff-only comments as an info alert below question descriptions in the assessment view
- Created a new jbuilder view template for the questions show endpoint
- Added a helper method to check if HTML text contains actual content
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/app/types/course/assessment/questions.ts | Added QuestionBaseDataWithUrl interface and staffOnlyComments field to QuestionData |
| client/app/bundles/course/assessment/pages/AssessmentShow/Question.tsx | Added UI to display staff-only comments in an Alert component |
| client/app/api/course/Assessment/Question/Questions.ts | Updated return type of fetch method to use QuestionBaseDataWithUrl |
| app/views/course/question_assessments/_question_assessment.json.jbuilder | Added conditional rendering of staffOnlyComments field using new helper |
| app/views/course/assessment/questions/show.json.jbuilder | Created new view template for questions show endpoint |
| app/helpers/application_formatters_helper.rb | Added html_text_blank? helper method |
| app/controllers/course/assessment/questions_controller.rb | Removed explicit render call to use implicit view rendering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def html_text_blank?(text) | ||
| return true if text.blank? | ||
|
|
||
| HTMLEntities.new.decode(ActionController::Base.helpers.strip_tags(text)).strip.blank? | ||
| end |
There was a problem hiding this comment.
The new html_text_blank? helper method lacks test coverage. The file already has comprehensive tests for format_rich_text_for_csv, and a similar testing pattern should be applied here to verify behavior with nil, empty strings, HTML-only markup (e.g., <p></p>), whitespace, and HTML entities.
| {question.staffOnlyComments && ( | ||
| <Alert className="[&_p]:m-0" icon={false} severity="info"> | ||
| <Typography | ||
| dangerouslySetInnerHTML={{ | ||
| __html: question.staffOnlyComments, | ||
| }} | ||
| variant="body2" | ||
| /> | ||
| </Alert> | ||
| )} |
There was a problem hiding this comment.
Using dangerouslySetInnerHTML with user-generated content poses an XSS risk. While format_ckeditor_rich_text on the backend does sanitize HTML, relying solely on server-side sanitization can be risky if the sanitization logic changes or has vulnerabilities. Consider using a client-side sanitization library like DOMPurify before rendering, or verify that the backend sanitization is comprehensive and XSS-safe.
Examples:
