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

Begin adding notification_first_name to Simple Forms models #20376

Merged

Conversation

Thrillberg
Copy link
Contributor

@Thrillberg Thrillberg commented Jan 21, 2025

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:

  1. Another PR to add methods for notification_email_address.
  2. Modify our 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

@va-vfs-bot va-vfs-bot temporarily deployed to add-notification-first-name-to-simple-forms-models/main/main January 21, 2025 19:30 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to add-notification-first-name-to-simple-forms-models/main/main January 21, 2025 21:08 Inactive
@Thrillberg Thrillberg force-pushed the add-notification-first-name-to-simple-forms-models branch from 33cadb9 to a372d8f Compare January 24, 2025 13:28
@va-vfs-bot va-vfs-bot temporarily deployed to add-notification-first-name-to-simple-forms-models/main/main January 24, 2025 13:35 Inactive
Copy link

github-actions bot commented Jan 24, 2025

1 Error
🚫 This PR changes 822 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, those exceeding
500 will not be reviewed, nor will they be allowed to merge. Please break this PR up into
smaller ones.

If you have reason to believe that this PR should be granted an exception, please see the
Submitting pull requests for approval - FAQ.

File Summary

Files

  • modules/simple_forms_api/app/models/simple_forms_api/base_form.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_20_10206.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_20_10207.rb (+9/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_21_0845.rb (+7/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_21_0966.rb (+7/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_21_0972.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_21_10210.rb (+9/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_21_4142.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_21p_0847.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_26_4555.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_40_0247.rb (+3/-0)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_40_10007.rb (+9/-216)

  • modules/simple_forms_api/app/models/simple_forms_api/vba_40_10007_attachment.rb (+230/-0)

  • modules/simple_forms_api/spec/models/base_form_spec.rb (+14/-0)

  • modules/simple_forms_api/spec/models/vba_20_10206_spec.rb (+18/-0)

  • modules/simple_forms_api/spec/models/vba_20_10207_spec.rb (+44/-0)

  • modules/simple_forms_api/spec/models/vba_21_0845_spec.rb (+30/-0)

  • modules/simple_forms_api/spec/models/vba_21_0966_spec.rb (+30/-0)

  • modules/simple_forms_api/spec/models/vba_21_0972_spec.rb (+18/-0)

  • modules/simple_forms_api/spec/models/vba_21_10210_spec.rb (+53/-0)

  • modules/simple_forms_api/spec/models/vba_21_4142_spec.rb (+19/-0)

  • modules/simple_forms_api/spec/models/vba_21p_0847_spec.rb (+18/-0)

  • modules/simple_forms_api/spec/models/vba_26_4555_spec.rb (+16/-1)

  • modules/simple_forms_api/spec/models/vba_40_0247_spec.rb (+13/-0)

  • modules/simple_forms_api/spec/models/vba_40_10007_spec.rb (+39/-1)

    Note: We exclude files matching the following when considering PR size:

    *.csv, *.json, *.tsv, *.txt, *.md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru, *.pdf
    

Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

@va-vfs-bot va-vfs-bot temporarily deployed to add-notification-first-name-to-simple-forms-models/main/main January 24, 2025 14:15 Inactive
@Thrillberg Thrillberg marked this pull request as ready for review January 24, 2025 15:16
@Thrillberg Thrillberg requested review from a team as code owners January 24, 2025 15:16
@va-vfs-bot va-vfs-bot temporarily deployed to add-notification-first-name-to-simple-forms-models/main/main January 24, 2025 15:36 Inactive
pennja
pennja previously approved these changes Jan 24, 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, great work! 👍

@stevenjcumming
Copy link
Contributor

@Thrillberg is highest_rank_ misspelled here?

highest_rank_a = @data.dig('application', 'veteran', 'service_records', 0, 'highest_rank_') || ''

@@ -3,6 +3,7 @@
require 'json'

module SimpleFormsApi
# rubocop:disable Metrics/ClassLength
Copy link
Contributor

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

@va-vfs-bot va-vfs-bot temporarily deployed to add-notification-first-name-to-simple-forms-models/main/main January 27, 2025 16:18 Inactive
@Thrillberg
Copy link
Contributor Author

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?

@stevenjcumming
Copy link
Contributor

stevenjcumming commented Jan 27, 2025

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 VBA4010007. Not sure what I was thinking there.

Perhaps you could rename the new class SimpleFormsApi::Attachments::VBA4010007? Or maybe there's a better name that I can't think of

@Thrillberg
Copy link
Contributor Author

It's related, there's a naming conflict, there's a class and module called VBA4010007. Not sure what I was thinking there.

Perhaps you could rename the new class SimpleFormsApi::Attachments::VBA4010007? Or maybe there's a better name that I can't think of

Oh haha of course! I've just renamed it to SimpleFormsApi::VBA4010007Attachment. I think we'll want to refactor all of these models (they're all pretty big) into their own directories with a module for each. But that can come later!

stevenjcumming
stevenjcumming previously approved these changes Jan 27, 2025
@Thrillberg
Copy link
Contributor Author

@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?

@stevenjcumming
Copy link
Contributor

@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')
Copy link
Contributor

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?

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 think I'd rather change the others to be data (lose the @) since it's an attribute anyway. I'll update.

@stevenjcumming stevenjcumming merged commit 23101f0 into master Jan 28, 2025
21 of 22 checks passed
@stevenjcumming stevenjcumming deleted the add-notification-first-name-to-simple-forms-models branch January 28, 2025 14:16
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