-
Notifications
You must be signed in to change notification settings - Fork 479
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
Validation that courses in family share course type #44062
Merged
Merged
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
91c44f5
trying to add validation to make sure family all has same course type
dmcavoy cf3a484
Add back in line at end of file
dmcavoy 5a112b6
Merge branch 'staging' into family-name-share-course-type
dmcavoy ecd60be
Return if the family name is not set
dmcavoy 44428ab
Get validation working for all things in family have same course type
dmcavoy 46234df
Updates to test for validation
dmcavoy 94f28cd
Us the cache for scripts so we can cut down on queries
dmcavoy a726bf7
Update query count tests
dmcavoy e7afd0f
Merge branch 'staging' into family-name-share-course-type
dmcavoy 848265a
Refactor validation of course type for family name
dmcavoy 47a8e13
Add helper methods for validation
dmcavoy 4ea3146
add tests for course type validation for family
dmcavoy fe205bd
Test the case of one course in the family
dmcavoy 3dd4581
Fix a couple tests
dmcavoy 17627d6
Merge branch 'staging' into family-name-share-course-type
dmcavoy 9d7018c
Remove serialized at from script seed test script_json
dmcavoy f63dd8e
move validate into the objects instead of in the concern
dmcavoy b6f6b3f
move back to using all instead of all_courses because of query count …
dmcavoy e45285c
Merge branch 'staging' into family-name-share-course-type
dmcavoy 9e95f85
Update to set up the cache for some tests
dmcavoy 0145989
Pull out populating cache into test helper
dmcavoy df7ae28
Trying to get tests to pass
dmcavoy 46fbb9c
Remove populate_cache_and disconnect_db from unit_group_test.rb
dmcavoy 80ee695
Just go back to using all instead of all_courses because of tests
dmcavoy b6319a7
Merge branch 'staging' into family-name-share-course-type
dmcavoy 2412c1d
Improve the error messages for course type validation
dmcavoy 626dd32
Improve error message lines
dmcavoy File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,40 @@ module Curriculum::CourseTypes | |
validates :participant_audience, acceptance: {accept: SharedCourseConstants::PARTICIPANT_AUDIENCE.to_h.values, message: 'must be facilitator, teacher, or student'} | ||
|
||
validate :cannot_have_same_audiences | ||
validate :must_have_same_course_type_as_family | ||
end | ||
|
||
# All courses in the same family name must have the save instruction_type, instructor_audience, and participant audience | ||
def must_have_same_course_type_as_family | ||
all_family_courses = get_family_courses | ||
return if all_family_courses.nil_or_empty? | ||
|
||
return unless all_family_courses.length > 1 | ||
errors.add(:instructor_audience, '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(:participant_audience, 'Participant Audience must be the same for all courses in a family.') if all_family_courses.map(&:participant_audience).uniq.length > 1 || all_family_courses.last&.participant_audience != participant_audience | ||
errors.add(:instruction_type, 'Instruction Type must be the same for all courses in a family.') if all_family_courses.map(&:instruction_type).uniq.length > 1 || all_family_courses.last&.instruction_type != instruction_type | ||
end | ||
|
||
# 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 | ||
end | ||
|
||
# If course we are check is a unit_group or a unit that is in a unit_group check the family_name on the UnitGroup. | ||
# If the course is a unit that is not in a unit_group check the unit for the family_name | ||
def get_family_courses | ||
family_name = get_course_family_name | ||
return nil if family_name.nil_or_empty? | ||
|
||
all_family_courses = nil | ||
|
||
if is_a?(UnitGroup) || (is_a?(Script) && unit_group) | ||
all_family_courses = UnitGroup.all_courses.select {|c| c.family_name == family_name} | ||
elsif is_a?(Script) | ||
all_family_courses = Script.get_family_from_cache(family_name) | ||
end | ||
dmcavoy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
all_family_courses | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the additions to this file look much easier to follow now, thanks for updating! |
||
end | ||
|
||
# Instructor and Participant Audience can not be equal unless they are nil | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
per your top-level comment, you could do this more generally as follows:
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.
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.