REFACTOR: Quick refactor of the webhook event emitter job (#7385)

REFACTOR: Quick refactor of the webhook event emitter job (#7385)

  • REFACTOR: Quick refactor of the webhook event emitter job
diff --git a/app/jobs/regular/emit_web_hook_event.rb b/app/jobs/regular/emit_web_hook_event.rb
index cdba6da..4095f3b 100644
--- a/app/jobs/regular/emit_web_hook_event.rb
+++ b/app/jobs/regular/emit_web_hook_event.rb
@@ -7,62 +7,39 @@ module Jobs
     RETRY_BACKOFF = 5
 
     def execute(args)
-      %i{
-        web_hook_id
-        event_type
-      }.each do |key|
-        raise Discourse::InvalidParameters.new(key) unless args[key].present?
-      end
-
-      @orig_args = args.dup
-
-      web_hook = WebHook.find_by(id: args[:web_hook_id])
-      raise Discourse::InvalidParameters.new(:web_hook_id) if web_hook.blank?
+      memoize_arguments(args)
+      validate_arguments!
 
-      unless ping_event?(args[:event_type])
-        return unless web_hook.active?
+      unless ping_event?(arguments[:event_type])
+        validate_argument!(:payload)
 
-        return if web_hook.group_ids.present? && (args[:group_id].present? ||
-          !web_hook.group_ids.include?(args[:group_id]))
-
-        return if web_hook.category_ids.present? && (!args[:category_id].present? ||
-          !web_hook.category_ids.include?(args[:category_id]))
-
-        return if web_hook.tag_ids.present? && (args[:tag_ids].blank? ||
-          (web_hook.tag_ids & args[:tag_ids]).blank?)
-
-        raise Discourse::InvalidParameters.new(:payload) unless args[:payload].present?
-        args[:payload] = JSON.parse(args[:payload])
+        return if webhook_inactive?
+        return if group_webhook_invalid?
+        return if category_webhook_invalid?
+        return if tag_webhook_invalid?
       end
 
-      web_hook_request(args, web_hook)
+      send_webhook!
     end
 
     private
 
-    def guardian
-      Guardian.new(Discourse.system_user)
+    def validate_arguments!
+      validate_argument!(:web_hook_id)
+      validate_argument!(:event_type)
+      raise Discourse::InvalidParameters.new(:web_hook_id) if web_hook.blank?
     end
 
-    def ping_event?(event_type)
-      PING_EVENT == event_type.to_s
+    def validate_argument!(key)
+      raise Discourse::InvalidParameters.new(key) unless arguments[key].present?
     end
 
-    def build_web_hook_body(args, web_hook)
-      body = {}
-      event_type = args[:event_type].to_s
-
-      if ping_event?(event_type)
-        body[:ping] = 'OK'
-      else
-        body[event_type] = args[:payload]
-      end
-
-      new_body = Plugin::Filter.apply(:after_build_web_hook_body, self, body)
-      MultiJson.dump(new_body)
+    def memoize_arguments(args)
+      @arguments = args
+      @retry_count = @arguments[:retry_count] || 0
     end
 
-    def web_hook_request(args, web_hook)
+    def send_webhook!
       uri = URI(web_hook.payload_url.strip)
 
       conn = Excon.new(
@@ -71,42 +48,17 @@ module Jobs
         retry_limit: 0
       )
 
-      body = build_web_hook_body(args, web_hook)
-      web_hook_event = WebHookEvent.create!(web_hook_id: web_hook.id, payload: body)
+      web_hook_body = build_webhook_body
+      web_hook_event = create_webhook_event(web_hook_body)
+      web_hook_headers = build_webhook_headers(uri, web_hook_body, web_hook_event)
+
       response = nil
 
       begin
-        content_type =
-          case web_hook.content_type
-          when WebHook.content_types['application/x-www-form-urlencoded']
-            'application/x-www-form-urlencoded'
-          else
-            'application/json'
-          end
-
-        headers = {
-          'Accept' => '*/*',
-          'Connection' => 'close',
-          'Content-Length' => body.bytesize,
-          'Content-Type' => content_type,
-          'Host' => uri.host,
-          'User-Agent' => "Discourse/#{Discourse::VERSION::STRING}",
-          'X-Discourse-Instance' => Discourse.base_url,
-          'X-Discourse-Event-Id' => web_hook_event.id,
-          'X-Discourse-Event-Type' => args[:event_type]
-        }
-
-        headers['X-Discourse-Event'] = args[:event_name].to_s if args[:event_name].present?
-
-        if web_hook.secret.present?
-          headers['X-Discourse-Event-Signature'] = "sha256=#{OpenSSL::HMAC.hexdigest("sha256", web_hook.secret, body)}"
-        end
-
         now = Time.zone.now
-        response = conn.post(headers: headers, body: body)
-
+        response = conn.post(headers: web_hook_headers, body: web_hook_body)
         web_hook_event.update!(
-          headers: MultiJson.dump(headers),
+          headers: MultiJson.dump(web_hook_headers),
           status: response.status,
           response_headers: MultiJson.dump(response.headers),
           response_body: response.body,
@@ -114,28 +66,114 @@ module Jobs
         )
       rescue => e
         web_hook_event.update!(
-          headers: MultiJson.dump(headers),
+          headers: MultiJson.dump(web_hook_headers),
           status: -1,
           response_headers: MultiJson.dump(error: e),
           duration: ((Time.zone.now - now) * 1000).to_i
         )
       end
 
+      publish_webhook_event(web_hook_event)
+      retry_web_hook if response&.status != 200
+    end
+
+    def retry_web_hook
+      if SiteSetting.retry_web_hook_events?
+        @retry_count += 1
+        return if @retry_count > MAX_RETRY_COUNT
+        delay = RETRY_BACKOFF**(@retry_count - 1)
+        Jobs.enqueue_in(delay.minutes, :emit_web_hook_event, arguments)
+      end
+    end
+
+    def publish_webhook_event(web_hook_event)
       MessageBus.publish("/web_hook_events/#{web_hook.id}", {
         web_hook_event_id: web_hook_event.id,
-        event_type: args[:event_type]
+        event_type: arguments[:event_type]
       }, user_ids: User.human_users.staff.pluck(:id))
+    end
 
-      retry_web_hook if response&.status != 200
+    def ping_event?(event_type)
+      PING_EVENT == event_type
     end
 
-    def retry_web_hook
-      if SiteSetting.retry_web_hook_events?
-        @orig_args[:retry_count] = (@orig_args[:retry_count] || 0) + 1
-        return if @orig_args[:retry_count] > MAX_RETRY_COUNT
-        delay = RETRY_BACKOFF**(@orig_args[:retry_count] - 1)
-        Jobs.enqueue_in(delay.minutes, :emit_web_hook_event, @orig_args)
+    def webhook_inactive?
+      !web_hook.active?
+    end
+
+    def group_webhook_invalid?
+      web_hook.group_ids.present? && (arguments[:group_id].present? ||
+        !web_hook.group_ids.include?(arguments[:group_id]))
+    end
+
+    def category_webhook_invalid?
+      web_hook.category_ids.present? && (!arguments[:category_id].present? ||
+        !web_hook.category_ids.include?(arguments[:category_id]))
+    end
+
+    def tag_webhook_invalid?
+      web_hook.tag_ids.present? && (arguments[:tag_ids].blank? ||
+        (web_hook.tag_ids & arguments[:tag_ids]).blank?)
+    end
+
+    def arguments
+      @arguments
+    end
+
+    def parsed_payload
+      @parsed_payload ||= JSON.parse(arguments[:payload])
+    end
+
+    def web_hook
+      @web_hook ||= WebHook.find_by(id: arguments[:web_hook_id])
+    end
+
+    def build_webhook_headers(uri, web_hook_body, web_hook_event)
+      content_type =
+        case web_hook.content_type
+        when WebHook.content_types['application/x-www-form-urlencoded']
+          'application/x-www-form-urlencoded'
+        else
+          'application/json'
+        end
+
+      headers = {
+        'Accept' => '*/*',
+        'Connection' => 'close',
+        'Content-Length' => web_hook_body.bytesize,
+        'Content-Type' => content_type,
+        'Host' => uri.host,
+        'User-Agent' => "Discourse/#{Discourse::VERSION::STRING}",
+        'X-Discourse-Instance' => Discourse.base_url,
+        'X-Discourse-Event-Id' => web_hook_event.id,
+        'X-Discourse-Event-Type' => arguments[:event_type]
+      }
+
+      headers['X-Discourse-Event'] = arguments[:event_name] if arguments[:event_name].present?
+
+      if web_hook.secret.present?
+        headers['X-Discourse-Event-Signature'] = "sha256=#{OpenSSL::HMAC.hexdigest("sha256", web_hook.secret, web_hook_body)}"
       end
+
+      headers
     end
+
+    def build_webhook_body
+      body = {}
+

[... diff too long, it was truncated ...]

GitHub sha: 02a9429c

This is quite strange to me since we’re wrapping a private method around @arguments when @arguments is directly accessible.

Do we need to memoize this when it is only called once? Also we could just assign it to the instance variable and access that variable directly.

I followed up in REFACTOR: Prefer accessing instance variables directly. · discourse/discourse@0210b0a · GitHub