-
Notifications
You must be signed in to change notification settings - Fork 523
[AITT-1343][Snapshot - CFU general] Programmatically determine which levels in a lesson are CFU #69977
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
Conversation
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 adds functionality to programmatically identify and fetch CFU (Check For Understanding) levels from lessons in the Student Snapshot feature. The implementation adds a new backend endpoint that filters script levels by their progression name and exposes this data to the frontend React component.
Key changes:
- Added new API endpoint
/student_snapshots/cfu_levels/:lesson_idto fetch CFU levels from a specific lesson - Implemented CFU level identification based on progression name matching "Check Your Understanding" or "Check For Understanding"
- Integrated CFU levels fetching in the StudentSnapshot React component with loading state management
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dashboard/config/routes.rb | Added route definition for the new cfu_levels endpoint |
| dashboard/app/controllers/student_snapshots_controller.rb | Implemented cfu_levels action that queries lessons for script levels with CFU progression names and returns level metadata |
| apps/src/templates/studentSnapshot/StudentSnapshot.tsx | Added TypeScript interfaces, API client function, state management, and useEffect hook to fetch and store CFU levels when lesson selection changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def cfu_levels | ||
| lesson_id = params[:lesson_id] | ||
| lesson = Lesson.find_by(id: lesson_id) | ||
| return render json: {error: "Can't find Lesson id=#{lesson_id}"}, status: :bad_request unless lesson | ||
|
|
||
| cfu_levels_data = [] | ||
| lesson.script_levels.each do |script_level| | ||
| # Check if this script_level has progression: "Check Your Understanding" or "Check For Understanding" | ||
| if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/i) | ||
| script_level.levels.each do |level| | ||
| cfu_levels_data << { | ||
| id: level.id, | ||
| name: level.name, | ||
| display_name: level.display_name || level.name, | ||
| type: level.type, | ||
| key: level.try(:key), | ||
| script_level_id: script_level.id, | ||
| progression: script_level.progression, | ||
| progression_display_name: script_level.progression ? I18n.t(script_level.progression, scope: %i[data progressions], default: script_level.progression) : nil | ||
| } | ||
| end | ||
| end | ||
| end | ||
|
|
||
| render json: {cfu_levels: cfu_levels_data} | ||
| end |
Copilot
AI
Dec 10, 2025
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.
The new endpoint cfu_levels lacks test coverage. The existing lessons endpoint in this controller has comprehensive tests (checking success cases, error cases, and edge cases), but no tests have been added for the new cfu_levels endpoint. Add tests to verify: 1) correct CFU levels are returned for a valid lesson_id, 2) appropriate error is returned for an invalid lesson_id, 3) empty array is returned for a lesson with no CFU levels, and 4) levels with both "Check Your Understanding" and "Check For Understanding" progressions are correctly identified.
| }, [selectedLessonId]); | ||
|
|
||
| // TODO: Use this in CFU widget | ||
| console.log(cfuLevels, isCfuLevelsLoading); |
Copilot
AI
Dec 10, 2025
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.
The console.log statement on line 108 should be removed before merging to production. Debug logging statements left in production code can expose potentially sensitive information and clutter browser console output. The TODO comment on line 107 indicates this is temporary code, but the console.log should be removed now rather than waiting until the CFU widget implementation is complete.
| console.log(cfuLevels, isCfuLevelsLoading); |
|
|
||
| # GET /student_snapshots/cfu_levels/:lesson_id | ||
| # Returns all CFU levels from the specified lesson | ||
| # CFU levels are identified by progression: "Check Your Understanding" |
Copilot
AI
Dec 10, 2025
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.
The comment on line 28 is inconsistent with the actual implementation. The comment states CFU levels are identified by progression "Check Your Understanding", but the implementation (line 37) also matches "Check For Understanding". Update the comment to accurately reflect that both patterns are supported, or clarify which is the canonical name.
| # CFU levels are identified by progression: "Check Your Understanding" | |
| # CFU levels are identified by progression: "Check Your Understanding" or "Check For Understanding" |
| cfu_levels_data = [] | ||
| lesson.script_levels.each do |script_level| | ||
| # Check if this script_level has progression: "Check Your Understanding" or "Check For Understanding" | ||
| if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/i) |
Copilot
AI
Dec 10, 2025
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.
The regex pattern /^Check\s+(Your|For)\s+Understanding$/i uses the case-insensitive flag i, which allows matches like "check your understanding", "CHECK YOUR UNDERSTANDING", etc. This may be more permissive than intended if progression names are expected to follow a specific capitalization convention. Consider whether case-insensitive matching is necessary, or if the pattern should enforce exact capitalization to ensure data consistency.
| if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/i) | |
| if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/) |
|
This is a super clever approach. Recieved the following from Jamila confirming we can count on this naming going forward. I will take a follow up task for us to build a way to mark this into the tool itself.
|
lfryemason
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.
Generally love your approach and the structure, my only main concern is about determining CFU status from name
| const [isLessonsLoading, setIsLessonsLoading] = useState<boolean>(false); | ||
| const [hasUnnumberedLessons, setHasUnnumberedLessons] = | ||
| useState<boolean>(false); | ||
| const [cfuLevels, setCfuLevels] = useState<CFULevel[]>([]); |
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.
Do you think we will want these CFU levels in multiple widgets or should we put this logic into the CFU widget?
Fine if we want to keep it this way for now and move it later.
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 put this here only because we don't have cfuWidget at all yet and I wanted to give a good way to check if this works locally. Once we have the widget - this can be moved into widget.
| cfu_levels_data = [] | ||
| lesson.script_levels.each do |script_level| | ||
| # Check if this script_level has progression: "Check Your Understanding" or "Check For Understanding" | ||
| if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/i) |
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.
Is there a way that we can determine whether it's a CFU level based on the level type instead of the progression name? Is there a convenient way to check whether a level is a match, free response or multiple choice level instead of regex on the name?
I'm always a little skeptical of using naming patterns because they could change later or be mis-spelled or be different depending on unit.
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.
Yeah, we're sharing the same thaughts on this. It's just that for now it seems to be the only reliable option.
Long term - I think it would be nice to actually set smth like levelType or isCfuLevel field to the level, as right now the field that we have - don't tied to cfu at all an the fact that levels is a free response, or anything else doesn't guarantee that it's actually a cfu level (which I think is fine, but still worth adding separate field indicating if level is cfu or not)
lfryemason
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.
I talked with Tess about this and it seems like this is the right approach for now. I would still like to eventually move away from regex on a name, but that requires a lot more work than we want for this initial snapshot v0.
Warning!!
We have entered Pixel Lock for Hour of AI! All merges to the
stagingbranch from Dec 2 through Dec 12 must go through live change review and be deemed critical for supporting the Hour of AI. External contributions will not be accepted at this time.For non-critical changes, please change your base to
staging-nextand delete this warning. We will mergestaging-nextintostagingon Dec 15, 2025.[AITT-1343][Snapshot - CFU general] Programmatically determine which levels in a lesson are CFU
2025-12-10.21.21.39.mov
Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Creation Checklist: