Merge pull request #16840 from code-dot-org/capture-levelbuilder-solu…

…tions

Save solution images into artist and playlab level files

apps/src/turtle/turtle.js

@@ -1810,15 +1810,7 @@ Artist.prototype.checkAnswer = function () {
       save_to_gallery: level.impressive
     };
 
-    // https://www.pivotaltracker.com/story/show/84171560
-    // Never send up frozen images for now.
-    var isFrozen = (this.skin.id === 'anna' || this.skin.id === 'elsa');
-
-    // Get the canvas data for feedback.
-    if (this.testResults >= TestResults.TOO_MANY_BLOCKS_FAIL &&
-      !isFrozen && (level.freePlay || level.impressive)) {
-      reportData.image = encodeURIComponent(this.getFeedbackImage_().split(',')[1]);
-    }
+    reportData = this.setReportDataImage_(level, reportData);
 
     this.studioApp_.report(reportData);
   }
@@ -1831,6 +1823,37 @@ Artist.prototype.checkAnswer = function () {
   // The call to displayFeedback() will happen later in onReportComplete()
 };
 
+/**
+ * Adds the feedback image to the report data if indicated by the level config.
+ * @param {Object} level Level config.
+ * @param {Object} reportData Original reportData.
+ * @returns {Object} Updated reportData, or original report data if not updated.
+ * @private
+ */
+Artist.prototype.setReportDataImage_ = function (level, reportData) {
+  // https://www.pivotaltracker.com/story/show/84171560
+  // Never send up frozen images for now.
+  var isFrozen = (this.skin.id === 'anna' || this.skin.id === 'elsa');
+
+  // Include the feedback image whenever a levelbuilder edits solution blocks.
+  const isEditingSolution = (level.editBlocks === 'solution_blocks');
+
+  const didPassLevel = this.testResults >= TestResults.TOO_MANY_BLOCKS_FAIL;
+
+  // Get the canvas data for feedback.
+  if (
+    isEditingSolution ||
+    (didPassLevel && !isFrozen && (level.freePlay || level.impressive))
+  ) {
+    const image = encodeURIComponent(this.getFeedbackImage_().split(',')[1]);
+    return {
+      ...reportData,
+      image,
+    };
+  }
+  return reportData;
+};
+
 Artist.prototype.getFeedbackImage_ = function (width, height) {
 
   var origWidth = this.ctxFeedback.canvas.width;

dashboard/app/controllers/activities_controller.rb

@@ -87,16 +87,7 @@ def milestone
       params[:lines] = MAX_LINES_OF_CODE if params[:lines] > MAX_LINES_OF_CODE
     end
 
-    # Store the image only if the image is set, and the image has not been saved
-    if params[:image] && @level_source.try(:id)
-      @level_source_image = LevelSourceImage.find_by(level_source_id: @level_source.id)
-      unless @level_source_image
-        @level_source_image = LevelSourceImage.new(level_source_id: @level_source.id)
-        unless @level_source_image.save_to_s3(Base64.decode64(params[:image]))
-          @level_source_image = nil
-        end
-      end
-    end
+    @level_source_image = find_or_create_level_source_image(params[:image], @level_source.try(:id))
 
     @new_level_completed = false
     if current_user

dashboard/app/controllers/levels_controller.rb

@@ -95,6 +95,7 @@ def update_blocks
     authorize! :update, @level
     blocks_xml = params[:program]
     type = params[:type]
+    set_solution_image_url(@level) if type == 'solution_blocks'
     blocks_xml = Blockly.convert_toolbox_to_category(blocks_xml) if type == 'toolbox_blocks'
     @level.properties[type] = blocks_xml
     @level.log_changes(current_user)
@@ -289,4 +290,13 @@ def level_params
     permitted_params.concat(Level.permitted_params)
     params[:level].permit(permitted_params)
   end
+
+  def set_solution_image_url(level)
+    level_source = LevelSource.find_identical_or_create(
+      level,
+      params[:program].strip_utf8mb4
+    )
+    level_source_image = find_or_create_level_source_image(params[:image], level_source.try(:id))
+    @level.properties['solution_image_url'] = level_source_image.s3_url if level_source_image
+  end
 end

dashboard/app/helpers/levels_helper.rb

@@ -770,4 +770,27 @@ def can_view_teacher_markdown?
   def include_multi_answers?(standalone)
     standalone || current_user.try(:should_see_inline_answer?, @script_level)
   end
+
+  # Finds the existing LevelSourceImage corresponding to the specified level
+  # source id if one exists, otherwise creates and returns a new
+  # LevelSourceImage using the image data in level_image.
+  #
+  # @param level_image [String] A base64-encoded image.
+  # @param level_source_id [Integer, nil] The id of a LevelSource or nil.
+  # @returns [LevelSourceImage] A level source image, or nil if one was not
+  # created or found.
+  def find_or_create_level_source_image(level_image, level_source_id)
+    level_source_image = nil
+    # Store the image only if the image is set, and the image has not been saved
+    if level_image && level_source_id
+      level_source_image = LevelSourceImage.find_by(level_source_id: level_source_id)
+      unless level_source_image
+        level_source_image = LevelSourceImage.new(level_source_id: level_source_id)
+        unless level_source_image.save_to_s3(Base64.decode64(level_image))
+          level_source_image = nil
+        end
+      end
+    end
+    level_source_image
+  end
 end

dashboard/config/scripts/levels/courseC_PlayLab_events1.level

@@ -41,11 +41,12 @@
     "hide_share_and_remix": "false",
     "disable_if_else_editing": "false",
     "authored_hints": "[{\"hint_class\":\"content\",\"hint_markdown\":\"Attach this block to the `when run` block:\\n\\n![](https://images.code.org/24043f33cb433506679d27acf991b7c1-image-1499792847837.06.43 AM.png)\",\"hint_id\":\"courseC_PlayLab_events1_a\",\"hint_type\":\"general\",\"tts_url\":\"https://tts.code.org/sharon22k/180/100/b7c0dffa28e40475021b7f070fc4aceb/courseC_PlayLab_events1.mp3\"}]",
+    "solution_image_url": "https://d3p74s6bwmy6t9.cloudfront.net/af4338d3357b7926d7966836de647c0d=development/952.png",
     "contained_level_names": null
   },
   "published": true,
   "notes": "",
