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

[CST] Use json_schemer in SavedClaim #19684

Merged
merged 44 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
24a8a06
Use json schemer in savedclaim
iandonovan Dec 3, 2024
45425b8
Adds actual feature flipper, fixes already-existing tests
iandonovan Dec 3, 2024
ef3176e
Fixes more existing tests
iandonovan Dec 3, 2024
3fbc7aa
Merge
iandonovan Dec 4, 2024
351f5e2
Fixes rubocop
iandonovan Dec 4, 2024
89f14d5
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 4, 2024
51ff351
Play more nicely with error array
iandonovan Dec 4, 2024
0c72f08
Merge branch '98065/replace-json-schema' of https://github.com/depart…
iandonovan Dec 4, 2024
38a91c8
More elegant error handling
iandonovan Dec 4, 2024
d6a9c96
Rubocop
iandonovan Dec 4, 2024
97f12cd
Error handling redux redux
iandonovan Dec 4, 2024
b27d1f6
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 9, 2024
1a2dc54
Stub, rather than actually disable, the feature flipper
iandonovan Dec 9, 2024
90d6657
Json schemer can detect fake emails
iandonovan Dec 9, 2024
4722fb4
Fixes some more specs. Getting there.
iandonovan Dec 9, 2024
9940f10
rubocop
iandonovan Dec 9, 2024
01b3481
Fixing some more specs, adding some stubs, learning
iandonovan Dec 10, 2024
f7ac1d9
See if tests pass with old flipper syntax
iandonovan Dec 10, 2024
990a401
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 10, 2024
78ad63a
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 12, 2024
5eea6cb
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 12, 2024
c8bbf0f
Merge branch '98065/replace-json-schema' of https://github.com/depart…
iandonovan Dec 12, 2024
b5ae484
Fixes more specs
iandonovan Dec 12, 2024
3993a33
More passing specs. This might be all of them?
iandonovan Dec 12, 2024
737e23e
Rubocop
iandonovan Dec 12, 2024
d1c7fb7
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 12, 2024
d004026
Adds some affirmative tests with the JSON Schemer flipper enabled
iandonovan Dec 12, 2024
d807d3d
Merge branch '98065/replace-json-schema' of https://github.com/depart…
iandonovan Dec 12, 2024
09aec2a
Rubocop
iandonovan Dec 12, 2024
60b85fa
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 12, 2024
b35d588
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 13, 2024
fae042d
Adds comments to json validation methods for clarity on the gem switch
iandonovan Dec 13, 2024
f27259b
Merge branch '98065/replace-json-schema' of https://github.com/depart…
iandonovan Dec 13, 2024
7472d43
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 13, 2024
4898542
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 13, 2024
845c03c
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 16, 2024
3e7eda7
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 16, 2024
7315ce7
Finishes adding affirmative (flipper on) tests. Uses true/false each …
iandonovan Dec 16, 2024
b2fc708
Merge branch '98065/replace-json-schema' of https://github.com/depart…
iandonovan Dec 16, 2024
085c581
Merge branch 'master' into 98065/replace-json-schema
iandonovan Dec 17, 2024
891a975
Standardize error object to use symbols for keys
iandonovan Dec 17, 2024
9414188
Rubocop
iandonovan Dec 17, 2024
f3a7443
Fixes last spec, hopefully
iandonovan Dec 17, 2024
619a9dc
Rubocop
iandonovan Dec 17, 2024
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
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)
mjknight50 marked this conversation as resolved.
Show resolved Hide resolved
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)
mjknight50 marked this conversation as resolved.
Show resolved Hide resolved
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)
mjknight50 marked this conversation as resolved.
Show resolved Hide resolved
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)
mjknight50 marked this conversation as resolved.
Show resolved Hide resolved
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
Loading