-
Notifications
You must be signed in to change notification settings - Fork 480
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
Update script dsl to new format and make sure script dsl tests pass #35678
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, @dmcavoy ! From a reviewer standpoint I really appreciate that you made a stopping point here. I hope it wasn't too much extra work to make it happen, and hopefully it brings you some peace of mind knowing that you've got part of this passing tests and signed off as you proceed. apologies for the number of comments inline, code quality looks great and most comments are just on style.
#recreate raw_lessons from raw_lesson_groups | ||
raw_lesson_groups.each do |lesson_group| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm excited to see that you went with this approach! I'm interested to hear at some point if it felt worth doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we recreating raw_lessons
because that's what's needed for the current code to work, or because we want to keep that for the long term? I think both are fine but I'm trying to understand what our strategy will be.
(I know we want to move away from the current seeding process long long term, but my impression is we'll probably have something similar to the current state for the next 6-12 months. Is this accurate?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per earlier discussion i had with Dani, this is only here to let the current code in add_script keep working, while changing the code the calls it, as a way to break up this task into smaller pieces.
Generally speaking, yes, the seed process will probably resemble it's current form for the next 6-12 months, except as changes to it are needed to unblock other work. We'll definitely want to tackle the seed problem head-on when we go to consolidate LB into prod.
I'd like to review as well, it shouldn't take too long, if that's ok. |
scriptlevels: @scriptlevels, | ||
# For scripts that don't use lesson groups create a blank non-user facing lesson group | ||
if name | ||
if @lesson_groups.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we already had logic for creating a default lesson group in add_script
. Will it be duplicated now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just makes sure we create a lesson group to hold the lessons in the DSL output if one is not specified by the curriculum writer.
#recreate raw_lessons from raw_lesson_groups | ||
raw_lesson_groups.each do |lesson_group| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we recreating raw_lessons
because that's what's needed for the current code to work, or because we want to keep that for the long term? I think both are fine but I'm trying to understand what our strategy will be.
(I know we want to move away from the current seeding process long long term, but my impression is we'll probably have something similar to the current state for the next 6-12 months. Is this accurate?)
# or none of the lessons have lesson groups. We know we have hit this case if | ||
# there are more than one lesson group for a script but the lesson group key | ||
# for the first lesson is blank | ||
if raw_lesson_groups[0][:key].nil? && raw_lesson_groups.length > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we might want to protect against getting into this state earlier, if we can, since the thing passed into add_script
is now raw_lesson_groups
, not lessons themselves. So it seems like at this point every lesson should already belong to a lesson group.
More generally I think it's a good goal to minimize the parts of the code that need to know or care about the "default lesson group" logic. I think it'd be reasonable to put that in the Script DSL side of things, since you could consider not specifying a lesson group to just be a shorthand for specifying an unnamed lesson group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some of the later work will hopefully address this concern but if it does not let me know.
https://codedotorg.atlassian.net/browse/PLAT-223
Background
As part of updating the position values for lessons and script levels I realized that I would want to create a through relationship for scripts to know about lessons. In looking into that work I realized that the way
add_script
worked where script.lessons was getting setting was not going to work with that new set up. In order to move toward a set up that supports that through relationship I need to update add_script. In looking at where I wanted to end up with add_script I also realized that the format of the data coming in from script DSL could be a lot cleaner and clear.Overview
This PR does the work to clean up the set up of the information coming from Script DSL into add_script in preparation for improvements to add_script. You can see the before and after below of some information. Some big things that changed are that lesson and lesson groups are no longer separate things being passed to add_script. In addition information has been moved to be with the correct object. For example we don't keep lesson name in the level anymore. Because of this change we no longer needed the raw_lessons parameter on add_script. So I removed that and updated a bunch of tests that call add_script to reflect that change.
Before:
After