Only update dsls.en.yml for levels we want to translate #27881

Hamms · 2019-04-04T19:42:21Z

Make it possible to render DSL levels without translations #27953 added the ability for DSL levels to still render even if their relevant data isn't in the translation system (ie, the dsls.en.yml file)
02506f6 stops regenerating dsls.en.yml as part of the build process (since we now no longer rely on that file as the exclusive source of truth)
b96e61e updates the levelbuilder "level save" operation to only add content to the dsls.en.yml file on save if it's in the subset of levels we know we want to translate

Right now, all DSL levels depend on I18n.t to render their content, and will fall back to `nil` if they can't find the translation. We want to be able to only sync data into the translation system if we want to actually translate it, so we add the ability for DSL levels to fall back to the database content if they can't find translation data.

Repeat of #24384 This broke things the first time because we depended on content being in the dsls.en.yml file in order to render successfully; we no longer do, so it should be safe to stop this.

First attempted in #24183 This broke all untranslated DSL levels at the time, because we were still depending on the i18n file for content. Now that we can safely fall back to databse data if i18n data is missing, we can try this again.

…test

Hamms · 2019-04-04T21:42:23Z

dashboard/test/helpers/levels_helper_test.rb

@@ -646,4 +646,90 @@ def stub_country(code)
    assert_equal new_toolbox, options[:level]["toolbox"]
    assert_equal new_start, options[:level]["startBlocks"]
  end
+
+  test 'string_or_image can retrieve plain text' do
+    assert_equal string_or_image('match', "test"), "test"


note that this is the only one of the added tests that is actually testing new functionality; all the others are testing existing functionality

bethanyaconnor · 2019-04-08T20:06:44Z

What will be the new "source of truth" for non-i18n levels?

bethanyaconnor

Overall looks good! I have a couple of questions, mostly for my own education. :)

bethanyaconnor · 2019-04-08T19:53:37Z

dashboard/app/helpers/levels_helper.rb

@@ -654,7 +654,7 @@ def string_or_image(prefix, text, source_level = nil)
      )
    else
      level_name = source_level ? source_level.name : @level.name
-      data_t(prefix + '.' + level_name, text)
+      data_t(prefix + '.' + level_name, text, text)


We pass text as both the key and the default because the key is the English text right?

Yep!

That means that this PR doesn't address the issue of overly long keys, nor does it make it any easier to change the way we store the data in a way that would help us address that issue

bethanyaconnor · 2019-04-08T19:55:41Z

dashboard/test/models/dsl_test.rb

@@ -59,7 +59,7 @@ class DslTest < ActiveSupport::TestCase
  test 'should encrypt when saving in levelbuilder and decrypt when parsing from file' do
    # don't actually write a file, but check that we are writing the encrypted version
    Rails.application.config.stubs(:levelbuilder_mode).returns true
-    File.expects(:write).twice.with do |pathname, contents|
+    File.expects(:write).once.with do |pathname, contents|


Why change this?

This test level isn't in a script we're trying to internationalize, so we no longer expect it to update the i18n file on save (only its actual level file)

Hamms · 2019-04-08T21:43:03Z

What will be the new "source of truth" for non-i18n levels?

The database/their individual level files in dashboard/config/scripts

joshlory · 2019-04-08T21:47:20Z

dashboard/app/models/levels/dsl_defined.rb

@@ -74,7 +75,7 @@ def self.create_from_level_builder(params, level_params, old_name = nil)
      # Save updated level data to external files
      if Rails.application.config.levelbuilder_mode
        level.rewrite_dsl_file(text)
-        rewrite_i18n_file(i18n)
+        rewrite_i18n_file(i18n) if level.script_levels.any? {|sl| ScriptConstants.i18n? sl.script.name}


Is the long-term plan to delete dsls.en.yml entirely and generate it during the sync-codeorg-in step? Or keep it for translated script levels only?

Commit 3d8eb04 seems like a clear step forward, and maybe is worth merging on its own...

I'm actually not sure!

I played around with the idea of deleting it entirely and just generating it during the sync-codeorg-in step, but ran into a couple challenges there. Specifically, that step is built entirely around the idea of being able to get all the data it needs directly from the files in the filesystem, without the need for syncing everything into a database and using ActiveRecord. Unfortunately, it turns out it's a lot more complicated than I thought it would be to get all the dsls data from the filesystem. (see #24369 (comment) for more info)

