diff --git a/app/controllers/admin/badges_controller.rb b/app/controllers/admin/badges_controller.rb index 00a47fc6705..eea4b45ba4c 100644 --- a/app/controllers/admin/badges_controller.rb +++ b/app/controllers/admin/badges_controller.rb @@ -48,33 +48,22 @@ class Admin::BadgesController < Admin::AdminController def create badge = Badge.new - update_badge_from_params(badge) - badge.id = nil - badge.save! - render_serialized(badge, BadgeSerializer, root: "badge") + errors = update_badge_from_params(badge, new: true) + + if errors.present? + render_json_error errors + else + render_serialized(badge, BadgeSerializer, root: "badge") + end end def update badge = find_badge - error = nil - Badge.transaction do - update_badge_from_params(badge) + errors = update_badge_from_params(badge) - # Perform checks to prevent bad queries - begin - BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger }) - rescue => e - # noinspection RubyUnusedLocalVariable - error = e.message - raise ActiveRecord::Rollback - end - - badge.save! - end - - if error - render_json_error error + if errors.present? + render_json_error errors else render_serialized(badge, BadgeSerializer, root: "badge") end @@ -91,16 +80,36 @@ class Admin::BadgesController < Admin::AdminController Badge.find(params[:id]) end - def update_badge_from_params(badge) - allowed = Badge.column_names.map(&:to_sym) - allowed -= [:id, :created_at, :updated_at, :grant_count] - allowed -= Badge.protected_system_fields if badge.system? - params.permit(*allowed) + # Options: + # :new - reset the badge id to nil before saving + def update_badge_from_params(badge, opts={}) + errors = [] + Badge.transaction do + allowed = Badge.column_names.map(&:to_sym) + allowed -= [:id, :created_at, :updated_at, :grant_count] + allowed -= Badge.protected_system_fields if badge.system? + params.permit(*allowed) - allowed.each do |key| - badge.send("#{key}=" , params[key]) if params[key] + allowed.each do |key| + badge.send("#{key}=" , params[key]) if params[key] + end + + # Badge query contract checks + begin + BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger }) + rescue => e + errors << [e.message] + raise ActiveRecord::Rollback + end + + badge.id = nil if opts[:new] + badge.save! end - badge + if badge.errors + errors.push(*badge.errors.full_messages) + end + + errors end end diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index 143780aebe6..2f896682b6c 100644 --- a/app/services/badge_granter.rb +++ b/app/services/badge_granter.rb @@ -155,6 +155,7 @@ class BadgeGranter end raise "Contract violation:\nQuery does not return a 'user_id' column" unless sql.match /user_id/ raise "Contract violation:\nQuery does not return a 'granted_at' column" unless sql.match /granted_at/ + raise "Contract violation:\nQuery ends with a semicolon. Remove the semicolon; your sql will be used in a subquery." if sql.match /;\s*\z/ end # Options: diff --git a/lib/json_error.rb b/lib/json_error.rb index e7411cf5c76..2abc927a9eb 100644 --- a/lib/json_error.rb +++ b/lib/json_error.rb @@ -11,6 +11,12 @@ module JsonError # If it looks like an activerecord object, extract its messages return {errors: obj.errors.full_messages } if obj.respond_to?(:errors) && obj.errors.present? + # If we're passed an array, it's an array of error messages + return {errors: obj.map {|e| e.to_s}} if obj.is_a?(Array) && obj.present? + + # Log a warning (unless obj is nil) + Rails.logger.warn("create_errors_json called with unrecognized type: #{obj.inspect}") if obj + # default to a generic error JsonError.generic_error end