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

add attachment_id to metadata #16726

Merged
merged 17 commits into from
May 16, 2024
Merged

add attachment_id to metadata #16726

merged 17 commits into from
May 16, 2024

Conversation

cloudmagic80
Copy link
Contributor

Summary

This PR adds attachment_id to the S3 PutObject metadata for DOCMP PEGA S3 Bucket. Each attachment will now have a corresponding "attachment_id" in their metadata. (see attached). If you visit the s3 admin console, you will see it as the supporting documents = "x-amz-meta-attachment-ids"

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:
    • 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

Screenshot 2024-05-13 at 1 25 31 PM Screenshot 2024-05-13 at 1 25 38 PM

What areas of the site does it impact?

It will only impact the DOCMP PEGA owned S3 buckets called
Box-stage
Box-prod
Box-dev

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 or Grafana (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

@cloudmagic80 cloudmagic80 marked this pull request as ready for review May 14, 2024 19:43
@cloudmagic80 cloudmagic80 requested review from a team as code owners May 14, 2024 19:43
@cloudmagic80 cloudmagic80 enabled auto-merge (squash) May 14, 2024 19:43
@cloudmagic80 cloudmagic80 marked this pull request as draft May 14, 2024 20:27
auto-merge was automatically disabled May 14, 2024 20:27

Pull request was converted to draft

@cloudmagic80 cloudmagic80 marked this pull request as ready for review May 14, 2024 21:07
@va-vfs-bot va-vfs-bot temporarily deployed to ivc_attachment_ids/main/main May 14, 2024 21:39 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to ivc_attachment_ids/main/main May 15, 2024 01:04 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to ivc_attachment_ids/main/main May 15, 2024 13:04 Inactive
@cloudmagic80 cloudmagic80 enabled auto-merge (squash) May 16, 2024 05:04
@cloudmagic80 cloudmagic80 merged commit 307f9b6 into master May 16, 2024
19 checks passed
@cloudmagic80 cloudmagic80 deleted the ivc_attachment_ids branch May 16, 2024 13:10
filler = IvcChampva::PdfFiller.new(form_number: form_id, form:)
attachment_ids = [form_id]

if form_id == 'vha_10_10d'
Copy link
Contributor

@balexandr balexandr May 16, 2024

Choose a reason for hiding this comment

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

This is only going to work for 10-10D? Shouldn't we write this to work for any form? If it's form specific this logic should go in the model it's specific to i.e. models/ivc_champva/vha_10_10d.rb and not in the controller. If it's logic that can be used across all of them it can go in the service module Attachments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If your form wants to use this logic, you will have to modify the data structure for your form

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to change the data structure of your form to : "applicants"=> and "applicant_supporting_documents"=> if you want to apply this method.

end

def get_form_id
form_number = params[:form_number]
raise 'missing form_number in params' unless form_number

FORM_NUMBER_MAP[form_number]
form_number_without_colon = form_number.sub(':', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we have to add this? The form map key/value above doesn't have colons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 1010D it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? The map is '10-10D' => 'vha_10_10d', when does the colon come into play?

@form_id = form_id
@metadata = metadata || {}
@file_paths = Array(file_paths)
@attachment_ids = attachment_ids
Copy link
Contributor

@balexandr balexandr May 16, 2024

Choose a reason for hiding this comment

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

Why couldn't we include this in the metadata itself? Why does it have to be a new argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some forms don't require attachments

metadata&.transform_values! { |value| value || '' }
metadata['attachment_ids'] = attachment_ids.empty? ? '' : attachment_ids.join(',')
Copy link
Contributor

@balexandr balexandr May 16, 2024

Choose a reason for hiding this comment

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

This could happen much sooner, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Not all forms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants