From a04be2de957ca2c680c6f8fe5a438eafab2ba813 Mon Sep 17 00:00:00 2001
From: Manu Vasconcelos <87862340+vasconsaurus@users.noreply.github.com>
Date: Mon, 23 Sep 2024 14:06:52 -0300
Subject: [PATCH] =?UTF-8?q?5120=20=E2=80=93=20Create=20Tags=20in=20the=20b?=
 =?UTF-8?q?ackground=20(#2005)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Context
While working on API/Zapier Error: The app did not respond in-time. It may or may not have completed successfully we thought tags creation could be causing the slow response.

We couldn’t pinpoint it as the cause, but it’s still a good performance improvement and should help improve the response time. So we are moving forward with it.

Sidekiq’s delayed extensions were deprecated in version 5 and removed in 7. So we are taking this opportunity to create a GenericJob, and eventually substitute the instances of .delay/.delay_for we have.

Implementation Notes
- We created a GenericWorker.
    - Used that to create Tags in the background while creating a ProjectMedia. We are making sure only 1 job is created.
    - Pass User.current to the GenericWorker, to make sure we are logging this action
- Added Sentry Notification and logging.
- We created a helper method so this is easier to read.
   - We did so by extending Class with a module.

Tests
rails test test/models/tag_test.rb:288
rails test test/workers/generic_worker_test.rb
rails test test/lib/generic_worker_helpers_test.rb

References: 5120, 4959
PR: 2005
---
 app/lib/generic_worker_helpers.rb             |  9 +++
 app/models/concerns/project_media_creators.rb |  8 +-
 app/models/project_media.rb                   | 14 +++-
 app/workers/generic_worker.rb                 | 18 +++++
 config/initializers/class_extensions.rb       |  3 +
 lib/sample_data.rb                            |  1 +
 test/lib/generic_worker_helpers_test.rb       | 34 ++++++++
 test/models/project_media_8_test.rb           | 72 +++++++++++++++++
 test/workers/generic_worker_test.rb           | 79 +++++++++++++++++++
 9 files changed, 235 insertions(+), 3 deletions(-)
 create mode 100644 app/lib/generic_worker_helpers.rb
 create mode 100644 app/workers/generic_worker.rb
 create mode 100644 config/initializers/class_extensions.rb
 create mode 100644 test/lib/generic_worker_helpers_test.rb
 create mode 100644 test/models/project_media_8_test.rb
 create mode 100644 test/workers/generic_worker_test.rb

diff --git a/app/lib/generic_worker_helpers.rb b/app/lib/generic_worker_helpers.rb
new file mode 100644
index 0000000000..0fbfeec70a
--- /dev/null
+++ b/app/lib/generic_worker_helpers.rb
@@ -0,0 +1,9 @@
+module GenericWorkerHelpers
+  def run_later(klass_method, *method_args)
+    GenericWorker.perform_async(self.to_s, klass_method, *method_args)
+  end
+
+  def run_later_in(time, klass_method, *method_args)
+    GenericWorker.perform_in(time, self.to_s, klass_method, *method_args)
+  end
+end
diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb
index 011bca7ef9..4652e01df8 100644
--- a/app/models/concerns/project_media_creators.rb
+++ b/app/models/concerns/project_media_creators.rb
@@ -263,7 +263,11 @@ def create_claim_description_and_fact_check
     fc
   end
 
-  def create_tags
-    self.set_tags.each { |tag| Tag.create!(annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array)
+  def create_tags_in_background
+    if self.set_tags.is_a?(Array)
+      project_media_id = self.id
+      tags_json = self.set_tags.to_json
+      ProjectMedia.run_later_in(1.second, 'create_tags', project_media_id, tags_json, user_id: self.user_id)
+    end
   end
 end
diff --git a/app/models/project_media.rb b/app/models/project_media.rb
index 8fc1969578..20674d674a 100644
--- a/app/models/project_media.rb
+++ b/app/models/project_media.rb
@@ -33,7 +33,7 @@ class ProjectMedia < ApplicationRecord
 
   before_validation :set_team_id, :set_channel, :set_project_id, on: :create
   before_validation :create_original_claim, if: proc { |pm| pm.set_original_claim.present? }, on: :create
-  after_create :create_annotation, :create_metrics_annotation, :send_slack_notification, :create_relationship, :create_team_tasks, :create_claim_description_and_fact_check, :create_tags
+  after_create :create_annotation, :create_metrics_annotation, :send_slack_notification, :create_relationship, :create_team_tasks, :create_claim_description_and_fact_check, :create_tags_in_background
   after_create :add_source_creation_log, unless: proc { |pm| pm.source_id.blank? }
   after_commit :apply_rules_and_actions_on_create, :set_quote_metadata, :notify_team_bots_create, on: [:create]
   after_commit :create_relationship, on: [:update]
@@ -528,6 +528,18 @@ def add_nested_objects(ms)
     ms.attributes[:requests] = requests
   end
 
+  def self.create_tags(project_media_id, tags_json)
+    project_media = ProjectMedia.find_by_id(project_media_id)
+
+    if !project_media.nil?
+      tags = JSON.parse(tags_json)
+      tags.each { |tag| Tag.create! annotated: project_media, tag: tag.strip, skip_check_ability: true }
+    else
+      error = StandardError.new("[ProjectMedia] Exception creating project media's tags in background. Project media is nil.")
+      CheckSentry.notify(error, project_media_id: project_media_id)
+    end
+  end
+
   # private
   #
   # Please add private methods to app/models/concerns/project_media_private.rb
diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb
new file mode 100644
index 0000000000..4915f97c26
--- /dev/null
+++ b/app/workers/generic_worker.rb
@@ -0,0 +1,18 @@
+class GenericWorker
+
+  include Sidekiq::Worker
+
+  def perform(klass_name, klass_method, *method_args)
+    klass = klass_name.constantize
+    options = method_args.extract_options!.with_indifferent_access
+    unless options.blank?
+      user_id = options.delete(:user_id) if options.key?(:user_id)
+      current_user = User.current
+      User.current = User.find_by_id(user_id)
+      klass.public_send(klass_method, *method_args, **options)
+      User.current = current_user
+    else
+      klass.public_send(klass_method)
+    end
+  end
+end
diff --git a/config/initializers/class_extensions.rb b/config/initializers/class_extensions.rb
new file mode 100644
index 0000000000..b6fffb32ff
--- /dev/null
+++ b/config/initializers/class_extensions.rb
@@ -0,0 +1,3 @@
+class Class
+include GenericWorkerHelpers
+end
diff --git a/lib/sample_data.rb b/lib/sample_data.rb
index a4e3499ac7..e7ba63368e 100644
--- a/lib/sample_data.rb
+++ b/lib/sample_data.rb
@@ -517,6 +517,7 @@ def create_project_media(options = {})
     end
     options[:skip_autocreate_source] = true unless options.has_key?(:skip_autocreate_source)
     pm.source = create_source({ team: options[:team], skip_check_ability: true }) if options[:skip_autocreate_source]
+    pm.set_tags = options[:tags] if options[:tags]
     pm.save!
     create_cluster_project_media({ cluster: options[:cluster], project_media: pm}) if options[:cluster]
     pm.reload
diff --git a/test/lib/generic_worker_helpers_test.rb b/test/lib/generic_worker_helpers_test.rb
new file mode 100644
index 0000000000..11d390d999
--- /dev/null
+++ b/test/lib/generic_worker_helpers_test.rb
@@ -0,0 +1,34 @@
+require_relative '../test_helper'
+
+class GenericWorkerHelpersTest < ActionView::TestCase
+  def setup
+    require 'sidekiq/testing'
+    Sidekiq::Worker.clear_all
+  end
+
+  def teardown
+  end
+
+  test "should run a job, without raising an error, for a method that takes a hash as a parameter" do
+    Sidekiq::Testing.inline!
+
+    assert_difference "Team.where(name: 'BackgroundTeam', slug: 'background-team').count" do
+      Team.run_later('create!', name: 'BackgroundTeam', slug: 'background-team')
+    end
+  end
+
+  test "should run a job, without raising an error, for a method that takes standalone parameters" do
+    Sidekiq::Testing.inline!
+
+    team = create_team
+    project = create_project team: team
+    pm = create_project_media project: project
+
+    project_media_id = pm.id
+    tags_json = ['one', 'two', 'three'].to_json
+
+    ProjectMedia.run_later_in(0.second, 'create_tags', project_media_id, tags_json, user_id: pm.user_id)
+
+    assert_equal 3, pm.annotations('tag').count
+  end
+end
diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb
new file mode 100644
index 0000000000..42f9b9c41d
--- /dev/null
+++ b/test/models/project_media_8_test.rb
@@ -0,0 +1,72 @@
+require_relative '../test_helper'
+
+class ProjectMedia8Test < ActiveSupport::TestCase
+  def setup
+    require 'sidekiq/testing'
+    Sidekiq::Worker.clear_all
+  end
+
+  def teardown
+  end
+
+  test ":create_tags should create tags when project media id and tags are present" do
+    team = create_team
+    project = create_project team: team
+    pm = create_project_media project: project
+
+    project_media_id = pm.id
+    tags_json = ['one', 'two'].to_json
+
+    assert_nothing_raised do
+      ProjectMedia.create_tags(project_media_id, tags_json)
+    end
+    assert_equal 2, pm.annotations('tag').count
+  end
+
+  test ":create_tags should not raise an error when no project media is sent" do
+    project_media_id = nil
+    tags_json = ['one', 'two'].to_json
+
+    assert_nothing_raised do
+      CheckSentry.expects(:notify).once
+      ProjectMedia.create_tags(project_media_id, tags_json)
+    end
+  end
+
+  test "when creating an item with tag/tags, after_create :create_tags_in_background callback should create tags in the background" do
+    Sidekiq::Testing.inline!
+
+    team = create_team
+    project = create_project team: team
+    pm = create_project_media project: project, tags: ['one']
+
+    assert_equal 1, pm.annotations('tag').count
+  end
+
+  test "when creating an item with multiple tags, after_create :create_tags_in_background callback should only schedule one job" do
+    Sidekiq::Testing.fake!
+
+    team = create_team
+    project = create_project team: team
+
+    assert_nothing_raised do
+      create_project_media project: project, tags: ['one', 'two', 'three']
+    end
+    assert_equal 1, GenericWorker.jobs.size
+  end
+
+  test "when using :run_later_in to schedule multiple tags creation, it should only schedule one job" do
+    Sidekiq::Testing.fake!
+
+    team = create_team
+    project = create_project team: team
+    pm = create_project_media project: project
+
+    project_media_id = pm.id
+    tags_json = ['one', 'two', 'three'].to_json
+
+    ProjectMedia.run_later_in(0.second, 'create_tags', project_media_id, tags_json, user_id: pm.user_id)
+
+    assert_equal 1, GenericWorker.jobs.size
+  end
+end
diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb
new file mode 100644
index 0000000000..628602a0da
--- /dev/null
+++ b/test/workers/generic_worker_test.rb
@@ -0,0 +1,79 @@
+require 'test_helper'
+
+class GenericWorkerTest < ActiveSupport::TestCase
+  def setup
+    require 'sidekiq/testing'
+    Sidekiq::Worker.clear_all
+  end
+
+  def teardown
+  end
+
+  test "should run a job, without raising an error, for a method that takes no parameters" do
+    Sidekiq::Testing.inline!
+
+    assert_difference 'Blank.count' do
+      GenericWorker.perform_async('Blank', 'create!')
+    end
+  end
+
+  test "should run a job, without raising an error, for a method that takes a hash as a parameter" do
+    Sidekiq::Testing.inline!
+
+    assert_difference "Team.where(name: 'BackgroundTeam', slug: 'background-team').count" do
+      GenericWorker.perform_async('Team', 'create!', name: 'BackgroundTeam', slug: 'background-team')
+    end
+  end
+
+  test "should run a job, without raising an error, for a method that takes standalone parameters" do
+    Sidekiq::Testing.inline!
+
+    t = create_team
+    p = create_project team: t
+    pm = create_project_media project: p
+
+    project_media_id = pm.id
+    tags_json = ['one', 'two'].to_json
+
+    assert_difference "Tag.where(annotation_type: 'tag').count", difference = 2 do
+      GenericWorker.perform_async('ProjectMedia', 'create_tags', project_media_id, tags_json, user_id: pm.user_id)
+    end
+  end
+
+  test "should schedule a job, without raising an error, for a method that takes no parameters" do
+    Sidekiq::Testing.fake!
+
+    assert_nothing_raised do
+      GenericWorker.perform_async('Media', 'types')
+    end
+
+    assert_equal 1, GenericWorker.jobs.size
+  end
+
+  test "should schedule a job, without raising an error, for a method that takes a hash as a parameter" do
+    Sidekiq::Testing.fake!
+
+    assert_nothing_raised do
+      GenericWorker.perform_async('Team', 'create!', name: 'BackgroundTeam', slug: 'background-team')
+    end
+
+    assert_equal 1, GenericWorker.jobs.size
+  end
+
+  test "should schedule a job, without raising an error, for a method that takes standalone parameters" do
+    Sidekiq::Testing.fake!
+
+    t = create_team
+    p = create_project team: t
+    pm = create_project_media project: p
+
+    project_media_id = pm.id
+    tags_json = ['one', 'two'].to_json
+
+    assert_nothing_raised do
+      GenericWorker.perform_async('ProjectMedia', 'create_tags', project_media_id, tags_json, user_id: pm.user_id)
+    end
+
+    assert_equal 1, GenericWorker.jobs.size
+  end
+end