-
Notifications
You must be signed in to change notification settings - Fork 58
feat: add prompt to grading screen #471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #471 +/- ##
===========================================
- Coverage 100.00% 99.90% -0.10%
===========================================
Files 107 108 +1
Lines 1086 1106 +20
Branches 160 164 +4
===========================================
+ Hits 1086 1105 +19
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the ORA (Open Response Assessment) data model to support multiple prompts instead of a single prompt. The change updates the API documentation, Redux state management, selectors, and UI components to handle an array of prompts with descriptions.
Key changes:
- Updated
oraMetadatastructure from a singlepromptstring to apromptsarray of objects withdescriptionfields - Modified Redux selectors to extract and map prompt descriptions from the array
- Enhanced
ResponseDisplaycomponent to conditionally render single or multiple prompts with appropriate UI components
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/data/services/lms/api.js | Updated API documentation comment to reflect prompts instead of prompt |
| src/data/redux/app/selectors.js | Changed selector from prompt to prompts with mapping logic to extract descriptions |
| src/data/redux/app/selectors.test.js | Updated test data and assertions to use prompts array structure |
| src/data/redux/app/reducer.js | Changed initial state from prompt: '' to prompts: [] |
| src/data/redux/app/reducer.test.js | Updated test expectations for new prompts array structure |
| src/containers/ResponseDisplay/index.jsx | Added prompt display logic with single/multiple prompt handling and refactored HTML formatting |
| src/containers/ResponseDisplay/index.test.jsx | Added tests for single and multiple prompt display scenarios |
| src/containers/ResponseDisplay/messages.js | Added internationalization message for prompt collapsible header |
| src/containers/ResponseDisplay/PromptDisplay.jsx | New component file with SinglePromptDisplay and MultiplePromptDisplay components |
| src/containers/ResponseDisplay/ResponseDisplay.scss | Added styling for single and multiple prompt displays |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nsprenkle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design nit: I'm not sure why a separate design is needed for single / multi-prompt. I also have some critiques / questions:
- Why do the single / multi-prompt headers use different colors / expand arrow placement? They should probably be the same.
- The multi-prompt design with prompt collapsed makes it look like the response is the prompt. Either keeping the same separated Prompt
<br>Response design or explicitly labelling the response might make that clearer.
| formattedHtml(text) { | ||
| const cleanedText = text.replaceAll(/\.\.\/asset/g, `${getConfig().LMS_BASE_URL}/asset`); | ||
| return parse(this.purify.sanitize(cleanedText)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: this is technically a sanitizedHtml function. No?
| const multiPrompt = prompts.length > 1; | ||
| return ( | ||
| <div className="response-display"> | ||
| {!multiPrompt && <SinglePromptDisplay prompt={prompts[0]} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: add a comment as to why this styling changes.
| {!multiPrompt && <SinglePromptDisplay prompt={prompts[0]} />} | |
| /* For single prompt ORAs, display the prompt at the top of the page */ | |
| {!multiPrompt && <SinglePromptDisplay prompt={prompts[0]} />} |
| * Returns the ORA Prompts | ||
| * @return {array[string]} - ORA prompt | ||
| */ | ||
| prompt: oraMetadataSelector(data => data.prompt), | ||
| prompts: oraMetadataSelector(data => (data.prompts ? data.prompts.map((oraPrompt) => oraPrompt.description) : [])), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require backend changes? If so, can you link them?
This PR adds the prompts to the ora grading screen.
In order to do so, I needed to fix the way we were loading the prompts from redux state, along with testing.
We were treating it as a single string, but actually it's being stored as a list of objects with the text in a "description" key:
For the prompt, I had behavior diverge when there is one vs multiple prompts. For one prompt, I added the prompt as a dropdown at the top of the screen, above the file uploads.
##Existing:

##New, open/collapsed


For multiple prompts, the prompt is included at the top of the text response to that prompt. I also added some padding between text submissions to improve the overall look.
##existing

##New, open / collapsed

