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

Fill in has_lesson_plan value based on lockable and save updates to has_lesson_plan in editor #38502

Merged
merged 39 commits into from Jan 23, 2021

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented Jan 11, 2021

Background

Follow up to #38490, and #38614 which :

  • Created a migration to add has_lesson_plan to lesson model
  • Set up ActivitiesEditor to respond to hasLessonPlan

Current PR

This is a big PR but a lot of that is the script (commit f51a840) and script_json files (commit b80a68f) (updated both in ce47f55 to account for script has_lesson_plan setting) . So hopefully that makes it less scary to review. I have tried to think about how to split it up but I don't think there is a way to not ship all of this as one.

I reviewed all the script and script_json files to make sure there were not any unwanted changes.

Setting has_lesson_plan

Set has_lesson_plan in the script and script_json files based on the lockable value and the script has_lesson_plan setting (if the script has_lesson_plan is false than all all lesson has_lesson_plan settings should be false otherwise if lockable true then has_lesson_plan is false and if lockable false has_lesson_plan true. Except in the case of the last lesson in CSP 21-22 scripts which we set has_lesson_plan to true.) We are holding off on updating the CSP 20-21 lessons which have lesson plans because we need to transition those over which will take some extra steps.

I generated these by updating my local database using:

Script.all.each do |script|
  script.lessons.all.each do |lesson|
    if script.has_lesson_plan
      lockable = !!lesson.lockable
      lesson.has_lesson_plan = !lockable
    else
      lesson.has_lesson_plan = false
    end
    lesson.save!
  end
end
scripts = ['csp1-2021', 'csp2-2021', 'csp3-2021', 'csp4-2021', 'csp5-2021', 'csp6-2021', 'csp7-2021', 'csp9-2021', 'csp10-2021']
scripts.each do |script_name|
  script = Script.find_by(name: script_name)
  next unless script
  lesson = script.lessons.last
  lesson.has_lesson_plan = true
  lesson.save!
end

Then I updated the script files using:

Script.all.each do |script|
  script.write_script_dsl
end

And the script_json by:

Script.where("properties -> '$.is_migrated' = ?", true).each do |script|
  script.write_script_json
end

Serializing has_lesson_plan

Adds has_lesson_plan to .script and .script_json files when script or lesson is saved by adding to the DSL and the LessonSerializer.

Saving has_lesson_plan in migrated scripts

#38614 set up the activities editor to respond to hasLessonPlan. This PR updates the reset of the LessonEditor to respond to hasLessonPlan and also adds some indicators on the LessonToken in ScriptEditor to tell if a lesson is lockable or has no lesson plan.

ScriptEditor:
Screen Shot 2021-01-17 at 8 16 04 AM

LessonEditor for Lesson Without Lesson Plan:
Screen Shot 2021-01-17 at 8 16 46 AM

Links

Testing story

Added and updated tests for react components for the editor experience.

Updated and added tests around the script DSL changes.

Added a basic UI test for editing a lesson without a lesson plan

I manually tested:

  • Updating has_lesson_plan on an existing lesson in script DSL
  • Adding a new lesson with and without has_lesson_plan set in DSL. If the levelbuilder does not specify has_lesson_plan will be true.
  • Adding a new lesson or updating an existing lesson plan in the new script/lesson editor experience.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • 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 dmcavoy changed the base branch from staging to add-activity-section-field January 15, 2021 13:10
@dmcavoy dmcavoy changed the base branch from add-activity-section-field to staging January 15, 2021 17:15
@dmcavoy dmcavoy changed the base branch from staging to add-activity-section-field January 16, 2021 01:58
@dmcavoy dmcavoy changed the base branch from add-activity-section-field to staging January 16, 2021 02:15
@dmcavoy dmcavoy changed the base branch from staging to has-lesson-plan-editor January 16, 2021 13:34
@dmcavoy dmcavoy changed the base branch from has-lesson-plan-editor to staging January 16, 2021 14:00
@dmcavoy dmcavoy changed the title Fill in has_lesson_plan value and make sure we keep track of it going forward Fill in has_lesson_plan value based on lockable and save updates to has_lesson_plan in editor Jan 17, 2021
@dmcavoy dmcavoy changed the base branch from staging to has-lesson-plan-editor January 17, 2021 12:58
@dmcavoy dmcavoy changed the base branch from has-lesson-plan-editor to staging January 17, 2021 13:03
Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, Dani! The new UI test is a great addition. This seems like just the time when we would need a UI test case, because the editor is doing something substantially different in this case.

@@ -1025,7 +1025,8 @@ def wait_for_jquery
data = JSON.parse(response)
@temp_script_name = data['script_name']
@temp_lesson_id = data['lesson_id']
puts "created temp migrated script named '#{@temp_script_name}' and temp lesson with id #{@temp_lesson_id}"
@temp_lesson_without_lesson_plan_id = data['lesson_without_lesson_plan_id']
puts "created temp migrated script named '#{@temp_script_name}', temp lesson with id #{@temp_lesson_id}, and temp lesson without a lesson plan with id #{@temp_lesson_without_lesson_plan_id}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debug statement?

@@ -3,6 +3,27 @@
@no_mobile

Feature: Using the Lesson Edit Page
Scenario: Save changes using the lesson edit page for lesson without lesson plan
Given I create a levelbuilder named "Levi"
And I create a temp migrated script and lesson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice updates to UI tests! nit: change to "script and lessons" or "script with lessons"?

And I wait until element "#show-container" is visible
And element "h2" contains text "Agenda"

And I delete the temp script and lesson
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "script and lessons" or "script with lessons"?

@dmcavoy dmcavoy merged commit c4b13ef into staging Jan 23, 2021
@dmcavoy dmcavoy deleted the fill-has-lesson-plan branch January 23, 2021 01:26
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

2 participants