-
Notifications
You must be signed in to change notification settings - Fork 69
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
POA decline email #20918
base: master
Are you sure you want to change the base?
POA decline email #20918
Conversation
…cation-in-va-notify-staging
A new rubocop rule has been introduced recently that requires hash key-values where the key and the value match like:a notification_type: notification_type, to be condensed to: notification_type:,
AccreditedRepresentativePortal::PowerOfAttorneyRequestEmailJob.perform_async( | ||
email_data.email_address, | ||
Settings.vanotify.services.va_gov.template_id.appoint_a_representative_digital_submit_confirmation_email, | ||
notification.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's an issue to place PII in Sidekiq job arguments (they show up in Sidekiq UI for example and potentially get sent to DD logs).
But overall, that idea of capturing the entity in a PowerOfAttorneyRequestNotification
and then enqueuing work based on just the ID of that entity applies here too.
The notification record itself can know about the claimant's email, and also the email template that it uses.
notification.claimant_email # or `recipient_email`?, chain calls or introduce delegation
notification.email_template
Email template can be based on the type of notification. STI is an option there or just a switch statement if the variation in behavior is really simplistic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think it was an issue to send the email address in the arguments. That matches the implementation of the VANotify::EmailJob
that is widespread in the app and that is the pattern I worked from.
I'll go ahead and make the change and go with a case statement in the notification model, there are only four cases to keep track of.
We can look it up from the POA request notification record.
Summary
Related issue(s)
Testing done
Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas) Most of the changes are in the
AccreditedRepresentativePortal
module. There are a few changes toSettings
for sending the email, and a small change to theRepresentationManagement
module to track the submit confirmation email.Acceptance criteria