So I decided to just keep it around for the translated script in #27885; the idea there is that this file would get updated when the levels get changed in levelbuilder (and only get updated there, rather than as a part of any build process) and then consumed by the i18n sync (and only get consumed there, rather than as a part of any build process).

Unfortunately, that approach doesn't work very well for contained levels, since it's harder for them to know whether they're in a script or not. My short-term plan at this point is just to get things working with the "keep a minimal version of dsls.en.yml around", but I don't yet know whether or not that's the long-term solution.

I'd love to get @islemaster's input here from the Learning Platform perspective:

Are "contained" levels something we plan to fully-support or deprecate going forward?

How are these levels being found during the yearly course version clone process?

I think @davidsbailey knows more about this than I do, but contained levels (a.k.a. prediction levels?) are used extensively in our system - I see more than 200, spread out across courses A, B, C, D, E, F, CSD, and Dance Party. If we're planning to deprecate them for some other representation, we've got a migration ahead of us.

Are "contained" levels something we plan to fully-support or deprecate going forward?

yes, contained levels (a.k.a. prediction levels) are something we plan to fully support moving forward.

if some linkage could be added in the rails models so that a contained level knew what level it was contained in, would that make this problem any easier to tackle?

Now that (as of #27881 and #27885) we are generating a version of that file that contains only the strings we know we want to translate

Hamms added 7 commits April 2, 2019 16:33

Don't reseed dsls on every build

02506f6

Repeat of #24384 This broke things the first time because we depended on content being in the dsls.en.yml file in order to render successfully; we no longer do, so it should be safe to stop this.

add some tests for string_or_image

b4312e0

update some tests that expected the i18n file to be updated

2907a60

ScriptConstants.i18n? takes a string, not an actual script

addb2a7

test should use minifiable_asset_path so it can pass on both dev and …

c29f58c

…test

Hamms commented Apr 4, 2019

View reviewed changes

Hamms mentioned this pull request Apr 4, 2019

Sync minimal dsls.en.yml #27885

Merged

Hamms changed the title ~~Fix dsls~~ Make dsls.en.yml only required for translations, not for basic site functionality Apr 4, 2019

Hamms marked this pull request as ready for review April 4, 2019 23:43

Hamms requested review from joshlory, nkiruka and bethanyaconnor April 4, 2019 23:44

bethanyaconnor reviewed Apr 8, 2019

View reviewed changes

bethanyaconnor approved these changes Apr 8, 2019

View reviewed changes

joshlory reviewed Apr 8, 2019

View reviewed changes

This was referenced Apr 9, 2019

Stop recreating dsl i18n on every seed #24384

Closed

Make it possible to render DSL levels without translations #27953

Merged

Hamms changed the title ~~Make dsls.en.yml only required for translations, not for basic site functionality~~ Only update dsls.en.yml for levels we want to translate Apr 10, 2019

Merge branch 'staging' into fix-dsls

4dc390c

Hamms merged commit d356d01 into staging Apr 16, 2019

Hamms deleted the fix-dsls branch April 16, 2019 19:38

Hamms added a commit that referenced this pull request Apr 19, 2019

Reenable syncing dsls.en.yml with crowdin

d8dc7e2

Now that (as of #27881 and #27885) we are generating a version of that file that contains only the strings we know we want to translate

Hamms mentioned this pull request Apr 19, 2019

Reenable syncing dsls.en.yml with crowdin #28101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only update dsls.en.yml for levels we want to translate #27881

Only update dsls.en.yml for levels we want to translate #27881

Hamms commented Apr 4, 2019 •

edited

Hamms Apr 4, 2019

bethanyaconnor commented Apr 8, 2019

bethanyaconnor left a comment

bethanyaconnor Apr 8, 2019

Hamms Apr 8, 2019

bethanyaconnor Apr 8, 2019

Hamms Apr 8, 2019

Hamms commented Apr 8, 2019

joshlory Apr 8, 2019

Hamms Apr 9, 2019

joshlory Apr 10, 2019

islemaster Apr 10, 2019

davidsbailey Apr 10, 2019

Only update dsls.en.yml for levels we want to translate #27881

Only update dsls.en.yml for levels we want to translate #27881

Conversation

Hamms commented Apr 4, 2019 • edited

Choose a reason for hiding this comment

bethanyaconnor commented Apr 8, 2019

bethanyaconnor left a 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

Hamms commented Apr 8, 2019

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

Hamms commented Apr 4, 2019 •

edited