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

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

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
eceeff9
Created a new enrollment system service to help with code readability
JoshingYou1 Dec 24, 2024
7fe7ebf
Merge branch 'master' of https://github.com/department-of-veterans-af…
JoshingYou1 Dec 24, 2024
1c48fca
Fixed linting issues
JoshingYou1 Dec 24, 2024
fd68cdd
Fixed linting issues
JoshingYou1 Dec 24, 2024
92a5f1f
Removed some unused test code and updated the swagger specs for HCA
JoshingYou1 Dec 24, 2024
993d293
Fixed some failing tests
JoshingYou1 Dec 24, 2024
874539a
Fixed a couple more failing tests
JoshingYou1 Dec 24, 2024
03066c6
Separated tests with flipper enabled and disabled
JoshingYou1 Dec 24, 2024
6f18f95
Fixed linting issue
JoshingYou1 Dec 26, 2024
efb5141
Fixed linting issue
JoshingYou1 Dec 26, 2024
f2a6d59
Fixed linting issues
JoshingYou1 Dec 26, 2024
49c107b
Fixed linting issues
JoshingYou1 Dec 26, 2024
ae3a419
Fixed linting issue
JoshingYou1 Dec 26, 2024
9347b9b
Removed some code changes that were made in a different branch
JoshingYou1 Jan 7, 2025
35b9cfb
Merged from master
JoshingYou1 Jan 7, 2025
7d05fde
Cleaned up some test code
JoshingYou1 Jan 7, 2025
afe1337
Fixed some linting errors
JoshingYou1 Jan 7, 2025
35a5789
Fixed some more linting errors
JoshingYou1 Jan 7, 2025
8fff4f5
Fixed another linting issue
JoshingYou1 Jan 7, 2025
4683176
Fixed another linting issue
JoshingYou1 Jan 7, 2025
827217d
Fixed some failing tests
JoshingYou1 Jan 8, 2025
19b426f
Tried to fix a linting issue
JoshingYou1 Jan 8, 2025
9efcd0a
Tried to fix a linting issue
JoshingYou1 Jan 8, 2025
761d5f2
test: trigger preview environment
JoshingYou1 Jan 14, 2025
d1599d5
test: trigger preview environment
JoshingYou1 Jan 15, 2025
ec715c9
Merge branch 'master' of https://github.com/department-of-veterans-af…
JoshingYou1 Jan 15, 2025
c891ec6
Merged from master
JoshingYou1 Jan 15, 2025
daf3fe7
Fixed linting issues
JoshingYou1 Jan 16, 2025
fa9eb40
Merge branch 'master' of https://github.com/department-of-veterans-af…
JoshingYou1 Jan 16, 2025
37e125d
Fixed linting issues
JoshingYou1 Jan 16, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/models/health_care_application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
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 seemed a bit more logical than calling a method that calls this line of code.


result = begin
HCA::Service.new(user).submit_form(parsed_form)
Expand Down Expand Up @@ -240,7 +240,7 @@ def prefill_fields

def submit_async
submission_job = email.present? ? 'SubmissionJob' : 'AnonSubmissionJob'
@parsed_form = override_parsed_form(parsed_form)
@parsed_form = HCA::OverridesParser.new(parsed_form).override

"HCA::#{submission_job}".constantize.perform_async(
self.class.get_user_identifier(user),
Expand Down
4 changes: 4 additions & 0 deletions config/features.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1424,6 +1424,10 @@ features:
actor_type: user
description: Use the original veteran_x models to power Appoint a Rep entity search
enable_in_development: true
va1010_forms_enrollment_system_service_enabled:
actor_type: user
description: Enables the VA1010Forms enrollment system service
enable_in_development: true
va_notify_custom_errors:
actor_type: user
description: Custom error classes instead of the generic Common::Exceptions::BackendServiceException
Expand Down
23 changes: 16 additions & 7 deletions lib/form1010_ezr/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require 'hca/configuration'
require 'hca/ezr_postfill'
require 'va1010_forms/utils'
require 'va1010_forms/enrollment_system/service'

module Form1010Ezr
class Service < Common::Client::Base
Expand Down Expand Up @@ -37,7 +38,13 @@ def submit_async(parsed_form)

def submit_sync(parsed_form)
res = with_monitoring do
es_submit(parsed_form, HealthCareApplication.get_user_identifier(@user), FORM_ID)
if Flipper.enabled?(:va1010_forms_enrollment_system_service_enabled)
VA1010Forms::EnrollmentSystem::Service.new(
HealthCareApplication.get_user_identifier(@user)
).submit(parsed_form, FORM_ID)
else
es_submit(parsed_form, HealthCareApplication.get_user_identifier(@user), FORM_ID)
end
end

# Log the 'formSubmissionId' for successful submissions
Expand Down Expand Up @@ -106,11 +113,8 @@ def validate_form(parsed_form)
)
end

log_validation_errors(parsed_form)
log_validation_errors(validation_errors, parsed_form)

Rails.logger.error(
"10-10EZR form validation failed. Form does not match schema. Error list: #{validation_errors}"
)
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 logger was moved into the log_validation_errors method.

raise Common::Exceptions::SchemaValidationErrors, validation_errors
end
end
Expand Down Expand Up @@ -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
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 seemed a bit more logical than calling a method that calls this line of code.

add_financial_flag(parsed_form)
end

Expand All @@ -167,9 +171,14 @@ def add_financial_flag(parsed_form)
end
end

def log_validation_errors(parsed_form)
# @param [Hash] errors
def log_validation_errors(errors, parsed_form)
StatsD.increment("#{Form1010Ezr::Service::STATSD_KEY_PREFIX}.validation_error")

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

PersonalInformationLog.create(
data: parsed_form,
error_class: 'Form1010Ezr ValidationError'
Expand Down
6 changes: 3 additions & 3 deletions lib/hca/enrollment_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

def veteran_to_save_submit_form(
veteran,
current_user,
user_identifier,
form_id
)
return {} if veteran.blank?

copy_spouse_address!(veteran)

request = build_form_for_user(current_user, form_id)
request = build_form_for_user(user_identifier, form_id)

veteran['attachments']&.each_with_index do |attachment, i|
guid = attachment['confirmationCode']
Expand Down
7 changes: 6 additions & 1 deletion lib/hca/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require 'hca/enrollment_system'
require 'hca/configuration'
require 'va1010_forms/utils'
require 'va1010_forms/enrollment_system/service'

module HCA
class Service < Common::Client::Base
Expand All @@ -24,7 +25,11 @@ def submit_form(form)
is_short_form = HealthCareApplication.new(form: form.to_json).short_form?

with_monitoring do
es_submit(form, @user, '10-10EZ')
if Flipper.enabled?(:va1010_forms_enrollment_system_service_enabled)
VA1010Forms::EnrollmentSystem::Service.new(@user).submit(form, '10-10EZ')
else
es_submit(form, @user, '10-10EZ')
end
rescue => e
increment_failure('submit_form_short_form', e) if is_short_form
raise e
Expand Down
87 changes: 87 additions & 0 deletions lib/va1010_forms/enrollment_system/service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# frozen_string_literal: true

require 'benchmark'
require 'common/client/base'
require 'hca/configuration'
require 'hca/overrides_parser'

module VA1010Forms
module EnrollmentSystem
class Service < Common::Client::Base
include ActionView::Helpers::NumberHelper

configuration HCA::Configuration

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

Choose a reason for hiding this comment

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

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

@user_identifier = user_identifier
end

def submit(parsed_form, form_id)
formatted = HCA::EnrollmentSystem.veteran_to_save_submit_form(
parsed_form,
@user_identifier,
form_id
)
submission_body = submission_body(formatted)
response = perform(:post, '', submission_body)

root = response.body.locate('S:Envelope/S:Body/submitFormResponse').first
form_submission_id = root.locate('formSubmissionId').first.text.to_i

{
success: true,
formSubmissionId: form_submission_id,
timestamp: root.locate('timeStamp').first&.text || Time.now.getlocal.to_s
}
rescue => e

Check failure on line 39 in lib/va1010_forms/enrollment_system/service.rb

View workflow job for this annotation

GitHub Actions / Linting and Security

Lint/UselessRescue: Useless `rescue` detected.
raise e
end

private

def soap
# Savon *seems* like it should be setting these things correctly
# from what the docs say. Our WSDL file is weird, maybe?
Savon.client(
wsdl: HCA::Configuration::WSDL,
env_namespace: :soap,
element_form_default: :qualified,
namespaces: {
'xmlns:tns': 'http://va.gov/service/esr/voa/v1'
},
namespace: 'http://va.gov/schema/esr/voa/v1'
)
end

def log_payload_info(formatted_form, submission_body)
form_name = formatted_form.dig('va:form', 'va:formIdentifier', 'va:value')
attachments = formatted_form.dig('va:form', 'va:attachments')
attachment_count = attachments&.length || 0
# Log the attachment sizes in descending order
if attachment_count.positive?
# Convert the attachments into xml format so they resemble what will be sent to VES
attachment_sizes =
attachments.map { |a| a.to_xml.size }.sort.reverse!.map { |size| number_to_human_size(size) }.join(', ')

Rails.logger.info("Attachment sizes in descending order: #{attachment_sizes}")
end

Rails.logger.info(
"Payload for submitted #{form_name}: Body size of #{number_to_human_size(submission_body.bytesize)} " \
"with #{attachment_count} attachment(s)"
)
end

def submission_body(formatted_form)
content = Gyoku.xml(formatted_form)
submission_body = soap.build_request(:save_submit_form, message: content).body
log_payload_info(formatted_form, submission_body)

submission_body
end
end
end
end
4 changes: 0 additions & 4 deletions lib/va1010_forms/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ def soap
)
end

def override_parsed_form(parsed_form)
HCA::OverridesParser.new(parsed_form).override
end

private

def submission_body(formatted_form)
Expand Down
Loading
Loading