Validation that courses in family share course type #44062
Conversation
dmcavoy
changed the title
|
@davidsbailey I just went back to using |
There was a problem hiding this comment.
Nice work, Dani!
I'm thinking there is a way to do this with CourseVersion and CourseOffering but this was the first way that came to mind. If you have a suggestion of a better way to do this with CourseVersion and CourseOffering let me know.
Good thought. Given the move to UnitGroup, this seems like the right strategic direction -- get all the code into one place (the concern), and then it will be easier to move onto UnitGroup later.
moving onto CourseOffering and CourseVersion via the get_course_version helper seems nice-to-have when possible -- I've noted one place where that seems like it can be changed, but I would suggest only doing that when it can be done easily.
| return if all_family_courses.nil_or_empty? | ||
|
|
||
| return unless all_family_courses.length > 1 | ||
| errors.add(:instructor_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:instructor_audience).uniq.length > 1 || all_family_courses.last&.instructor_audience != instructor_audience |
There was a problem hiding this comment.
if you want to keep these all on one line, how about something a bit shorter, like:
| errors.add(:instructor_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:instructor_audience).uniq.length > 1 || all_family_courses.last&.instructor_audience != instructor_audience | |
| errors.add(:instructor_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:instructor_audience).uniq != [instructor_audience] |
or
| errors.add(:instructor_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:instructor_audience).uniq.length > 1 || all_family_courses.last&.instructor_audience != instructor_audience | |
| errors.add(:instructor_audience, 'must be the same for all courses in a family.') if all_family_courses.map(&:instructor_audience).any? {|audience| audience != instructor_audience} |
| all_family_courses = Script.get_family_from_cache(family_name) | ||
| end | ||
|
|
||
| all_family_courses |
There was a problem hiding this comment.
the additions to this file look much easier to follow now, thanks for updating!
| if is_a?(UnitGroup) || (is_a?(Script) && unit_group) | ||
| all_family_courses = UnitGroup.all.select {|c| c.family_name == family_name} | ||
| elsif is_a?(Script) | ||
| all_family_courses = Script.get_family_from_cache(family_name) |
There was a problem hiding this comment.
this section looks hard to move onto get_course_version -- I wouldn't worry about this for now, and we can tackle it when we move all Scripts onto UnitGroup.
|
|
||
| # Get the family name for the course based on if its set on the UnitGroup or Unit | ||
| def get_course_family_name | ||
| is_a?(Script) && unit_group ? unit_group.family_name : family_name |
There was a problem hiding this comment.
per your top-level comment, you could do this more generally as follows:
| is_a?(Script) && unit_group ? unit_group.family_name : family_name | |
| get_course_version&.family_name |
There was a problem hiding this comment.
apologies @dmcavoy but I'm going to backtrack and suggest that you punt on this for now -- this will make more activerecord calls, which could turn up as extra queries... and you've already had to deal with enough of that in this PR. practically speaking this is just as good, assuming that we are moving all Scripts onto UnitGroups some time soon.
This is a prep step for moving unit redirection to work for all participants instead of just students. In order to be able to redirect correctly we really need to guarantee that all CourseVersions in the same CourseOffering have the same instruction_type, participant_audience, and instructor_audience. This PR adds validation to check that that case is true.
Notes on a couple choices I made:
Links
Eng Plan
Product Spec
Testing story