When generating PLC course set all course type and published state information #44478
Conversation
…n type, and audience are all set
dmcavoy
changed the title
There was a problem hiding this comment.
this looks like the right idea to me @dmcavoy , but given the potentially fragile nature of this solution I'd be in favor of beefing up the unit tests a bit.
| assert_equal 'teacher', unit_group.participant_audience | ||
| assert_equal 'self_paced', unit_group.instruction_type | ||
| assert_equal 'stable', unit_group.published_state | ||
| end |
There was a problem hiding this comment.
are there any cases we care about where the plc unit already has a unit group (from a previous invocation of generate_plc_objects) and we need to change published state, instruction type or audience? My reading of get_instruction_type is that it would look to unit group first, and so a change to the unit's instruction type wouldn't actually get picked up.
Unless we are confident this will never happen, my suggestion would be to add a test case covering these values changing, whatever the desired behavior is in that case (e.g. it could be to raise an exception if these values are not supposed to change).
There was a problem hiding this comment.
Thanks for pushing on this. We should be just directly checking instruction_type instead of going to get_instruction_type here because we only care about what the script has set for instruction_type.
There was a problem hiding this comment.
per offline conversation, this seems good in terms of unit tests. there are some cases which may make sense to either add safeguards for or else carefully document:
- DLP courses are usually created by cloning. by default cloned courses start in the
in_developmentstate. need to be clear about how to get these intobeta(either by clearly documenting that an engineer should do this right after cloning, or by modifying the clone script)
dashboard/app/models/script.rb
Outdated
| unit_group.published_state = get_published_state if unit_group.published_state != get_published_state | ||
| unit_group.instruction_type = get_instruction_type if unit_group.instruction_type != get_instruction_type | ||
| unit_group.participant_audience = get_participant_audience if unit_group.participant_audience != get_participant_audience | ||
| unit_group.instructor_audience = get_instructor_audience if unit_group.instructor_audience != get_instructor_audience |
There was a problem hiding this comment.
are the if unit_group.foo != foo clauses actually doing anything, given you already have the if unit_group.changed? check below? my suggestion would be to add test cases covering all the behavior you care about, and then consider removing the if clauses on lines 211-214 if you can do so without breaking the tests.
Scripts that are part of PLC courses generate a Unit Group with the same name as the one saved in the
professional_learning_courseon script. The Unit Group is created ingenerate_plc_objectsand is not serialized. We need a way to set things like the audiences on the unit group in order to be able to make sure access to the plc course is set to the right audiences (as well as published states and instruction type). Therefore we must rely on the script to get the unit groups settings for instructor_audience, participant_audience, instruction_type and published_state. This is not the ideal solution to fix this, but because we do no currently have time to invest in cleaning up the plc course models and this set up of unit groups for plc courses, this is the best we can do right now. By setting the unit group based on the settings of the script which has just been saved we could end up with multiple scripts in the same plc course with different settings. We will be creating a guide to publishing plc courses to hopefully help prevent issues here.This will be followed by a PR with a migration to update the current UnitGroups for PLC Courses in all environments: #44479
Links
Slack Thread
Testing story