Skip to content
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

Post-Deadline notification #4475

Merged
merged 11 commits into from Apr 12, 2024
Merged

Conversation

rickreyhsig
Copy link
Contributor

@rickreyhsig rickreyhsig commented Apr 11, 2024

Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
Copy link

Heroku app: https://gyr-review-app-4475-ab50c51f4205.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4475 (optionally add --tail)

Ricardo Kreyhsig added 2 commits April 11, 2024 13:33
Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
rickreyhsig and others added 2 commits April 11, 2024 17:42
Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
Copy link
Contributor

@mpidcock mpidcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like you to double check the cutoff logic

.where.not(email_address: nil)
.where.not(email_address_verified_at: nil)
.where(unsubscribed_from_email: false)
.where("#{base_class.underscore.pluralize}.message_tracker #> '{messages.state_file.post_deadline_reminder}' IS NULL")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? I thought the send_only_once? config would take care of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% not needed; added for performance. Since intakes that have this message sent already anyway, I don't see why we should clog up system resources in DelayedJobs for emails that won't get sent anyway. Thinking forward for next years, this could be a TON of intakes that get loaded into memory for no reason. Thanks for bringing it up though.

class_object = base_class.constantize
intakes_to_notify += class_object.left_joins(:efile_submissions)
.where(efile_submissions: { id: nil })
.where("#{base_class.underscore.pluralize}.created_at < ?", cutoff_time_ago)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From reading the requirements, I don't think there is a cutoff for how long ago the intake was created. Only a cutoff for if they already got the reminder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

spec/tasks/send_post_deadline_reminders_spec.rb Outdated Show resolved Hide resolved
Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
Co-authored-by: Ricardo Kreyhsig <rkreyhsig@codeforamerica.org>
Copy link
Contributor

@mpidcock mpidcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍰

@rickreyhsig rickreyhsig merged commit d17e150 into main Apr 12, 2024
7 checks passed
@rickreyhsig rickreyhsig deleted the post-deadline-notification#187355955 branch April 12, 2024 17:59
jenny-heath added a commit that referenced this pull request Apr 12, 2024
2024-04-12 Last release of tax season 2024

* Send reject resolution message on 4/13 and 4/22 for clients that have been notified of rejection and not yet been accepted (#4479)
* Update GYR banner for closing season (#4473)
* Always showing download button after April 25 #187355990  (#4481)
* Post-Deadline notification (#4475)
* Pre-Deadline notification (#4469)
* truncate 3rd party designee name (#4462)
* Change logic for Reminder notification (#4447)
* Close fyst #187355990 (#4472)
* Fix submission and skip flaky tests #187278919 (#4468)
* Refine AZ excise credit #186927846 (#4418)
* pre-populate w2 info (#4457)
* Center content on desktop #186632607 (#4444)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants