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

Add error logging for SendNotificationEmailJob #21028

Merged
merged 8 commits into from
Mar 5, 2025

Conversation

Thrillberg
Copy link
Contributor

Summary

This PR adds error logging for the Simple Forms SendNotificationEmailJob, which I think will illuminate an error we're seeing in the Benefits Intake Status Job.

@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main February 28, 2025 18:31 Inactive
@Thrillberg Thrillberg force-pushed the logging-for-benefits-intake-status-job branch from 9f499d2 to 51a49d7 Compare February 28, 2025 19:07
@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main February 28, 2025 19:08 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main February 28, 2025 19:28 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main February 28, 2025 21:12 Inactive
@Thrillberg Thrillberg marked this pull request as ready for review March 1, 2025 01:03
@Thrillberg Thrillberg requested review from a team as code owners March 1, 2025 01:03
pennja
pennja previously approved these changes Mar 3, 2025
Copy link
Contributor

@pennja pennja left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

def handle_exception(e)
Rails.logger.error(
'Error sending simple forms notification email',
message: e.message,
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to confirm there's no possibility for PII / sensitive information being leaked in this log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope this should be good for PII. The error message would not include PII unless it somehow accesses the form data from inside the form submission on its own.

@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main March 4, 2025 21:02 Inactive
pennja
pennja previously approved these changes Mar 4, 2025
Copy link
Contributor

@pennja pennja left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main March 4, 2025 21:23 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main March 4, 2025 21:56 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main March 5, 2025 11:29 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to logging-for-benefits-intake-status-job/main/main March 5, 2025 16:43 Inactive
@rjohnson2011 rjohnson2011 self-requested a review March 5, 2025 17:08
@Thrillberg Thrillberg merged commit 6543c98 into master Mar 5, 2025
27 checks passed
@Thrillberg Thrillberg deleted the logging-for-benefits-intake-status-job branch March 5, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants