Prevent renaming lesson and lesson groups keys in scripts that are stable and i18n #36219

dmcavoy · 2020-08-11T01:53:45Z

Background

For a long time we have used the the name of the lesson as both the key for the lesson and then display name. This leads to challenges. One example of these challenges is in translation. In order to keep the key consistent for translations we don't want levelbuilders to change the name in the Script DSL. Instead an engineer needs to update the display name in scripts.en.yml.

Change

In order to get away from using the name as both the key and the value we are going to add a separate key value to lessons. This is going to take a couple steps:

Done Migrations to add key column to lesson(stages) table (See Add key column to lesson (stage) table #36013)
Done Copy the value in the name column to the key column for all lessons (See Add key column to lesson (stage) table #36013)
Done Update the Script DSL to use new format where a levelbuilder specifies both the key and name. Update all the existing script files to use this new format. (See Use lesson key in script dsl and use lesson name to store the display name #36020)
Done Update the name field to store the display name which was previously only kept in scripts.en.yml. Update places where lesson name was used in the past in a way that should become key for the future. (See Use lesson key in script dsl and use lesson name to store the display name #36020)
Allow updating the display name of lessons (and lesson groups) but prevent levelbuilder from adding/changing the key of lessons and lesson groups in scripts which are marked as stable and included in the i18n pipeline. <--- We are here

Testing

-Add tests to check that you can't add new lesson group or lesson to a script that is stable and i18n
-Add test to check you can't change the key of a lesson in a stable and i18n
-Add test to make sure we don't have a blank display name provided by levelbuilder in dsl
-Add test to make sure that changing the display name of a lesson updates correctly in scripts.en.yml

…le and in i18n list

…esson

dmcavoy · 2020-08-17T19:48:05Z

dashboard/test/models/script_test.rb

@@ -2035,13 +2035,11 @@ class SummarizeVisibleAfterScriptTests < ActiveSupport::TestCase
    assert_equal 'Expect all lesson groups to have display names. The following lesson group does not have a display name: content1', raise.message
  end

-  test 'raises error if lesson group key already exists and try to change the display name' do


This looks really weird. I deleted an old test and added a new tests. They are not really connected.

… changing the key

dashboard/app/models/lesson.rb

davidsbailey · 2020-08-17T21:06:45Z

dashboard/app/models/lesson_group.rb

@@ -92,6 +86,12 @@ def self.add_lesson_groups(raw_lesson_groups, script, new_suffix, editor_experim
    end
  end

+  def self.prevent_changing_stable_i18n_key(script, raw_lesson_group)
+    if script.is_stable && ScriptConstants.i18n?(script.name) && !(I18n.t "data.script.name.#{script.name}.lesson_groups").keys.map(&:to_s).include?(raw_lesson_group[:key])


same as above: can use ... #{raw_lesson_group[:key]} instead of .keys.map...include? ?

even better, would it be possible to DRY this up a bit? I think a Policy Object might be appropriate here

davidsbailey · 2020-08-17T22:31:29Z

dashboard/app/models/script.rb

@@ -1021,7 +1021,7 @@ def self.base_name(name)
  def copy_and_write_i18n(new_name)
    scripts_yml = File.expand_path('config/locales/scripts.en.yml')
    i18n = File.exist?(scripts_yml) ? YAML.load_file(scripts_yml) : {}
-    i18n.deep_merge!(summarize_i18n_for_copy(new_name)) {|_, old, _| old}
+    i18n.deep_merge!(summarize_i18n_for_copy(new_name))


why are we removing the block passed to deep_merge! here? I'm not saying it's wrong, but it looks a bit tricky and related to a thing I need to do to the summarize methods next so hoping to learn something.

So removing the {|_, old, _| old} will make it so if there is the same key in both i18n and summarize_i18n_for_copy it will default to the one in summarize_i18n_for_copy. That seemed to me like what we would want but you know this area of the code better than I do so maybe thats incorrect?

My concern was that this change might expose some bug like the one we saw where the i18n system did not immediately reflect a new value after one was written, i.e. the scripts.en.yml file would be guaranteed to be fresh but whatever you got from summarize_i18n_for_copy might not be because it calls I18n.t.

I dug in github commit history and ended up with this gem: 4cb8b73 the commit description points to 2 other commits which flip flop between using or not using this block, both of which were merged to staging back in 2014 without a PR and without comment! It really was the wild west back then :-)

So based on all of that I don't want to give any special credit to the way the code was, although it still needs a small amount of credit since we know that it's been working for about 4 years.

I would say, go ahead and make this change. If i am reading correctly, this code line won't have any effect most of the time, and I think your change may actually fix a problem by updating scripts.en.yml to actually properly copy of all the i18n when we copy a script. we can verify that when we focus on script copy in December or January.

uponthesun

I don't think I have anything to add beyond Dave's review.

dmcavoy added 2 commits August 10, 2020 21:50

Use new name instead of defaulting to the old name in scripts.en.yml

c5ef411

All lessons must have display name

f763fc1

dmcavoy requested a review from a team as a code owner August 11, 2020 01:53

dmcavoy added 8 commits August 10, 2020 22:07

Prevent changing a lesson or lesson group key in a script that is tab…

429f49a

…le and in i18n list

Pull prevent changing key out into method like other prevents

92117f9

Add test to check that new keys are not created for stable and i18n l…

8825ecf

…esson

Add 2 more tests

f06c0e4

Merge branch 'add-key-lesson-2' into prevent-rename-key

1e3ad08

Merge branch 'add-key-lesson-2' into prevent-rename-key

9b80ac1

Merge branch 'add-key-lesson-2' into prevent-rename-key

d6a811a

Merge branch 'add-key-lesson-2' into prevent-rename-key

c89bd6e

dmcavoy changed the base branch from staging to add-key-lesson-2 August 12, 2020 23:49

Base automatically changed from add-key-lesson-2 to staging August 17, 2020 18:20

Merge branch 'staging' into prevent-rename-key

83dbec6

dmcavoy changed the title ~~Prevent rename key~~ Prevent renaming lesson and lesson groups keys in scripts that are stable and i18n Aug 17, 2020

dmcavoy requested review from davidsbailey, Hamms and uponthesun August 17, 2020 19:32

Add test to check that lesson has a display name

7f0a835

dmcavoy commented Aug 17, 2020

View reviewed changes

Add a test to make sure we can update the lesson display name without…

a788e8d

… changing the key

davidsbailey reviewed Aug 17, 2020

View reviewed changes

dashboard/app/models/lesson.rb Outdated Show resolved Hide resolved

davidsbailey reviewed Aug 17, 2020

View reviewed changes

dashboard/app/models/lesson.rb Outdated Show resolved Hide resolved

davidsbailey reviewed Aug 17, 2020

View reviewed changes

davidsbailey approved these changes Aug 17, 2020

View reviewed changes

davidsbailey mentioned this pull request Aug 17, 2020

Mark scripts as translateable #36287

Closed

7 tasks

uponthesun approved these changes Aug 17, 2020

View reviewed changes

dmcavoy added 2 commits August 17, 2020 21:09

Fix things from PR feedback

a142536

Merge branch 'staging' into prevent-rename-key

305a59d

Fix the translation check

6b7d8bd

dmcavoy merged commit da7adce into staging Aug 19, 2020

dmcavoy deleted the prevent-rename-key branch August 19, 2020 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent renaming lesson and lesson groups keys in scripts that are stable and i18n #36219

Prevent renaming lesson and lesson groups keys in scripts that are stable and i18n #36219

dmcavoy commented Aug 11, 2020 •

edited

dmcavoy Aug 17, 2020

davidsbailey Aug 17, 2020

Hamms Aug 17, 2020

davidsbailey Aug 17, 2020

dmcavoy Aug 18, 2020

davidsbailey Aug 18, 2020

uponthesun left a comment •

edited

Prevent renaming lesson and lesson groups keys in scripts that are stable and i18n #36219

Prevent renaming lesson and lesson groups keys in scripts that are stable and i18n #36219

Conversation

dmcavoy commented Aug 11, 2020 • edited

Background

Change

Testing

dmcavoy Aug 17, 2020

Choose a reason for hiding this comment

davidsbailey Aug 17, 2020

Choose a reason for hiding this comment

Hamms Aug 17, 2020

Choose a reason for hiding this comment

davidsbailey Aug 17, 2020

Choose a reason for hiding this comment

dmcavoy Aug 18, 2020

Choose a reason for hiding this comment

davidsbailey Aug 18, 2020

Choose a reason for hiding this comment

uponthesun left a comment • edited

Choose a reason for hiding this comment

dmcavoy commented Aug 11, 2020 •

edited

uponthesun left a comment •

edited