Update add_script and Curriculum Model Relationships

[dmcavoy]

Background

As we added lesson groups between scripts and lessons we created the possibility of two sources of truth for the position of a lesson. In looking to improve the positions values in lessons in order to get rid of the possibility of two sources of truth we realized that we would need to update the add_script method which was adding lessons by script.lessons =. Instead we needed to add lessons to their lesson groups and have script access lessons through lesson groups.

Overview

This breaks up the old add_script method which is used when you create a new script or when you seed scripts into multiple methods, one for each object type.

In addition is updates the associations between objects in the curriculum data model. Script has many lessons through lesson_groups and Script has many script_levels through lessons.

When I ran bundle exec rake build it came up with an error for ui-test-course.course because it has no scripts in it (I created this when I was testing something on levelbuilder). When I deleted the file it passed so I have deleted the file here. Is that alright? Will I need to go into each environment to clean this up? The same issue seems to be happening on my staging branch.

Future work

In the future we will use this to update position values for lesson and script_level.

Links

• jira

Testing story

• Made sure all tests passed (Updated a couple tests as needed. See comments in files)
• Ran bundle exec rake build locally to make sure that it worked and no changes resulted from it
• Edited a script locally and saved

Should I create tests for each add_blah method that I created or is the testing on add_script enough?

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

[dmcavoy]

Missed one test that needed lesson group added to it.

[dmcavoy]

I would love for someone to help me confirm that this update is ok but I think its because of the changes to the associations to the through relationships.

[davidsbailey]

this seems fine -- I wouldn't want to block a large refactor like this on an added query to a non-critical codepath.

[dmcavoy]

Moved where the check for this is so we don't have the lesson information as easily.

[dmcavoy]

Updated the wording on this error to make it more clear.

[dmcavoy]

Needed to save here in order for the curriculum_path to take effect.

[davidsbailey]

Hey Dani, this looks like great progress. One top-level comment I want to share is that I think this gets you suuuuuper close to being able to just use the new associations to do the ordering properly, so if you want to just finish this off and finish the "fix positioning" task, I think that option should definitely be on the table. maybe we can check in about what's left to do there once this PR is over the hump.

[dmcavoy]

The UI tests were failing because I was incorrectly setting chapter values for script levels. Since this was not caught by a unit test I added a new unit test in script_test.rb to check that chapter and position values of script levels are correctly set by add_script. I tested it before and after the change to fix the chapter values and it failed before and passed after :)

[uponthesun]

Looks really good overall!

My main concern is making sure there's no unexpected behavior changes from this refactor, especially since we had a bug that was only caught by UI tests just now. I have the impression @islemaster has some process to verify big seeding changes like this. I thought he had a way to verify the results of seeding everything, before and after a change. Can you check with him on that before merging?

[uponthesun]

This pattern of passing in all the position counters (lockable_count, non_lockable_count, lesson_position, position, chapter) into the lower-level methods, and then afterwards figuring out how to update them in the higher-level methods, seems a bit redundant and fragile to me. The lower-level methods already incremented the values locally so ideally we shouldn't repeat that work somewhere else.

One option I can think of is to create a single "Counters" Struct to hold these values, and to pass that object into each of the lower-level methods instead of passing them all separately. Then, when the lower-level methods increment the counters, those changes will automatically be reflected in the higher-level methods already, since everything is sharing that one "Counters" object.

This would also have the side benefit of shortening the parameter lists for some of these methods and making them more readable.

[dmcavoy]

I will follow up with another PR to do this but I want to get these changes in first.

[dmcavoy]

https://codedotorg.atlassian.net/browse/PLAT-252

[islemaster]

I have the impression @islemaster has some process to verify big seeding changes like this.

Are you thinking of this process? The general approach was to dump the contents of relevant seeded tables before and after the code change, using a difftool to prove that the outcome of the process was the same.

If I understand this change correctly, we expect the outcome (at least in terms of database rows) to be different. You could probably do something similar to verify that only the expected change occurs, but it'll take some thinking.

[davidsbailey]

a few more miscellaneous comments. this is shaping up really well!

[davidsbailey]

these associations look great!

[davidsbailey]

now that you've got tests passing, is it possible to remove this reload without breaking anything?

[dmcavoy]

no unfortunately the tests don't pass if I remove it

[davidsbailey]

If I understand this change correctly, we expect the outcome (at least in terms of database rows) to be different. You could probably do something similar to verify that only the expected change occurs, but it'll take some thinking.

Good point Brad. Since we are touching "seed" but not "serialize", perhaps this could be tested manually by going into a few script edit pages in levelbuilder and saving them, and confirming that no differences appear in the .script files?

[uponthesun]

If I understand this change correctly, we expect the outcome (at least in terms of database rows) to be different. You could probably do something similar to verify that only the expected change occurs, but it'll take some thinking.

Good point Brad. Since we are touching "seed" but not "serialize", perhaps this could be tested manually by going into a few script edit pages in levelbuilder and saving them, and confirming that no differences appear in the .script files?

I thought this was just a refactor of the seeding logic which preserved the same behavior? What are some of the changes we expect?

[dmcavoy]

If I understand this change correctly, we expect the outcome (at least in terms of database rows) to be different. You could probably do something similar to verify that only the expected change occurs, but it'll take some thinking.

Good point Brad. Since we are touching "seed" but not "serialize", perhaps this could be tested manually by going into a few script edit pages in levelbuilder and saving them, and confirming that no differences appear in the .script files?

I thought this was just a refactor of the seeding logic which preserved the same behavior? What are some of the changes we expect?

I don't expect any changes. It should be the same output.

[uponthesun]

I don't expect any changes. It should be the same output.

Great. @islemaster, do you have a sense of how much work it'd take to adapt what you did for custom levels to this change?

From my understanding of it, it seems like the main differences would be:

1. how we set up the db state before we start the part of the seed we're trying to verify
2. the sql queries used to dump the contents of the relevant tables for verification
3. It'd probably take a lot more wall clock time since we'd be seeding all scripts

[islemaster]

@uponthesun the differences you list sound exactly right. The db setup may require a few extra seed steps to support script seeding, and you might want to dump different tables to verify this change, omitting columns like id, created_at and updated_at that you'd expect to change between seeding runs. I can't think of anything else you'd need to change!

[dmcavoy]

If I understand this change correctly, we expect the outcome (at least in terms of database rows) to be different. You could probably do something similar to verify that only the expected change occurs, but it'll take some thinking.

Good point Brad. Since we are touching "seed" but not "serialize", perhaps this could be tested manually by going into a few script edit pages in levelbuilder and saving them, and confirming that no differences appear in the .script files?

Tested allthethings and csd2-2019 scripts and I get the same changes on staging that I get in staging. I checked coursef-2020 and csp9-2020 and both had no changes to the script files.

[davidsbailey]

Tested allthethings and csd2-2019 scripts and I get the same changes on staging that I get in staging. I checked coursef-2020 and csp9-2020 and both had no changes to the script files.

Great! I did misunderstand when I suggested this, thinking that there might be some expected DB content differences. but this is still good evidence that the PR isn't breaking anything in those scripts.

[dmcavoy]

Verifying correctness

1. Pulled current staging

2. bundle exec rake seed:scripts

3. mysql dashboard_development -u root -e 'select id, name, wrapup_video_id, hidden, user_id, login_required, properties, n_name, family_name from scripts; select id,key, script_id, user_facing, position, properties from lesson_groups; select id, name, absolute_position, script_id, lockable, relative_position, properties, lesson_group_id from stages; select id, script_id, chapter, stage_id, position, assessment, properties, named_level, bonus from script_levels; select id, game_id, name, level_num, ideal_level_source_id, user_id, properties, type, md5, published, notes, audit_log from levels;' > ~/sql_dump_before.txt

4. Check out feature branch

5. Merge current staging

6. bundle exec rake seed:scripts

7. mysql dashboard_development -u root -e 'select id, name, wrapup_video_id, hidden, user_id, login_required, properties, n_name, family_name from scripts; select id,key, script_id, user_facing, position, properties from lesson_groups; select id, name, absolute_position, script_id, lockable, relative_position, properties, lesson_group_id from stages; select id, script_id, chapter, stage_id, position, assessment, properties, named_level, bonus from script_levels; select id, game_id, name, level_num, ideal_level_source_id, user_id, properties, type, md5, published, notes, audit_log from levels;' > ~/sql_dump_after.txt

8. diff ~/sql_dump_before.txt ~/sql_dump_after.txt

Result:
5271a5272,5274

4382 Algorithms Detour - Minimum Spanning Tree 8 269 0 8 {} 2171
4383 Algorithms Detour - Shortest Path 9 269 0 9 {} 2171
4384 Algorithms Detour - How Routers Learn 10 269 0 10 {} 2171
53438d53440
< 22671 43 - CSD U3 Card Examples_2018_2019_pilot_2020 NULL NULL NULL {"options":{"skip_dialog":true},"display_name":"Making an Interactive Card","title":"Making an Interactive Card","content1":"description here","markdown":"# Your Interactive Card\n\nIn the next few levels, you'll be completing your own interactive card. Here are some examples to give you some ideas. Don't forget to look at the code to see how they work.\n\n## Examples\n\n\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\u0026nbsp;\n"} External NULL 0 NULL NULL

Fixed a couple files see commit d4108ae. After those changes there were no diffs. I'm also going to add in a check to prevent empty lessons getting added as the script overview page does not like empty lessons.

[uponthesun]

🎉 Thanks for doing the additional verification!

[davidsbailey]