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] - Update failed document upload logic #20808

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

pmclaren19
Copy link
Contributor

@pmclaren19 pmclaren19 commented Feb 14, 2025

Summary

Found that the Type 2 errors for upload docs were not populating the failed_date in the template_metadata so the failure email job was not working for those.

  • Updated the polling job so that we set this field.

  • Updated code to set acknowledgment_date for evss and lighthouse type 1 and 2 errors

  • Updated code to set error_message for evss and lighthouse type 1 and 2 errors

  • Updated code to set failed_date for evss and lighthouse type 1 and 2 errors

  • Updated code to set template_metadata -> personalisation -> date_failed for evss and lighthouse type 1 and 2 errors

  • Updated app/sidekiq/evss/document_upload.rb so that failed_date, acknowledgement_date and error_message are set when an evss document upload fails.

  • Updated app/sidekiq/lighthouse/evidence_submissions/document_upload.rb so that failed_date, acknowledgement_date and error_message are set when an evss document upload fails.

  • Updated lib/lighthouse/benefits_documents/upload_status_updater.rb so that template_metadata -> personalisation -> date_failed is set. Also imported helpers so that we can use the helper function.

  • Updated spec/factories/lighthouse/benefits_documents/evidence_submission.rb so that the factory represented each document upload error type. Also changed bd_evidence_submission_pending to have a date_submitted that was accurate to what we set in the code.

  • Updated the tests in spec/lib/lighthouse/benefits_documents/upload_status_updater_spec.rb

  • Updated the tests in spec/sidekiq/evss/document_upload_spec.rb

  • Updated the tests in spec/sidekiq/lighthouse/evidence_submissions/document_upload_spec.rb

  • Added more tests to spec/sidekiq/lighthouse/evidence_submissions/failure_notification_email_job_spec.rb for the different error types

  • Update the tests in spec/sidekiq/lighthouse/evidence_submissions/evidence_submission_document_upload_polling_job_spec.rb

Related issue(s)

Testing done

  • New code is covered by unit tests

How to Test LH document upload failures for type 1 work correctly and are picked up by cron job

  1. Make sure benefits_documents_use_lighthouse is enabled
  2. Make sure cst_synchronous_evidence_uploads is disabled
  3. Make sure cst_send_evidence_failure_emails is enabled
  4. Make sure cst_send_evidence_submission_failure_emails is enabled
  5. Locally make sure your settings.local.yml has a vanotify section
  6. Update lib/lighthouse/benefits_claims/service.rb with an icn of a user in staging EX: @icn = '1012830712V627751' # icn for user 19
  7. Update document_upload so that sidekiq retries are set to 0 for testing locally
  8. Update app/sidekiq/lighthouse/evidence_submissions/failure_notification_email_job.rb so that in notify_client.send_email() we replace recipient_identifier: { id_value: icn, id_type: 'ICN' } with email_address: 'YOUR_EMAIL',
  9. Make sure lib/lighthouse/benefits_documents/configuration.rb is NOT updated to have a participantId --> this is how we will generate an error
  10. Run vets-api and vets-website locally and upload a file
  11. Add a document via CST and should see 1 record was added/updated for evidence_submission table with a FAILED status, acknowledgment_date, failed_date, error_message and template_metadata -> personalisation -> date_failed
  12. Go to rails console and run Lighthouse::EvidenceSubmissions::FailureNotificationEmailJob.perform_async
  13. Youll see you record in the evidence_submission table now it has a va_notify_id and va_notify_date and should receive an email

How to Test LH document upload failures for type 2 work correctly and are picked up by cron job

Screenshots

Note: Optional

What areas of the site does it impact?

CST

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?

Copy link

github-actions bot commented Feb 14, 2025

1 Warning
⚠️ This PR changes 404 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding
200. Expect some delays getting reviews.

File Summary

Files

  • app/sidekiq/evss/document_upload.rb (+3/-0)

  • app/sidekiq/lighthouse/evidence_submissions/document_upload.rb (+3/-0)

  • app/sidekiq/lighthouse/evidence_submissions/failure_notification_email_job.rb (+1/-7)

  • lib/lighthouse/benefits_documents/upload_status_updater.rb (+10/-1)

  • lib/lighthouse/benefits_documents/utilities/helpers.rb (+6/-0)

  • spec/factories/lighthouse/benefits_documents/evidence_submission.rb (+47/-5)

  • spec/lib/lighthouse/benefits_documents/upload_status_updater_spec.rb (+19/-1)

  • spec/sidekiq/evss/document_upload_spec.rb (+9/-1)

  • spec/sidekiq/lighthouse/evidence_submissions/document_upload_spec.rb (+13/-1)

  • spec/sidekiq/lighthouse/evidence_submissions/evidence_submission_document_upload_polling_job_spec.rb (+24/-27)

  • spec/sidekiq/lighthouse/evidence_submissions/failure_notification_email_job_spec.rb (+183/-43)

    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

samcoforma
samcoforma previously approved these changes Feb 14, 2025
Copy link
Contributor

@samcoforma samcoforma left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -55,16 +54,6 @@ def send_failed_evidence_submissions
nil
end

# This will be used to send an upload failure email
# We created a new personalisation with the obfuscated_file_name so the filename is hidden in the email
def create_personalisation_from_upload(upload)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a helper for this

allow(Rails.logger).to receive(:error)
allow(StatsD).to receive(:increment)
context 'when the FAILED record is for EVSS and doesnt have a va_notify_date' do
context 'when an error occurs' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a way to wrap these tests in a shared_example? not quite sure what that'd look like but i think its make things a lot cleaner.

it 'successfully enqueues a failure notification mailer to send to the veteran' do
expect(EvidenceSubmission.count).to eq(1)
expect(EvidenceSubmission.va_notify_email_not_queued.length).to eq(1)
expect(vanotify_service).to receive(:send_email).with(send_email_params)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would like to add a .and_return() here but I cant with how we stubbed out the vanotify_service not sure how we can make this better. Let me know your thoughts.

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.

3 participants