Parametrize import script by model and add vocab import logic #38486
Conversation
bin/oneoff/import_unit_details.rb
Outdated
| Example: runner.rb -l -u csp2-2021,csp3-2021,csp4-2021 | ||
| Example: runner.rb -u coursea-2021 -m LessonGroup,Lesson,Activity,Resource,Objective,Vocabulary | ||
| Example: runner.rb -l -u csd1-2021 -m Activity,Resource,Objective | ||
| Example: runner.rb -l -u csp2-2021,csp3-2021,csp4-2021 -m Lesson |
There was a problem hiding this comment.
I just noticed that these all say runner.rb. could use #{$0} to get the right command name?
lib/cdo/lesson_import_helper.rb
Outdated
| raise unless [:development, :adhoc, :levelbuilder].include? rack_env | ||
| raise unless lesson.script.hidden | ||
|
|
||
| raise "Must specify models to import" if models_to_import.blank? |
There was a problem hiding this comment.
this seems like an odd place for this check -- for example, if I do not specify any models, I hit this error. but if I specify -m LessonGroup, this check will pass, even though no lesson-related models are being imported. how about moving it up to the main import_unit_details.rb script?
| lesson.assessment_opportunities = cb_lesson_data['assessment'] unless cb_lesson_data['assessment'].blank? | ||
| lesson.objectives = cb_lesson_data['objectives'].map do |o| | ||
| Objective.new(key: SecureRandom.uuid, description: o["name"]) | ||
| if models_to_import.include?('Lesson') |
There was a problem hiding this comment.
nit: this is just my opinion, but I think it would be a little easier to read if you did the if models_to_import.include?('Lesson') as the outer conditional, and if cb_lesson_data.empty? as the inner one.
There was a problem hiding this comment.
That makes a lot of sense! I'll rearrange
There was a problem hiding this comment.
Oh actually, this doesn't work. Lockable lessons cause other blocks to crash because the entry in the hash doesn't exist. We shouldn't be trying to import anything other than lesson information for lockable lessons so I'm going to switch this back (mostly)
There was a problem hiding this comment.
ohh, I see. I don't know if I have just become totally allergic to nested ifs, but if you are on board with it another option would be to do an early return from this method if cb_lesson_data.empty?. otherwise how you have it is fine. "as simple as possible, but not simpler". :-)
There was a problem hiding this comment.
Haha I went back and forth between an early return and the way it is now. They both feel kind of bad to me so I'm not particularly attached to either
There was a problem hiding this comment.
as the PR author, you have the burden of deciding :-)
There was a problem hiding this comment.
I'm going to leave as-is then. It's very slightly clearer this way to me
First part of PLAT-359 (the second part is the actual import). In order to import vocab, we still need the logic in
import_unit_detailsthat pairs lessons but we don't want to update most objects now that they're been imported. 2db91b1 adds a command line flag to specify which models to update, with an empty list meaning no updates. This will allow us to run the script with the-m Vocabularyflag in order to only update vocabulary.Relies on code-dot-org/curriculumbuilder#335 on the curriculumbuilder side to export vocab for levelbuilder to consume.
Links
Testing story
Reviewer Checklist: