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
Merged

Add is_stable to course #28275

merged 8 commits into from Apr 30, 2019

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Apr 29, 2019

Adds the is_stable property to the Course model, and marks the following courses as stable:

  • csd-2017
  • csd-2018
  • csp-2017
  • csp-2018

@maddiedierker maddiedierker marked this pull request as ready for review April 29, 2019 23:29
Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

Great tests!

One small question, otherwise LGTM!

# 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
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.

@maddiedierker maddiedierker changed the title Add course stable Add is_stable to course Apr 29, 2019
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Glad to see this happen!

@@ -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

@maddiedierker maddiedierker merged commit c9e1b88 into staging Apr 30, 2019
@maddiedierker maddiedierker deleted the add-course-stable branch April 30, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants