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

Add is_stable to course #28275

Merged
merged 8 commits into from Apr 30, 2019
16 changes: 11 additions & 5 deletions dashboard/app/models/course.rb
Expand Up @@ -37,6 +37,7 @@ def skip_name_format_validation
has_verified_resources
family_name
version_year
is_stable
)

def to_param
Expand All @@ -55,6 +56,14 @@ def localized_version_title
I18n.t("data.course.name.#{name}.version_title", default: version_year)
end

# Any course with a plc_course or no family_name is considered stable.
# All other courses must specify an is_stable boolean property.
def stable_or_default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a comment of mine disappeared -- this name seems confusing because it is redundant, because "default" means "latest stable". Can we call it stable? instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, good call! i'll update the naming now

return true if plc_course || !family_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we consider version_year here at all, or is there an assumption that family_name ⇒ version_year?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great question. so far from what i understand, we've been going on that assumption, but @davidsbailey might have more insight here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depending on family_name seems fine for now. currently, the presence of family_name implies the presence of version year in the two courses that have family names, but there's nothing programmatically enforcing this. I'd say, go ahead with this for now and maybe later we can add some checks to enforce any assumptions.


is_stable || false
end

def self.file_path(name)
Rails.root.join("config/courses/#{name}.course")
end
Expand Down Expand Up @@ -188,8 +197,7 @@ def assignable_info(user = nil)
info[:assignment_family_title] = localized_assignment_family_title
info[:version_year] = version_year || ScriptConstants::DEFAULT_VERSION_YEAR
info[:version_title] = localized_version_title
# For now, all course versions visible in the UI are stable.
info[:is_stable] = true
info[:is_stable] = stable_or_default
info[:category] = I18n.t('courses_category')
info[:script_ids] = user ?
scripts_for_user(user).map(&:id) :
Expand Down Expand Up @@ -296,9 +304,7 @@ def summarize_versions(user = nil)
version_year: c.version_year,
version_title: c.localized_version_title,
can_view_version: c.can_view_version?(user),
# TODO: (madelynkasula) Update is_stable to no longer be hard-coded once
# properties[:is_stable] is implemented for courses.
is_stable: true
is_stable: stable_or_default
}
end

Expand Down
3 changes: 2 additions & 1 deletion dashboard/config/courses/csd-2017.course
Expand Up @@ -21,6 +21,7 @@
],
"has_verified_resources": true,
"family_name": "csd",
"version_year": "2017"
"version_year": "2017",
"is_stable": true
}
}
3 changes: 2 additions & 1 deletion dashboard/config/courses/csd-2018.course
Expand Up @@ -21,6 +21,7 @@
],
"has_verified_resources": true,
"family_name": "csd",
"version_year": "2018"
"version_year": "2018",
"is_stable": true
}
}
3 changes: 2 additions & 1 deletion dashboard/config/courses/csp-2017.course
Expand Up @@ -40,6 +40,7 @@
],
"has_verified_resources": true,
"family_name": "csp",
"version_year": "2017"
"version_year": "2017",
"is_stable": true
}
}
3 changes: 2 additions & 1 deletion dashboard/config/courses/csp-2018.course
Expand Up @@ -23,6 +23,7 @@
],
"has_verified_resources": true,
"family_name": "csp",
"version_year": "2018"
"version_year": "2018",
"is_stable": true
}
}
26 changes: 25 additions & 1 deletion dashboard/test/models/course_test.rb
Expand Up @@ -61,7 +61,7 @@ class NameValidationTests < ActiveSupport::TestCase
end

test "should serialize to json" do
course = create(:course, name: 'my-course')
course = create(:course, name: 'my-course', is_stable: true)
create(:course_script, course: course, position: 1, script: create(:script, name: "script1"))
create(:course_script, course: course, position: 2, script: create(:script, name: "script2"))
create(:course_script, course: course, position: 3, script: create(:script, name: "script3"))
Expand All @@ -71,6 +71,30 @@ class NameValidationTests < ActiveSupport::TestCase
obj = JSON.parse(serialization)
assert_equal 'my-course', obj['name']
assert_equal ['script1', 'script2', 'script3'], obj['script_names']
assert obj['properties']['is_stable']
end

test "stable_or_default: true if course has plc_course" do
course = Course.new(family_name: 'plc')
course.plc_course = Plc::Course.new(course: course)
course.save

assert course.stable_or_default
end

test "stable_or_default: true if course is not in a family" do
course = create :course
assert course.stable_or_default
end

test "stable_or_default: true if course in family has is_stable set" do
course = create :course, family_name: 'csd', is_stable: true
assert course.stable_or_default
end

test "stable_or_default: defaults to false if course in family does not have is_stable set" do
course = create :course, family_name: 'csd'
refute course.stable_or_default
end

class UpdateScriptsTests < ActiveSupport::TestCase
Expand Down