From 18243581c8439c9e2f40289131f37d61a26b8eb4 Mon Sep 17 00:00:00 2001 From: Mike Sanders Date: Thu, 4 Dec 2025 15:37:51 +0100 Subject: [PATCH 1/2] stricter tests for broken scraper check and better query --- app/mailers/curation_mailer.rb | 7 +-- app/models/content_provider.rb | 11 +++++ test/mailers/curation_mailer_test.rb | 68 ++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 6 deletions(-) diff --git a/app/mailers/curation_mailer.rb b/app/mailers/curation_mailer.rb index 584d90735..399ad2a70 100644 --- a/app/mailers/curation_mailer.rb +++ b/app/mailers/curation_mailer.rb @@ -53,12 +53,7 @@ def check_broken_scrapers(user, cut_off_time) source_names = TeSS::Config.ingestion[:sources].filter { |s| s[:enabled] }.map { |s| s[:provider] }.uniq source_names += Source.includes(:content_provider).enabled.approved.map{ |s| s.content_provider.title} - @providers = ContentProvider - .left_joins(%i[events materials]) - .where(title: source_names) - .where('events.updated_at < ?', cut_off_time) - .where('materials.updated_at < ?', cut_off_time) - .distinct + @providers = ContentProvider.with_broken_scrapers(source_names, cut_off_time) subject = t('mailer.check_broken_scrapers.subject') mail(subject:, to: user.email) do |format| format.html diff --git a/app/models/content_provider.rb b/app/models/content_provider.rb index 54ffeede7..157d989aa 100644 --- a/app/models/content_provider.rb +++ b/app/models/content_provider.rb @@ -161,4 +161,15 @@ def approved_editors= values editors.each { |item| remove_editor(item) if !editors_list.include?(item) } end + def self.with_broken_scrapers(source_names, cut_off_time) + ContentProvider + .left_joins(%i[events materials]) + .where(title: source_names) + .group("content_providers.id") + .having('(COUNT(events.id) > 0 OR COUNT(materials.id) > 0)') + .having('(COUNT(events.id) = 0 OR MAX(events.updated_at) < :cutoff)', cutoff: cut_off_time) + .having('(COUNT(materials.id) = 0 OR MAX(materials.updated_at) < :cutoff)', cutoff: cut_off_time) + .to_a + .uniq + end end diff --git a/test/mailers/curation_mailer_test.rb b/test/mailers/curation_mailer_test.rb index d3062701c..485e1642d 100644 --- a/test/mailers/curation_mailer_test.rb +++ b/test/mailers/curation_mailer_test.rb @@ -250,3 +250,71 @@ class CurationMailerTest < ActionMailer::TestCase end end end + +class ContentProvidersWithBrokenScrapersTest < ActionMailer::TestCase + setup do + @cutoff = 1.week.ago + @user = users(:regular_user) + @provider = ContentProvider.create!(title: 'Goblet', user: @user, url: 'http://www.google.com#1') + @provider_2 = ContentProvider.create!(title: 'Two', user: @user, url: 'http://www.google.com#1') + @params = {title: 'my_title', description: 'my_description', url: 'http://www.google.com#1', user: @user} + end + + test "excludes providers with no events and no materials" do + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_empty result + end + + test "includes provider only with events all before cutoff" do + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.events.create!(@params.merge({updated_at: 3.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + end + + test "excludes provider with any event after cutoff" do + @provider.events.create!(@params.merge({updated_at: 2.days.ago})) + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + refute_includes result, @provider + end + + test "includes provider only with materials all before cutoff" do + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 3.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + end + + test "excludes provider with any material after cutoff" do + @provider.materials.create!(@params.merge({updated_at: 2.days.ago})) + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + refute_includes result, @provider + end + + test "includes provider with both events and materials all before cutoff" do + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.events.create!(@params.merge({updated_at: 3.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 3.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + end + + test "excludes provider with mixed cases where one event or material is too new" do + @provider.events.create!(@params.merge({updated_at: 2.days.ago})) + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + refute_includes result, @provider + end + + test "filters correctly by title" do + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider_2.events.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + refute_includes result, @provider_b + end +end From 43768fa0c787c242fdaef8c28f10198d62ac3627 Mon Sep 17 00:00:00 2001 From: Mike Sanders Date: Thu, 4 Dec 2025 16:20:47 +0100 Subject: [PATCH 2/2] move with_broken_scrapers tests to model test --- test/mailers/curation_mailer_test.rb | 68 ---------------------------- test/models/content_provider_test.rb | 68 ++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 68 deletions(-) diff --git a/test/mailers/curation_mailer_test.rb b/test/mailers/curation_mailer_test.rb index 485e1642d..d3062701c 100644 --- a/test/mailers/curation_mailer_test.rb +++ b/test/mailers/curation_mailer_test.rb @@ -250,71 +250,3 @@ class CurationMailerTest < ActionMailer::TestCase end end end - -class ContentProvidersWithBrokenScrapersTest < ActionMailer::TestCase - setup do - @cutoff = 1.week.ago - @user = users(:regular_user) - @provider = ContentProvider.create!(title: 'Goblet', user: @user, url: 'http://www.google.com#1') - @provider_2 = ContentProvider.create!(title: 'Two', user: @user, url: 'http://www.google.com#1') - @params = {title: 'my_title', description: 'my_description', url: 'http://www.google.com#1', user: @user} - end - - test "excludes providers with no events and no materials" do - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - assert_empty result - end - - test "includes provider only with events all before cutoff" do - @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) - @provider.events.create!(@params.merge({updated_at: 3.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - assert_includes result, @provider - end - - test "excludes provider with any event after cutoff" do - @provider.events.create!(@params.merge({updated_at: 2.days.ago})) - @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - refute_includes result, @provider - end - - test "includes provider only with materials all before cutoff" do - @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) - @provider.materials.create!(@params.merge({updated_at: 3.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - assert_includes result, @provider - end - - test "excludes provider with any material after cutoff" do - @provider.materials.create!(@params.merge({updated_at: 2.days.ago})) - @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - refute_includes result, @provider - end - - test "includes provider with both events and materials all before cutoff" do - @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) - @provider.events.create!(@params.merge({updated_at: 3.weeks.ago})) - @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) - @provider.materials.create!(@params.merge({updated_at: 3.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - assert_includes result, @provider - end - - test "excludes provider with mixed cases where one event or material is too new" do - @provider.events.create!(@params.merge({updated_at: 2.days.ago})) - @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) - @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - refute_includes result, @provider - end - - test "filters correctly by title" do - @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) - @provider_2.events.create!(@params.merge({updated_at: 2.weeks.ago})) - result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) - assert_includes result, @provider - refute_includes result, @provider_b - end -end diff --git a/test/models/content_provider_test.rb b/test/models/content_provider_test.rb index 23fce1620..9e609dc4e 100644 --- a/test/models/content_provider_test.rb +++ b/test/models/content_provider_test.rb @@ -130,3 +130,71 @@ class ContentProviderTest < ActiveSupport::TestCase end end end + +class ContentProvidersWithBrokenScrapersTest < ActiveSupport::TestCase + setup do + @cutoff = 1.week.ago + @user = users(:regular_user) + @provider = ContentProvider.create!(title: 'Goblet', user: @user, url: 'http://www.google.com#1') + @provider_2 = ContentProvider.create!(title: 'Two', user: @user, url: 'http://www.google.com#1') + @params = {title: 'my_title', description: 'my_description', url: 'http://www.google.com#1', user: @user} + end + + test "excludes providers with no events and no materials" do + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_empty result + end + + test "includes provider only with events all before cutoff" do + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.events.create!(@params.merge({updated_at: 3.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + end + + test "excludes provider with any event after cutoff" do + @provider.events.create!(@params.merge({updated_at: 2.days.ago})) + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + refute_includes result, @provider + end + + test "includes provider only with materials all before cutoff" do + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 3.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + end + + test "excludes provider with any material after cutoff" do + @provider.materials.create!(@params.merge({updated_at: 2.days.ago})) + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + refute_includes result, @provider + end + + test "includes provider with both events and materials all before cutoff" do + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.events.create!(@params.merge({updated_at: 3.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 3.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + end + + test "excludes provider with mixed cases where one event or material is too new" do + @provider.events.create!(@params.merge({updated_at: 2.days.ago})) + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider.materials.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + refute_includes result, @provider + end + + test "filters correctly by title" do + @provider.events.create!(@params.merge({updated_at: 2.weeks.ago})) + @provider_2.events.create!(@params.merge({updated_at: 2.weeks.ago})) + result = ContentProvider.with_broken_scrapers(["Goblet"], @cutoff) + assert_includes result, @provider + refute_includes result, @provider_2 + end +end \ No newline at end of file