Upload (and update) of Project images

only1chunts commented 4 years ago

Is your feature request related to a problem? Please describe. when a dataset needs to be linked to an existing project that we dont already have in the GigaDB list we need to add it, the admin pages have the ability to add the name and URL, but not the logo of the project.

Describe the solution you'd like when creating or updating a project link in the admin pages (http://gigadb.org/adminProject/admin) there should be the option to upload an image for it.

Additional context Prior to Jesse leaving the images were just sent to him and he added them to the relevant place in the webserver manually.

User story

As a admin user I want to be able to add and update external project links So that I can ensure the project links presented on the dataset pages are upto date with links and logos

Acceptance criteria

Given a project exists in GigaDB When I visit the admin page for that project, e.g. http://gigadb.org/adminProject/update/id/38 Then I am able to upload a new logo image and/or update the URL to which the project links

Given a project does not exist already in GigaDB When I click create new project link Then I can insert the new project details that I wish to display, including the project name, logo and link URL

Given I have a project image that fits the required size When I upload the image in the form then the form is submitted successfully

Given I have a project image that does not fits the required size When I upload the image in the form then the form doesn't submit and show an error message about incorrect image size

Additional Info

the project form should allow a curator to an upload an image and the system will make it a available on the cloud and the on the CDN and on the website.
There is standard size for logos (that look good on the web page). Mention on the form that the image should be of that size, and validate the upload image is of that size
The height matters the most (use Genome 10K logo as template)

Product Backlog Item Ready Checklist

[ ] Business value is clearly articulated
[ ] Item is understood enough by the IT team so it can make an informed decision as to whether it can complete this item
[ ] Dependencies are identified and no external dependencies would block this item from being completed
[ ] At the time of the scheduled sprint, the IT team has the appropriate composition to complete this item
[ ] This item is estimated and small enough to comfortably be completed in one sprint
[ ] Acceptance criteria are clear and testable
[ ] Performance criteria, if any, are defined and testable
[ ] The Scrum team understands how to demonstrate this item at the sprint review

Product Backlog Item Done Checklist

[ ] Item(s) in increment pass all Acceptance Criteria
[ ] Code is refactored to best practices and coding standards
[ ] Documentation is updated as needed
[ ] Data security has not been compromised (with particular reference to the personal information we hold in GigaDB)
[ ] No deviation from the team technology stack and software architecture has been introduced
[ ] The product is in a releasable state (i.e. the increment has not broken anything)

rija commented 1 month ago

@luistoptal maybe we can make a Vue.js app that reuse the Uppy component here for image upload, which means we may even be able to do image validation and pre-processing on the client-side

luistoptal commented 1 month ago

@rija it would be nice to use uppy so admins have a diverse set of options to upload files. I assume we need less features than in the existing file uploader since we're just uploading what is expected to be small sized logos, e.g. tus for chunking file uploads, checksums,

so to summarize:

tasks:

add a vue widget in the http://gigadb.gigasciencejournal.com/adminProject/create (and update) page for logo upload
validate logo size and width clientside
remove "image location" field
on submit, send image to the backend. I assume it is added to a S3 bucket and then the url added to the DB, there should exist controller logic that handles that
add tests for the core use case, and update any affected acceptance tests

assumptions:

advanced uppy features (tus, checksum) are not needed, only the ability to upload an image from multiple sources
image validation will take this image height and width as a template, to determine logo validation: https://assets.gigadb-cdn.net/live/images/projects/genome_10k/G10Klogo.jpg. I'll test mock logos to allow some flexibility while ensuring the logos look good on the site

Does this cover everything?

only1chunts commented 1 month ago

many of the existing project thumbnail images are wider than the G10K logo (e.g.), so being able to allow wider logos would be useful, its really the hieght that should be fixed with the width being variable to maintain the aspect ratio.

rija commented 1 month ago

Hi @luistoptal,

Your breakdown of the tasks looks very reasonable. Maybe just accommodate for @only1chunts' comment regarding constraining the image height only.

The image will be stored in a new S3 bucket, and the path should include a UUID segment to avoid image files of the same name to clash.

Something else to consider, is that in the future, we may want to re-use the component to upload the dataset thumbnail image from the dataset form.

In both cases, Uppy's advanced features used for File Upload Wizard won't be needed here. That includes TUS-based resumability, checksum calculation, multiple file upload.

Like for the dataset thumbnail image, we will want to serve the project images from our Cloudflare CDN (so we will need to connect the S3 bucket to Cloudflare too) to save money.

Finally, we should be able to replace an existing project image by uploading a new image, which will effectively delete the previous image ; and when we click the delete icon on https://gigadb.org/adminProject/admin, the image should be deleted too, but that could be another story maybe?

luistoptal commented 1 month ago

Hi @rija I have been looking into this and I realize that to initiate this likely there's some devops work previous, i.e. setup a new js app and the corresponding docker environment, separate from the fuw app

I think it would be a good idea to have a screenshare about that to understand how to set things up, or maybe any other suggestion to get started

I noticed I can just reuse the infrastructure for the fuw app and add in new components for this separate feature, but it feels like a hack and probably a new Vue instance should be spinned for this to keep both unrelated features separate

rija commented 1 month ago

Hi @luistoptal, sure, you can drop in for our VOH meetings, or alternatively email me to let me know your availability for a call

luistoptal commented 1 month ago

@rija I can join today's VOH

luistoptal commented 1 month ago

@rija I managed to setup a basic vite + vue dockerized app. I think that to upload files I need an endpoint that uploads the logo to a S3 bucket and (probably) returns the image url

do we have an endpoint for this?

rija commented 2 weeks ago

Hi @luistoptal

here's the class we already use for saving images to S3: protected/models/Image.php (look at the write() function)

An example call can be found at protected/controllers/AdminDatasetController.php in the block starting L106.

We want to use the same bucket used there.

The issue is the write() function is hard-coded to write dataset images into DOI keyed directories and is in the dataset images model class,

while the project images are not dataset specific, and they needs to be written to /[dev|staging|live]/images/projects

I suggest you duplicate protected/models/Image.php's write() into writeLogo() in protected/models/Project.php and adjust the path.

protected/models/Project.php is the model class for project logos.

Then in your controller, you can call $model->writeLogo(...) in a similar pattern to L106 of protected/controllers/AdminDatasetController.php

The new writeLogo() function doesn't need all the parameters that was used in write() as the images are all in the same directory, so the signature will be more like:

public function writeLogo(Filesystem $targetStorage, CUploadedFile $uploadedFile): bool

luistoptal commented 1 week ago

@rija I've spent some time with this, and I notice that uppy is specially designed to enable advanced file upload features such as: batch upload multiple files, upload large files, fetch files from third parties, etc

The upload step of uppy involves first uploading a file to a server (either third party or own servers) through an endpoint specific for that, for example /adminProject/uploadLogo

Our use case is submitting one single small file along with a form in a post request, and essentially the only requirement is client side validation. I realized we don't really seem to need any of the uppy features, plus the native file input can handle this well enough

Implementing uppy for this seems relatively complex because I find it runs against the usual uppy flow (file upload), whereas the flow for native html input is very straightforward

So actually I don't think we need a vue widget at all, plain html and some lines of jquery for client side validation should be enough

Is there something I am missing?

rija commented 1 week ago

Hi @luistoptal,

Is there something I am missing?

your reasoning is quite reasonable, and I'd go with it in this use case. We still need to store the image on S3, alongside all the other image assets. In my mind, Uppy would have enabled the upload to S3 directly in addition to potential UX bells-and-whistles (e.g: cropping), but there is no requirement for those at this stage.

So, If using html file upload, the controller code will need to copy the uploaded file blob to S3 which is what we are already doing for the dataset images, and the software design I have described in my previous approach is still (even more so now) valid.

luistoptal commented 1 week ago

@rija indeed with Uppy the natural flow is to upload image (separate from the form submission with the rest of the data). If eventually we want to implement the flow:

allow to crop / edit image before upload
Upload image first (separate from form submission)

If eventually we want to adopt that flow then I actually rectify my previous comment and I think it would be a good idea to use Uppy now

what I was finding challenging and tricky with Uppy was to submit the image alongside the other data to the actionCreate method

so I think the plan is now:

create a new method in the AdminProjectController called actionUploadLogo
implement vite + vue + uppy (already setup)
enable the plugin that allows the user to edit (crop, resize) and image before upload. I think this makes a lot of sense if we impose dimension restrictions and I see how that could be very useful to the users
when image is uploaded, a POST request to actionUploadLogo method call uploads the image to a S3 bucket and returns the logo URL (will have to mock this since I don't have access to S3)
add logo URL as a hidden field in the form, and allow the user to submit the rest of the fields as usual, this will be handled by the actionCreate method which will remain unchanged

rija commented 1 week ago

Hi @luistoptal,

The steps looks right. and they assume, that Uppy is going to be used with https://uppy.io/docs/xhr-upload/

For the copy to S3, You should use the existing Flysystem already used for the dataset images. then faking S3 on your local deployment is just a configuration change: if you look at this file ops/configuration/yii-conf/web.dev.CI.php.dist (which up.sh transforms into protected/config/yii2/web.php)

you can implement a fake S3 by using the content of the fs key for the cloudstore key in the config protected/config/yii2/web.php

Regarding the URL, it doesn't need to be returned as it is predictable and therefore can be calculated. You can see an example of that done for dataset image in protected/models/Image.php in the block starting line 114. We will need to do something similar here (the value of BUCKET is actually the same, so we can directly reference Image::BUCKET constant in the Project controller)

luistoptal commented 1 week ago

Hi @rija

Regarding the URL, it doesn't need to be returned as it is predictable

for the create page, since we're uploading an image before callng the create endpoint that instanciates a project, the project id does not yet exist, so i initalize a random uuid and return it in the upload logo endpoint response

luistoptal commented 1 week ago

@rija I see one potential pitfall where the user uploads a logo and then simply refreshes or leaves the page, in which case the new logo is left dangling forever and it's not possible to know it should be deleted. So there needs to be an intermediate step

logo uploaded added to temp folder in storage
project is actually created from form submission
logo from storage is copied to deterministic folder path

then from time to time a cron job can delete from the temp folder any file older than one day, for example

rija commented 2 days ago

@rija I see one potential pitfall where the user uploads a logo and then simply refreshes or leaves the page, in which case the new logo is left dangling forever and it's not possible to know it should be deleted. So there needs to be an intermediate step

logo uploaded added to temp folder in storage

project is actually created from form submission

logo from storage is copied to deterministic folder path

then from time to time a cron job can delete from the temp folder any file older than one day, for example

Hi @luistoptal,

make sense, but the last step feels unnecessary, we could simply copy the file from the temp folder to S3 directly? so that the only location that needs housekeeping is the temp folder (and for that we could literally use the /tmp/ folder on linux and leverage system housekeeping mechanism maybe)

luistoptal commented 1 day ago

@rija I'm not too familiar with that. How would it go? storing the temporary files not in a S3 bucket but in /var/tmp in the first step? Which if I understand correctly gets automatically cleaned up without the need to set it up. And then copy the file from /var/tmp to the S3 bucket?

i think it would be possible, the frontend just needs to receive the random uuid generated in the backend to know where to locate the temp file to copy

It is also possible to generate the uuid in the client and send it to the back as metadata (then the backend doesn't need to send it back), but it don't think it matters

anyway I completed the implementation (as I listed in my last comment) in the PR

gigascience / gigadb-website