-  "audit_log": "[{\"changed_at\":\"2017-06-14 20:07:10 +0000\",\"changed\":[\"start_blocks\",\"toolbox_blocks\",\"solution_blocks\",\"success_condition\",\"contained_level_names\"],\"changed_by_id\":302,\"changed_by_email\":\"mara.downing@code.org\"},{\"changed_at\":\"2017-07-11 17:07:39 +0000\",\"changed\":[\"start_blocks\",\"toolbox_blocks\",\"solution_blocks\",\"contained_level_names\"],\"changed_by_id\":302,\"changed_by_email\":\"mara.downing@code.org\"}]",
+  "audit_log": "[{\"changed_at\":\"2017-06-14 20:07:10 +0000\",\"changed\":[\"start_blocks\",\"toolbox_blocks\",\"solution_blocks\",\"success_condition\",\"contained_level_names\"],\"changed_by_id\":302,\"changed_by_email\":\"mara.downing@code.org\"},{\"changed_at\":\"2017-07-11 17:07:39 +0000\",\"changed\":[\"start_blocks\",\"toolbox_blocks\",\"solution_blocks\",\"contained_level_names\"],\"changed_by_id\":302,\"changed_by_email\":\"mara.downing@code.org\"},{\"changed_at\":\"2017-08-04 15:16:23 -0700\",\"changed\":[],\"changed_by_id\":1,\"changed_by_email\":\"dave@code.org\"}]",
   "level_concept_difficulty": {
     "sequencing": 2
   }

dashboard/config/scripts/levels/courseC_artist_prog7.level

@@ -39,6 +39,7 @@
     "video_key": "CSF_artist_angles",
     "hide_share_and_remix": "false",
     "disable_if_else_editing": "false",
+    "solution_image_url": "https://d3p74s6bwmy6t9.cloudfront.net/8cbbc4899e479126d6c97114489d398f=development/967.png",
     "contained_level_names": null
   },
   "published": true,
@@ -257,4 +258,4 @@
       </xml>
     </predraw_blocks>
   </blocks>
-</Artist>
+</Artist>

dashboard/test/controllers/levels_controller_test.rb

@@ -13,6 +13,15 @@ class LevelsControllerTest < ActionController::TestCase
     @levelbuilder = create(:levelbuilder)
     sign_in(@levelbuilder)
     @program = '<hey/>'
+
+    enable_level_source_image_s3_urls
+
+    @default_update_blocks_params = {
+      level_id: @level.id,
+      game_id: @level.game.id,
+      type: 'toolbox_blocks',
+      program: @program,
+    }
   end
 
   test "should get index" do
@@ -289,26 +298,38 @@ class LevelsControllerTest < ActionController::TestCase
   end
 
   test "should update blocks" do
-    post :update_blocks, params: {
-      level_id: @level.id,
-      game_id: @level.game.id,
-      type: 'toolbox_blocks',
-      program: @program
-    }
+    post :update_blocks, params: @default_update_blocks_params
+    assert_response :success
+    level = assigns(:level)
+    assert_equal @program, level.properties['toolbox_blocks']
+    assert_nil level.properties['solution_image_url']
+  end
+
+  test "should update solution image when updating solution blocks" do
+    post :update_blocks, params: @default_update_blocks_params.merge(
+      type: 'solution_blocks',
+      image: 'stub-image-data',
+    )
     assert_response :success
     level = assigns(:level)
-    assert_equal level.properties[:toolbox_blocks.to_s], @program
+    assert_equal @program, level.properties['solution_blocks']
+    assert_s3_image_url level.properties['solution_image_url']
+  end
+
+  test "should not update solution image when updating toolbox blocks" do
+    post :update_blocks, params: @default_update_blocks_params.merge(
+      image: 'stub-image-data',
+    )
+    assert_response :success
+    level = assigns(:level)
+    assert_equal @program, level.properties['toolbox_blocks']
+    assert_nil level.properties['solution_image_url']
   end
 
   test "should not update blocks if not levelbuilder" do
     [@not_admin, @admin].each do |user|
       sign_in user
-      post :update_blocks, params: {
-        level_id: @level.id,
-        game_id: @level.game.id,
-        type: 'toolbox_blocks',
-        program: @program
-      }
+      post :update_blocks, params: @default_update_blocks_params
       assert_response :forbidden
     end
   end
@@ -318,12 +339,10 @@ class LevelsControllerTest < ActionController::TestCase
     can_edit = Ability.new(@levelbuilder).can? :edit, level
     assert_equal false, can_edit
 
-    post :update_blocks, params: {
+    post :update_blocks, params: @default_update_blocks_params.merge(
       level_id: level.id,
       game_id: level.game.id,
-      type: 'toolbox_blocks',
-      program: @program
-    }
+    )
     assert_response :forbidden
   end
 
@@ -706,4 +725,30 @@ class LevelsControllerTest < ActionController::TestCase
     get :embed_blocks, params: {level_id: level, block_type: :solution_blocks}
     assert_response :success
   end
+
+  private
+
+  # Assert that the url is a real S3 url, and not a placeholder.
+  def assert_s3_image_url(url)
+    assert(
+      %r{#{LevelSourceImage::S3_URL}.*\.png}.match(url),
+      "expected #{url.inspect} to be an S3 URL"
+    )
+  end
+
+  # Allow our update_blocks tests to verify that real S3 urls are being
+  # generated when solution images are uploaded. We don't want to actually
+  # upload any S3 images in our tests, so just enable the codepath where an
+  # existing LevelSourceImage is found based on the program contents.
+  def enable_level_source_image_s3_urls
+    # Allow LevelSourceImage to return real S3 urls.
+    CDO.stubs(:disable_s3_image_uploads).returns(false)
+
+    # Make sure there is a LevelSourceImage associated with the program.
+    create(:level_source, :with_image, level: @level, data: @program)
+
+    # Because we cleared disable_s3_image_uploads, there's a chance we'll
+    # accidentally try to upload an image to S3. Make sure this never happens.
+    LevelSourceImage.any_instance.expects(:save_to_s3).never
+  end
 end