draft case contact cleanup #5918
Conversation
Smullz622
requested review from
compwron,
FireLemons,
littleforest and
elasticspoon
as code owners
| @@ -0,0 +1,6 @@ | |||
| desc "delete draft case contacts marked for clean up, run by heroku scheduler." | |||
There was a problem hiding this comment.
for later PR thought - we may want to move away from heroku scheduler towards something like
include Sidekiq::Job
sidekiq_options queue: :general_12h
def self.crontab
"0 8 * * *"
end
There was a problem hiding this comment.
I'm not that familiar with heroku scheduler but would like to learn so I'm curious, what's the rationale for moving away from it in favor of Sidekiq?
There was a problem hiding this comment.
Most of my feedback is pretty minor BUT I took a closer look at the production data and want to double check that stakeholders are fine with the deletion timelines. (The request for changes is mainly to ensure this does not get accidentally merged.)
There are a surprising amount of case contacts that are in draft state that have good info in them but are old. I am not sure we actually want to delete those.
app/models/case_contact.rb
Outdated
| .where("case_contacts.created_at < ?", 1.week.ago) | ||
| .or(where(draft_case_ids: []).where("case_contacts.created_at < ?", 1.day.ago)) |
There was a problem hiding this comment.
I'm not sure what the best names would be but can we turn 1.week.ago into named constants as opposed to magic values?
app/models/case_contact.rb
Outdated
| scope :drafts_for_removal, -> { | ||
| where.not(status: "active") | ||
| .where("case_contacts.created_at < ?", 1.week.ago) | ||
| .or(where(draft_case_ids: []).where("case_contacts.created_at < ?", 1.day.ago)) | ||
| } | ||
|
|
There was a problem hiding this comment.
We aren't the most consistent with it in this code base but I think we should keep business logic out of our scopes. In this case the designation of what is "for removal" is that business logic.
Most of the other scopes are pretty generic.
scope :created_max_ago, ->(time_range = nil) {
where("case_contacts.created_at > ?", time_range) if time_range.present?
}I think this query belongs in a service object or maybe just some plain ruby object.
There was a problem hiding this comment.
Got it. I'll move it into a service class and do the destroy there as well so the rake task just calls the service method. I'll update tests accordingly
spec/models/case_contact_spec.rb
Outdated
| let!(:new_draft_case_contact) { create(:case_contact, status: "started", created_at: Time.now, draft_case_ids: []) } | ||
| let!(:draft_case_contact_without_draft_case_id) { create(:case_contact, status: "started", created_at: Time.now - 2.days, draft_case_ids: []) } | ||
| let!(:draft_case_contact_with_draft_case_id) { create(:case_contact, status: "started", created_at: Time.now - 2.days, draft_case_ids: [1]) } | ||
| let!(:eight_day_draft_case_contact) { create(:case_contact, status: "details", created_at: Time.now - 8.days) } |
There was a problem hiding this comment.
can this also be status: "started" for consistency?
There was a problem hiding this comment.
I actually did that intentionally to mimic a draft that was saved correctly after step one (moving it to 'details') but was then abandoned & was over a week old to cover the second half of the ticket (remove any drafts older than a week)
spec/models/case_contact_spec.rb
Outdated
| let!(:past_expiration) { Time.now - 8.days } | ||
| let!(:past_exp_without_case) { Time.now - 2.days } |
There was a problem hiding this comment.
You can also do 8.days.ago, etc.
| draft_expiration = 1.week.ago | ||
| draft_expiration_without_case = 1.day.ago |
There was a problem hiding this comment.
I meant constants like DRAFT_EXPIRATION_DELAY and ORPHANED_DRAFT_EXPIRATION_DELAY
Not sure if orphaned is a better name
|
Cool stuff :) |
| ORPHANED_DRAFT_EXPIRATION_DELAY_WITHOUT_CASE = 1.day.ago | ||
|
|
||
| def self.call | ||
| drafts_for_removal = CaseContact.where.not(status: "active").where("case_contacts.created_at < ?", ORPHANED_DRAFT_EXPIRATION_DELAY) |
There was a problem hiding this comment.
I was just looking at some enum things:
| drafts_for_removal = CaseContact.where.not(status: "active").where("case_contacts.created_at < ?", ORPHANED_DRAFT_EXPIRATION_DELAY) | |
| drafts_for_removal = CaseContact.not_active.where("case_contacts.created_at < ?", ORPHANED_DRAFT_EXPIRATION_DELAY) |
status enum should provide this scope, and will not break if enum definition changes, such as switch to integer column. (Unlikely to happen, just thought this is useful to know). Same could be used below.
What github issue is this PR for, if any?
Resolves #5442
What changed, and why?
Added a scope to CaseContact to return drafts without an attached case as well as drafts older than one week. The rake task actually deletes the case contact. The task will need to be added to Heroku task scheduler.
How is this tested? (please write tests!) 💖💪
There's a model test on CaseContact that tests different scenarios and ensures only case contacts that should be cleaned up are returned.
Screenshots please :)
Before running rake task:
After running rake task:
Feelings gif (optional)