diff --git a/app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6 b/app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6 new file mode 100644 index 00000000000..e141dd24e40 --- /dev/null +++ b/app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6 @@ -0,0 +1,49 @@ +export default Ember.Controller.extend({ + needs: ['modal'], + + sample: Em.computed.alias('model.sample'), + errors: Em.computed.alias('model.errors'), + count: Em.computed.alias('model.grant_count'), + + count_warning: function() { + if (this.get('count') <= 10) { + return this.get('sample.length') !== this.get('count'); + } else { + return this.get('sample.length') !== 10; + } + }.property('count', 'sample.length'), + + has_query_plan: function() { + return !!this.get('model.query_plan'); + }.property('model.query_plan'), + + query_plan_html: function() { + var raw = this.get('model.query_plan'), + returned = "
";
+
+    _.each(raw, function(linehash) {
+      returned += Handlebars.Utils.escapeExpression(linehash["QUERY PLAN"]);
+      returned += "
"; + }); + + returned += "
"; + return returned; + }.property('model.query_plan'), + + processed_sample: Ember.computed.map('model.sample', function(grant) { + var i18nKey = 'admin.badges.preview.grant.with', + i18nParams = { username: Handlebars.Utils.escapeExpression(grant.username) }; + + if (grant.post_id) { + i18nKey += "_post"; + i18nParams.link = "" + Handlebars.Utils.escapeExpression(grant.title) + ""; + } + + if (grant.granted_at) { + i18nKey += "_time"; + i18nParams.time = Handlebars.Utils.escapeExpression(moment(grant.granted_at).format(I18n.t('dates.long_with_year'))); + } + + return I18n.t(i18nKey, i18nParams); + }) +}); diff --git a/app/assets/javascripts/admin/controllers/admin-badges.js.es6 b/app/assets/javascripts/admin/controllers/admin-badges.js.es6 index 199c7eab1ee..c679db8402b 100644 --- a/app/assets/javascripts/admin/controllers/admin-badges.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-badges.js.es6 @@ -78,21 +78,6 @@ export default Ember.ArrayController.extend({ actions: { - preview: function(badge) { - // TODO wire modal and localize - Discourse.ajax('/admin/badges/preview.json', { - method: 'post', - data: {sql: badge.query, target_posts: !!badge.target_posts} - }).then(function(json){ - if(json.error){ - bootbox.alert(json.error); - } else { - bootbox.alert(json.grant_count + " badges to be assigned"); - } - }); - - }, - /** Create a new badge and select it. @@ -128,16 +113,21 @@ export default Ember.ArrayController.extend({ 'enabled', 'show_posts', 'target_posts', 'name', 'description', 'icon', 'query', 'badge_grouping_id', - 'trigger', 'badge_type_id']; + 'trigger', 'badge_type_id'], + self = this; - if(this.get('selectedItem.system')){ + if (this.get('selectedItem.system')){ var protectedFields = this.get('protectedSystemFields'); fields = _.filter(fields, function(f){ return !_.include(protectedFields,f); }); } - this.get('selectedItem').save(fields); + this.get('selectedItem').save(fields).catch(function(error) { + // this shows the admin-badge-preview modal with the error + // kinda weird, but it consolidates the display logic for badge errors + self.send('saveError', error); + }); } }, diff --git a/app/assets/javascripts/admin/routes/admin_badges_route.js b/app/assets/javascripts/admin/routes/admin_badges_route.js index ba8fb4f9c57..4ef15173d36 100644 --- a/app/assets/javascripts/admin/routes/admin_badges_route.js +++ b/app/assets/javascripts/admin/routes/admin_badges_route.js @@ -15,8 +15,39 @@ Discourse.AdminBadgesRoute = Discourse.Route.extend({ }, actions: { - editGroupings: function(model){ + editGroupings: function(model) { Discourse.Route.showModal(this, 'admin_edit_badge_groupings', model); + }, + + saveError: function(jqXhr) { + if (jqXhr.status === 422) { + Discourse.Route.showModal(this, 'admin_badge_preview', jqXhr.responseJSON); + } else { + Em.Logger.error(jqXhr); + bootbox.alert(I18n.t('errors.description.unknown')); + } + }, + + preview: function(badge, explain) { + var self = this; + + badge.set('preview_loading', true); + Discourse.ajax('/admin/badges/preview.json', { + method: 'post', + data: { + sql: badge.query, + target_posts: !!badge.target_posts, + trigger: badge.trigger, + explain: explain + } + }).then(function(json) { + badge.set('preview_loading', false); + Discourse.Route.showModal(self, 'admin_badge_preview', json); + }).catch(function(error) { + badge.set('preview_loading', false); + Em.Logger.error(error); + bootbox.alert("Network error"); + }); } } diff --git a/app/assets/javascripts/admin/templates/badges.js.handlebars b/app/assets/javascripts/admin/templates/badges.js.handlebars index 57d7ae71008..1f0a42725e2 100644 --- a/app/assets/javascripts/admin/templates/badges.js.handlebars +++ b/app/assets/javascripts/admin/templates/badges.js.handlebars @@ -76,7 +76,12 @@ {{#if hasQuery}} - {{i18n admin.badges.preview}} + {{i18n admin.badges.preview.link_text}} + | + {{i18n admin.badges.preview.plan_text}} + {{#if preview_loading}} + {{i18n loading}}... + {{/if}}
diff --git a/app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars b/app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars new file mode 100644 index 00000000000..12d78af55d1 --- /dev/null +++ b/app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars @@ -0,0 +1,44 @@ +
+ {{#if errors}} +

{{i18n admin.badges.preview.sql_error_header}}

+ +
+ {{errors}} +
+ + {{else}} +

{{{i18n admin.badges.preview.grant_count count=count}}}

+ + {{#if count_warning}} +
+

{{i18n admin.badges.preview.bad_count_warning.header}}

+

{{i18n admin.badges.preview.bad_count_warning.text}}

+
+ {{/if}} + + {{#if sample}} +

+ {{i18n admin.badges.preview.sample}} +

+
    + {{#each html in processed_sample}} +
  • {{{html}}}
  • + {{/each}} +
+ {{/if}} + + {{#if has_query_plan}} +
+ {{{query_plan_html}}} +
+ {{/if}} + {{/if}} +
diff --git a/app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js b/app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js new file mode 100644 index 00000000000..0d890b3d663 --- /dev/null +++ b/app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js @@ -0,0 +1,5 @@ + +Discourse.AdminBadgePreviewView = Discourse.ModalBodyView.extend({ + templateName: 'admin/templates/modal/admin_badge_preview', + title: I18n.t('admin.badges.preview.modal_title') +}); diff --git a/app/assets/javascripts/discourse/models/badge.js b/app/assets/javascripts/discourse/models/badge.js index 3752bf5a57e..43ca8d5e750 100644 --- a/app/assets/javascripts/discourse/models/badge.js +++ b/app/assets/javascripts/discourse/models/badge.js @@ -146,10 +146,10 @@ Discourse.Badge = Discourse.Model.extend({ self.set('savingStatus', I18n.t('saved')); self.set('saving', false); return self; - }, function(error){ - self.set('savingStatus', ''); + }).catch(function(error) { + self.set('savingStatus', I18n.t('failed')); self.set('saving', false); - bootbox.alert(error.responseText); + throw error; }); }, diff --git a/app/assets/stylesheets/common/admin/admin_base.scss b/app/assets/stylesheets/common/admin/admin_base.scss index 28342d4b210..748c21ff8a8 100644 --- a/app/assets/stylesheets/common/admin/admin_base.scss +++ b/app/assets/stylesheets/common/admin/admin_base.scss @@ -419,6 +419,38 @@ section.details { } } +.badge-query-preview { + .grant-count, .sample, .error-header { + margin-left: 10px; + } + + .badge-query-plan, .badge-errors { + padding: 4px; + background-color: scale-color-diff(); + } + + .badge-query-plan { + font-size: 80%; + } + .badge-errors { + font-family: monospace; + } + + .count-warning { + background-color: dark-light-diff(rgba($danger,.7), $secondary, 50%, -60%); + margin: 0 0 7px 0; + padding: 10px 20px; + + p { + margin: 0; + } + .heading { + color: $danger; + font-weight: bold; + } + } +} + // Customise area .customize { .nav.nav-pills { diff --git a/app/controllers/admin/badges_controller.rb b/app/controllers/admin/badges_controller.rb index cd5170ee98d..00a47fc6705 100644 --- a/app/controllers/admin/badges_controller.rb +++ b/app/controllers/admin/badges_controller.rb @@ -14,7 +14,10 @@ class Admin::BadgesController < Admin::AdminController end def preview - render json: BadgeGranter.preview(params[:sql], target_posts: params[:target_posts] == "true") + render json: BadgeGranter.preview(params[:sql], + target_posts: params[:target_posts] == "true", + explain: params[:explain] == "true", + trigger: params[:trigger].to_i) end def badge_types @@ -53,9 +56,28 @@ class Admin::BadgesController < Admin::AdminController def update badge = find_badge - update_badge_from_params(badge) - badge.save! - render_serialized(badge, BadgeSerializer, root: "badge") + + error = nil + Badge.transaction do + 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 + else + render_serialized(badge, BadgeSerializer, root: "badge") + end end def destroy diff --git a/app/models/badge.rb b/app/models/badge.rb index 5c398ff5ff3..a31580231df 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -31,6 +31,18 @@ class Badge < ActiveRecord::Base PostRevision = 2 TrustLevelChange = 4 UserChange = 8 + + def self.is_none?(trigger) + [0].include? trigger + end + + def self.uses_user_ids?(trigger) + [4, 8].include? trigger + end + + def self.uses_post_ids?(trigger) + [1, 2].include? trigger + end end module Queries diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index ade8d503fa7..46155e79cea 100644 --- a/app/services/badge_granter.rb +++ b/app/services/badge_granter.rb @@ -132,9 +132,38 @@ class BadgeGranter "badge_queue".freeze end + # Options: + # :target_posts - whether the badge targets posts + # :trigger - the Badge::Trigger id + def self.contract_checks!(sql, opts = {}) + return unless sql.present? + if Badge::Trigger.uses_post_ids?(opts[:trigger]) + raise "Contract violation:\nQuery triggers on posts, but does not reference the ':post_ids' array" unless sql.match /:post_ids/ + end + if Badge::Trigger.uses_user_ids?(opts[:trigger]) + raise "Contract violation:\nQuery triggers on users, but does not reference the ':user_ids' array" unless sql.match /:user_ids/ + end + if opts[:trigger] && !Badge::Trigger.is_none?(opts[:trigger]) + raise "Contract violation:\nQuery is triggered, but does not reference the ':backfill' parameter.\n(Hint: if :backfill is TRUE, you should ignore the :post_ids/:user_ids)" unless sql.match /:backfill/ + end + + # TODO these three conditions have a lot of false negatives + if opts[:target_posts] + raise "Contract violation:\nQuery targets posts, but does not return a 'post_id' column" unless sql.match /post_id/ + 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/ + end + + # Options: + # :target_posts - whether the badge targets posts + # :trigger - the Badge::Trigger id + # :explain - return the EXPLAIN query def self.preview(sql, opts = {}) params = {user_ids: [], post_ids: [], backfill: true} + BadgeGranter.contract_checks!(sql, opts) + # hack to allow for params, otherwise sanitizer will trigger sprintf count_sql = "SELECT COUNT(*) count FROM (#{sql}) q WHERE :backfill = :backfill" grant_count = SqlBuilder.map_exec(OpenStruct, count_sql, params).first.count.to_i @@ -156,23 +185,34 @@ class BadgeGranter LIMIT 10" end + query_plan = nil + query_plan = ActiveRecord::Base.exec_sql("EXPLAIN #{sql}", params) if opts[:explain] + sample = SqlBuilder.map_exec(OpenStruct, grants_sql, params).map(&:to_h) - {grant_count: grant_count, sample: sample} + sample.each do |result| + raise "Query returned a non-existent user ID:\n#{result[:id]}" unless User.find(result[:id]).present? + raise "Query did not return a badge grant time\n(Try using 'current_timestamp granted_at')" unless result[:granted_at] + if opts[:target_posts] + raise "Query did not return a post ID" unless result[:post_id] + raise "Query returned a non-existent post ID:\n#{result[:post_id]}" unless Post.find(result[:post_id]).present? + end + end + + {grant_count: grant_count, sample: sample, query_plan: query_plan} rescue => e - {error: e.to_s} + {errors: e.message} end MAX_ITEMS_FOR_DELTA = 200 def self.backfill(badge, opts=nil) return unless badge.query.present? && badge.enabled + post_ids = user_ids = nil + post_ids = opts[:post_ids] if opts user_ids = opts[:user_ids] if opts - post_ids = nil unless post_ids.present? - user_ids = nil unless user_ids.present? - # safeguard fall back to full backfill if more than 200 if (post_ids && post_ids.length > MAX_ITEMS_FOR_DELTA) || (user_ids && user_ids.length > MAX_ITEMS_FOR_DELTA) @@ -180,6 +220,9 @@ class BadgeGranter user_ids = nil end + post_ids = nil unless post_ids.present? + user_ids = nil unless user_ids.present? + full_backfill = !user_ids && !post_ids post_clause = badge.target_posts ? "AND (q.post_id = ub.post_id OR NOT :multiple_grant)" : "" diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 967de72cf3d..0c7ddc673b0 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -204,6 +204,7 @@ en: disable: "Disable" undo: "Undo" revert: "Revert" + failed: "Failed" banner: close: "Dismiss this banner." @@ -2024,7 +2025,6 @@ en: target_posts: Query targets posts auto_revoke: Run revocation query daily show_posts: Show post granting badge on badge page - preview: Preview badge trigger: Trigger trigger_type: none: "Update daily" @@ -2032,6 +2032,25 @@ en: post_revision: "When a user edits or creates a post" trust_level_change: "When a user changes trust level" user_change: "When a user is edited or created" + preview: + link_text: "Preview granted badges" + plan_text: "Preview with query plan" + modal_title: "Badge Query Preview" + sql_error_header: "There was an error with the query." + error_help: "See the following links for help with badge queries." + bad_count_warning: + header: "WARNING!" + text: "There are missing grant samples. This happens when the badge query returns user IDs or post IDs that do not exist. This may cause unexpected results later on - please double-check your query." + grant_count: + zero: "No badges to be assigned." + one: "1 badge to be assigned." + other: "%{count} badges to be assigned." + sample: "Sample:" + grant: + with: %{username} + with_post: %{username} for post in %{link} + with_post_time: %{username} for post in %{link} at %{time} + with_time: %{username} at %{time} lightbox: download: "download"