Revert "Revert "add simple audit log to levels""

dashboard/app/controllers/levels_controller.rb

@@ -97,6 +97,7 @@ def update_blocks
     type = params[:type]
     blocks_xml = Blockly.convert_toolbox_to_category(blocks_xml) if type == 'toolbox_blocks'
     @level.properties[type] = blocks_xml
+    @level.log_changes(current_user)
     @level.save!
     render json: {redirect: level_url(@level)}
   end
@@ -110,7 +111,13 @@ def update
       # do not allow case-only changes in the level name because that confuses git on OSX
       @level.errors.add(:name, 'Cannot change only the capitalization of the level name (it confuses git on OSX)')
       render json: @level.errors, status: :unprocessable_entity
-    elsif @level.update(level_params)
+      return
+    end
+
+    @level.assign_attributes(level_params)
+    @level.log_changes(current_user)
+
+    if @level.save
       redirect = params["redirect"] || level_url(@level, show_callouts: 1)
       render json: {redirect: redirect}
     else

dashboard/app/models/levels/dsl_defined.rb

@@ -124,7 +124,7 @@ def dsl_text
     self.class.decrypt_dsl_text_if_necessary(File.read(file_path)) if file_path && File.exist?(file_path)
   end
 
-  def update(params)
+  def assign_attributes(params)
     if params[:dsl_text].present?
       self.class.create_from_level_builder({dsl_text: params.delete(:dsl_text)}, params, name)
     else

dashboard/app/models/levels/external.rb

@@ -55,8 +55,4 @@ def properties_with_replaced_markdown(user)
     user_id = user.try(:id).to_s
     properties.merge({'markdown' => properties['markdown'].gsub(USER_ID_REPLACE_STRING, user_id)})
   end
-
-  def update(params)
-    super(params)
-  end
 end

dashboard/app/models/levels/level.rb

@@ -372,6 +372,40 @@ def strip_name
     self.name = name.to_s.strip unless name.nil?
   end
 
+  def log_changes(user=nil)
+    return unless changed?
+
+    log = JSON.parse(audit_log || "[]")
+
+    # gather all field changes; if the properties JSON blob is one of the things
+    # that changed, rather than including just 'properties' in the list, include
+    # all of those attributes within properties that changed.
+    latest_changes = changed.dup
+    if latest_changes.include?('properties') && changed_attributes['properties']
+      latest_changes.delete('properties')
+      changed_attributes['properties'].each do |key, value|
+        latest_changes.push(key) unless properties[key] == value
+      end
+    end
+
+    entry = {
+      changed_at: Time.now,
+      changed: latest_changes
+    }
+    unless user.nil?
+      entry[:changed_by_id] = user.id
+      entry[:changed_by_email] = user.email
+    end
+    log.push(entry)
+
+    # Because this ever-growing log is stored in a limited column and because we
+    # will tend to care a lot less about older entries than newer ones, we will
+    # here drop older entries until this log gets down to a reasonable size
+    log.shift while JSON.dump(log).length >= 65535
+
+    self.audit_log = JSON.dump(log)
+  end
+
   def remove_empty_script_levels
     script_levels.each do |script_level|
       if script_level.levels.length == 1 && script_level.levels[0] == self

dashboard/test/models/dsl_test.rb

@@ -38,7 +38,7 @@ class DslTest < ActiveSupport::TestCase
   test 'name should not be modifiable' do
     level = External.create_from_level_builder({}, {dsl_text: "name 'test external'\ntitle 'test'"})
     assert_raises RuntimeError do
-      level = level.update(dsl_text: "name 'new test name'\ntitle 'abc'")
+      level.update(dsl_text: "name 'new test name'\ntitle 'abc'")
     end
     assert_equal 'test external', level.name
     assert_equal 'test', level.properties['title']
@@ -47,7 +47,8 @@ class DslTest < ActiveSupport::TestCase
 
   test 'should set serialized_attributes' do
     level = External.create_from_level_builder({}, {dsl_text: "name 'test external 2'"})
-    level = level.update(dsl_text: "name 'test external 2'\ntitle 'abc'", video_key: 'zzz')
+    level.update(dsl_text: "name 'test external 2'\ntitle 'abc'", video_key: 'zzz')
+    level.reload
     assert_equal 'zzz', level.video_key
     assert_equal 'abc', level.properties['title']
     assert_nil level.properties['name']

dashboard/test/models/level_test.rb

@@ -602,4 +602,47 @@ def create_maze
     assert_equal data[:description], level.description
     assert_equal data[:description], I18n.t("data.unplugged.#{data[:name]}.desc")
   end
+
+  test "audit log is initially null" do
+    data = @custom_maze_data.dup
+    data[:name] = "test audit log nul"
+    level = Level.create(data)
+    assert level.valid?
+    assert_nil level.audit_log
+  end
+
+  test "audit log will explode properties" do
+    data = @custom_maze_data.dup
+    data[:name] = "test audit log properties"
+    level = Level.create(data)
+
+    level.skin = "bee"
+    level.log_changes
+
+    assert_equal 1, JSON.parse(level.audit_log).length
+    assert_equal ["skin"], JSON.parse(level.audit_log).first["changed"]
+  end
+
+  test "audit log will self-truncate" do
+    data = @custom_maze_data.dup
+    data[:name] = "test audit log truncation"
+    level = Level.create(data)
+
+    # Create an audit log that is almost the max length
+    huge_array = ["test"] * 9360
+    level.audit_log = JSON.dump(huge_array)
+
+    assert_equal 65521, level.audit_log.length
+    assert_equal 9360, JSON.parse(level.audit_log).length
+
+    # add a new entry that will put us over the limit
+    level.instructions = "new actual instructions"
+    level.log_changes
+
+    # audit log should have dropped off several entries in order get back under
+    # the limit, since the test entries are individually much smaller than the
+    # new actual entry
+    assert_equal 65533, level.audit_log.length
+    assert_equal 9351, JSON.parse(level.audit_log).length
+  end
 end