Fix and test PL unit completion for contained levels

[bethanyaconnor]

Fixes a case where, when a unit contained a bubble choice level, greater than 100% completion was being shown. To fix that, I changed the logic to look for the levels, instead of the units, associated with a UserLevel.

This did come with a side effect of not counting predict levels correctly. Predict levels are set up such that the Blockly level is in the ScriptLevel and contains the FreeResponse/Multi level. However, the UserLevel is created for the FreeResponse/Multi level. I added some special handling for that case.

I added a test for both BubbleChoice and predict levels.

I largely used the investigation in #57121 as a starting point here!

Warning!!

The AP CSP Create Performance Task is in progress. The most critical dates are from April 3 - April 30, 2024. Please consider any risk introduced by this PR that could affect our students taking AP CSP. Code.org students taking AP CSP primarily use App Lab for their Create Task, however a small percent use Game Lab. Carefully consider whether your change has any risk of alterering, changing, or breaking anything in these two labs. Even small changes, such as a different button color, are considered significant during this time period. Reach out to the Student Learning team or Curriculum team for more details.

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• 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
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[bethanyaconnor]

Tagging Teacher Tools in case they have any insight!

[etaderhold]

This change LGTM.

I do wonder about the difference between this behavior of looking at all contained levels individually, vs. what we do on the progress table where we just look at the first contained level for non-bubble-choice levels. Are there good reasons to do things differently in these two places, or would it be better to standardize on one way?

code-dot-org/dashboard/app/models/levels/level.rb

Line 809 in 7c97b3f

def get_level_for_progress(student = nil, script = nil)

[bethanyaconnor]

I do wonder about the difference between this behavior of looking at all contained levels individually, vs. what we do on the progress table where we just look at the first contained level for non-bubble-choice levels.

I think, in practice, levels can only have one (or zero) contained level, so they're effectively the same. But I would love to standardize on that and actually enforce it. Do you want me to change this logic to only look at the first contained level?

[etaderhold]

I think, in practice, levels can only have one (or zero) contained level, so they're effectively the same.

I ran Level.all.filter{|level| !level.is_a?(BubbleChoice)}.filter {|level| (level.contained_levels.length rescue 0) > 1 } in a Rails console in my local dev environment and it came back with just two levels with more than one contained level. So yes, agreed that it's nearly always the same in practice.

But I would love to standardize on that and actually enforce it. Do you want me to change this logic to only look at the first contained level?

Sounds fine!