Skip to content

Conversation

@levadadenys
Copy link
Contributor

@levadadenys levadadenys commented Dec 10, 2025

Warning!!

We have entered Pixel Lock for Hour of AI! All merges to the staging branch 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-next and delete this warning. We will merge staging-next into staging on Dec 15, 2025.

[AITT-1343][Snapshot - CFU general] Programmatically determine which levels in a lesson are CFU

  • feat: add CFU levels fetching functionality to StudentSnapshot component
2025-12-10.21.21.39.mov

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Creation Checklist:

  • Tests provide adequate coverage
  • Privacy impacts have been documented
  • Security impacts have been documented
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

Copilot AI left a 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_id to 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.

Comment on lines +29 to +54
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
Copy link

Copilot AI Dec 10, 2025

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.

Copilot uses AI. Check for mistakes.
}, [selectedLessonId]);

// TODO: Use this in CFU widget
console.log(cfuLevels, isCfuLevelsLoading);
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
console.log(cfuLevels, isCfuLevelsLoading);

Copilot uses AI. Check for mistakes.

# GET /student_snapshots/cfu_levels/:lesson_id
# Returns all CFU levels from the specified lesson
# CFU levels are identified by progression: "Check Your Understanding"
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
# CFU levels are identified by progression: "Check Your Understanding"
# CFU levels are identified by progression: "Check Your Understanding" or "Check For Understanding"

Copilot uses AI. Check for mistakes.
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)
Copy link

Copilot AI Dec 10, 2025

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.

Suggested change
if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/i)
if script_level.progression&.match?(/^Check\s+(Your|For)\s+Understanding$/)

Copilot uses AI. Check for mistakes.
@tess323
Copy link

tess323 commented Dec 10, 2025

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.

yes! we've been actually trying to use that phrase to refer to them anyway to make it clear to teachers

Copy link
Contributor

@lfryemason lfryemason left a 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[]>([]);
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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)

Copy link
Contributor

@lfryemason lfryemason left a 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.

@levadadenys levadadenys merged commit 309f6a6 into staging-next Dec 12, 2025
3 checks passed
@levadadenys levadadenys deleted the denys/aitt/aitt-1343 branch December 12, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants