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

Seed programming expressions and programming environments #39274

Merged
merged 48 commits into from
Mar 12, 2021

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented Feb 28, 2021

Create .json files for each ProgrammingExpression that we want to seed. This includes: App Lab and Game Lab blocks from the dropletConfig, Sprite Lab blocks which are defined in config/blocks as well as Sprite Lab blocks that are defined in the general Blockly library, and Web Lab expressions which match the documentation set currently on CB.

Create .json files for each ProgrammingEnvironment that we want to seed.

Use the .json files to seed ProgrammingExpressions and ProgrammingEnvironments.

Background & Context

When we originally set out to seed ProgrammingExpressions, the hope was that we could do it in a way that was more connected to the way we already define blocks for our environments. However time limitations have made us consider this route as the best options. Below is a list of challenges and what action was taken to deal with the challenges.

Challenge 1: There is no easy way to use dropletConfig.js (for App Lab and Game Lab) to seed ProgrammingExpressions without extracting that information into a shared space.

Action: We used the definition of the blocks in dropletConfig to create the pullOutBlocks.js one-off script to generate the .json files. Is it possible in the future we could use the .json files to set up dropletConfig.js for App Lab and Game Lab?

Challenge 2: The .json files for Sprite Lab which live in config/blocks/GamelabJr do not capture all the Sprite Lab blocks that need documentation. There are Blockly blocks that don't have json files.

Action: Made json files for each of the Blockly blocks we currently have documentation for on CB. Had to put them in config/programming_expressions/spritelab because adding them to config/blocks/GamelabJr caused issues with seeing the Blocks model which is used for block pools in Sprite Lab, Dance, and Minecraft. This means we now have two places that are capturing the json files used to seed ProgrammingExpressions /config/blocks/ and /config/programming_expressions. Ideally these would all be able to be together.

Links

Testing story

  • Ran seed:all, seed:programming_expressions and seed:blocks locally to make sure both work

Follow-up work

  • Import Lesson-Block associations from CB
  • Show Programming Expressions in Lesson Plan and Student Lesson Plan
  • Allow editors to add and delete Programming Expressions associated with lesson

PR 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

{
func: 'randomNumber_min_max',
block: 'randomNumber(1, 10)',
category: 'Math',
docFunc: 'randomNumber'
docFunc: 'randomNumber_min_max'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have as separate documentation page for randomNumber_min_max which covers the params. We should use it here.

@dmcavoy dmcavoy requested review from davidsbailey, ajpal and a team March 5, 2021 19:59
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.

a few initial comments...

Challenge 1: There is no easy way to use dropletConfig.js (for App Lab and Game Lab) to seed ProgrammingExpressions without extracting that information into a shared space.

Action: We used the definition of the blocks in dropletConfig to create the pullOutBlocks.js one-off script to generate the .json files. Is it possible in the future we could use the .json files to set up dropletConfig.js for App Lab and Game Lab?

in the future, it should be possible to generate dropletConfig.js from some ruby file droplet_config.rb, the same way that we currently generate sharedConstants.js from shared_constants.rb. Totally understand that there is not time for this right now. I don't think we need to track this, but it is valuable to have this idea captured somewhere so that we know what direction to head in later if needed.

@davidsbailey
Copy link
Member

Challenge 2: The .json files for Sprite Lab which live in config/blocks/GamelabJr do not capture all the Sprite Lab blocks that need documentation. There are Blockly blocks that don't have json files.

Action: Made json files for each of the Blockly blocks we currently have documentation for on CB. Had to put them in config/programming_expressions/spritelab because adding them to config/blocks/GamelabJr caused issues with seeing the Blocks model which is used for block pools in Sprite Lab, Dance, and Minecraft. This means we now have two places that are capturing the json files used to seed ProgrammingExpressions /config/blocks/ and /config/programming_expressions. Ideally these would all be able to be together.

This definitely sounds fine in the short term. In the future if we wanted to get the spritelab blocks all into one place, one idea would be to add a flag "is blockly default block" within the block json that essentially tells the spritelab block editing code to ignore those blocks during seeding / block editing.

@dmcavoy
Copy link
Contributor Author

dmcavoy commented Mar 9, 2021

Is it an issue that some of the file names have "." in the name?

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.

Is it an issue that some of the file names have "." in the name?

it shouldn't be -- most programs only treat the part after the final . as the file extension.

@@ -0,0 +1 @@
{"func":"soundSensor.start","category":"Circuit","noAutocomplete":true}
Copy link
Member

Choose a reason for hiding this comment

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

Just commenting for posterity that it looks like Circuit Playground is keeping the "default" block names (without prefix) and only microbit blocks are getting a prefix on their docFunc -- this sounds great to me, so that we don't have to change the urls of the existing Circuit Playground blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i added microbit- on all the microbit blocks since many of them have the same name as circuit playground blocks.

@dmcavoy
Copy link
Contributor Author

dmcavoy commented Mar 10, 2021

@davidsbailey Could you take a look at this tomorrow? Thanks!

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.

Awesome progress Dani! I have a bunch of comments, so would like to review again once you've had a chance to address them.

...createLedProps,
type: 'either',
docFunc: 'createLedNoAssign'
},
Copy link
Member

Choose a reason for hiding this comment

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

Is adding these going to have any effect on what shows up in Maker apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the docFunc is just a way to specify if the documentation link is different than the block name

dashboard/app/models/programming_environment.rb Outdated Show resolved Hide resolved
dashboard/scripts/pullOutBlocks.js Outdated Show resolved Hide resolved
@@ -419,5 +425,5 @@ namespace :seed do
timed_task incremental: [:videos, :concepts, :scripts_incremental, :callouts, :school_districts, :schools, :secret_words, :secret_pictures, :courses, :ap_school_codes, :ap_cs_offerings, :ib_school_codes, :ib_cs_offerings, :state_cs_offerings, :donors, :donor_schools, :foorms, :standards]

desc "seed only dashboard data required for tests"
timed_task test: [:videos, :games, :concepts, :secret_words, :secret_pictures, :school_districts, :schools, :standards, :foorms]
timed_task test: [:videos, :games, :concepts, :programming_expressions, :secret_words, :secret_pictures, :school_districts, :schools, :standards, :foorms]
Copy link
Member

Choose a reason for hiding this comment

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

this does not look like it belongs here. this looks like the list of things we seed for unit tests. instead, please add this to UI_TEST_SEED_TASKS and FULL_SEED_TASKS. that will make this data get seeded during rake build on localhost and during the deploy to every chef-managed environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok its already added to UI_TEST_SEED_TASKS and FULL_SEED_TASKS by being included in SCRIPTS_DEPENDENCIES further up the file. I will remove it here

@@ -8,6 +8,37 @@
# created_at :datetime not null
# updated_at :datetime not null
#
#
# @attr [String] editor_type - Type of editor one of the following: 'text-based', 'droplet', 'blockly'
Copy link
Member

Choose a reason for hiding this comment

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

Is "text-based" just for weblab? If so, what do you think about changing it to "bramble" (the weblab editor)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be used for CSA too in the future.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I'm not totally clear on how this field will get used, for example during the seed code you check the environment name rather than the editor name. but this seems easy enough to change in the future if you do want separate ones for weblab and javalab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that we wanted to use editor_type to determine how to show the blocks/text in markdown. So droplet blocks would look different than blockly blocks etc.

# General
when 'Advanced'
'#19c3e1'
# Gamelab and Applab have different blue colors for this. Waiting to hear thoughts on what to do
Copy link
Member

Choose a reason for hiding this comment

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

can we solve this by making this method take in both category and environment_name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advanced category does not show in the toolbox. I think it would be good for someone from @code-dot-org/starlabs to weigh in on if we can actually just standardize to one color of blue here. The curriculum team did not seem very concerned about this since its not in the toolbox. https://codedotorg.slack.com/archives/C1B3PNDL7/p1614718268052000

Copy link
Contributor Author

@dmcavoy dmcavoy Mar 10, 2021

Choose a reason for hiding this comment

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

Also for context other categories that are shared like Variables and Math the colors match

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks for the context. I agree that we don't need to consider the programming environment in this method right now.

dashboard/app/models/programming_expression.rb Outdated Show resolved Hide resolved
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.

Awesome progress Dani!

# General
when 'Advanced'
'#19c3e1'
# Gamelab and Applab have different blue colors for this. Waiting to hear thoughts on what to do
Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks for the context. I agree that we don't need to consider the programming environment in this method right now.

var fs = require("fs");

var applabBlocksToImport = [
// Add dropletConfig definition of the blocks you want to import here
Copy link
Member

Choose a reason for hiding this comment

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

What exactly do you have to add here? I could imagine pasting in snippets from */dropletConfig.js, however that sounds challenging because it looks like those sometimes depend on constants defined elsewhere in those files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya you have to do some work to remove things that are dynamically generated such as dropdowns or have constants. Right now those fields were not needed so I just ran the script and when it complained about a line for those reasons and I didn't need it I just deleted the line.

Copy link
Member

Choose a reason for hiding this comment

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

If it would save you time, one idea would be to change our application code to print out the droplet config, at which point it would have all the constants already substituted.

@@ -8,6 +8,37 @@
# created_at :datetime not null
# updated_at :datetime not null
#
#
# @attr [String] editor_type - Type of editor one of the following: 'text-based', 'droplet', 'blockly'
Copy link
Member

Choose a reason for hiding this comment

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

OK. I'm not totally clear on how this field will get used, for example during the seed code you check the environment name rather than the editor name. but this seems easy enough to change in the future if you do want separate ones for weblab and javalab.

@@ -0,0 +1 @@
{"func":"Math.abs","category":"Math","type":"value","params":["__"],"docFunc":"mathAbs"}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how easy these are to regenerate, but I think life will generally be easier if we "pretty print" the json here like we do in other config files, so that the diffs are a little easier to read when things change. I'm also not sure how often these will be updated, so feel free to use your judgment here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you make something pretty print? It is not ideal to regenerate all of these but I understand the value of what you are saying

console.log(JSON.stringify(block));
fs.writeFile(
`../../dashboard/config/programming_expressions/applab/${block.func}.json`,
JSON.stringify(block),
Copy link
Member

Choose a reason for hiding this comment

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

@dmcavoy to pretty print, you would do:

Suggested change
JSON.stringify(block),
JSON.stringify(block, null, 2),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used this to pretty print everything.

function readFiles(dirname, onFileContent, onError) {
  fs.readdir(dirname, function(err, filenames) {
    if (err) {
      onError(err);
      return;
    }
    filenames.forEach(function(filename) {
      fs.readFile(dirname + filename, "utf-8", function(err, content) {
        if (err) {
          onError(err);
          return;
        }
        onFileContent(filename, content);
      });
    });
  });
}
var data = {};
readFiles(
  "dashboard/config/programming_expressions/spritelab/",
  function(filename, content) {
    //data[filename] = content;
    let blockInfo = JSON.parse(content);
    fs.writeFile(
      `dashboard/config/programming_expressions/spritelab/${filename}`,
      JSON.stringify(blockInfo, null, 2),
      function(err) {
        if (err) {
          throw err;
        }
        console.log(
          `Saved! dashboard/config/programming_expressions/spritelab/${filename}`
        );
      }
    );
  },
  function(err) {
    throw err;
  }
);

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