Lesson Feedback - fix missing params #70726
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the lesson feedback flow to require/persist section_id when fetching AI-generated lesson feedback, and simplifies the frontend by removing the unused lesson feedback hook.
Changes:
- Backend: derive
teacher_idfromcurrent_userand requiresection_idvia request params. - Frontend: pass
sectionIdthrough Student Snapshot → Lesson Feedback widget and include it in the AI feedback request. - Frontend: remove
useLessonFeedbackhook usage (and delete the hook file).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| dashboard/app/controllers/student_snapshots_controller.rb | Switches to current_user for teacher_id and requires section_id from params for AI feedback endpoint. |
| apps/src/templates/studentSnapshot/lessonFeedbackWidget/useLessonFeedback.ts | Removes the custom hook (no longer used by the widget). |
| apps/src/templates/studentSnapshot/lessonFeedbackWidget/LessonFeedbackWidget.tsx | Adds sectionId prop, appends it to AI feedback fetch URL, removes hook usage, and adjusts widget behaviors. |
| apps/src/templates/studentSnapshot/StudentSnapshot.tsx | Passes sectionId down into LessonFeedbackWidget. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resource_link: '', | ||
| }, | ||
| ]); | ||
| const [isLoading, setIsLoading] = React.useState<boolean>(false); |
There was a problem hiding this comment.
isLoading is set to true when the effect runs, but there’s no corresponding setIsLoading(false) shown after fetchLessonFeedback completes (or when the guard prevents fetching). This can leave the widget in a perpetual loading state. Consider moving setIsLoading(true) inside the if (lessonId && studentId && unitId && sectionId) block and ensuring setIsLoading(false) is set in a finally block within fetchLessonFeedback (and also in any early-return paths).
There was a problem hiding this comment.
NOTE: I took this feedback
|
|
||
| // Fetch lesson feedback from backend, and if not found, try generating ai feedback | ||
| React.useEffect(() => { | ||
| setIsLoading(true); |
There was a problem hiding this comment.
isLoading is set to true when the effect runs, but there’s no corresponding setIsLoading(false) shown after fetchLessonFeedback completes (or when the guard prevents fetching). This can leave the widget in a perpetual loading state. Consider moving setIsLoading(true) inside the if (lessonId && studentId && unitId && sectionId) block and ensuring setIsLoading(false) is set in a finally block within fetchLessonFeedback (and also in any early-return paths).
| if (lessonId && studentId && unitId && sectionId) { | ||
| fetchLessonFeedback(); | ||
| } | ||
| }, [lessonId, sectionId, studentId, unitId]); |
There was a problem hiding this comment.
isLoading is set to true when the effect runs, but there’s no corresponding setIsLoading(false) shown after fetchLessonFeedback completes (or when the guard prevents fetching). This can leave the widget in a perpetual loading state. Consider moving setIsLoading(true) inside the if (lessonId && studentId && unitId && sectionId) block and ensuring setIsLoading(false) is set in a finally block within fetchLessonFeedback (and also in any early-return paths).
| lesson_id = params[:lesson_id] | ||
| unit_id = params[:unit_id] | ||
| student_id = params[:student_id] | ||
|
|
||
| section_id = nil | ||
| teacher_id = nil | ||
| if student_id && unit_id | ||
| student = User.find_by(id: student_id) | ||
| if student | ||
| section = student.sections_as_student.joins(:script).where(scripts: {id: unit_id}).first | ||
| section_id = section&.id | ||
| teacher_id = section&.teacher&.id | ||
| end | ||
| end | ||
| teacher_id = current_user.id | ||
| section_id = params[:section_id] | ||
|
|
||
| return render json: {error: "Missing required parameters"}, status: :bad_request unless lesson_id && unit_id && student_id && section_id |
There was a problem hiding this comment.
The endpoint now trusts section_id coming from the client. Because this value is user-controlled, it should be authorized server-side (e.g., ensure the section_id belongs to current_user, and the student_id is actually enrolled in that section) before using it to generate or fetch feedback. A concrete approach is to look up the section through current_user.sections (or equivalent) by section_id, and verify the student is in it; otherwise return a 403.
There was a problem hiding this comment.
I had to do some research on this but I actually think this might be a good suggestion... I took it.
| onSendToStudent={() => { | ||
| console.log('Send to student:', feedbackText, resourceData); | ||
| }} |
There was a problem hiding this comment.
Leaving a console.log in the user action handler will likely ship debug output to production and doesn’t perform the intended action. If sending isn’t implemented yet, consider wiring this back to the prior handler (or a stub that shows a UI message), or gating logs behind a dev-only check.
There was a problem hiding this comment.
This is intentional for now, the "send to student" functionality will come later. The old handler was also just a console log statement.
There was a problem hiding this comment.
I originally used this file to keep track of many states in the LessonFeedback component. Since then, I have refactored it to reduce the number of states used AND move states to some child components. I did a double check to make sure this file was not needed any more.
kobryan0619
changed the title
2 participants
In yesterday's bug bash, lesson feedback was not showing up in some situations. Specifically, the network tab was showing
"Missing required parameters".After some digging, it appeared to come from the fact that I was checking that a student was in a section with the unit we were getting feedback on assigned. I modified this so that teachers can get AI generated feedback for students in any unit.
I imagine in the future we will want to refine this further, but for right now this will support "one step forward"