Add one_email_per_occurrence option and migration task #53
Conversation
Introduce the one_email_per_occurrence configuration option to limit email notifications to one per occurrence. Updated README and error controller to support the new feature, and added Rake task to handle migrations.
lib/solid_errors/db/migrate/20240716154238_add_prev_resolved_at_to_solid_errors.rb
Outdated
Show resolved
Hide resolved
Thank you so much for taking the time to review this and to add all your helpful edits and comments. Much much appreciated! I am not sure if you need me to/if I should make some commits in my branch and redo my PR (this is my first PR to a gem so please forgive any stupidity or missteps on my part!). Thanks, again, so much for your work. Please know how much my team appreciates Solid Errors! |
|
Yeah, you've done such a great job with the PR that I'd like you to finish it yourself and have full credit! |
AWESOME! Man, really appreciate it. I will make the changes and commit them shortly |
- Converted prev_resolved_at to a datetime from a timestamp in migration - Added a method `should_send_email?` to determine if an email goes instead of a string of conditionals - Cleaned up guard clauses - Improved the datetime comparison in an AR query
The refactor involved removing a segment which forced a specific migration to run in favor of Rails' mechanism of alerting when migrations are pending. The prior method was too rigid and hard-coded. Now, the system still copies the migration over but doesn't automatically run it.
The 'prev_resolved_at' parameter has been removed from the error_params in the errors_controller, and subsequently from the error buttons in the views. Meanwhile, in the subscriber, 'prev_resolved_at' is now updated with the current 'resolved_at' before this is set to nil, maintaining the history of resolution times.
|
For the Basically, I removed that element from the buttons in the Maybe less intrusive and more seamless? Totally open to other alternatives of course! Finally, for the migration, I just kept the rake task the same in terms of copying the migration files over. But, I killed the running of them (I have some commented out code as an alternative approach to run just the specific migration if desired) because Rails will, of course, scream and yell at us that we have pending migrations to run ... duh on my part 🤣 |
Simplified the condition check whether to send an email in SolidErrors::Occurrence. The method should_send_email? now has guard clauses if SolidErrors does not send emails or if no email recipient is set. Additionally, the instance method one_email_per_occurrence is renamed to one_email_per_occurrence? to follow the naming convention like `send_emails?`.
The config option that limits the email notifications to one email per error occurrence in the SolidErrors has been removed and, instead, that approach is used automatically (sending one per occurrence). Corrections were also made to the post-install message in the gemspec file to remove the ``*and* runs` from the note since we are not auto-running migrations.
|
We still need a note in the README explaining the behavior for email sending |
Just pushed a commit with an edited note about sending emails. Let me know if you think that fits the bill or needs some added massaging, etc. |
…ng process In the 'Occurrence' model, the condition checking if an error has only one occurrence has been simplified by replacing 'count == 1' with 'one?'. In the rake task, the file copying process from Solid Errors to Rails application has been altered so that instead of copying the file directly, it's named with a timestamp before copying to avoid potential naming conflicts. The migration file name was also simplified by removing the timestamp so that we can dynamically add one.
This patch replaces the original file existence check so that it now checks if there are any files with the same original filename (no timestamp) in the entire directory structure under the destination path. This avoids unnecessary file copy operations.
|
I know we are taking a while polishing the final exacting details, but we are close and this is a great PR. Thanks for the patience |
Happy to! I like getting things polished and as close to ideal as possible. No problems at all on my end! I'll commit again later tonight. Time to go walk and feed the pups! |
Don't need nested directory in glob
Co-authored-by: Stephen Margheim <stephen.margheim@gmail.com>
Co-authored-by: Stephen Margheim <stephen.margheim@gmail.com>
Co-authored-by: Stephen Margheim <stephen.margheim@gmail.com>
Added instructions to README file for running migration installer after updating the gem. Also, refactored 'solid_errors_tasks.rake' by simplifying the filename generation process during migration. These changes facilitate smoother gem updates and migrations.
|
Ok, Stephen, I think I got that last round of changes in place. Thanks for catching the spelling error(s) 🤦🏻 Let me know how that README line sounds about the post-install message, etc. Or any other changes you find need made. Thanks! |
|
I want to chat with @bensheldon some about how he manages versions with schema changes before merging and releasing. I want a clear plan. |
|
👋🏻 Happy to chat. I can try to sketch out a few things (I've been wanting to blog about this forever!) and maybe invite further chatting. Upfront: GoodJob does a lot of migrations. A lot! I want to say there have been about 30 migration files over its lifespan so far. That's more than like Active Storage which has... 4. So take this with a grain of salt if your gem is more like Active Storage than GoodJob.
Sorry, that's a wall of text 😅 |
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
fyi, Rails Engines have this functionality built in. I don't use it in GoodJob, but that was more out of ignorance at the time than anything else.
https://guides.rubyonrails.org/engines.html#engine-setup
Solid Queue does it really well:
There was a problem hiding this comment.
This is really great! Thanks so much @bensheldon (particularly for the great write-up above!)
This version (x.003) has changes that ensure the migration is copied to the containing application.
|
I pushed a small update (well, two) yesterday evening. These two ensure that, in the current approach to the migration, it does copy over correctly. Note: I did bump the version on this branch ( Let me know, please, if you have any thoughts or approaches you'd like for this PR's final push. Thanks! |
|
I'm on vacation for the week. Will wrap up when I get back. Thanks for the great work. |
Motivation:
We are happily using Solid Errors in our production app but because that app connects to some external APIs we can run into situations where we are sent a lot of emails for the same error's reoccurrence.
Details:
Introduced a new configuration option
one_email_per_occurrencethat serves to limit email notifications to one per occurrence. If an error is resolved but reoccurs, an email will be sent, again, for that first reoccurrence.In order to do this, a migration was created to add a new column/attribute to the
solid_errorstable calledprev_resolved_at.This column/attribute is updated to match the
resolved_atvalue whenever it is set - but it is never nill'ed out likeresolved_atis when the issue is resolved.A Rake task,
solid_errors:install_migrations, was also added to copy any new migrations from thelib/solid_errors/db/migratedirectory and then immediately run them.The README was updated as was the error controller to support the new feature.
The
.gemspecwas also updated to add a note reminding users to ensure they've run the task to copy over the migration(s).Please let me know if there's anything you'd change, do better, etc. Thank you for your excellent work on Solid Errors. We are grateful to have it!