Seeding for CourseVersions for .script files #35980

uponthesun · 2020-07-24T07:26:18Z

Create, update, and delete CourseVersions during seeding process, based on the presence of is_course true in the .script file. Does not handle .course files yet, but it should work similarly for them.

Future work

Change the key format, for example, from csp-2020 to just 2020, once the new Course model is added.
Seeding for CourseVersions for .course files
Add other properties to the CourseVersion object during seeding as appropriate

Testing story

added unit tests

Reviewer Checklist:

Tests provide adequate coverage
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

…rsion

dmcavoy · 2020-07-24T19:12:05Z

dashboard/test/fixtures/test-script-course-version.script

@@ -0,0 +1,3 @@
+family_name 'xyz'
+version_year '1234'
+is_course true


Is there any benefit to having some lesson groups, lessons and levels in here if this is meant to be used for an integration test?

I'm not sure I see it, even though I'm calling it an "integration test" for lack of better terms, I want to test something relatively specific around seeding and creating CourseVersions. I don't think lesson groups / lessons / levels should affect that. Were you thinking of anything in particular?

No, I didn't have anything specific was just wondering

dmcavoy · 2020-07-24T19:14:07Z

dashboard/app/models/script.rb

@@ -144,6 +144,7 @@ def generate_plc_objects
    project_sharing
    curriculum_umbrella
    tts
+    is_course


Should is_course be a column or a property? It seems like we might want to be able to search for all the scripts that are courses easily? Does content root allow us to do that so we aren't worried about using this?

this field seems like redundant state. how about this instead:

def is_course !!course_version end

Oh, I remember why I did this now. This field is only really needed to determine whether we should create a CourseVersion to associate with this Script in the seeding process. (Looking at presence of version_year doesn't work there because scripts which aren't content roots have those too).

This is the easiest way to get that piece of information from the .script file into the seeding logic. I could explicitly avoid storing it in the DB by leaving it off the properties here, though it'd still have to be included in build_property_hash (or we'd have to refactor how this works.) Not sure if that's any better.

Which do you prefer? Any other ideas?

Good points Winter. I think the redundant state is ok as long as it serves a purpose, which it sounds like it does. So either option seems fine, so in that sense it doesn't seem worth redoing. Another idea (since you asked 😉 ) would be to add a validation enforcing that !!course_version == is_course.

I feel like this validation might cause problems when we update the Script object from the DSL properties, which happens before we would create the corresponding CourseVersion objects.

I could believe there's a good way to refactor things to make this all better but it's not obvious to me right now. I'm open to continuing to iterate on this, but I'll leave this as-is for this PR.

uponthesun · 2020-07-24T19:22:26Z

dashboard/app/models/course_version.rb

@@ -18,4 +18,31 @@

 class CourseVersion < ApplicationRecord
  belongs_to :content_root, polymorphic: true
+
+  def self.update_course_version(content_root)
+    # TODO: Once the Course model is added, change this to just be version_year, since then the


Does this seem right? I was thinking having the family name in the key will be redundant long-term.

I'm wondering why CourseVersion would not be the owner of version year instead of script

I'm wondering why CourseVersion would not be the owner of version year instead of script

IIRC the way we shortened the timeline for this undertaking was to have these properties continue to live on the content root, and as we add new features, we could look for them on the CourseVersion and the CourseVersion could delegate the call to the content root. it would look like this:

class CourseVersion < ApplicationRecord delegate :family_name, to: :content_root

And then instance methods (but not this class method), could just call family_name instead of content_root.family_name.

In the future, we would move the family_name fully into the CourseVersion, in which case I'm not sure how the code below would need to be updated because the content_root here might not have a Course Version yet.

# TODO: Once the Course model is added, change this to just be version_year, since then the
# unique index will be on (course_id, key).

Does this seem right? I was thinking having the family name in the key will be redundant long-term.

This makes sense to me. Once we have created the new Course model, won't it make more sense to have version_year be it's own db field and just index directly on (course_id, version_year), though?

In that option, would we just get rid of key? I was thinking that logically it'd be the same as what you're saying, we'd just call it key.

agreed, I think those two solutions would be equivalent, just with the column named either version_year or key.

uponthesun · 2020-07-24T19:22:44Z

dashboard/app/models/course_version.rb

+      content_root: content_root,
+    )
+
+    # Delete old CourseVersion if the key has been changed to something else


Not sure the right way to handle this, this was my attempt.

dmcavoy · 2020-07-24T19:29:44Z

dashboard/app/models/course_version.rb

+
+    # Delete old CourseVersion if the key has been changed to something else
+    content_root.course_version.destroy if course_version != content_root.course_version
+    content_root.course_version = course_version


If you just do this does it not delete the old course version?

It does not, as far as I can tell when I was testing this out.

Here's what I get when I try that. I think what happens is ActiveRecord just tries to set the CourseVersion object's content_root to nil. So it tries to unassociate it rather than delete it. Not sure if there's some setting or some other approach for getting it to delete instead.

[1] pry(CourseVersion)> content_root => #<Script:0x00007fc0d62287a8 id: 56, name: "test-all-properties", created_at: Tue, 11 Aug 2020 05:02:28 UTC +00:00, updated_at: Tue, 11 Aug 2020 05:02:28 UTC +00:00, wrapup_video_id: nil, hidden: true, user_id: nil, login_required: false, properties: {}, new_name: nil, family_name: nil> [2] pry(CourseVersion)> content_root.course_version => #<CourseVersion:0x00007fc112679960 id: 7, key: "-2020", display_name: "2020", properties: nil, content_root_type: "Script", content_root_id: 56, created_at: Tue, 11 Aug 2020 05:02:28 UTC +00:00, updated_at: Tue, 11 Aug 2020 05:02:28 UTC +00:00> [3] pry(CourseVersion)> content_root.course_version = nil ActiveRecord::StatementInvalid: Mysql2::Error: Column 'content_root_id' cannot be null: UPDATE `course_versions` SET `content_root_id` = NULL, `updated_at` = '2020-08-11 05:02:48' WHERE `course_versions`.`id` = 7

Good question Dani, I was curious about this too. It's not explicitly spelled out in the docs, but it's possible that the dependent: :destroy option is what makes this work the way you're describing in other situations. Here are the parts of the docs which seem relevant (this is a has_one and is also polymorphic)

https://guides.rubyonrails.org/association_basics.html#options-for-has-one-dependent These docs mention destroying the associated object when the parent is deleted, but does not explicitly say the same will happen upon assignment.

https://guides.rubyonrails.org/association_basics.html#polymorphic-associations doesn't mention destroying so not very helpful

By contrast, the docs for for has_many ... dependent: :destroy are more explicit:

https://guides.rubyonrails.org/association_basics.html#dependent

The :destroy and :delete_all options also affect the semantics of the collection.delete and collection= methods by causing them to destroy associated objects when they are removed from the collection.

dmcavoy · 2020-07-24T19:32:48Z

dashboard/app/models/course_version.rb

+    # unique index will be on (course_id, key).
+    key = "#{content_root.family_name}-#{content_root.version_year}"
+
+    unless content_root.is_course?


Is this here if someone were to change is_course from true to false? Maybe a comment explaining that would be helpful. Also again wondering does do the content_root.course_version = delete the old object and if so could we just set that to nil for this case?

Maybe it is just too early in the morning (😆 ) but I think I am getting confused about what is_course means. In a script file, it made sense to me as "this is a script which represents an entire course". what would it mean for unit_group.is_course? to be true?

Separately, I don't see where unit_group.is_course? would be defined so it looks like this might not be working when the content root is a unit group. Am I missing something?

Yea, I haven't added that case yet (listed under future work). I think the implementation there should just be adding an is_course method to the UnitGroup class which always returns true, since AFAIK every unit group we have right now corresponds to a top level CourseVersion.

Do you think this makes sense? Is there a clearer way to do this?

I think that makes sense, but in that case i think what's missing here is an "interface" defining the methods and contracts that unit_group and script are both required to implement in order to be able to be a content root. I can't remember what we found earlier about whether this is possible to accomplish in ruby, but if it's not possible to truly implement an interface then some comments on the content_root association describing the interface could be an acceptable substitute.

I added comments to document the "Interface" as it is so far in course_version.rb. I can explore the unit test based "interface" enforcement when I work on adding seeding for .course files.

davidsbailey

Sorry for the long delay on reviewing this!

davidsbailey · 2020-07-29T23:16:37Z

dashboard/app/models/course_version.rb

@@ -18,4 +18,31 @@

 class CourseVersion < ApplicationRecord
  belongs_to :content_root, polymorphic: true
+
+  def self.update_course_version(content_root)
+    # TODO: Once the Course model is added, change this to just be version_year, since then the


I'm wondering why CourseVersion would not be the owner of version year instead of script

IIRC the way we shortened the timeline for this undertaking was to have these properties continue to live on the content root, and as we add new features, we could look for them on the CourseVersion and the CourseVersion could delegate the call to the content root. it would look like this:

class CourseVersion < ApplicationRecord delegate :family_name, to: :content_root

And then instance methods (but not this class method), could just call family_name instead of content_root.family_name.

In the future, we would move the family_name fully into the CourseVersion, in which case I'm not sure how the code below would need to be updated because the content_root here might not have a Course Version yet.

davidsbailey · 2020-07-29T23:30:56Z

dashboard/app/models/course_version.rb

+    content_root.course_version.destroy if course_version != content_root.course_version
+    content_root.course_version = course_version
+
+    # TODO: add relevant properties from content root to course_version


What does this mean exactly -- are we talking about the "delegate" syntax from earlier?

For the short term, yes, and for the long term, to move the properties to actually be stored on this model.

davidsbailey · 2020-07-29T23:34:41Z

dashboard/app/models/script.rb

@@ -144,6 +144,7 @@ def generate_plc_objects
    project_sharing
    curriculum_umbrella
    tts
+    is_course


this field seems like redundant state. how about this instead:

def is_course !!course_version end

davidsbailey · 2020-07-29T23:36:26Z

dashboard/app/models/course_version.rb

@@ -18,4 +18,31 @@

 class CourseVersion < ApplicationRecord
  belongs_to :content_root, polymorphic: true
+
+  def self.update_course_version(content_root)


Can you please add some comments on this method saying what it does?

davidsbailey · 2020-07-29T23:45:49Z

dashboard/app/models/course_version.rb

@@ -18,4 +18,31 @@

 class CourseVersion < ApplicationRecord
  belongs_to :content_root, polymorphic: true
+
+  def self.update_course_version(content_root)
+    # TODO: Once the Course model is added, change this to just be version_year, since then the


# TODO: Once the Course model is added, change this to just be version_year, since then the
# unique index will be on (course_id, key).

Does this seem right? I was thinking having the family name in the key will be redundant long-term.

This makes sense to me. Once we have created the new Course model, won't it make more sense to have version_year be it's own db field and just index directly on (course_id, version_year), though?

davidsbailey · 2020-08-11T16:00:25Z

dashboard/app/models/script.rb

@@ -144,6 +144,7 @@ def generate_plc_objects
    project_sharing
    curriculum_umbrella
    tts
+    is_course


Good points Winter. I think the redundant state is ok as long as it serves a purpose, which it sounds like it does. So either option seems fine, so in that sense it doesn't seem worth redoing. Another idea (since you asked 😉 ) would be to add a validation enforcing that !!course_version == is_course.

davidsbailey · 2020-08-11T16:07:23Z

dashboard/app/models/script.rb

@@ -919,7 +920,7 @@ def self.setup(custom_files, new_suffix: nil, new_properties: {})
      added_scripts = scripts_to_add.sort_by.with_index {|args, idx| [args[0][:id] || Float::INFINITY, idx]}.map do |options, raw_lesson_groups|
        add_script(options, raw_lesson_groups, new_suffix: new_suffix, editor_experiment: new_properties[:editor_experiment])
      rescue => e
-        raise e, "Error adding script named '#{options[:name]}': #{e}"
+        raise e, "Error adding script named '#{options[:name]}': #{e}", e.backtrace


for my own information, did this end up being helpful? based on the PR description where I added this, it looks like I was getting the backtraces already: #35660 but I'm not clear based on my output whether some of it just became part of the "message" part of the exception.

I posted my before and after above, not sure why I was getting a different result from you. I can dig into this a bit more later.

davidsbailey · 2020-08-11T16:17:56Z

dashboard/app/models/course_version.rb

+    # unique index will be on (course_id, key).
+    key = "#{content_root.family_name}-#{content_root.version_year}"
+
+    unless content_root.is_course?


Maybe it is just too early in the morning (😆 ) but I think I am getting confused about what is_course means. In a script file, it made sense to me as "this is a script which represents an entire course". what would it mean for unit_group.is_course? to be true?

Separately, I don't see where unit_group.is_course? would be defined so it looks like this might not be working when the content root is a unit group. Am I missing something?

davidsbailey · 2020-08-11T16:45:19Z

dashboard/app/models/course_version.rb

+
+    # Delete old CourseVersion if the key has been changed to something else
+    content_root.course_version.destroy if course_version != content_root.course_version
+    content_root.course_version = course_version


Good question Dani, I was curious about this too. It's not explicitly spelled out in the docs, but it's possible that the dependent: :destroy option is what makes this work the way you're describing in other situations. Here are the parts of the docs which seem relevant (this is a has_one and is also polymorphic)

https://guides.rubyonrails.org/association_basics.html#options-for-has-one-dependent These docs mention destroying the associated object when the parent is deleted, but does not explicitly say the same will happen upon assignment.

https://guides.rubyonrails.org/association_basics.html#polymorphic-associations doesn't mention destroying so not very helpful

By contrast, the docs for for has_many ... dependent: :destroy are more explicit:

https://guides.rubyonrails.org/association_basics.html#dependent

The :destroy and :delete_all options also affect the semantics of the collection.delete and collection= methods by causing them to destroy associated objects when they are removed from the collection.

davidsbailey

Great work, @uponthesun !

davidsbailey · 2020-08-11T23:08:58Z

dashboard/config/scripts/valentine.script

+version_year 2019
+is_course true


davidsbailey · 2020-08-11T23:13:24Z

dashboard/test/models/course_version_test.rb

+    assert_nil CourseVersion.find_by(key: 'csz-2050')
+  end
+
+  test "update_course_version does nothing for script without CourseVersion if is_course is false" do


great test cases, especially the descriptions! these really helped me understand the expected behavior.

dmcavoy · 2020-08-12T00:15:25Z

dashboard/test/models/course_version_test.rb

+    assert_equal '1234', course_version.display_name
+  end
+
+  test "update_course_version creates CourseVersion for script that doesn't have one if is_course is true" do


it looks like you updated the name of update_course_version method so should it get updated in the name of the tests?

uponthesun · 2020-08-17T06:42:59Z

@davidsbailey

yeah, it seems like is_course? doesn't need to be in the interface, doesn't need to be called by CourseVersion, and the Script model could call this something more specific like is_single_unit_course or something. It's up to you whether to tackle that now or not. If the call here to is_course? can be safely removed, then it does kind of seem worth doing. I could understand keeping it for now if we're not 100% confident on that, though.

Meanwhile, I would expect this interface documentation comment to at least include the fields (and, if applicable, methods) on content_root which you call from this class. Right now that looks like it should include: name, family_name, version_year. Can you please add those to these comments? They don't need to be long.

I'll clean this up and add more documentation once I add the Course model and seeding.

Winter Dong added 5 commits July 22, 2020 23:31

Add is_course to Script DSL and Script model

bdbe3cd

Fix serialization output + add unit test on serialization

c803ece

Add basic logic for adding course version

dfc3dda

Add unit tests on update_course_version

6018a15

Add integration test on seeding from .script file creating a CourseVe…

104b60a

…rsion

uponthesun requested review from dmcavoy and davidsbailey July 24, 2020 07:26

dmcavoy reviewed Jul 24, 2020

View reviewed changes

uponthesun commented Jul 24, 2020

View reviewed changes

dmcavoy reviewed Jul 24, 2020

View reviewed changes

davidsbailey reviewed Jul 29, 2020

View reviewed changes

Winter Dong added 6 commits August 10, 2020 21:32

Add comment on deleting CourseVersion

fde080f

Rename method + add comment

20338df

Preserve original backtrace when handling errors in add_script

83e4d16

Simplify logic for deleting CourseVersion when is_course is set to false

07c0bbb

Add validation on family name and version_year being set

995d9a0

Fix validation + unit test

3929549

uponthesun requested review from dmcavoy and davidsbailey August 11, 2020 06:30

davidsbailey reviewed Aug 11, 2020

View reviewed changes

davidsbailey approved these changes Aug 11, 2020

View reviewed changes

dmcavoy reviewed Aug 12, 2020

View reviewed changes

dmcavoy approved these changes Aug 12, 2020

View reviewed changes

Winter Dong added 3 commits August 12, 2020 21:43

Revert accidental changes to valentine.script

2a86e3b

Document the content_root interface

290055e

Fix method name in unit tests

bae069c

uponthesun merged commit 5cf1f2a into staging Aug 13, 2020

uponthesun deleted the course-version-seeding branch August 13, 2020 17:14

uponthesun mentioned this pull request Aug 17, 2020

Set is_course true for CSF script files #36300

Merged

7 tasks

Seeding for CourseVersions for .script files #35980

Seeding for CourseVersions for .script files #35980

Conversation

uponthesun commented Jul 24, 2020

Future work

Testing story

Reviewer Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcavoy Jul 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uponthesun Jul 24, 2020 • edited

Choose a reason for hiding this comment

dmcavoy Jul 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uponthesun commented Aug 17, 2020

dmcavoy Jul 24, 2020 •

edited

uponthesun Jul 24, 2020 •

edited

dmcavoy Jul 24, 2020 •

edited