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

Handle tiffs correctly #3050

Merged
merged 36 commits into from
Dec 4, 2020
Merged

Handle tiffs correctly #3050

merged 36 commits into from
Dec 4, 2020

Conversation

aug24
Copy link
Contributor

@aug24 aug24 commented Dec 3, 2020

What does this change?

Existing code does not handle tiffs correctly.
In an attempt to extract an optimised file for use in the UI, an error meant that the optimised file was uploaded in place of the original.

This PR completely rewrites the upload process in idiomatic futures which makes the process clearer.

Specifically, instead of nested flatmaps across futures, the process now uses a for comprehension to complete the futures. Futures which can be parallelised are created outside the for comp, those which are necessarily sequential (ie depend on prior results) are created inside.

This PR also adds tests, which unfortunately do not run in CI, pending a docker or similar environment with imagemagic/graphicsmagic.

How can success be measured?

Tiff files (and any other kind of file) should

  1. have their original file uploaded to the image bucket.
  2. If not suitable for use in the UI, have an optimised (PNG) version uploaded to /optimised in the image bucket
  3. have a thumbnail (JPG) version uploaded to the thumb bucket.

Screenshots (if applicable)

Who should look at this?

Tested?

  • locally
  • on TEST

@aug24 aug24 requested a review from akash1810 December 3, 2020 11:58
val mimeType: MimeType
val meta: Map[String, String]
}
case class StorableThumbImage(id: String, file: File, mimeType: MimeType, meta: Map[String, String] = Map.empty) extends StorableImage
Copy link
Contributor

Choose a reason for hiding this comment

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

We've refactored here to codify the three possible sorts of images in types. In this way, it's harder to make an error by mistaking one StorableImage for another.

)

case class ImageUploadOpsDependencies(
config: ImageUploadOpsCfg,
Copy link
Member

Choose a reason for hiding this comment

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

minor: So much whitespace! Can we be consistent here? There's a setting in IntelliJ to control this, if only I could remember which it is...

Copy link
Contributor

Choose a reason for hiding this comment

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

Not one for this PR, but it would be great to add Scalafmt or similar to the project.

image-loader/app/model/Uploader.scala Outdated Show resolved Hide resolved
image-loader/test/scala/model/ImageUploadTest.scala Outdated Show resolved Hide resolved
Copy link
Contributor

@sihil sihil left a comment

Choose a reason for hiding this comment

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

Some helpful comments I hope. LMK if you want to run through IRL.

image-loader/test/scala/model/ImageUploadTest.scala Outdated Show resolved Hide resolved
image-loader/test/scala/model/ImageUploadTest.scala Outdated Show resolved Hide resolved
docs/06-objects-of-interest/06.01-tiffs.md Outdated Show resolved Hide resolved
image-loader/app/model/Uploader.scala Outdated Show resolved Hide resolved
image-loader/app/model/Uploader.scala Show resolved Hide resolved
@prout-bot
Copy link

Seen on kahuna, metadata-editor (merged by @aug24 16 minutes and 3 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on collections, auth, image-loader-projection (merged by @aug24 16 minutes and 8 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on image-loader (merged by @aug24 16 minutes and 17 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on usage (merged by @aug24 16 minutes and 21 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on leases, media-api (merged by @aug24 16 minutes and 26 seconds ago) Please check your changes!

@prout-bot
Copy link

Seen on cropper (merged by @aug24 17 minutes and 6 seconds ago) Please check your changes!

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.

7 participants