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

Upload ODK submission user photos to S3 for easy access #1744

Merged
merged 11 commits into from
Aug 14, 2024

Conversation

Sujanadh
Copy link
Collaborator

@Sujanadh Sujanadh commented Aug 8, 2024

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

Describe this PR

This PR creates a function to upload submission images to s3

  • run background task to upload images whenever submission table is opened
  • checks if the object already exists in s3 and avoids task to run in background
  • after successfully uploading images , it will create database record in submission_photos table with fields project_id, task_id, submission_id, and s3_path

Screenshots

N/A

Alternative Approaches Considered

Did you attempt any other approaches that are not documented in code?

Review Guide

  • Make a submission with an image
  • Download submissions on FMTM
  • Check the S3 to see if the image is present

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@Sujanadh Sujanadh added enhancement New feature or request backend Related to backend code dependency:osm-fieldwork Requires updates in osm-fieldwork labels Aug 8, 2024
@Sujanadh Sujanadh self-assigned this Aug 8, 2024
@spwoodcock spwoodcock changed the title Upload images in submission to s3 Upload ODK submission user photos to S3 for easy access Aug 9, 2024
for idx, filename in enumerate(attachments):
s3_key = f"{s3_base_path}/{instance_id}/{idx+1}.jpeg"

if object_exists(s3_bucket, s3_key):
Copy link
Member

Choose a reason for hiding this comment

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

This will make a lot of calls to S3 potentially!

Not a problem with Minio, but slow if external S3.

Could we do a comparison for what the submission says should exist against the database table instead?

We would need the stat approach if we didn't gave the db table πŸ‘

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah makes sense! πŸ€”


# Insert the record into submission_photos table
sql = text("""
INSERT INTO submission_photos (
Copy link
Member

Choose a reason for hiding this comment

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

Also this could be batched for efficiency.

It's generally good to not make db queries in a for loop - instead gather the data (images uploaded) there in a dict or something, then do a batch db table insert in one go πŸ˜„

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay

-- Start a transaction
BEGIN;

CREATE TABLE submission_photos (
Copy link
Member

Choose a reason for hiding this comment

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

Check this SQL doesn't error if you run it twice πŸ‘

Maybe create table if not exists will fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay

@Sujanadh
Copy link
Collaborator Author

  • Addressed all the mentioned things

@spwoodcock
Copy link
Member

Looks good! Merging πŸ’«

@spwoodcock spwoodcock merged commit ab22dc8 into development Aug 14, 2024
5 checks passed
@spwoodcock spwoodcock deleted the feat/upload-image-to-s3 branch August 14, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code dependency:osm-fieldwork Requires updates in osm-fieldwork enhancement New feature or request frontend Related to frontend code migration Contains a DB migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants