Save solution images into artist and playlab level files #16840

davidsbailey · 2017-08-04T18:50:17Z

Background

For stage extras (see spec), we want to be able to show images of the bonus levels that the students can choose from. These images should show the solution of the level in question. These levels could be artist or playlab levels. A different solution is already being used to obtain these images for maze levels.

Description

This PR proposes saving the image at the time that the levelbuilder edits the level solution. This guarantees that the solution image stays up to date when the solution is edited, and puts the solution image url in a predictable place in the database where it can be retrieved when needed. It also has the downside that each of the ~100 levels that will be included in stage extras will need to have their solutions manually run in levelbuilder once in order to generate the solution image and url.

Verification

I've run one artist solution and one playlab solution to generate the thumbnail image urls for those levels. You can see them here:

artist script level: https://studio.code.org/s/coursec/stage/6/puzzle/8
- level file changes: c973dee
- thumbnail url: https://d3p74s6bwmy6t9.cloudfront.net/8cbbc4899e479126d6c97114489d398f=development/967.png
playlab script level: https://studio.code.org/s/coursec/stage/13/puzzle/1
- level file changes: 9c8934f
- solution image url: https://d3p74s6bwmy6t9.cloudfront.net/af4338d3357b7926d7966836de647c0d=development/952.png

Future work

This PR only generates solution images for artist and playlab levels. A slightly different approach may be needed to obtain the actual image for other level types, but the solution image url will still be stored in the same way in the level file for those other level types.

…updated

davidsbailey · 2017-08-04T18:57:57Z

@aoby , can you please review the rails code? @islemaster , can you please review the javascript? @joshlory , please take a look at the overall approach to make sure that this meets your needs.

islemaster

JSLGTM.

islemaster · 2017-08-04T19:09:18Z

apps/src/turtle/turtle.js

+    return {
+      ...reportData,
+      image,
+    };


👍 for not mutating arguments!

davidsbailey · 2017-08-04T19:38:03Z

For clarification, the level file edited in this PR can be seen here in the product: https://studio.code.org/s/coursec/stage/6/puzzle/8 , and the solution image linked from the PR description represents the solution to that level.

ashercodeorg · 2017-08-04T19:42:17Z

dashboard/test/controllers/levels_controller_test.rb

+    }
+    assert_response :success
+    level = assigns(:level)
+    assert_equal level.properties[:toolbox_blocks.to_s], @program


It should be assert_equal expected, actual, it appears as if you have the arguments flipped.

Also why :toolbox_blocks.to_s? If you need the string, just specify it directly: 'toolbox_blocks'. Same below

ashercodeorg · 2017-08-04T19:43:44Z

dashboard/test/controllers/levels_controller_test.rb

+      game_id: @level.game.id,
+      type: 'toolbox_blocks',
+      program: @program,
+      image: 'stub-image-data',


These parameters appear to be (mostly) duplicated across tests. It might make sense to define a default_params in the setup block, then modify it appropriately for each test. In addition to being DRY, it makes it more clear what part of the param is under test.

I've extracted default params for the 5 tests pertaining to update_blocks.

ashercodeorg · 2017-08-04T19:45:12Z

dashboard/app/helpers/levels_helper.rb

@@ -770,4 +770,19 @@ def can_view_teacher_markdown?
  def include_multi_answers?(standalone)
    standalone || current_user.try(:should_see_inline_answer?, @script_level)
  end
+
+  def get_level_source_image(level_image, level_source_id)


Please document the function. Is level_image a string? That level_source_id is an integer or nil. It returns a LevelSourceImage or nil. Etc.

aoby · 2017-08-04T19:48:42Z

dashboard/app/helpers/levels_helper.rb

@@ -770,4 +770,19 @@ def can_view_teacher_markdown?
  def include_multi_answers?(standalone)
    standalone || current_user.try(:should_see_inline_answer?, @script_level)
  end
+
+  def get_level_source_image(level_image, level_source_id)


Please add a function header comment here. This function is rather complex, and has some side effects I wouldn't necessarily expect from a get_ method.

I'm not clear to me what it's doing. It seems to take an image, save it to S3, and construct a LevelSourceImage model around that? Or if a LevelSourceImage already exists, then it finds and returns that and ignores the supplied image?

renamed to find_or_create_level_source_image

aoby · 2017-08-04T20:05:09Z

dashboard/test/controllers/levels_controller_test.rb

+
+    CDO.stubs(:disable_s3_image_uploads).returns(false)
+    LevelSourceImage.any_instance.expects(:save_to_s3).never
+    create(:level_source, :with_image, level: @level, data: @program)


What does the comment relate to? Are we ensuring that we get a real S3 url? I don't see anything to that effect.

The preceding comment relates to these 3 lines. I was trying to only tell you why, but maybe I took it too far. I've extracted a method for this and documented the steps more clearly.

aoby · 2017-08-04T20:06:25Z

dashboard/test/controllers/levels_controller_test.rb

+    }
+    assert_response :success
+    level = assigns(:level)
+    assert_equal level.properties[:toolbox_blocks.to_s], @program


Also why :toolbox_blocks.to_s? If you need the string, just specify it directly: 'toolbox_blocks'. Same below

davidsbailey · 2017-08-04T21:05:53Z

@aoby @ashercodeorg PTAL. I've added comments and addressed other feedback.

aoby · 2017-08-04T21:20:18Z

dashboard/test/controllers/levels_controller_test.rb

@@ -3,6 +3,8 @@
 class LevelsControllerTest < ActionController::TestCase
  include Devise::Test::ControllerHelpers

+  default_update_blocks_params = nil


This won't be accessible anywhere. Member variables in ruby must be preceded with a @, and code here in the class will be executed when the class is loaded, not instantiated.

Since setup runs before each test case you don't need to initialize the member, just define it directly. If you want to use it without the @ you can add an accessor with attr_reader :default_update_blocks_params, or instead of in setup define it as a private method so it's lazily constructed as in this example.

I've followed your suggestion, but FYI that tests were passing for some reason the way that I had it. Not sure I can explain why exactly.

Weird. I played around with it and confirmed that it worked. But not for the reason it seems. This has to do with the closures generated for the blocks sent to the test command when the class is defined.

See this example:

class C temp_var = 'value' define_method :with_closure do puts "with closure: #{defined?(temp_var)}" end def without_closure puts "without closure: #{defined?(temp_var)}" end define_method :nested do print "nested, " without_closure end end c = C.new c.with_closure c.without_closure c.nested

The output is:

with closure: local-variable without closure: nested, without closure:

It shows up in the closure as if it were a local variable, but doesn't show up anywhere else.
That is only AFAICT because the blocks were defined in the same context. That variable is only in scope while the class is loaded, not accessible anywhere else later.

Similarly,

class C @outside = nil def initialize @inside = nil end def m puts "outside: #{defined?(@outside)}, inside: #{defined?(@inside)}" end end C.new.m

outputs

outside: , inside: instance-variable

The instance variable only persists when it's defined in a method. The one defined in the class body is lost.

aoby

LGTM, once the default test params are fixed and all tests pass. Thanks for the explanatory refactor!

davidsbailey · 2017-08-04T22:59:01Z

Quick update: this is now working in playlab also, since we are already sending the feedback image to the server when the solution blocks are updated in playlab here. I've added a solution image url for a single playlab level to this PR, and updated the PR description to point to it.

davidsbailey added 6 commits August 4, 2017 11:29

send feedback image to the server when updating artist solution_blocks

f71f0c9

extract Artist.prototype.setReportDataImage_

4742455

extract get_level_source_image from ActivitiesController to LevelsHelper

13b0e92

save solution_image_url to level properties when solution_blocks are …

3391639

…updated

Add tests for solution_image_url to LevelsControllerTest

b52e255

generate a solution image for one artist level

c973dee

davidsbailey requested review from aoby and islemaster August 4, 2017 18:52

islemaster approved these changes Aug 4, 2017

View reviewed changes

apps/src/turtle/turtle.js

return {

...reportData,

image,

};

Copy link

Contributor

islemaster Aug 4, 2017

Choose a reason for hiding this comment

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

👍 for not mutating arguments!

ashercodeorg reviewed Aug 4, 2017

View reviewed changes

aoby reviewed Aug 4, 2017

View reviewed changes

davidsbailey added 2 commits August 4, 2017 13:09

extract default_update_blocks_params in LevelsControllerTest

afeb5e7

code review feedback

be2d6e0

aoby reviewed Aug 4, 2017

View reviewed changes

aoby approved these changes Aug 4, 2017

View reviewed changes

davidsbailey added 2 commits August 4, 2017 14:31

rename default_update_blocks_params to @default_update_blocks_params

11132e7

generate a solution image for one playlab level [ci skip]

9c8934f

davidsbailey changed the title ~~Save solution images into artist level files~~ Save solution images into artist and playlab level files Aug 4, 2017

remove stray puts [ci skip]

2cf4f22

davidsbailey merged commit 0d65f1f into staging Aug 4, 2017

davidsbailey deleted the capture-levelbuilder-solutions branch August 4, 2017 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save solution images into artist and playlab level files #16840

Save solution images into artist and playlab level files #16840

davidsbailey commented Aug 4, 2017 •

edited

davidsbailey commented Aug 4, 2017

islemaster left a comment

islemaster Aug 4, 2017

davidsbailey commented Aug 4, 2017 •

edited

ashercodeorg Aug 4, 2017

aoby Aug 4, 2017

ashercodeorg Aug 4, 2017

davidsbailey Aug 4, 2017

ashercodeorg Aug 4, 2017

aoby Aug 4, 2017

davidsbailey Aug 4, 2017

aoby Aug 4, 2017

davidsbailey Aug 4, 2017

aoby Aug 4, 2017

davidsbailey commented Aug 4, 2017

aoby Aug 4, 2017 •

edited

davidsbailey Aug 4, 2017

aoby Aug 7, 2017

davidsbailey Aug 8, 2017

aoby left a comment

davidsbailey commented Aug 4, 2017

Save solution images into artist and playlab level files #16840

Save solution images into artist and playlab level files #16840

Conversation

davidsbailey commented Aug 4, 2017 • edited

Background

Description

Verification

Future work

davidsbailey commented Aug 4, 2017

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey commented Aug 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsbailey commented Aug 4, 2017

aoby Aug 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoby left a comment

Choose a reason for hiding this comment

davidsbailey commented Aug 4, 2017

davidsbailey commented Aug 4, 2017 •

edited

davidsbailey commented Aug 4, 2017 •

edited

aoby Aug 4, 2017 •

edited