Skip to content
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 27 commits into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from 8 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 Dec 15, 2021
cf3a484
Add back in line at end of file
dmcavoy Dec 16, 2021
5a112b6
Merge branch 'staging' into family-name-share-course-type
dmcavoy Dec 16, 2021
ecd60be
Return if the family name is not set
dmcavoy Dec 16, 2021
44428ab
Get validation working for all things in family have same course type
dmcavoy Dec 16, 2021
46234df
Updates to test for validation
dmcavoy Dec 16, 2021
94f28cd
Us the cache for scripts so we can cut down on queries
dmcavoy Dec 16, 2021
a726bf7
Update query count tests
dmcavoy Dec 16, 2021
e7afd0f
Merge branch 'staging' into family-name-share-course-type
dmcavoy Dec 17, 2021
848265a
Refactor validation of course type for family name
dmcavoy Dec 17, 2021
47a8e13
Add helper methods for validation
dmcavoy Dec 17, 2021
4ea3146
add tests for course type validation for family
dmcavoy Dec 17, 2021
fe205bd
Test the case of one course in the family
dmcavoy Dec 17, 2021
3dd4581
Fix a couple tests
dmcavoy Dec 17, 2021
17627d6
Merge branch 'staging' into family-name-share-course-type
dmcavoy Jan 4, 2022
9d7018c
Remove serialized at from script seed test script_json
dmcavoy Jan 5, 2022
f63dd8e
move validate into the objects instead of in the concern
dmcavoy Jan 5, 2022
b6f6b3f
move back to using all instead of all_courses because of query count …
dmcavoy Jan 6, 2022
e45285c
Merge branch 'staging' into family-name-share-course-type
dmcavoy Jan 6, 2022
9e95f85
Update to set up the cache for some tests
dmcavoy Jan 6, 2022
0145989
Pull out populating cache into test helper
dmcavoy Jan 6, 2022
df7ae28
Trying to get tests to pass
dmcavoy Jan 7, 2022
46fbb9c
Remove populate_cache_and disconnect_db from unit_group_test.rb
dmcavoy Jan 8, 2022
80ee695
Just go back to using all instead of all_courses because of tests
dmcavoy Jan 8, 2022
b6319a7
Merge branch 'staging' into family-name-share-course-type
dmcavoy Jan 8, 2022
2412c1d
Improve the error messages for course type validation
dmcavoy Jan 10, 2022
626dd32
Improve error message lines
dmcavoy Jan 13, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions dashboard/app/models/concerns/curriculum/course_types.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,28 @@ 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 = nil
family_name = self.family_name

if (is_a?(Script) && unit_group) || is_a?(UnitGroup)
family_name = unit_group.family_name if is_a?(Script) && unit_group
return if family_name.nil_or_empty?
all_family_courses = UnitGroup.all.select {|c| c.family_name == family_name}
dmcavoy marked this conversation as resolved.
Show resolved Hide resolved
elsif is_a?(Script)
return if family_name.nil_or_empty?
all_family_courses = Script.get_family_from_cache(family_name)
end
dmcavoy marked this conversation as resolved.
Show resolved Hide resolved

if all_family_courses
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
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
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
end
end

# Instructor and Participant Audience can not be equal unless they are nil
Expand Down
2 changes: 1 addition & 1 deletion dashboard/test/controllers/scripts_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ class ScriptsControllerTest < ActionController::TestCase
is_course: 'on',
peer_reviews_to_complete: 1,
curriculum_path: 'fake_curriculum_path',
family_name: 'coursea',
family_name: 'fake-family-z',
version_year: '2020',
pilot_experiment: 'fake-pilot-experiment',
editor_experiment: 'fake-editor-experiment',
Expand Down
2 changes: 1 addition & 1 deletion dashboard/test/integration/db_query_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def setup
student.assign_script(script)
sign_in student

assert_cached_queries(6) do
assert_cached_queries(7) do
get "/s/#{script.name}"
assert_response :success
end
Expand Down
3 changes: 2 additions & 1 deletion dashboard/test/lib/services/script_seed_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,13 @@ class ScriptSeedTest < ActiveSupport::TestCase
# 1 query to get all the programming environments
# 1 query to get all the standards frameworks
# 1 query to check for a CourseOffering. (Would be a few more if is_course was true)
# 1 query to check if units in family have the same course type settings
# LevelsScriptLevels has queries which scale linearly with the number of rows.
# As far as I know, to get rid of those queries per row, we'd need to load all Levels into memory. I think
# this is slower for most individual Scripts, but there could be a savings when seeding multiple Scripts.
# For now, leaving this as a potential future optimization, since it seems to be reasonably fast as is.
# The game queries can probably be avoided with a little work, though they only apply for Blockly levels.
assert_queries(86) do
assert_queries(87) do
ScriptSeed.seed_from_json(json)
end

Expand Down