-
Notifications
You must be signed in to change notification settings - Fork 481
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
Add controller for ai evaluation teacher feedback #54830
Add controller for ai evaluation teacher feedback #54830
Conversation
…bs down or on a thumbs up Also pull in the aiInfo object to get the id of the associated AI Evaluation
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.
Awesome work Ceara, the new controller looks great! I did have a few comments on the JS side.
@@ -47,6 +47,7 @@ export default function RubricContainer({ | |||
return response.json(); | |||
}) | |||
.then(data => { | |||
console.log(data); |
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.
remove debug code?
HttpClient.post(`${baseUrl}`, bodyData, true, { | ||
'Content-Type': 'application/json', | ||
}).catch(error => { | ||
console.log(error); |
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 not going to do anything with this, it would be better practice to let the js error propagate, so that it shows up in our error reporting.
HttpClient.post(`${baseUrl}`, bodyData, true, { | ||
'Content-Type': 'application/json', | ||
}).catch(error => { | ||
console.log(error); |
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.
per previous comment, please let js error propagate and do not catch it here, unless you are going to catch it and prompt the user to try again
const icon = require('@cdo/static/ai-bot.png'); | ||
|
||
export default function AiAssessment({ | ||
isAiAssessed, | ||
studentName, | ||
aiUnderstandingLevel, | ||
aiConfidence, | ||
learningGoalKey, | ||
aiInfo, |
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 we please call this aiEval
or aiEvalInfo
, to make it a bit more specific and to more closely match the name of aiEvaluationShape?
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.
also, any reason we still need aiUnderstandingLevel and aiConfidence as props, now that we have the aiEvaluation, which contains both of these?
code-dot-org/apps/src/templates/rubrics/rubricShapes.jsx
Lines 45 to 49 in 6c6897e
export const aiEvaluationShape = PropTypes.shape({ | |
learning_goal_id: PropTypes.number.isRequired, | |
understanding: PropTypes.number.isRequired, | |
ai_confidence: PropTypes.number, | |
}); |
if not, you could get rid of those props, and do something like this here:
const aiConfidence = aiEval.ai_confidence;
...
const thumbsupval = 'thumbsup'; | ||
const thumbsdownval = 'thumbsdown'; | ||
export default function AiAssessmentFeedback({aiInfo}) { | ||
const radioGroupName = `ai-assessment-feedback-${aiInfo.id}`; |
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.
this object appears to be of type aiEvaluationShape, but I don't see id
defined on that shape. can you please add it here?
code-dot-org/apps/src/templates/rubrics/rubricShapes.jsx
Lines 45 to 49 in 6c6897e
export const aiEvaluationShape = PropTypes.shape({ | |
learning_goal_id: PropTypes.number.isRequired, | |
understanding: PropTypes.number.isRequired, | |
ai_confidence: PropTypes.number, | |
}); |
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.
That's already in this PR!
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.
apologies! 😊
const props = { | ||
learningGoalKey: 'learning_key', | ||
aiInfo: mock_aiInfo, |
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.
style nit: please avoid underscores in JS variable names when possible. in this case the variable name can just be mockAiInfo
. we try to only use snake_case when it sneaks through in JSON that comes from the server, and even in those cases the better practice is to have the server do the work to convert any key names in JSON to camelCase.
test_user_gets_response_for :get_by_ai_evaluation_id, params: -> {{learningGoalAiEvaluationId: @learning_goal_ai_evaluation.id}}, user: :student, response: :forbidden | ||
test_user_gets_response_for :get_by_ai_evaluation_id, params: -> {{learningGoalAiEvaluationId: @learning_goal_ai_evaluation.id}}, user: -> {@teacher}, response: :success | ||
test_user_gets_response_for :get_by_ai_evaluation_id, params: -> {{learningGoalAiEvaluationId: @learning_goal_ai_evaluation.id}}, user: :teacher, response: :not_found | ||
end |
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.
great tests! 💯
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.
Looks good! If you are just adding a comment, I think it is ok to merge without waiting for another drone run.
aiFeedbackApproval: aiFeedback, | ||
falsePositive: aiFalsePos, | ||
falseNegative: aiFalseNeg, | ||
Vague: aiVague, |
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.
Please add a brief comment indicating that the capitalization here is to work around a bug. even if we don't fully understand it, it would be nice to tip ourselves off that lower-casing this key name will cause problems.
This creates a controller for learning_goal_ai_evaluation_feedbacks, and sends data to it when a teacher gives feedback on an AI evaluation via the thumbs up/down buttons and options.
Links
Addresses these two Jira tickets:
https://codedotorg.atlassian.net/browse/AITT-307
https://codedotorg.atlassian.net/browse/AITT-323
Testing story
This includes tests for the backend controller, and updated unit tests for the app
Deployment strategy
Follow-up work
Working on this demonstrated a potential bug/ unexpected behavior with associating JSON key names with the table key names (vague ❌ ; Vague ✅ ). This has a workaround but the original problem is still mysterious.
Privacy
Security
Caching
PR Checklist: