Levelbuilders can edit courses #15167
Conversation
| i18n = File.exist?(courses_yml) ? YAML.load_file(courses_yml) : {} | ||
|
|
||
| i18n.deep_merge!({'en' => {'data' => {'course' => {'name' => {name => course_strings.to_h}}}}}) | ||
| File.write(courses_yml, "# Autogenerated scripts locale file.\n" + i18n.to_yaml(line_width: -1)) |
There was a problem hiding this comment.
We have this pattern repeated in a number of different models. I wonder if it makes sense to pull it out into a concern or its own model (not ActiveRecord-backed).
I know this is copying over existing code, but this also isn't thread safe.
There was a problem hiding this comment.
I've attempted to clean up some of the existing usages (for scripts.en.yml) in the past, and found it quite challenging. I think there is probably opportunity to make this cleaner, but that it's more work than I'm willing to put in right now.
Non-thread safe is a good point, though again this is already a problem with our existing usages for scripts.en.yml (the good news is we expect courses to be edit even more infrequently than scripts).
dashboard/app/models/course.rb
Outdated
| # @param scripts [Array<String>] - Updated list of names of scripts in this course | ||
| # @param course_strings[Hash{String => String}] | ||
| def update_and_persist(scripts, course_strings) | ||
| Course.update_strings(name, course_strings) |
There was a problem hiding this comment.
Why does this need to be static?
There was a problem hiding this comment.
I made it static because we're updating a file (courses.en.yml) that is shared amongst all courses.
There was a problem hiding this comment.
I'm not sure I follow. Making it static doesn't provide any additional thread safety. As an instance method you don't need to pass in name. No strong preference here, just trying to understand.
There was a problem hiding this comment.
Yeah, I wasn't doing it for thread safety. My intent was conceptual clarity. Strings for a course are all stored in a single file, so conceptually it made sense for me to have string persistence be a static method on the Course model rather than a method on the course instance.
dashboard/app/models/course.rb
Outdated
| # the set of scripts that are in the course, then writes out our serialization | ||
| # @param scripts [Array<String>] - Updated list of names of scripts in this course | ||
| # @param course_strings[Hash{String => String}] | ||
| def update_and_persist(scripts, course_strings) |
There was a problem hiding this comment.
Not immediately clear that this will mutate scripts. I don't think there's too much harm, but a bit of a weird side effect.
There was a problem hiding this comment.
Maybe name it more clearly, something like update_and_persist_scripts?
| @@ -43,7 +43,25 @@ def create | |||
| end | |||
| end | |||
|
|
|||
| def update | |||
| course = Course.find_by_name!(params[:course_name]) | |||
| course.update_and_persist(params[:scripts], i18n_params) | |||
There was a problem hiding this comment.
A pattern we use elsewhere is to have an after_save :write_serialization vs. calling the serialization explicitly.
There was a problem hiding this comment.
That works in places like level, because the serialization is captured entirely in the level object. Course (and script) are weird in that in addition to serialization to course_name.course, we also need to save our localizeable data to a yml file.
We could save our localizeable data explicitly, and then have it do the serialization to course_name.course in an after_save.
There was a problem hiding this comment.
Note that you can have multiple after_save methods in a model.
dashboard/test/models/course_test.rb
Outdated
| name: 'this-course', | ||
| script_names: ['script1', 'script2'] | ||
| }.to_json | ||
| # File.any_instance.stub(:read) { serialization } |
dashboard/app/models/course.rb
Outdated
| def update_and_persist(scripts, course_strings) | ||
| Course.update_strings(name, course_strings) | ||
| update_scripts(scripts) | ||
| save |
There was a problem hiding this comment.
Do you want the non-bang save here? If so, add a @return to the function doc, explaining the save may fail.
There was a problem hiding this comment.
I think I probably want the bang save.
This PR allows level builders to add localizeable text to courses, as well as to set what scripts are in them. Ideally I'd get this into today's DTP so that we can start creating a couple of courses.
The PR could still use some additional test coverage that I'm going to work on in a follow on PR.