surface errors to ui when script update fails

[davidsbailey]

background

This PR addresses the following issue:

1. go to https://levelbuilder-studio.code.org/s/csp1-2021/edit
2. under "Publishing Settings", select "Visible in Teacher Dashboard"
3. press "Save and Keep Editing"

EXPECTED: fails with error explaining that migrated scripts cannot be marked visible (yet)
ACTUAL: fails silently with success message

root cause

this validation was failing:

code-dot-org/dashboard/app/models/script.rb

Lines 123 to 125 in 8845df6

	def set_is_migrated_only_for_migrated_scripts
	if !!is_migrated && !hidden
	errors.add(:is_migrated, "Can't be set on a course that is visible")

an ActiveRecord::RecordInvalid error was being raised:

code-dot-org/dashboard/app/models/script.rb

Line 1154 in 8845df6

script.update!(options.merge(skip_name_format_validation: true))

the error was being caught and added to the list of errors on the script object:

code-dot-org/dashboard/app/models/script.rb

Lines 1183 to 1184 in 8845df6

	rescue StandardError => e
	errors.add(:base, e.to_s)

the errors were even being returned to the client:

code-dot-org/dashboard/app/controllers/scripts_controller.rb

Line 132 in 36bf378

render json: @script.errors

but, because the status code was not a 4xx, the client treated the response as a success.

fix

The fix is just to set the right status code in the server response. The client code then takes care of displaying the error message.

before

after

Future work

in the process of debugging this, I discovered that there are nested transactions in this codepath:

• code-dot-org/dashboard/app/models/script.rb

  Lines 1163 to 1168 in def7f2c

  	def update_text(script_params, script_text, metadata_i18n, general_params)
  	script_name = script_params[:name]
  	begin
  	transaction do
  	script_data, i18n = ScriptDSL.parse(script_text, 'input', script_name)
  	Script.add_script(

• code-dot-org/dashboard/app/models/script.rb

  Lines 989 to 990 in def7f2c

  	def self.add_script(options, raw_lesson_groups, new_suffix: nil, editor_experiment: nil)
  	transaction do

and that with default options, nested transactions are bad:

• https://pragtob.wordpress.com/2017/12/12/surprises-with-nested-transactions-rollbacks-and-activerecord/
• https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Nested+transactions

Fixed in #38775

Testing story

existing test coverage, plus manually verifying the result shown in the screenshots above.

Reviewer 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

[dmcavoy]

Thank you for finding this!