Fix check force delete lift bug #2418
Conversation
|
Changes preview: Legend: |
thiagoyoussef
force-pushed
the
2413-force-delete-lift-bug
branch
2 times, most recently
from
495b068 to
1bc743c
Compare
thiagoyoussef
force-pushed
the
2413-force-delete-lift-bug
branch
from
1bc743c to
70ff87b
Compare
lib/tasks/check_force_delete.rake
Outdated
| @@ -3,8 +3,7 @@ task check_force_delete: :environment do | |||
| validations = ValidationEvent.failed.where(validation_eventable_type: 'Contact').uniq(&:validation_eventable_id) | |||
|
|
|||
| invalid_contact_ids = validations.select do |validation| | |||
| contact = validation.validation_eventable | |||
| contact.need_to_start_force_delete? || contact.need_to_lift_force_delete? | |||
| validation.validation_eventable.need_to_start_force_delete? | |||
There was a problem hiding this comment.
where did you hide the check for need_to_lift_force_delete? how will he understand that force delete can be removed on a specific domain?
There was a problem hiding this comment.
I added on the validation_event after create callback. It's the way we did before I refactor it
I moved back because it seems that after updating to a valid email, the domain didn't have the FD status notes refreshed
app/models/validation_event.rb
Outdated
| @@ -35,6 +35,8 @@ class ValidationEvent < ApplicationRecord | |||
| scope :smtp, -> { where('event_data @> ?', { 'check_level': 'smtp' }.to_json) } | |||
| scope :by_object, ->(object) { where(validation_eventable: object) } | |||
|
|
|||
| after_create :check_force_delete_lift | |||
|
|
|||
There was a problem hiding this comment.
I think that is a bad idea.
The last time we put checks for force delete and lift force delete into callbacks, firstly there were performance issues, and secondly, unexpected behavior happened when lift force delete worked in a situation where it was not supposed to.
In addition, there is an opinion that using callback is not a good practice in terms of support, code testing. But that's just an opinion, everyone has their own opinion :)
There was a problem hiding this comment.
I'll add one more comment to make it a little clearer.
We have a validation_event model, we also have a verify_email rake task. When we run this task, the mail is validated and the results are written to the validation_event table. The problem is that checking mail is a very productive operation. And if, in addition to checking mail, writing the result data, you also check for check force delete lift, this will be a VERY highly productive operation. My idea is to remove after_create :check_force_delete_lift. It should not be launched every time after checking each email and record results to validation event table, because it takes a LOT of resources. It is enough that all emails are checked and the results are recorded in the validation table. We can run the Force Delete check and removal separately, we don’t need to run the force delete lift check in the valiation_event model
thiagoyoussef
force-pushed
the
2413-force-delete-lift-bug
branch
from
dc57f47 to
df7a112
Compare
thiagoyoussef
force-pushed
the
2413-force-delete-lift-bug
branch
from
df7a112 to
c6ff1ad
Compare
thiagoyoussef
force-pushed
the
2413-force-delete-lift-bug
branch
from
cb66ad1 to
71d1e9b
Compare
| domain.reload | ||
|
|
||
| assert_equal domain.status_notes[DomainStatus::FORCE_DELETE], invalid_email |
There was a problem hiding this comment.
Why were we checking if the status notes had the invalid email? It seems strange to me, as the purpose of the test is to check if the invalid email was removed from the status notes.
Changed the test to assert_nil
app/models/validation_event.rb
Outdated
| @@ -35,6 +35,8 @@ class ValidationEvent < ApplicationRecord | |||
| scope :smtp, -> { where('event_data @> ?', { 'check_level': 'smtp' }.to_json) } | |||
| scope :by_object, ->(object) { where(validation_eventable: object) } | |||
|
|
|||
| after_create :check_force_delete_lift | |||
|
|
|||
There was a problem hiding this comment.
I'll add one more comment to make it a little clearer.
We have a validation_event model, we also have a verify_email rake task. When we run this task, the mail is validated and the results are written to the validation_event table. The problem is that checking mail is a very productive operation. And if, in addition to checking mail, writing the result data, you also check for check force delete lift, this will be a VERY highly productive operation. My idea is to remove after_create :check_force_delete_lift. It should not be launched every time after checking each email and record results to validation event table, because it takes a LOT of resources. It is enough that all emails are checked and the results are recorded in the validation table. We can run the Force Delete check and removal separately, we don’t need to run the force delete lift check in the valiation_event model
|
an issue detected - if user has the same invalid email with multiple contact objects related to a single domain registration the amount of false validation_event records is unlimited (seemingly) instead of three records. This is not a rare case in production |
close #2413