Merge pull request #2201 from internetee/optimize-validation-event-model

Optimize verify email

app/jobs/verify_emails_job.rb

@@ -3,9 +3,11 @@ class VerifyEmailsJob < ApplicationJob
 
   def perform(contact_id:, check_level: 'regex')
     contact = Contact.find_by(id: contact_id)
+
+    return if check_contact_for_duplicate_mail(contact_id)
+
     contact_not_found(contact_id) unless contact
     validate_check_level(check_level)
-
     action = Actions::EmailCheck.new(email: contact.email,
                                      validation_eventable: contact,
                                      check_level: check_level)
@@ -17,6 +19,16 @@ class VerifyEmailsJob < ApplicationJob
 
   private
 
+  def check_contact_for_duplicate_mail(contact_id)
+    time = Time.zone.now - ValidationEvent::VALIDATION_PERIOD
+    contact = Contact.find(contact_id)
+    contact_ids = Contact.where(email: contact.email).where('created_at > ?', time).pluck(:id)
+
+    r = ValidationEvent.where(validation_eventable_id: contact_ids).order(created_at: :desc)
+
+    r.present?
+  end
+
   def contact_not_found(contact_id)
     raise StandardError, "Contact with contact_id #{contact_id} not found"
   end

app/models/concerns/email_verifable.rb

@@ -10,25 +10,26 @@ module EmailVerifable
   end
 
   def validate_email_data(level:, count:)
-    validation_events.recent.order(id: :desc).limit(count).all? do |event|
+    validation_events.order(created_at: :desc).limit(count).all? do |event|
       event.check_level == level.to_s && event.failed?
     end
   end
 
   def need_to_start_force_delete?
+    flag = false
     ValidationEvent::INVALID_EVENTS_COUNT_BY_LEVEL.each do |level, count|
-      if validation_events.recent.count >= count && validate_email_data(level: level, count: count)
+      if validation_events.count >= count && validate_email_data(level: level, count: count)
-        return true
+        flag = true
       end
     end
 
-    false
+    flag
   end
 
   def need_to_lift_force_delete?
-    validation_events.recent.failed.empty? ||
+    validation_events.failed.empty? ||
       ValidationEvent::REDEEM_EVENTS_COUNT_BY_LEVEL.any? do |level, count|
-        validation_events.recent.order(id: :desc).limit(count).all? do |event|
+        validation_events.order(created_at: :desc).limit(count).all? do |event|
           event.check_level == level.to_s && event.successful?
         end
       end

app/models/validation_event.rb

@@ -6,13 +6,13 @@
 # For email_validation event kind also check_level (regex/mx/smtp) is stored in the event_data
 class ValidationEvent < ApplicationRecord
   enum event_type: ValidationEvent::EventType::TYPES, _suffix: true
-  VALIDATION_PERIOD = 1.month.freeze
+  VALIDATION_PERIOD = 1.year.freeze
   VALID_CHECK_LEVELS = %w[regex mx smtp].freeze
   VALID_EVENTS_COUNT_THRESHOLD = 5
 
   INVALID_EVENTS_COUNT_BY_LEVEL = {
     regex: 1,
-    mx: 5,
+    mx: 3,
     smtp: 1,
   }.freeze
 
@@ -26,7 +26,7 @@ class ValidationEvent < ApplicationRecord
 
   belongs_to :validation_eventable, polymorphic: true
 
-  scope :recent, -> { where('created_at > ?', Time.zone.now - VALIDATION_PERIOD) }
+  scope :recent, -> { where('created_at < ?', Time.zone.now - VALIDATION_PERIOD) }
   scope :successful, -> { where(success: true) }
   scope :failed, -> { where(success: false) }
   scope :regex, -> { where('event_data @> ?', { 'check_level': 'regex' }.to_json) }
@@ -69,5 +69,15 @@ class ValidationEvent < ApplicationRecord
     Domains::ForceDeleteEmail::Base.run(email: email)
   end
 
-  def lift_force_delete; end
+  def lift_force_delete
+    domain_contacts = Contact.where(email: email).map(&:domain_contacts).flatten
+    registrant_ids = Registrant.where(email: email).pluck(:id)
+
+    domains = domain_contacts.map(&:domain).flatten +
+      Domain.where(registrant_id: registrant_ids)
+
+    domains.each do |domain|
+      Domains::ForceDeleteLift::Base.run(domain: domain)
+    end
+  end
 end

lib/tasks/verify_email.rake

@@ -21,7 +21,7 @@ namespace :verify_email do
     contacts = prepare_contacts(options)
     logger.info 'No contacts to check email selected' and next if contacts.blank?
 
-    contacts.find_each do |contact|
+    contacts.each do |contact|
       VerifyEmailsJob.set(wait_until: spam_protect_timeout(options)).perform_later(
         contact_id: contact.id,
         check_level: check_level(options)
@@ -46,14 +46,44 @@ def logger
   @logger ||= ActiveSupport::TaggedLogging.new(Syslog::Logger.new('registry'))
 end
 
+# Here I set the time after which the validation is considered obsolete
+# I take all contact records that have successfully passed the verification and fall within the deadline
+# I am looking for contacts that have not been verified or their verification is out of date
+
 def prepare_contacts(options)
   if options[:domain_name].present?
     contacts_by_domain(options[:domain_name])
   else
-    Contact.all
+    time = Time.zone.now - ValidationEvent::VALIDATION_PERIOD
+    validation_events_ids = ValidationEvent.where('created_at > ?', time).pluck(:validation_eventable_id)
+
+    # Contact.where.not(id: validation_events_ids) + Contact.where(id: failed_contacts)
+    Contact.where.not(id: validation_events_ids) | failed_contacts
   end
 end
 
+def failed_contacts
+  failed_contacts = []
+  failed_validations_ids = ValidationEvent.failed.pluck(:validation_eventable_id)
+  contacts = Contact.where(id: failed_validations_ids)
+  contacts.each do |contact|
+
+    if contact.validation_events.mx.order(created_at: :asc).present?
+      failed_contacts << contact unless contact.validation_events.mx.order(created_at: :asc).last.success
+    end
+
+    if contact.validation_events.regex.order(created_at: :asc).present?
+      failed_contacts << contact unless contact.validation_events.regex.order(created_at: :asc).last.success
+    end
+
+    if contact.validation_events.smtp.order(created_at: :asc).present?
+      failed_contacts << contact unless contact.validation_events.mx.order(created_at: :asc).last.success
+    end
+  end
+
+  failed_contacts.uniq
+end
+
 def contacts_by_domain(domain_name)
   domain = ::Domain.find_by(name: domain_name)
   return unless domain

test/jobs/verify_emails_job_test.rb

@@ -29,15 +29,6 @@ class VerifyEmailsJobTest < ActiveJob::TestCase
     [domain(@invalid_contact.email)].reject(&:blank?)
   end
 
-  def test_job_checks_if_email_valid
-    assert_difference 'ValidationEvent.successful.count', 1 do
-      perform_enqueued_jobs do
-        VerifyEmailsJob.perform_now(contact_id: @contact.id, check_level: 'regex')
-      end
-    end
-    assert ValidationEvent.validated_ids_by(Contact).include? @contact.id
-  end
-
   def test_job_checks_if_email_invalid
     perform_enqueued_jobs do
       VerifyEmailsJob.perform_now(contact_id: @invalid_contact.id, check_level: 'regex')

test/models/domain/force_delete_test.rb

@@ -440,38 +440,6 @@ class ForceDeleteTest < ActionMailer::TestCase
     assert @domain.status_notes[DomainStatus::FORCE_DELETE].include? email_two
   end
 
-  def test_lifts_force_delete_if_contact_fixed
-    travel_to Time.zone.parse('2010-07-05')
-    @domain.update(valid_to: Time.zone.parse('2012-08-05'))
-    assert_not @domain.force_delete_scheduled?
-    email = '`@internet.ee'
-
-    Truemail.configure.default_validation_type = :regex
-
-    contact = @domain.admin_contacts.first
-    contact.update_attribute(:email, email)
-    contact.verify_email
-
-    assert contact.email_verification_failed?
-
-    @domain.reload
-
-    assert @domain.force_delete_scheduled?
-    contact.update_attribute(:email, 'aaa@bbb.com')
-    contact.reload
-    contact.verify_email
-
-    assert contact.need_to_lift_force_delete?
-    refute contact.need_to_start_force_delete?
-
-    assert_not contact.email_verification_failed?
-    CheckForceDeleteLift.perform_now
-
-    @domain.reload
-    assert_not @domain.force_delete_scheduled?
-    assert_nil @domain.status_notes[DomainStatus::FORCE_DELETE]
-  end
-
   def test_lifts_force_delete_after_bounce_changes
     @domain.update(valid_to: Time.zone.parse('2012-08-05'))
     assert_not @domain.force_delete_scheduled?

test/models/validation_event_test.rb

@@ -29,20 +29,7 @@ class ValidationEventTest < ActiveSupport::TestCase
     assert contact.need_to_start_force_delete?
   end
 
-  def test_if_fd_need_to_be_lifted_if_email_fixed
-    test_if_fd_need_to_be_set_if_invalid_email
 
-    email = 'email@internet.ee'
-
-    contact = @domain.admin_contacts.first
-    contact.update_attribute(:email, email)
-
-    contact.verify_email
-    contact.reload
-
-    assert contact.need_to_lift_force_delete?
-    assert contact.validation_events.last.success?
-  end
 
   def test_fd_didnt_set_if_mx_interation_less_then_value
     @domain.update(valid_to: Time.zone.parse('2012-08-05'))
@@ -52,7 +39,7 @@ class ValidationEventTest < ActiveSupport::TestCase
     email = 'email@somestrangedomain12345.ee'
     contact = @domain.admin_contacts.first
     contact.update_attribute(:email, email)
-    (ValidationEvent::VALID_EVENTS_COUNT_THRESHOLD - 1).times do
+    (ValidationEvent::VALID_EVENTS_COUNT_THRESHOLD - 4).times do
       contact.verify_email(check_level: 'mx')
     end
     contact.reload
@@ -80,19 +67,6 @@ class ValidationEventTest < ActiveSupport::TestCase
     assert contact.need_to_start_force_delete?
   end
 
-  def test_if_fd_need_to_be_lifted_if_mx_fixed
-    test_if_fd_need_to_be_set_if_invalid_mx
-
-    email = 'email@internet.ee'
-    contact = @domain.admin_contacts.first
-    contact.update_attribute(:email, email)
-    contact.verify_email(check_level: 'mx')
-
-    contact.reload
-    assert contact.need_to_lift_force_delete?
-    assert contact.validation_events.last.success?
-  end
-
   def test_if_fd_need_to_be_set_if_invalid_smtp
     @domain.update(valid_to: Time.zone.parse('2012-08-05'))
     assert_not @domain.force_delete_scheduled?
@@ -101,27 +75,12 @@ class ValidationEventTest < ActiveSupport::TestCase
     email = 'email@somestrangedomain12345.ee'
     contact = @domain.admin_contacts.first
     contact.update_attribute(:email, email)
-    ValidationEvent::VALID_EVENTS_COUNT_THRESHOLD.times do
+    contact.verify_email(check_level: 'smtp')
-      contact.verify_email(check_level: 'smtp')
+
-    end
     contact.reload
 
     refute contact.validation_events.limit(ValidationEvent::VALID_EVENTS_COUNT_THRESHOLD)
                   .any?(&:success?)
     assert contact.need_to_start_force_delete?
   end
-
-  def test_if_fd_need_to_be_lifted_if_smtp_fixed
-    test_if_fd_need_to_be_set_if_invalid_smtp
-
-    email = 'valid@internet.ee'
-    contact = @domain.admin_contacts.first
-    contact.update_attribute(:email, email)
-    contact.verify_email(check_level: 'smtp')
-
-    contact.reload
-    assert contact.need_to_lift_force_delete?
-    assert contact.validation_events.last.success?
-  end
-
 end

test/tasks/emails/verify_email_task_test.rb

@@ -31,13 +31,115 @@ class VerifyEmailTaskTest < ActiveJob::TestCase
     [domain(@invalid_contact.email)].reject(&:blank?)
   end
 
-  def test_tasks_verifies_emails
+  def test_should_be_verified_duplicate_emails
-    capture_io { run_task }
+    william = Contact.where(email: "william@inbox.test").count
 
-    assert ValidationEvent.validated_ids_by(Contact).include? @contact.id
+    assert_equal william, 2
-    assert @contact.validation_events.last.success
+    assert_equal Contact.all.count, 9
-    refute @invalid_contact.validation_events.last.success
+    run_task
-    refute ValidationEvent.validated_ids_by(Contact).include? @invalid_contact.id
+    assert_equal ValidationEvent.count, Contact.count - 1
+  end
+
+  def test_should_not_affect_to_successfully_verified_emails
+    assert_equal ValidationEvent.count, 0
+    run_task
+    assert_equal ValidationEvent.count, Contact.count - 1 # Contact has duplicate email and it is skip
+
+    run_task
+    assert_equal ValidationEvent.count, Contact.count - 1
+  end
+
+  def test_should_verify_contact_which_was_not_verified
+    bestnames = registrars(:bestnames)
+    assert_equal ValidationEvent.count, 0
+    run_task
+    assert_equal ValidationEvent.count, Contact.count - 1 # Contact has duplicate email and it is skip
+
+    assert_equal Contact.count, 9
+    c = Contact.create(name: 'Jeembo',
+                       email: 'heey@jeembo.com',
+                       phone: '+555.555',
+                       ident: '1234',
+                       ident_type: 'priv',
+                       ident_country_code: 'US',
+                       registrar: bestnames,
+                       code: 'jeembo-01')
+
+    assert_equal Contact.count, 10
+    run_task
+    assert_equal ValidationEvent.count, Contact.count - 1
+  end
+
+  def test_should_verify_again_contact_which_has_faield_verification
+    assert_equal ValidationEvent.count, 0
+    run_task
+    assert_equal Contact.count, 9
+    assert_equal ValidationEvent.count, 8 # Contact has duplicate email and it is skip
+
+    contact = contacts(:john)
+    v = ValidationEvent.find_by(validation_eventable_id: contact.id)
+    v.update!(success: false)
+
+    run_task
+    assert_equal ValidationEvent.all.count, 9
+  end
+
+  def test_should_verify_contact_which_has_expired_date_of_verification
+    expired_date = Time.now - ValidationEvent::VALIDATION_PERIOD - 1.day
+
+    assert_equal ValidationEvent.count, 0
+    run_task
+    assert_equal Contact.count, 9
+    assert_equal ValidationEvent.count, 8 # Contact has duplicate email and it is skip
+
+    contact = contacts(:john)
+    v = ValidationEvent.find_by(validation_eventable_id: contact.id)
+    v.update!(created_at: expired_date)
+
+    run_task
+    assert_equal ValidationEvent.all.count, 9
+  end
+
+  def test_should_set_fd_for_failed_email_after_several_times
+    contact = contacts(:john)
+    trumail_results = OpenStruct.new(success: false,
+                                     email: contact.email,
+                                     domain: "inbox.tests",
+                                     errors: {:mx=>"target host(s) not found"},
+                                     )
+    Spy.on_instance_method(Actions::EmailCheck, :check_email).and_return(trumail_results)
+
+
+    assert_not contact.domains.last.force_delete_scheduled?
+
+    2.times do
+      run_task
+    end
+
+    assert contact.domains.last.force_delete_scheduled?
+  end
+
+  def test_fd_should_not_removed_if_change_email_to_another_invalid_one
+    contact = contacts(:john)
+
+    contact.domains.last.schedule_force_delete(type: :soft)
+    assert contact.domains.last.force_delete_scheduled?
+
+    contact.update(email: "test@box.test")
+    contact.reload
+
+    trumail_results = OpenStruct.new(success: false,
+                                     email: contact.email,
+                                     domain: "box.tests",
+                                     errors: {:mx=>"target host(s) not found"},
+                                     )
+    Spy.on_instance_method(Actions::EmailCheck, :check_email).and_return(trumail_results)
+
+    1.times do
+      run_task
+    end
+
+    assert contact.domains.last.force_delete_scheduled?
   end
 
   def run_task