Skip to content
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

Use code studio lesson plans #42496

Merged
merged 3 commits into from
Sep 16, 2021
Merged

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Sep 15, 2021

starts https://codedotorg.atlassian.net/browse/PLAT-856 . the first step is to add a flag which when unchecked indicates that, although the unit is migrated, it should still use lesson plans on CB. The HelpTip text in the PR diff also adds a bit of detail about what's going on.

screenshots

unmigrated unit, checkbox disabled

Screen Shot 2021-09-15 at 3 33 54 PM

migrated unit, unchecked

Screen Shot 2021-09-15 at 3 34 21 PM

migrated unit, checked

Screen Shot 2021-09-15 at 3 34 40 PM

Testing story

manually verified that the setting persists after checking/unchecking, saving, and reloading the unit edit page. also verified the setting appears in the script_json.

Deployment strategy

once this change reaches levelbuilder, I'll use the rails console on the levelbuilder machine to set this flag for all units which are currently migrated:

Script.all.select(&:is_migrated).each do |u| 
  u.update!(use_code_studio_lesson_plans: true)
  u.write_script_json
end

that'll get us into a state where we can safely start to show code studio lesson plans only when use_code_studio_lesson_plans is true.

@davidsbailey davidsbailey requested a review from a team September 16, 2021 06:20
@davidsbailey davidsbailey merged commit e3707e9 into staging Sep 16, 2021
@davidsbailey davidsbailey deleted the use-code-studio-lesson-plans branch September 16, 2021 16:57
@dmcavoy
Copy link
Contributor

dmcavoy commented Sep 16, 2021

I know this is already merged but I'm wondering about the wording here. "Use Code Studio Lesson Plans" feels like its should be the default. It seems like you should only have to check something for not using Code Studio Lesson Plans. I'm also wondering if we want to give editors the ability to pick not using code studio lesson plans for future courses. It seems like a message that says "This course uses older lesson plan formats. If you would like to update this please talk to an engineer." Would be safer for us.

@davidsbailey
Copy link
Member Author

Hey Dani, sorry for jumping the gun on this. I think the wording here is important and I'd be happy to make a change to it. Do you have any opinions for what the wording should be to replace "use code studio lesson plans"? do you think we should also change the flag name we use under the covers? ideas:

  • use_cb_lesson_plans
  • use_custom_lesson_plans
  • has_custom_curriculum_path

Good point about new units. at the very least, new units should have code studio lesson plans enabled, which I forgot to account for in this chain of PRs. to your point, it would be ideal if we didn't even allow new courses to use curriculum builder lesson plans. the simplest idea might be to have the checkbox be disabled when "use code studio lesson plans" are selected, and warn the user before they select "use code studio lesson plans" that once they save it, they won't be able to undo it without the help of an engineer.

@davidsbailey
Copy link
Member Author

another idea:

  • use_legacy_lesson_plans / "use legacy lesson plans"

@dmcavoy
Copy link
Contributor

dmcavoy commented Sep 16, 2021

I like use_legacy_lesson_plans because that captures both CB lesson plans and google doc lesson plans.

With the checkbox on the edit page:
I worry about adding another checkbox that I don't think we will use very often and could lead to possible problems. Are we adding the checkbox just so that if someone is trying to move over lesson plans from CB to LB for an old course they can use the checkbox to flip the switch when ready? It seems like that use case is pretty small and something that having an engineer do would keep us safer. Thats why I'm in favor of a warning message for the older courses so newer courses don't have to worry about this.

@davidsbailey
Copy link
Member Author

davidsbailey commented Sep 16, 2021

great, let's do use_legacy_lesson_plans on the backend.

here is another idea -- when use_legacy_lesson_plans is false, do not show anything in the unit edit page. when it is true, show a button (rather than a checkbox) indicating "enable code studio lesson plans" (in addition to the existing curriculum_path field). this can help give the idea that it is a one-way process. does that sound ok, or would you still be worried about things going wrong if someone decided to press the button when they shouldn't have?

@dmcavoy
Copy link
Contributor

dmcavoy commented Sep 16, 2021

i like the button idea that feels safe enough and gives editors the power to move to the code studio lesson plans on their own

@davidsbailey
Copy link
Member Author

addressed in #42534

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants