Merge pull request #1188 from DaanVanVugt/bugfix/broken_scraper_check

stricter tests for broken scraper check and better query

Author: fbacall

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

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

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