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

98603 Callbacks for ZSF logging - IVC CHAMPVA forms #20015

Conversation

michaelclement
Copy link
Contributor

@michaelclement michaelclement commented Dec 23, 2024

Summary

This PR adds a custom callback_class per this VA Notify documentation. The main change is that calls to monitor.log_silent_failure_avoided will now feature a completed email_confirmed property so that when they are viewed in this dashboard they are categorized properly.

Related issue(s)

Testing done

  • New code is covered by unit tests
  • Describe what the old behavior was prior to the change
  • Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
  • If this work is behind a flipper:
    • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
    • What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

@va-vfs-bot va-vfs-bot temporarily deployed to 98603-add-logging-to-capture-callback-from-va-notify-when-a-user-has-received-a-zsf-email/main/main December 30, 2024 16:24 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 98603-add-logging-to-capture-callback-from-va-notify-when-a-user-has-received-a-zsf-email/main/main January 2, 2025 18:26 Inactive
Comment on lines +180 to +182
champva_vanotify_custom_callback:
actor_type: user
description: Enables the custom callback_klass when sending IVC CHAMPVA failure emails with VA Notify
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feature flag so we can test in staging.

Comment on lines +65 to +76

if Flipper.enabled?(:champva_vanotify_custom_callback, @current_user)
form_data = form_data.merge(
{
callback_klass: 'IvcChampva::ZsfEmailNotificationCallback',
callback_metadata: {
statsd_tag: 'veteran-ivc-champva-forms',
additional_context:
}
}
)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing this callback class to VA Notify when we send the email allows us to gather the email delivery status, which is then going to the DD silent failures dashboard.

ActiveRecord::Base.transaction do
if IvcChampva::Email.new(form_data).send_email
fetch_forms_by_uuid(form[:form_uuid]).update_all(email_sent: true) # rubocop:disable Rails/SkipsModelValidations
else
additional_context = { form_id: form[:form_number], form_uuid: form[:form_uuid] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now being passed in to the function above rather than created here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual callback class that gets run via VA Notify when an email's status is determined (modified from this example).

The only side effects from this callback are more logging calls.

Comment on lines -29 to +34
send_failure_email(form, template_id)
additional_context = { form_id: form[:form_number], form_uuid: form[:form_uuid] }
monitor.log_silent_failure_avoided(additional_context)
monitor.track_missing_status_email_sent(form[:form_number])
unless Flipper.enabled?(:champva_vanotify_custom_callback, @current_user)
monitor.log_silent_failure_avoided(additional_context)
monitor.track_missing_status_email_sent(form[:form_number])
end
send_failure_email(form, template_id, additional_context)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These calls to the monitor have been moved inside our callback class so they have access to the email delivery status. send_failure_email has been updated to pass the callback_klass to VA Notify.

When our flipper is NOT enabled, we default to logging here per existing functionality.

@va-vfs-bot va-vfs-bot temporarily deployed to 98603-add-logging-to-capture-callback-from-va-notify-when-a-user-has-received-a-zsf-email/main/main January 2, 2025 19:30 Inactive
cloudmagic80
cloudmagic80 previously approved these changes Jan 3, 2025
@michaelclement michaelclement marked this pull request as ready for review January 3, 2025 13:54
@michaelclement michaelclement requested review from a team as code owners January 3, 2025 13:54
@@ -23,7 +23,7 @@ def initialize(data)
@data = data
end

def send_email
def send_email # rubocop:disable Metrics/MethodLength
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to avoid inline disabling rubocop rules. Is there a way to break this up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to streamline how I'm accessing that object (updated in this commit) - please let me know if there's anything else I can modify. Thanks! @LindseySaari

cloudmagic80
cloudmagic80 previously approved these changes Jan 3, 2025
@va-vfs-bot va-vfs-bot temporarily deployed to 98603-add-logging-to-capture-callback-from-va-notify-when-a-user-has-received-a-zsf-email/main/main January 6, 2025 14:29 Inactive
cloudmagic80
cloudmagic80 previously approved these changes Jan 6, 2025
@va-vfs-bot va-vfs-bot temporarily deployed to 98603-add-logging-to-capture-callback-from-va-notify-when-a-user-has-received-a-zsf-email/main/main January 6, 2025 15:02 Inactive
@michaelclement
Copy link
Contributor Author

Good morning, @LindseySaari! When you get a chance, could I please have another review of this PR? I updated some testing related issues and fixed the request above. Thank you!

@michaelclement michaelclement changed the title 98603 Callbacks for ZSF logging - IVC CHAMPV forms 98603 Callbacks for ZSF logging - IVC CHAMPVA forms Jan 6, 2025
@michaelclement michaelclement merged commit 5d4bab5 into master Jan 9, 2025
22 of 23 checks passed
@michaelclement michaelclement deleted the 98603-add-logging-to-capture-callback-from-va-notify-when-a-user-has-received-a-zsf-email branch January 9, 2025 15:06
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.

5 participants