-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: added ReviewExamAttemptButton #11
Conversation
- added call to backend api - developing change to redux state
> | ||
<ModalDialog.Header> | ||
<ModalDialog.Title> | ||
{formatMessage({ |
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.
can you move these to the messages.js file
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.
I was able to move most of these, however some of these messages (not just in this file but in others) have values that are passed in. Not sure how to handle those yet.
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.
it's mostly the same you just pass a map of values intl.formatMessage(message_id, { valuesMapping })
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.
I've resolved this by following your suggestion of breaking up the info in messages that get values passed in into separate messages is messages.js
@ilee2u can you add some screenshots with this? |
Here's a screen recording of the button in action: Screen.Recording.2023-08-03.at.1.28.59.PM.mov |
const capitalizeFirstLetter = (string) => string.charAt(0).toUpperCase() + string.slice(1); | ||
const cleanString = (string) => { | ||
// Remove underscores, capitalize the first letter of each word, and return as a single string. | ||
return string.split("_").map(string => string.charAt(0).toUpperCase() + string.slice(1)).join(" "); |
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.
if we're going down the path of formatting these we should probably just create a map between the backend value and UI labels since this method isn't internationalizable (if that's a word...)
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.
I have improved this by adding this mapping in data/constants.js
> | ||
<ModalDialog.Header> | ||
<ModalDialog.Title> | ||
{formatMessage({ |
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.
it's mostly the same you just pass a map of values intl.formatMessage(message_id, { valuesMapping })
92fafc4
to
7605b11
Compare
reject: 'reject', | ||
}; | ||
|
||
export const ExamAttemptStatus = { |
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.
sorry to keep nit picking on these but I think there's two different types of constants that belong in different places. One is just the statuses we get from the backend (so lower/snake case values) and the other is UI labels for those statuses. I think the former makes sense here like the ExamAttemptActions you have above but the UI labels should probably go nearer to the component that's using them and likely named accordingly. I think someone looking at this file the first time would assume these are all the backend values since it's in the data folder.
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.
I've moved the UI label mapping to AttemptsList, and kept the backend string mapping in constants.js
- Fixed attempt data values to match backend constants
Codecov Report
@@ Coverage Diff @@
## main #11 +/- ##
==========================================
+ Coverage 67.29% 70.19% +2.89%
==========================================
Files 20 21 +1
Lines 211 255 +44
Branches 12 19 +7
==========================================
+ Hits 142 179 +37
- Misses 66 72 +6
- Partials 3 4 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
looking good!
Ticket: https://2u-internal.atlassian.net/jira/software/projects/MST/boards/584?selectedIssue=MST-1989
Attempts in the ‘second_review_required’ state will need an additional button to ‘review’ the attempt. Based on the findings in this discovery: https://2u-internal.atlassian.net/wiki/spaces/PT/pages/485916748/Discovery+Instructor+Gradebook+Reviews
TODO: UX for this button.
suggestion, we actually make the ‘Second review required’ text a bright noticeable button instead of using the gear icon or a dropdown in the action column. It’s important instructors are able to quickly identify and resolve these exams.
Include import { Warning } from '@edx/paragon/icons'; with color danger 500
Clicking review opens a model similar to the reset button. This model should include the following text:
username
exam name
suspicion level
Text directing user to the review dashboard for more details. (we could link)
And two actions:
Verify
Reject
Verify/Reject will call the existing /exams/attempt api with ‘verified’ or ‘rejected’ as the action parameter.