#90355 [10-10 Health Apps] Improve code quality for shared code #20022

JoshingYou1 · 2024-12-24T17:37:38Z

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

The majority of the code written in this PR was to split unit tests out for when the flipper is enabled and disabled.

This work is behind a feature toggle (flipper): YES
(Summarize the changes that have been made to the platform)

Utilizes the new VA1010Forms::EnrollmentSystem::Service for the 10-10EZ and 10-10EZR.
Moves some tests called conformance tests from spec/lib/hca/service_spec.rb to spec/lib/va1010_forms/enrollment_system/service_spec.rb because they don't actually test anything functionality that belongs to the HCA::Service class.
Updates existing tests.

(If bug, how to reproduce)
(What is the solution, why is this the solution?)
(Which team do you work for, does your team own the maintenance of this component?)
10-10 Health apps
(If introducing a flipper, what is the success criteria being targeted?)
The success criteria is that the app functions the same way with the toggle on and off.

Related issue(s)

https://github.com/orgs/department-of-veterans-affairs/projects/1433/views/35?pane=issue&itemId=85113784&issue=department-of-veterans-affairs%7Cva.gov-team%7C90355
Link to previous change of the code/bug (if applicable)
Link to epic if not included in ticket

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:
Yes. Tests are written for when the flipper is on and when it's off
- 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?

10-10EZ and 10-10EZR forms

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?

…fairs/vets-api into 90355_1010_apps_improve_code_quality

github-actions · 2024-12-24T17:38:24Z

	1 Error
🚫	This PR changes `1345` 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 config/features.yml (+4/-0) lib/form1010_ezr/service.rb (+24/-17) lib/hca/service.rb (+6/-1) spec/lib/form1010_ezr/service_spec.rb (+237/-225) spec/lib/hca/service_spec.rb (+345/-159) spec/requests/swagger_spec.rb (+59/-21) spec/requests/v0/health_care_applications_spec.rb (+166/-81) 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

JoshingYou1 · 2024-12-24T19:55:46Z

app/models/health_care_application.rb

@@ -71,7 +71,7 @@ def send_failure_email?
  end

  def submit_sync
-    @parsed_form = override_parsed_form(parsed_form)
+    @parsed_form = HCA::OverridesParser.new(parsed_form).override


This seemed a bit more logical than calling a method that calls this line of code.

JoshingYou1 · 2024-12-24T19:56:15Z

lib/form1010_ezr/service.rb


-        Rails.logger.error(
-          "10-10EZR form validation failed. Form does not match schema. Error list: #{validation_errors}"
-        )


This logger was moved into the log_validation_errors method.

JoshingYou1 · 2024-12-24T19:56:32Z

lib/form1010_ezr/service.rb

@@ -155,7 +159,7 @@ def configure_and_validate_form(parsed_form)
      post_fill_fields(parsed_form)
      validate_form(parsed_form)
      # Due to overriding the JSON form schema, we need to do so after the form has been validated
-      override_parsed_form(parsed_form)
+      HCA::OverridesParser.new(parsed_form).override


This seemed a bit more logical than calling a method that calls this line of code.

JoshingYou1 · 2024-12-24T19:59:42Z

lib/hca/enrollment_system.rb

@@ -865,18 +865,18 @@ def add_attachment(file, id, is_dd214)
    end

    # @param [Hash] veteran data in JSON format
-    # @param [Account] current_user
+    # @param [Hash] user_identifier
    # @param [String] form_id


current_user was not a record, but instead a Hash. I changed the name of this variable because it was kind of misleading.

JoshingYou1 · 2024-12-24T20:00:27Z

lib/va1010_forms/enrollment_system/service.rb

+      # @param [Hash] user_identifier
+      # @example { 'icn' => user.icn, 'edipi' => user.edipi }
+      def initialize(user_identifier = nil)
+        super()


Lint was complaining about not havingsuper, so I added it 😅

