Merge pull request #2741 from riking/badges_create_checks

FIX: Apply contract checks when first creating a badge

app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6

@@ -19,7 +19,7 @@ export default Ember.Controller.extend({
 
   query_plan_html: function() {
     var raw = this.get('model.query_plan'),
-        returned = "<pre>";
+        returned = "<pre class='badge-query-plan'>";
 
     _.each(raw, function(linehash) {
       returned += Handlebars.Utils.escapeExpression(linehash["QUERY PLAN"]);

app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars

@@ -2,9 +2,8 @@
   {{#if errors}}
     <p class="error-header">{{i18n admin.badges.preview.sql_error_header}}</p>
 
-    <div class="badge-errors">
-      {{errors}}
-    </div>
+    <pre class="badge-errors">{{errors}}</pre>
+
     <!--
     TODO we want some help pages for this, link to those instead
     <p>

app/assets/stylesheets/common/admin/admin_base.scss

@@ -424,16 +424,18 @@ section.details {
     margin-left: 10px;
   }
 
-  .badge-query-plan, .badge-errors {
+  .badge-errors {
+    font-size: 14px;
+    line-height: 16px;
     padding: 4px;
     background-color: scale-color-diff();
   }
 
   .badge-query-plan {
-    font-size: 80%;
-  }
-  .badge-errors {
-    font-family: monospace;
+    font-size: 12px;
+    line-height: 13px;
+    padding: 4px;
+    background-color: scale-color-diff();
   }
 
   .count-warning {

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

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:

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