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

[SIMPLE-FORMS] fix: updates to S3 presigned URL and directory naming logic #19454

Merged
merged 6 commits into from
Nov 18, 2024

Conversation

pennja
Copy link
Contributor

@pennja pennja commented Nov 13, 2024

Summary

  • This work is behind a feature toggle (flipper): YES
  • This pull request updates the S3 client to generate presigned URLs without creating new files or directories, streamlining the file handling and S3 upload logic within the simple_forms_api service. It also modifies the date-based file and directory naming conventions to improve efficiency.
  • To reproduce the previous behavior, attempt to generate presigned URLs using the existing S3 client, which would create temporary files and directories.
  • The solution involves refactoring the S3 client to implement a new method, retrieve_presigned_url, which accesses existing archive data directly. This eliminates the need for temporary file generation and consolidates directory naming logic. This approach is chosen for its efficiency and clarity in handling S3 paths.
  • I work for the Veteran Facing Forms team, which owns the maintenance of this component.
  • The success criteria for the feature toggle include ensuring that the S3 client retrieves presigned URLs correctly without generating files, and that the new naming logic functions as intended.

Related issue(s)

Testing done

  • New code is covered by unit tests
  • Prior to this change, generating a presigned URL would create temporary files and directories, which was inefficient.
  • To verify the changes:
    1. Test the S3 client to ensure it retrieves presigned URLs without generating any files.
    2. Check that the date-based naming logic works correctly by providing different submission creation dates.
    3. Run the RSpec tests in s3_client_spec.rb and submission_archive_spec.rb to confirm that all new and modified methods function as expected.

What areas of the site does it impact?

  • This update impacts the S3 file handling and upload logic within the simple_forms_api service, specifically in how presigned URLs are generated and how files and directories are named.

Acceptance criteria

  • I fixed unit tests and integration tests for each feature.
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution.
  • Documentation has been updated.
  • No sensitive information is captured in logging, hardcoded, or specs.
  • Feature has a monitor built into Datadog.
  • I logged into a local build and verified all authenticated routes work as expected.

Requested Feedback

I would appreciate feedback on the new naming conventions and whether the changes to the S3 client meet the expected efficiency improvements.

@va-vfs-bot va-vfs-bot temporarily deployed to jap/simple-forms/fix-dated-directory/main/main November 13, 2024 22:49 Inactive
@pennja pennja changed the title WIP [] fix: updates to S3 presigned URL and directory naming logic Nov 14, 2024
@pennja pennja changed the title [] fix: updates to S3 presigned URL and directory naming logic [SIMPLE-FORMS] fix: updates to S3 presigned URL and directory naming logic Nov 14, 2024
@pennja pennja marked this pull request as ready for review November 14, 2024 17:56
@pennja pennja requested review from a team as code owners November 14, 2024 17:56
Copy link

Backend-review-group approval confirmed.

@pennja pennja merged commit 3a4e8d7 into master Nov 18, 2024
26 checks passed
@pennja pennja deleted the jap/simple-forms/fix-dated-directory branch November 18, 2024 15:54
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.

4 participants