Skip to content

Commit

Permalink
[CST] Use json_schemer in SavedClaim (#19684)
Browse files Browse the repository at this point in the history
* Use json schemer in savedclaim

* Adds actual feature flipper, fixes already-existing tests

* Fixes more existing tests

* Fixes rubocop

* Play more nicely with error array

* More elegant error handling

* Rubocop

* Error handling redux redux

* Stub, rather than actually disable, the feature flipper

* Json schemer can detect fake emails

* Fixes some more specs. Getting there.

* rubocop

* Fixing some more specs, adding some stubs, learning

* See if tests pass with old flipper syntax

* Fixes more specs

* More passing specs. This might be all of them?

* Rubocop

* Adds some affirmative tests with the JSON Schemer flipper enabled

* Rubocop

* Adds comments to json validation methods for clarity on the gem switch

* Finishes adding affirmative (flipper on) tests. Uses true/false each block.

* Standardize error object to use symbols for keys

* Rubocop

* Fixes last spec, hopefully

* Rubocop
  • Loading branch information
iandonovan authored Dec 18, 2024
1 parent e459d58 commit 8a998c8
Show file tree
Hide file tree
Showing 17 changed files with 3,583 additions and 3,298 deletions.
59 changes: 58 additions & 1 deletion app/models/saved_claim.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ def form_matches_schema
unless validation_errors.empty?
Rails.logger.error('SavedClaim form did not pass validation', { guid:, errors: validation_errors })
end

schema_errors.empty? && validation_errors.empty?
end

def to_pdf(file_name = nil)
Expand Down Expand Up @@ -147,14 +149,57 @@ def va_notification?(email_template_id)

private

# Depending on the feature flipper, validate the *entire schema*
# via either the json_schema or json_schemer gem.
# This is tied to vets-api #19684
def validate_schema(schema)
if Flipper.enabled?(:validate_saved_claims_with_json_schemer)
validate_schema_with_json_schemer(schema)
else
validate_schema_with_json_schema(schema)
end
end

# Depending on the feature flipper, validate the *parsed form*
# via either the json_schema or the json_schemer gem.
# This is tied to vets-api #19684
def validate_form(schema, clear_cache)
if Flipper.enabled?(:validate_saved_claims_with_json_schemer)
validate_form_with_json_schemer(schema)
else
validate_form_with_json_schema(schema, clear_cache)
end
end

# For json_schemer, the default behavior is not to raise an exception
# on validation, so we return an array of errors if they exist.
# This method validates the *entire schema*.
def validate_schema_with_json_schemer(schema)
errors = JSONSchemer.validate_schema(schema).to_a
return [] if errors.empty?

reformatted_schemer_errors(errors)
end

# For json_schema, validation errors raise an exception.
# This method validates the *entire schema*.
def validate_schema_with_json_schema(schema)
JSON::Validator.fully_validate_schema(schema, { errors_as_objects: true })
rescue => e
Rails.logger.error('Error during schema validation!', { error: e.message, backtrace: e.backtrace, schema: })
raise
end

def validate_form(schema, clear_cache)
# This method validates the *parsed form* with json_schemer.
def validate_form_with_json_schemer(schema)
errors = JSONSchemer.schema(schema).validate(parsed_form).to_a
return [] if errors.empty?

reformatted_schemer_errors(errors)
end

# This method validates the *parsed form* with json_schema.
def validate_form_with_json_schema(schema, clear_cache)
JSON::Validator.fully_validate(schema, parsed_form, { errors_as_objects: true, clear_cache: })
rescue => e
PersonalInformationLog.create(data: { schema:, parsed_form:, params: { errors_as_objects: true, clear_cache: } },
Expand All @@ -164,6 +209,18 @@ def validate_form(schema, clear_cache)
raise
end

# This method exists to change the json_schemer errors
# to be formatted like json_schema errors, which keeps
# the error logging smooth and identical for both options.
def reformatted_schemer_errors(errors)
errors.map!(&:symbolize_keys)
errors.each do |error|
error[:fragment] = error[:data_pointer]
error[:message] = error[:error]
end
errors
end

def attachment_keys
[]
end
Expand Down
4 changes: 4 additions & 0 deletions config/features.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1537,6 +1537,10 @@ features:
actor_type: user
description: When enabled, the VAProfile::V3::ContactInformation will be enabled
enable_in_development: true
validate_saved_claims_with_json_schemer:
actor_type: user
description: When enabled, Saved Claims will be validated using the JSON Schemer gem rather than JSON Schema
enable_in_development: false
veteran_onboarding_beta_flow:
actor_type: user
description: Conditionally display the new veteran onboarding flow to user
Expand Down
45 changes: 36 additions & 9 deletions modules/pensions/spec/models/pensions/saved_claim_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,18 +49,45 @@
)
end

describe '#process_attachments!' do
it 'sets the attachments saved_claim_id' do
expect(Lighthouse::SubmitBenefitsIntakeClaim).not_to receive(:perform_async).with(claim.id)
claim.process_attachments!
expect(claim.persistent_attachments.size).to eq(2)
context 'using JSON Schema' do
before do
allow(Flipper).to receive(:enabled?).with(:validate_saved_claims_with_json_schemer).and_return(false)
end

describe '#process_attachments!' do
it 'sets the attachments saved_claim_id' do
expect(Lighthouse::SubmitBenefitsIntakeClaim).not_to receive(:perform_async).with(claim.id)
claim.process_attachments!
expect(claim.persistent_attachments.size).to eq(2)
end
end

describe '#destroy' do
it 'also destroys the persistent_attachments' do
claim.process_attachments!
expect { claim.destroy }.to change(PersistentAttachment, :count).by(-2)
end
end
end

describe '#destroy' do
it 'also destroys the persistent_attachments' do
claim.process_attachments!
expect { claim.destroy }.to change(PersistentAttachment, :count).by(-2)
context 'using JSON Schemer' do
before do
allow(Flipper).to receive(:enabled?).with(:validate_saved_claims_with_json_schemer).and_return(true)
end

describe '#process_attachments!' do
it 'sets the attachments saved_claim_id' do
expect(Lighthouse::SubmitBenefitsIntakeClaim).not_to receive(:perform_async).with(claim.id)
claim.process_attachments!
expect(claim.persistent_attachments.size).to eq(2)
end
end

describe '#destroy' do
it 'also destroys the persistent_attachments' do
claim.process_attachments!
expect { claim.destroy }.to change(PersistentAttachment, :count).by(-2)
end
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/factories/gids_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
versioned_school_certifying_officials: [
{
priority: 'Primary',
email: 'test@edu_sample.com'
email: 'user@school.edu'
},
{
priority: 'Secondary',
email: 'test@edu_sample.com'
email: 'user@school.edu'
}
]
}
Expand Down
6 changes: 3 additions & 3 deletions spec/factories/va10203.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
schoolCity: 'Test',
schoolState: 'TN',
schoolCountry: 'USA',
schoolEmailAddress: 'test@edu_sample.com',
schoolEmailAddress: 'user@school.edu',
schoolStudentId: '01010101',
isActiveDuty: true,
veteranAddress: {
Expand Down Expand Up @@ -58,7 +58,7 @@
schoolCity: 'Test 2',
schoolState: 'SC',
schoolCountry: 'USA',
schoolEmailAddress: 'test@edu_sample.com',
schoolEmailAddress: 'user@school.edu',
schoolStudentId: '01010101',
isActiveDuty: true,
veteranAddress: {
Expand Down Expand Up @@ -98,7 +98,7 @@
schoolCity: 'Test 2',
schoolState: 'SC',
schoolCountry: 'USA',
schoolEmailAddress: 'test@edu_sample.com',
schoolEmailAddress: 'user@school.edu',
schoolStudentId: '01010101',
isActiveDuty: true,
veteranAddress: {
Expand Down
Loading

0 comments on commit 8a998c8

Please sign in to comment.