From 97d2cd326b1e40048a3c6356b9699f4ffd96f585 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 26 Aug 2024 13:59:08 -0300 Subject: [PATCH 01/26] wip: create GenericWorker I tried to create a general structure of the worker and two tests: - one test just to make sure the GenericWorker works - one test to make sure tags are created in the background BUT: - The tests are wonky - The worker test passes but shouldn't - The tag tests doesn't work because create_tags is private: - I think that is the place where/when we create the tags, seem we use it in a after_create callback. But I'm not completelly sure, and if I'm right, not sure the best way to test it. --- app/models/concerns/project_media_creators.rb | 3 ++- app/workers/generic_worker.rb | 16 ++++++++++++++++ test/models/tag_test.rb | 11 +++++++++++ test/workers/generic_worker_test.rb | 19 +++++++++++++++++++ 4 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 app/workers/generic_worker.rb create mode 100644 test/workers/generic_worker_test.rb diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 6b4e405b7a..0fcbc469ed 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -263,6 +263,7 @@ def create_claim_description_and_fact_check 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) + # self.set_tags.each { |tag| Tag.create!(annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) + self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'tag', 'create!', annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) end end diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb new file mode 100644 index 0000000000..52416408a7 --- /dev/null +++ b/app/workers/generic_worker.rb @@ -0,0 +1,16 @@ +class GenericWorker + + include Sidekiq::Worker + + def perform(klass_name, *args) + # require 'byebug' + # byebug + klass = klass_name.constantize + options = args.extract_options!(args) + if options + klass.public_send(*args, **options) + else + klass.public_send(*args) + end + end +end diff --git a/test/models/tag_test.rb b/test/models/tag_test.rb index 0439f55832..0b447d60ff 100644 --- a/test/models/tag_test.rb +++ b/test/models/tag_test.rb @@ -284,4 +284,15 @@ def setup create_tag tag: ' foo', annotated: pm2 end end + + test "tags should be created in the background" do + t = create_team + p = create_project team: t + pm = create_project_media project: p + + pm.set_tags = ['foo'] + pm.create_tags + + 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..fa37a7d340 --- /dev/null +++ b/test/workers/generic_worker_test.rb @@ -0,0 +1,19 @@ +require 'test_helper' + +class GenericWorkerTest < ActiveSupport::TestCase + def setup + super + require 'sidekiq/testing' + end + + test "should schedule a job for any class" do + t = create_team + p = create_project team: t + pm = create_project_media project: p + + assert_nothing_raised do + GenericWorker.perform_async('Tag', 'create!', annotated: pm, tag: 'test_tag', skip_check_ability: true) + end + assert_equal 1, GenericWorker.jobs.size + end +end From 5ab4d0c1ee50c662a05cf5eb59f2cb91db6ca6f0 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 26 Aug 2024 17:13:50 -0300 Subject: [PATCH 02/26] Test creating the tag while creating project ProjectMedia I think the tags are created by a after_create callback inside ProjectMedia. I was having a bit of trouble testing that, so I added the creating of tags to the sample_data method, ans tested that it worked. Now that I know that works, I'll try moving it to the background. --- app/models/concerns/project_media_creators.rb | 6 ++++-- lib/sample_data.rb | 1 + test/models/tag_test.rb | 8 ++++---- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 0fcbc469ed..4b15918e64 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -263,7 +263,9 @@ def create_claim_description_and_fact_check 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) - self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'tag', 'create!', annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) + self.set_tags.each { |tag| Tag.create!(annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) + # require 'byebug' + # byebug + # self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'Tag', 'create!', annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) end end diff --git a/lib/sample_data.rb b/lib/sample_data.rb index 37c5eca8f3..eddfea34dd 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/models/tag_test.rb b/test/models/tag_test.rb index 0b447d60ff..5b5001900a 100644 --- a/test/models/tag_test.rb +++ b/test/models/tag_test.rb @@ -288,11 +288,11 @@ def setup test "tags should be created in the background" do t = create_team p = create_project team: t - pm = create_project_media project: p + pm = create_project_media project: p, tags: ['one'] - pm.set_tags = ['foo'] - pm.create_tags + # assert_equal 1, GenericWorker.jobs.size - assert_equal 1, GenericWorker.jobs.size + assert_equal 'one', Tag.last.tag_text + assert_equal pm.id, Tag.last.annotated_id end end From 7bc2a5f0b6027c4349c303fb438c8ffc16a54a1f Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 26 Aug 2024 18:19:23 -0300 Subject: [PATCH 03/26] wip: try to move create_tags to the background --- app/models/concerns/project_media_creators.rb | 4 ++-- app/workers/generic_worker.rb | 4 +--- test/models/tag_test.rb | 6 +++--- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 4b15918e64..6b4aeafc51 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -263,9 +263,9 @@ def create_claim_description_and_fact_check 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) + # self.set_tags.each { |tag| Tag.create!(annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) # require 'byebug' # byebug - # self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'Tag', 'create!', annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) + self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'Tag', 'create!', annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) end end diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb index 52416408a7..64cfcb83b3 100644 --- a/app/workers/generic_worker.rb +++ b/app/workers/generic_worker.rb @@ -3,10 +3,8 @@ class GenericWorker include Sidekiq::Worker def perform(klass_name, *args) - # require 'byebug' - # byebug klass = klass_name.constantize - options = args.extract_options!(args) + options = args.extract_options! if options klass.public_send(*args, **options) else diff --git a/test/models/tag_test.rb b/test/models/tag_test.rb index 5b5001900a..81b7f21c47 100644 --- a/test/models/tag_test.rb +++ b/test/models/tag_test.rb @@ -290,9 +290,9 @@ def setup p = create_project team: t pm = create_project_media project: p, tags: ['one'] - # assert_equal 1, GenericWorker.jobs.size + assert_equal 1, GenericWorker.jobs.size - assert_equal 'one', Tag.last.tag_text - assert_equal pm.id, Tag.last.annotated_id + # assert_equal 'one', Tag.last.tag_text + # assert_equal pm.id, Tag.last.annotated_id end end From ee8d6a31123e105c299ea7b975fc7e8d014e5c3f Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Tue, 27 Aug 2024 10:03:01 -0300 Subject: [PATCH 04/26] edit worker and tag test - I added a new test for GenericWorker and specified which models we are testing. I think that might be easier than a too generic test. - I think now the tests make sense (though I'm not sure the tags test should be in there or inside project media tests) - Both tests trying to create tags fail, because we need to pass a whole object to annotations, so that is an issue --- test/models/tag_test.rb | 11 +++++------ test/workers/generic_worker_test.rb | 13 +++++++++++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/test/models/tag_test.rb b/test/models/tag_test.rb index 81b7f21c47..93d6212d14 100644 --- a/test/models/tag_test.rb +++ b/test/models/tag_test.rb @@ -286,13 +286,12 @@ def setup end test "tags should be created in the background" do + Sidekiq::Testing.inline! + t = create_team p = create_project team: t - pm = create_project_media project: p, tags: ['one'] - - assert_equal 1, GenericWorker.jobs.size - - # assert_equal 'one', Tag.last.tag_text - # assert_equal pm.id, Tag.last.annotated_id + assert_nothing_raised do + create_project_media project: p, tags: ['one'] + end end end diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index fa37a7d340..9e75c9d96a 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -6,7 +6,9 @@ def setup require 'sidekiq/testing' end - test "should schedule a job for any class" do + test "should schedule a job for Tag creation" do + Sidekiq::Testing.inline! + t = create_team p = create_project team: t pm = create_project_media project: p @@ -14,6 +16,13 @@ def setup assert_nothing_raised do GenericWorker.perform_async('Tag', 'create!', annotated: pm, tag: 'test_tag', skip_check_ability: true) end - assert_equal 1, GenericWorker.jobs.size + end + + test "should schedule a job for Team creation" do + Sidekiq::Testing.inline! + + assert_nothing_raised do + GenericWorker.perform_async('Team', 'create!', name: 'BackgroundTeam', slug: 'background-team') + end end end From 88661befd207dec76ec250ee82944fd8c3f4a894 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Tue, 27 Aug 2024 10:21:24 -0300 Subject: [PATCH 05/26] Updates Tag creation to not send the entire object I'm unsure if it's ok to change this, but looking in our GraphQL wiki we should be able to create a Tag by sending the annotated_type and id. If we update create_tags to send those two values instead of the entire object, we are able to do it in the background job. --- app/models/concerns/project_media_creators.rb | 5 +---- test/workers/generic_worker_test.rb | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 6b4aeafc51..4262f01409 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -263,9 +263,6 @@ def create_claim_description_and_fact_check 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) - # require 'byebug' - # byebug - self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'Tag', 'create!', annotated: self, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) + self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'Tag', 'create!', annotated_type: 'ProjectMedia' , annotated_id: self.id, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) end end diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index 9e75c9d96a..94faa232d6 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -14,7 +14,7 @@ def setup pm = create_project_media project: p assert_nothing_raised do - GenericWorker.perform_async('Tag', 'create!', annotated: pm, tag: 'test_tag', skip_check_ability: true) + GenericWorker.perform_async('Tag', 'create!', annotated_type: 'ProjectMedia' , annotated_id: pm.id, tag: 'test_tag', skip_check_ability: true) end end From 370d30013914dac9dfd3bd6fb7b91bb3507fb563 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 2 Sep 2024 14:43:59 -0300 Subject: [PATCH 06/26] update GenericWorker method signature --- app/workers/generic_worker.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb index 64cfcb83b3..c5f7c78eb6 100644 --- a/app/workers/generic_worker.rb +++ b/app/workers/generic_worker.rb @@ -2,13 +2,13 @@ class GenericWorker include Sidekiq::Worker - def perform(klass_name, *args) + def perform(klass_name, klass_method, *method_args) klass = klass_name.constantize - options = args.extract_options! + options = method_args.extract_options! if options - klass.public_send(*args, **options) + klass.public_send(klass_method, **options) else - klass.public_send(*args) + klass.public_send(klass_method) end end end From dedd1aebbcf2372668d10ce54229e9b2a2e05407 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 2 Sep 2024 14:54:38 -0300 Subject: [PATCH 07/26] Add new test: make sure item's tags creation run in the same job When creating tags while creating an item, we should wrap the whole execution in a single worker run. Each tag in a separate background job run means background job overhead for a small task, and it can also lead to inconsistencies --- test/models/tag_test.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/models/tag_test.rb b/test/models/tag_test.rb index 93d6212d14..49ccd3c24e 100644 --- a/test/models/tag_test.rb +++ b/test/models/tag_test.rb @@ -294,4 +294,15 @@ def setup create_project_media project: p, tags: ['one'] end end + + test "when creating multiple tags for the same item only one job should be scheduled" do + Sidekiq::Testing.fake! + + t = create_team + p = create_project team: t + assert_nothing_raised do + create_project_media project: p, tags: ['one', 'two', 'three'] + end + assert_equal 1, GenericWorker.jobs.size + end end From 61f48d04564a942c94ef49547fb692e2dc86abef Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 2 Sep 2024 15:54:20 -0300 Subject: [PATCH 08/26] move tests from TagTest to ProjectMedia8Test What we are testing is that tags are being created when we create a project media. The method we use to do that is a method called by project_media, so I thought it made more sense to move the tests. --- test/models/project_media_8_test.rb | 29 +++++++++++++++++++++++++++++ test/models/tag_test.rb | 21 --------------------- 2 files changed, 29 insertions(+), 21 deletions(-) create mode 100644 test/models/project_media_8_test.rb diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb new file mode 100644 index 0000000000..f35ae19296 --- /dev/null +++ b/test/models/project_media_8_test.rb @@ -0,0 +1,29 @@ +require_relative '../test_helper' + +class ProjectMedia8Test < ActiveSupport::TestCase + def setup + super + require 'sidekiq/testing' + end + + test "when creating an item with tag/tags, tags should be created in the background" do + Sidekiq::Testing.inline! + + t = create_team + p = create_project team: t + assert_nothing_raised do + create_project_media project: p, tags: ['one'] + end + end + + test "when creating an item with multiple tags, only one job should be scheduled" do + Sidekiq::Testing.fake! + + t = create_team + p = create_project team: t + assert_nothing_raised do + create_project_media project: p, tags: ['one', 'two', 'three'] + end + assert_equal 1, GenericWorker.jobs.size + end +end diff --git a/test/models/tag_test.rb b/test/models/tag_test.rb index 49ccd3c24e..0439f55832 100644 --- a/test/models/tag_test.rb +++ b/test/models/tag_test.rb @@ -284,25 +284,4 @@ def setup create_tag tag: ' foo', annotated: pm2 end end - - test "tags should be created in the background" do - Sidekiq::Testing.inline! - - t = create_team - p = create_project team: t - assert_nothing_raised do - create_project_media project: p, tags: ['one'] - end - end - - test "when creating multiple tags for the same item only one job should be scheduled" do - Sidekiq::Testing.fake! - - t = create_team - p = create_project team: t - assert_nothing_raised do - create_project_media project: p, tags: ['one', 'two', 'three'] - end - assert_equal 1, GenericWorker.jobs.size - end end From e0e5873031647bb5dffe94814eb3f8b2826a3f54 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 2 Sep 2024 15:56:56 -0300 Subject: [PATCH 09/26] Create all tags in one background job Each tag in a separate background job run means background job overhead for a small task, and it can also lead to inconsistencies --- app/models/concerns/project_media_creators.rb | 4 +++- app/models/project_media.rb | 7 +++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 4262f01409..0fb58650b4 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -263,6 +263,8 @@ def create_claim_description_and_fact_check end def create_tags - self.set_tags.each { |tag| GenericWorker.perform_in(1.second, 'Tag', 'create!', annotated_type: 'ProjectMedia' , annotated_id: self.id, tag: tag.strip, skip_check_ability: true) } if self.set_tags.is_a?(Array) + if self.set_tags.is_a?(Array) + GenericWorker.perform_in(1.second, 'ProjectMedia', 'create_tags_in_background', project_media_id: self.id, tags_json: self.set_tags.to_json) + end end end diff --git a/app/models/project_media.rb b/app/models/project_media.rb index 8fc1969578..5b12398e52 100644 --- a/app/models/project_media.rb +++ b/app/models/project_media.rb @@ -528,6 +528,13 @@ def add_nested_objects(ms) ms.attributes[:requests] = requests end + + def self.create_tags_in_background(**params) + project_media = ProjectMedia.find(params['project_media_id']) + tags = JSON.parse(params['tags_json']) + tags.each { |tag| Tag.create! annotated: project_media, tag: tag.strip, skip_check_ability: true } + end + # private # # Please add private methods to app/models/concerns/project_media_private.rb From 9a2986dde286998eccd2733cbcde99c6162f0530 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 5 Sep 2024 12:33:26 -0300 Subject: [PATCH 10/26] avoid raising an error if project media is nil --- app/models/project_media.rb | 8 +++++--- test/models/project_media_8_test.rb | 12 ++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/models/project_media.rb b/app/models/project_media.rb index 5b12398e52..e1a6d5ea43 100644 --- a/app/models/project_media.rb +++ b/app/models/project_media.rb @@ -530,9 +530,11 @@ def add_nested_objects(ms) def self.create_tags_in_background(**params) - project_media = ProjectMedia.find(params['project_media_id']) - tags = JSON.parse(params['tags_json']) - tags.each { |tag| Tag.create! annotated: project_media, tag: tag.strip, skip_check_ability: true } + project_media = ProjectMedia.find_by_id(params['project_media_id']) + unless project_media.nil? + tags = JSON.parse(params['tags_json']) + tags.each { |tag| Tag.create! annotated: project_media, tag: tag.strip, skip_check_ability: true } + end end # private diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index f35ae19296..d36fbcfbd0 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -6,6 +6,18 @@ def setup require 'sidekiq/testing' end + test "create tags when project media id and tags are present" do + t = create_team + p = create_project team: t + pm = create_project_media project: p + + ProjectMedia.create_tags_in_background(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) + end + + test "does not raise an error when no project media is sent" do + ProjectMedia.create_tags_in_background(project_media_id: nil, tags_json: ['one', 'two'].to_json) + end + test "when creating an item with tag/tags, tags should be created in the background" do Sidekiq::Testing.inline! From 81e95ee08cbdc6aab4ee6efa325633666ebab526 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 5 Sep 2024 13:17:07 -0300 Subject: [PATCH 11/26] Add Sentry Notification and tests We should notify Sentry when we fail to create the tags. (CheckSentry.notify also logs the error) Added tests: - make sure create_tags_in_background creates tags associated with the project_media - make sure an error is not raised, but an error is sent to Sentry Added .with_indifferent_access to make sure we are able to get the data from the params hash. --- app/models/project_media.rb | 4 +++- test/models/project_media_8_test.rb | 11 +++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/app/models/project_media.rb b/app/models/project_media.rb index e1a6d5ea43..ac2ea4d9f5 100644 --- a/app/models/project_media.rb +++ b/app/models/project_media.rb @@ -528,13 +528,15 @@ def add_nested_objects(ms) ms.attributes[:requests] = requests end - def self.create_tags_in_background(**params) + params = params.with_indifferent_access project_media = ProjectMedia.find_by_id(params['project_media_id']) unless project_media.nil? tags = JSON.parse(params['tags_json']) tags.each { |tag| Tag.create! annotated: project_media, tag: tag.strip, skip_check_ability: true } end + error = StandardError.new("[ProjectMedia] Exception creating project media's tags in background. Project media is nil.") + CheckSentry.notify(error) end # private diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index d36fbcfbd0..8dc8dfbeac 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -11,11 +11,18 @@ def setup p = create_project team: t pm = create_project_media project: p - ProjectMedia.create_tags_in_background(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) + assert_nothing_raised do + ProjectMedia.create_tags_in_background(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) + end + assert_equal pm.id, Tag.last.annotated_id + assert_equal 'two', Tag.last.tag_text end test "does not raise an error when no project media is sent" do - ProjectMedia.create_tags_in_background(project_media_id: nil, tags_json: ['one', 'two'].to_json) + assert_nothing_raised do + CheckSentry.expects(:notify).once + ProjectMedia.create_tags_in_background(project_media_id: nil, tags_json: ['one', 'two'].to_json) + end end test "when creating an item with tag/tags, tags should be created in the background" do From ef52d2f1ebe345527c07464598920fbf9c5a5fe1 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 5 Sep 2024 16:59:03 -0300 Subject: [PATCH 12/26] Pass User.current to the GenericWorker We log the revisions for tag creation in case User.current exists otherwise no logs for this action --- app/models/concerns/project_media_creators.rb | 2 +- app/models/project_media.rb | 1 + app/workers/generic_worker.rb | 7 ++++++- test/models/project_media_8_test.rb | 2 ++ test/workers/generic_worker_test.rb | 2 +- 5 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 0fb58650b4..5c9898b7e2 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -264,7 +264,7 @@ def create_claim_description_and_fact_check def create_tags if self.set_tags.is_a?(Array) - GenericWorker.perform_in(1.second, 'ProjectMedia', 'create_tags_in_background', project_media_id: self.id, tags_json: self.set_tags.to_json) + GenericWorker.perform_in(1.second, 'ProjectMedia', 'create_tags_in_background', project_media_id: self.id, tags_json: self.set_tags.to_json, user_id: self.user_id) end end end diff --git a/app/models/project_media.rb b/app/models/project_media.rb index ac2ea4d9f5..83b3ca02b7 100644 --- a/app/models/project_media.rb +++ b/app/models/project_media.rb @@ -531,6 +531,7 @@ def add_nested_objects(ms) def self.create_tags_in_background(**params) params = params.with_indifferent_access project_media = ProjectMedia.find_by_id(params['project_media_id']) + unless project_media.nil? tags = JSON.parse(params['tags_json']) tags.each { |tag| Tag.create! annotated: project_media, tag: tag.strip, skip_check_ability: true } diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb index c5f7c78eb6..a1d1890a80 100644 --- a/app/workers/generic_worker.rb +++ b/app/workers/generic_worker.rb @@ -4,9 +4,14 @@ class GenericWorker def perform(klass_name, klass_method, *method_args) klass = klass_name.constantize - options = method_args.extract_options! + options = method_args.extract_options!.with_indifferent_access if options + if options.key?(:user_id) + user_id = options.delete(:user_id) + User.current = User.find_by_id(user_id) + end klass.public_send(klass_method, **options) + User.current = nil else klass.public_send(klass_method) end diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index 8dc8dfbeac..c33e1060e9 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -30,6 +30,7 @@ def setup t = create_team p = create_project team: t + assert_nothing_raised do create_project_media project: p, tags: ['one'] end @@ -40,6 +41,7 @@ def setup t = create_team p = create_project team: t + assert_nothing_raised do create_project_media project: p, tags: ['one', 'two', 'three'] end diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index 94faa232d6..ad9ce8a219 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -14,7 +14,7 @@ def setup pm = create_project_media project: p assert_nothing_raised do - GenericWorker.perform_async('Tag', 'create!', annotated_type: 'ProjectMedia' , annotated_id: pm.id, tag: 'test_tag', skip_check_ability: true) + GenericWorker.perform_async('Tag', 'create!', annotated_type: 'ProjectMedia' , annotated_id: pm.id, tag: 'test_tag', skip_check_ability: true, user_id: pm.user_id) end end From 6781a9c94248de6df448283e752a3d3da9df62ad Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 5 Sep 2024 17:07:41 -0300 Subject: [PATCH 13/26] update tests --- test/models/project_media_8_test.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index c33e1060e9..c938035cb6 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -14,8 +14,7 @@ def setup assert_nothing_raised do ProjectMedia.create_tags_in_background(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) end - assert_equal pm.id, Tag.last.annotated_id - assert_equal 'two', Tag.last.tag_text + assert_equal 2, pm.annotations('tag').count end test "does not raise an error when no project media is sent" do @@ -30,10 +29,9 @@ def setup t = create_team p = create_project team: t + pm = create_project_media project: p, tags: ['one'] - assert_nothing_raised do - create_project_media project: p, tags: ['one'] - end + assert_equal 1, pm.annotations('tag').count end test "when creating an item with multiple tags, only one job should be scheduled" do From ffd4a14226669e4357dc0bd8b7b41045626399ae Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 12 Sep 2024 15:38:29 -0300 Subject: [PATCH 14/26] update how we notify Sentry - call the notify inside else, before I was always sending a sentry error - it's better to send the project_media_id to Sentry, even if there isn't a project media object, an id might exist --- app/models/project_media.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/project_media.rb b/app/models/project_media.rb index 83b3ca02b7..f5b6467466 100644 --- a/app/models/project_media.rb +++ b/app/models/project_media.rb @@ -532,12 +532,13 @@ def self.create_tags_in_background(**params) params = params.with_indifferent_access project_media = ProjectMedia.find_by_id(params['project_media_id']) - unless project_media.nil? + if !project_media.nil? tags = JSON.parse(params['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: params['project_media_id']) end - error = StandardError.new("[ProjectMedia] Exception creating project media's tags in background. Project media is nil.") - CheckSentry.notify(error) end # private From 3a67d1851e8451f4b704709b9ad4228c068b3e87 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 12 Sep 2024 15:40:34 -0300 Subject: [PATCH 15/26] update how we set User.current --- app/workers/generic_worker.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb index a1d1890a80..b88ffc68b5 100644 --- a/app/workers/generic_worker.rb +++ b/app/workers/generic_worker.rb @@ -6,12 +6,11 @@ def perform(klass_name, klass_method, *method_args) klass = klass_name.constantize options = method_args.extract_options!.with_indifferent_access if options - if options.key?(:user_id) - user_id = options.delete(:user_id) - User.current = User.find_by_id(user_id) - end + 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, **options) - User.current = nil + User.current = current_user else klass.public_send(klass_method) end From c8a0dd26d74f5f8dc3d83558b6f6d195f6ea980a Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 12 Sep 2024 16:08:17 -0300 Subject: [PATCH 16/26] switch the methods' names I think the method that was called 'in_background' is actually foreground and vice-versa. --- app/models/concerns/project_media_creators.rb | 4 ++-- app/models/project_media.rb | 4 ++-- test/models/project_media_8_test.rb | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 5c9898b7e2..c63009722f 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -262,9 +262,9 @@ def create_claim_description_and_fact_check fc end - def create_tags + def create_tags_in_background if self.set_tags.is_a?(Array) - GenericWorker.perform_in(1.second, 'ProjectMedia', 'create_tags_in_background', project_media_id: self.id, tags_json: self.set_tags.to_json, user_id: self.user_id) + GenericWorker.perform_in(1.second, 'ProjectMedia', 'create_tags', project_media_id: self.id, tags_json: self.set_tags.to_json, user_id: self.user_id) end end end diff --git a/app/models/project_media.rb b/app/models/project_media.rb index f5b6467466..e78e4c5e8e 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,7 +528,7 @@ def add_nested_objects(ms) ms.attributes[:requests] = requests end - def self.create_tags_in_background(**params) + def self.create_tags(**params) params = params.with_indifferent_access project_media = ProjectMedia.find_by_id(params['project_media_id']) diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index c938035cb6..929b63ad1e 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -12,7 +12,7 @@ def setup pm = create_project_media project: p assert_nothing_raised do - ProjectMedia.create_tags_in_background(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) + ProjectMedia.create_tags(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) end assert_equal 2, pm.annotations('tag').count end @@ -20,7 +20,7 @@ def setup test "does not raise an error when no project media is sent" do assert_nothing_raised do CheckSentry.expects(:notify).once - ProjectMedia.create_tags_in_background(project_media_id: nil, tags_json: ['one', 'two'].to_json) + ProjectMedia.create_tags(project_media_id: nil, tags_json: ['one', 'two'].to_json) end end From 8a6efa5214c638a1780f06dc20678cb22ac792c8 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Fri, 13 Sep 2024 11:20:05 -0300 Subject: [PATCH 17/26] create a wrapper method so this is easier to read --- app/models/concerns/project_media_creators.rb | 2 +- config/initializers/class_extensions.rb | 5 ++ test/models/project_media_8_test.rb | 50 ++++++++++++++----- 3 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 config/initializers/class_extensions.rb diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index c63009722f..61efc3bbd0 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -264,7 +264,7 @@ def create_claim_description_and_fact_check def create_tags_in_background if self.set_tags.is_a?(Array) - GenericWorker.perform_in(1.second, 'ProjectMedia', 'create_tags', project_media_id: self.id, tags_json: self.set_tags.to_json, user_id: self.user_id) + ProjectMedia.run_later_in(1.second, 'create_tags', project_media_id: self.id, tags_json: self.set_tags.to_json, user_id: self.user_id) end end end diff --git a/config/initializers/class_extensions.rb b/config/initializers/class_extensions.rb new file mode 100644 index 0000000000..461cf05ceb --- /dev/null +++ b/config/initializers/class_extensions.rb @@ -0,0 +1,5 @@ +class Class + 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/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index 929b63ad1e..854c911656 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -6,10 +6,10 @@ def setup require 'sidekiq/testing' end - test "create tags when project media id and tags are present" do - t = create_team - p = create_project team: t - pm = create_project_media project: p + 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 assert_nothing_raised do ProjectMedia.create_tags(project_media_id: pm.id, tags_json: ['one', 'two'].to_json) @@ -17,32 +17,56 @@ def setup assert_equal 2, pm.annotations('tag').count end - test "does not raise an error when no project media is sent" do + test ":create_tags should not raise an error when no project media is sent" do assert_nothing_raised do CheckSentry.expects(:notify).once ProjectMedia.create_tags(project_media_id: nil, tags_json: ['one', 'two'].to_json) end end - test "when creating an item with tag/tags, tags should be created in the background" do + 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! - t = create_team - p = create_project team: t - pm = create_project_media project: p, tags: ['one'] + 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, only one job should be scheduled" do + test "when creating an item with multiple tags, after_create :create_tags_in_background callback should only schedule one job" do Sidekiq::Testing.fake! - t = create_team - p = create_project team: t + team = create_team + project = create_project team: team assert_nothing_raised do - create_project_media project: p, tags: ['one', 'two', 'three'] + 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 + + ProjectMedia.run_later_in(0.second, 'create_tags', project_media_id: pm.id, tags_json: ['one', 'two', 'three'].to_json, user_id: pm.user_id) + + assert_equal 1, GenericWorker.jobs.size + end + + test "when using :run_later_in to schedule multiple tags creation, tags should be created" do + Sidekiq::Testing.inline! + + team = create_team + project = create_project team: team + pm = create_project_media project: project + + ProjectMedia.run_later_in(0.second, 'create_tags', project_media_id: pm.id, tags_json: ['one', 'two', 'three'].to_json, user_id: pm.user_id) + + assert_equal 3, pm.annotations('tag').count + end end From 93963ab6f2950cbb4c844093a72ade86813ddbc4 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 16 Sep 2024 15:15:12 -0300 Subject: [PATCH 18/26] change `create_tags` to receive specific standalone parameters instead of **params while doing this I found a bug: GenericWorker worked for methods that took hashes as a parameter, but not for methods that took standalone parameters. (not sure if standalone parameters is the correct way to call this) --- app/models/concerns/project_media_creators.rb | 4 +++- app/models/project_media.rb | 9 ++++----- app/workers/generic_worker.rb | 2 +- test/models/project_media_8_test.rb | 20 +++++++++++++++---- test/workers/generic_worker_test.rb | 19 ++++++++++++++++-- 5 files changed, 41 insertions(+), 13 deletions(-) diff --git a/app/models/concerns/project_media_creators.rb b/app/models/concerns/project_media_creators.rb index 61efc3bbd0..6f61c2a471 100644 --- a/app/models/concerns/project_media_creators.rb +++ b/app/models/concerns/project_media_creators.rb @@ -264,7 +264,9 @@ def create_claim_description_and_fact_check def create_tags_in_background if self.set_tags.is_a?(Array) - ProjectMedia.run_later_in(1.second, 'create_tags', project_media_id: self.id, tags_json: self.set_tags.to_json, user_id: self.user_id) + 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 e78e4c5e8e..20674d674a 100644 --- a/app/models/project_media.rb +++ b/app/models/project_media.rb @@ -528,16 +528,15 @@ def add_nested_objects(ms) ms.attributes[:requests] = requests end - def self.create_tags(**params) - params = params.with_indifferent_access - project_media = ProjectMedia.find_by_id(params['project_media_id']) + 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(params['tags_json']) + 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: params['project_media_id']) + CheckSentry.notify(error, project_media_id: project_media_id) end end diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb index b88ffc68b5..72e182a2f7 100644 --- a/app/workers/generic_worker.rb +++ b/app/workers/generic_worker.rb @@ -9,7 +9,7 @@ def perform(klass_name, klass_method, *method_args) 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, **options) + klass.public_send(klass_method, *method_args, **options) User.current = current_user else klass.public_send(klass_method) diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index 854c911656..4bd0e3c131 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -11,16 +11,22 @@ def setup 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: pm.id, tags_json: ['one', 'two'].to_json) + 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: nil, tags_json: ['one', 'two'].to_json) + ProjectMedia.create_tags(project_media_id, tags_json) end end @@ -53,7 +59,10 @@ def setup project = create_project team: team pm = create_project_media project: project - ProjectMedia.run_later_in(0.second, 'create_tags', project_media_id: pm.id, tags_json: ['one', 'two', 'three'].to_json, user_id: pm.user_id) + 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 @@ -65,7 +74,10 @@ def setup project = create_project team: team pm = create_project_media project: project - ProjectMedia.run_later_in(0.second, 'create_tags', project_media_id: pm.id, tags_json: ['one', 'two', 'three'].to_json, user_id: pm.user_id) + 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 diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index ad9ce8a219..454d676c8c 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -6,7 +6,7 @@ def setup require 'sidekiq/testing' end - test "should schedule a job for Tag creation" do + test "should schedule a job for a method that takes a hash: Tag creation" do Sidekiq::Testing.inline! t = create_team @@ -18,11 +18,26 @@ def setup end end - test "should schedule a job for Team creation" do + test "should schedule a job for a method that takes a hash: Team creation" do Sidekiq::Testing.inline! assert_nothing_raised do GenericWorker.perform_async('Team', 'create!', name: 'BackgroundTeam', slug: 'background-team') end end + + test "should schedule a job 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_nothing_raised do + GenericWorker.perform_async('ProjectMedia', 'create_tags', project_media_id, tags_json, user_id: pm.user_id) + end + end end From e35c643df5ad3ea00d07d3b1cedf79087a967465 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 16 Sep 2024 15:36:53 -0300 Subject: [PATCH 19/26] keep setup and teardown to the bare minimum Tests will run faster if you don't run everything that super runs. request here: https://github.com/meedan/check-api/pull/2005#discussion_r1759631178 --- test/models/project_media_8_test.rb | 5 ++++- test/workers/generic_worker_test.rb | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/test/models/project_media_8_test.rb b/test/models/project_media_8_test.rb index 4bd0e3c131..0f2c818b69 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -2,8 +2,11 @@ class ProjectMedia8Test < ActiveSupport::TestCase def setup - super 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 diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index 454d676c8c..61c90353f8 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -2,8 +2,11 @@ class GenericWorkerTest < ActiveSupport::TestCase def setup - super require 'sidekiq/testing' + Sidekiq::Worker.clear_all + end + + def teardown end test "should schedule a job for a method that takes a hash: Tag creation" do From 324ad39f7767bda6ba47090ee37b04d365a288a6 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 16 Sep 2024 15:46:00 -0300 Subject: [PATCH 20/26] add test to ensure job is queued and clean up tests --- test/workers/generic_worker_test.rb | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index 61c90353f8..3ea8b0f38f 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -9,28 +9,41 @@ def setup def teardown end - test "should schedule a job for a method that takes a hash: Tag creation" do + test "should run a job, without raising an error, for a method that takes a hash as a parameter" do + Sidekiq::Testing.inline! + + assert_nothing_raised 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_nothing_raised do - GenericWorker.perform_async('Tag', 'create!', annotated_type: 'ProjectMedia' , annotated_id: pm.id, tag: 'test_tag', skip_check_ability: true, user_id: pm.user_id) + GenericWorker.perform_async('ProjectMedia', 'create_tags', project_media_id, tags_json, user_id: pm.user_id) end end - test "should schedule a job for a method that takes a hash: Team creation" do - Sidekiq::Testing.inline! + 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 for a method that takes standalone parameters" do - Sidekiq::Testing.inline! + 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 @@ -42,5 +55,7 @@ def teardown 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 From b510a7b65f5ed7c9464e114c09d81fa6d03bd1c7 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 16 Sep 2024 15:50:47 -0300 Subject: [PATCH 21/26] change assert_nothing_raised to assert_difference this way we make sure the team was created, the background job did what it was supposed to. --- test/workers/generic_worker_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index 3ea8b0f38f..f5972ec9da 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -12,7 +12,7 @@ def teardown test "should run a job, without raising an error, for a method that takes a hash as a parameter" do Sidekiq::Testing.inline! - assert_nothing_raised do + assert_difference "Team.where(name: 'BackgroundTeam', slug: 'background-team').count" do GenericWorker.perform_async('Team', 'create!', name: 'BackgroundTeam', slug: 'background-team') end end From 935f44f6cc572d6494bf4f6389a3ba5cbae7e692 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Thu, 19 Sep 2024 16:54:09 -0300 Subject: [PATCH 22/26] move run_later methods to a module --- app/lib/generic_worker_helpers.rb | 9 +++++++++ config/initializers/class_extensions.rb | 4 +--- 2 files changed, 10 insertions(+), 3 deletions(-) create mode 100644 app/lib/generic_worker_helpers.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/config/initializers/class_extensions.rb b/config/initializers/class_extensions.rb index 461cf05ceb..b6fffb32ff 100644 --- a/config/initializers/class_extensions.rb +++ b/config/initializers/class_extensions.rb @@ -1,5 +1,3 @@ class Class - def run_later_in(time, klass_method, *method_args) - GenericWorker.perform_in(time, self.to_s, klass_method, *method_args) - end +include GenericWorkerHelpers end From 2c9c1d0e8c8e93d86657a7cc3b060059990f2a8e Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Fri, 20 Sep 2024 09:21:59 -0300 Subject: [PATCH 23/26] add GenericWorkerHelpersTest --- test/lib/generic_worker_helpers_test.rb | 34 +++++++++++++++++++++++++ test/models/project_media_8_test.rb | 15 ----------- 2 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 test/lib/generic_worker_helpers_test.rb 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 index 0f2c818b69..42f9b9c41d 100644 --- a/test/models/project_media_8_test.rb +++ b/test/models/project_media_8_test.rb @@ -69,19 +69,4 @@ def teardown assert_equal 1, GenericWorker.jobs.size end - - test "when using :run_later_in to schedule multiple tags creation, tags should be created" 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 From ff016199cca5eac443752acf434f7d705ee4e2e3 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Fri, 20 Sep 2024 14:55:12 -0300 Subject: [PATCH 24/26] add test for check-api/app/workers/generic_worker.rb:15 klass.public_send(klass_method) --- test/workers/generic_worker_test.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index f5972ec9da..0042e94845 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -9,6 +9,14 @@ def setup def teardown end + test "should run a job, without raising an error, for a method that takes no parameters" do + Sidekiq::Testing.inline! + + assert_nothing_raised do + GenericWorker.perform_async('Media', 'types') + 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! @@ -32,6 +40,16 @@ def teardown 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! From 5c1c9413557111cbe493f2d324785d8b274bba82 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 23 Sep 2024 09:31:38 -0300 Subject: [PATCH 25/26] change conditional options is always a hash, so `if options` is always `true`. replaced this conditional by `unless options.blank?`, because when there are no parameters, options value is `{}`. --- app/workers/generic_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/generic_worker.rb b/app/workers/generic_worker.rb index 72e182a2f7..4915f97c26 100644 --- a/app/workers/generic_worker.rb +++ b/app/workers/generic_worker.rb @@ -5,7 +5,7 @@ class GenericWorker def perform(klass_name, klass_method, *method_args) klass = klass_name.constantize options = method_args.extract_options!.with_indifferent_access - if options + 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) From 9f1141e3fbcb273e8c608d1de9fa7b40fd961ae6 Mon Sep 17 00:00:00 2001 From: manu vasconcelos Date: Mon, 23 Sep 2024 10:26:21 -0300 Subject: [PATCH 26/26] update tests to assert_difference --- test/workers/generic_worker_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/workers/generic_worker_test.rb b/test/workers/generic_worker_test.rb index 0042e94845..628602a0da 100644 --- a/test/workers/generic_worker_test.rb +++ b/test/workers/generic_worker_test.rb @@ -12,8 +12,8 @@ def teardown test "should run a job, without raising an error, for a method that takes no parameters" do Sidekiq::Testing.inline! - assert_nothing_raised do - GenericWorker.perform_async('Media', 'types') + assert_difference 'Blank.count' do + GenericWorker.perform_async('Blank', 'create!') end end @@ -35,7 +35,7 @@ def teardown project_media_id = pm.id tags_json = ['one', 'two'].to_json - assert_nothing_raised do + 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