JoshingYou1 · 2024-12-24T20:05:03Z

spec/lib/va1010_forms/enrollment_system/service_spec.rb

+    end
+  end
+
+  describe '#submission_body' do


Even though this is a private method, we have some tests in the HCA service spec called conformance tests, which essentially ensure the json form is being formatted into the proper xml. Well, this method is what handles that formatting, so I added them here because it's more of an integration test in the HCA service spec as opposed to a unit test like it is here.

JoshingYou1 · 2024-12-24T20:51:58Z

spec/requests/swagger_spec.rb

+
+          allow_any_instance_of(HCA::Service).to receive(:submit_form) do
+            raise Common::Client::Errors::HTTPError, 'error message'
+          end


This is different than what we had before adding the new enrollment service. The code on line 872 did not work. I'm wondering why we didn't allow it to receive submit_form in the original implementation instead of :post 🤔 This seems much more straight forward 🤷‍♂️ Please let me know if I'm missing something.

It works for the original because the es_submit is an included method on the class, so it is the same as having the HCA::Service call the post method. With the toggle on, you're instantiating your new VA1010Forms::EnrollmentSystem::Service class which is making the call, but it's this new service class that is making the post call now, and not the HCA::Service class anymore. Does that make sense?

So in this case, I do want to call out that this spec isn't really testing the flipper logic, but is rather just testing that the submit_form method is called on the HCA::Service. I think it's fine since we are testing that logic in the service elsewhere, but that does make this test kind of awkward at the moment. You could delete the toggle and this one would still pass. However, it's probably still a good test since once we remove the toggle the old one will fail since we are no longer making the post call from the HCA::Service, but rather the new VA1010Forms::EnrollmentSystem::Service.

So all that to say I think these updated specs here look good as far as I can tell😄

JoshingYou1 · 2024-12-24T20:52:30Z

spec/requests/swagger_spec.rb

+
+          allow_any_instance_of(HCA::Service).to receive(:post) do
+            raise Common::Client::Errors::HTTPError, 'error message'
+          end


This is what I was referring to in the previous comment ⬆️

spec/requests/v0/health_care_applications_spec.rb

…fairs/vets-api into 90355_1010_apps_improve_code_quality

stevenjcumming · 2025-01-16T16:44:35Z

@JoshingYou1 there are some linting warnings

…fairs/vets-api into 90355_1010_apps_improve_code_quality

JoshingYou1 · 2025-01-16T18:12:27Z

@JoshingYou1 there are some linting warnings

They have been addressed. Thank you!

JoshingYou1 added 2 commits December 24, 2024 11:32

Created a new enrollment system service to help with code readability

eceeff9

Merge branch 'master' of https://github.com/department-of-veterans-af…

7fe7ebf

…fairs/vets-api into 90355_1010_apps_improve_code_quality

github-actions bot added lint-failure test-failure labels Dec 24, 2024

va-vfs-bot temporarily deployed to 90355_1010_apps_improve_code_quality/main/main December 24, 2024 18:01 Inactive

Fixed linting issues

1c48fca

va-vfs-bot temporarily deployed to 90355_1010_apps_improve_code_quality/main/main December 24, 2024 19:25 Inactive

Fixed linting issues

fd68cdd

va-vfs-bot temporarily deployed to 90355_1010_apps_improve_code_quality/main/main December 24, 2024 19:46 Inactive

JoshingYou1 commented Dec 24, 2024

View reviewed changes

Removed some unused test code and updated the swagger specs for HCA

92a5f1f

va-vfs-bot temporarily deployed to 90355_1010_apps_improve_code_quality/main/main December 24, 2024 20:51 Inactive

JoshingYou1 commented Dec 24, 2024

View reviewed changes

JoshingYou1 marked this pull request as ready for review December 24, 2024 20:52

JoshingYou1 requested review from a team as code owners December 24, 2024 20:52

github-actions bot added the require-backend-approval label Dec 24, 2024

Fixed some failing tests

993d293

va-vfs-bot temporarily deployed to 90355_1010_apps_improve_code_quality/main/main December 24, 2024 21:08 Inactive

Fixed a couple more failing tests

874539a

JoshingYou1 commented Dec 24, 2024

View reviewed changes

spec/requests/v0/health_care_applications_spec.rb Outdated Show resolved Hide resolved

test: trigger preview environment

761d5f2

va-vfs-bot requested a deployment to 90355_1010_apps_improve_code_quality/main/main January 14, 2025 22:46 Pending

test: trigger preview environment

d1599d5

va-vfs-bot had a problem deploying to 90355_1010_apps_improve_code_quality/main/main January 15, 2025 17:48 Error

Merge branch 'master' of https://github.com/department-of-veterans-af…

ec715c9

…fairs/vets-api into 90355_1010_apps_improve_code_quality

github-actions bot added test-failure and removed test-passing ready-for-backend-review labels Jan 15, 2025

va-vfs-bot had a problem deploying to 90355_1010_apps_improve_code_quality/main/main January 15, 2025 20:02 Error

Merged from master

c891ec6

JoshingYou1 dismissed coope93’s stale review via c891ec6 January 15, 2025 20:41

github-actions bot added the lint-failure label Jan 15, 2025

va-vfs-bot had a problem deploying to 90355_1010_apps_improve_code_quality/main/main January 15, 2025 20:58 Error

platform-support-slack-integration bot mentioned this pull request Jan 15, 2025

I keep getting failed deployments for my PR. Can someone ple... department-of-veterans-affairs/va.gov-team#101211

Closed

coope93 previously approved these changes Jan 16, 2025

View reviewed changes

github-actions bot added test-passing and removed test-failure labels Jan 16, 2025

Fixed linting issues

daf3fe7

JoshingYou1 dismissed coope93’s stale review via daf3fe7 January 16, 2025 17:33

JoshingYou1 added 2 commits January 16, 2025 12:33

Merge branch 'master' of https://github.com/department-of-veterans-af…

fa9eb40

…fairs/vets-api into 90355_1010_apps_improve_code_quality

Fixed linting issues

37e125d

github-actions bot added ready-for-backend-review test-failure and removed lint-failure test-passing ready-for-backend-review labels Jan 16, 2025

va-vfs-bot deployed to 90355_1010_apps_improve_code_quality/main/main January 16, 2025 18:15 View deployment

github-actions bot added the test-passing label Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#90355 [10-10 Health Apps] Improve code quality for shared code #20022

#90355 [10-10 Health Apps] Improve code quality for shared code #20022

JoshingYou1 commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 24, 2024 •

edited

Loading

Files

JoshingYou1 Dec 24, 2024

JoshingYou1 Dec 24, 2024

JoshingYou1 Dec 24, 2024

JoshingYou1 Dec 24, 2024

JoshingYou1 Dec 24, 2024

JoshingYou1 Dec 24, 2024

JoshingYou1 Dec 24, 2024

coope93 Jan 13, 2025

JoshingYou1 Dec 24, 2024

stevenjcumming commented Jan 16, 2025

JoshingYou1 commented Jan 16, 2025

#90355 [10-10 Health Apps] Improve code quality for shared code #20022

Are you sure you want to change the base?

#90355 [10-10 Health Apps] Improve code quality for shared code #20022

Conversation

JoshingYou1 commented Dec 24, 2024 • edited Loading

Summary

The majority of the code written in this PR was to split unit tests out for when the flipper is enabled and disabled.

Related issue(s)

Testing done

Screenshots

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

github-actions bot commented Dec 24, 2024 • edited Loading

Files

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenjcumming commented Jan 16, 2025

JoshingYou1 commented Jan 16, 2025

JoshingYou1 commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 24, 2024 •

edited

Loading