-
Notifications
You must be signed in to change notification settings - Fork 66
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
Begin adding notification_first_name to Simple Forms models #20376
Begin adding notification_first_name to Simple Forms models #20376
Conversation
33cadb9
to
a372d8f
Compare
Generated by 🚫 Danger |
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.
Looks good, great work! 👍
@Thrillberg is
|
@@ -3,6 +3,7 @@ | |||
require 'json' | |||
|
|||
module SimpleFormsApi | |||
# rubocop:disable Metrics/ClassLength |
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.
@Thrillberg, can you look into getting this file under 400 lines?
May I suggest moving the contents of create_attachment_page
to a new model
module SimpleFormsApi
module VBA4010007
class Attachment
attr_reader :file_path, :data
def initialize(file_path:, data:)
@file_path = file_path
@data = @data
end
def create
veteran_sex = get_gender(@data.dig('application', 'veteran', 'gender'))
# etc etc
then you can do this in the form model
def create_attachment_page(file_path)
SimpleFormsApi::VBA4010007::Attachment.new(file_path:, data: @data).create
end
Thanks @stevenjcumming ! I've corrected the typo (confirmed by @ksantiagoBAH on the team that owns this form to be a likely typo) and refactored as you recommended. I think since I still have to override Rubocop in some spots, it could use some more refactoring and it sounds like @ksantiagoBAH has that on his team's backlog now. I'm seeing test failures that look unrelated. Do you think they are? |
It's related, there's a naming conflict, there's a class and module called Perhaps you could rename the new class |
Oh haha of course! I've just renamed it to |
@stevenjcumming This PR is ready for re-review! It's gotten quite huge though. Do you think I should break out the 40-10007 changes into a separate PR to merge first and then get to the main part of this one? |
@Thrillberg no it looks like it's just copy and paste move to another file. It does look much better though |
@@ -12,5 +12,9 @@ def initialize(data) | |||
@data = data | |||
@signature_date = Time.current.in_time_zone('America/Chicago') | |||
end | |||
|
|||
def notification_first_name | |||
data.dig('veteran_full_name', 'first') |
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.
Nit: Do you want to use the instance variable @data
just to be consistent with the other notification_first_name method
?
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 think I'd rather change the others to be data
(lose the @
) since it's an attribute anyway. I'll update.
7faeeee
to
b40f091
Compare
Summary
This PR adds a method to every Simple Forms form model (including the base model),
notification_first_name
. This method is meant to specify the first name of the recipient of email notifications.There is a bit of followup work from this:
notification_email_address
.NotificationEmail
to look at these methods instead of holding all the logic itself.Related issue(s)
https://github.com/orgs/department-of-veterans-affairs/projects/1443/views/3?sliceBy%5Bvalue%5D=Thrillberg&pane=issue&itemId=90296153&issue=department-of-veterans-affairs%7CVA.gov-team-forms%7C1908