-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix check force delete lift bug #2418
Changes from 3 commits
ff1dded
3e4b677
70ff87b
7328ea2
929ccf6
c6ff1ad
71d1e9b
39022ea
9d7bf04
0d7e9eb
dc75a94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added on the |
||
end.pluck(:validation_eventable_id) | ||
|
||
CheckForceDeleteJob.perform_later(invalid_contact_ids) if invalid_contact_ids.present? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -517,6 +517,29 @@ def test_lifts_force_delete_after_bounce_changes | |
assert_not @domain.force_delete_scheduled? | ||
end | ||
|
||
def test_lifts_force_delete_after_changing_to_valid_email | ||
@domain.update(valid_to: Time.zone.parse('2012-08-05')) | ||
assert_not @domain.force_delete_scheduled? | ||
travel_to Time.zone.parse('2010-07-05') | ||
@domain.registrant.update_attribute(:email, '`@internet.ee') | ||
3.times { @domain.registrant.verify_email } | ||
perform_check_force_delete_job(@domain.registrant.id) | ||
@domain.reload | ||
|
||
assert @domain.force_delete_scheduled? | ||
|
||
@domain.registrant.update(email: '[email protected]') | ||
@domain.registrant.verify_email | ||
@domain.reload | ||
|
||
assert @domain.registrant.need_to_lift_force_delete? | ||
|
||
perform_enqueued_jobs { CheckForceDeleteLift.perform_now } | ||
@domain.reload | ||
|
||
assert_not @domain.force_delete_scheduled? | ||
end | ||
|
||
def prepare_bounced_email_address(email) | ||
@bounced_mail = BouncedMailAddress.new | ||
@bounced_mail.email = email | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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