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

Support image-based R&C for Paper LPAs #200

Merged
merged 2 commits into from
May 14, 2024

Conversation

gregtyler
Copy link
Contributor

  • Alter schema to change fields available based on form channel
  • Add FileUpload and File struct types
    • FileUpload is used in submissions, File is used to pass back
  • Add S3Client.UploadFile to copy base64-encoded files into S3
  • On submission of Paper LPAs, store FileUpload objects in S3 and return a reference to them in File
  • Update the fixtures UI to support uploads

For CTC-146 #minor

@gregtyler gregtyler requested a review from a team as a code owner May 10, 2024 15:11
internal/objectstore/client.go Fixed Show fixed Hide fixed
internal/objectstore/client.go Fixed Show fixed Hide fixed
@gregtyler gregtyler force-pushed the CTC-146-support-rc-images-for-paper-lpas branch from a375a54 to 82daaa9 Compare May 10, 2024 15:26
- Alter schema to change fields available based on form channel
- Add `FileUpload` and `File` struct types
  - `FileUpload` is used in submissions, `File` is used to pass back
- Add `S3Client.UploadFile` to copy base64-encoded files into S3
- On submission of Paper LPAs, store `FileUpload` objects in S3 and return a reference to them in `File`
- Update the fixtures UI to support uploads
- Fixed some misplaced if/then/else statements in the JSONSchema

For CTC-146 #minor
@gregtyler gregtyler closed this May 13, 2024
@gregtyler gregtyler force-pushed the CTC-146-support-rc-images-for-paper-lpas branch from 82daaa9 to eab1525 Compare May 13, 2024 13:03
@gregtyler gregtyler reopened this May 13, 2024
Copy link
Contributor

@acsauk acsauk left a comment

Choose a reason for hiding this comment

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

This will be cool to see working 🙌

A couple of Qs

}

hash := sha256.New()
hash.Write(imgData)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be handling the (potential) error returned here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I somehow missed that that could return an error. Handled now.

if data.Channel == shared.ChannelPaper && len(input.RestrictionsAndConditionsImages) > 0 {
data.RestrictionsAndConditionsImages = make([]shared.File, len(input.RestrictionsAndConditionsImages))
for i, image := range input.RestrictionsAndConditionsImages {
path := fmt.Sprintf("%s/scans/rc_%d_%s", data.Uid, i, image.Filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ever likely to get identical filenames?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the inclusion of i should prevent that, since {uid}/scans/rc_{i} should itself be a unique identifier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, missed that! All good then 👍

Copy link
Contributor

@acsauk acsauk left a comment

Choose a reason for hiding this comment

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

:shipit:

@gregtyler gregtyler merged commit 61a0bcc into main May 14, 2024
23 checks passed
@gregtyler gregtyler deleted the CTC-146-support-rc-images-for-paper-lpas branch May 14, 2024 08:22
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.

2 participants