Open luistoptal opened 1 week ago
https://github.com/gigascience/gigadb-website/issues/376 has some interesting comments that should serve to improve this PR
I will apply them
@rija as for this comment: https://github.com/gigascience/gigadb-website/issues/376#issuecomment-2456826542
I think it's better if I implement that in a separate PR, because that is a set of changes I'm not entire sure how it should work, and I want to have it reviewed separately rather than mix with the many changes this PR already has
Pull request for issue: #376
This is a pull request for the following functionalities:
How to test?
Local testing:
SETUP
protected/config/yii2/web.php
to use the local file system (this file is reset every time theup.sh
script is run)protected/models/Project.php
, modify the following:NOTE: the above steps need to be done to mock the S3 bucket by the local filesystem. Should be done when running the app locally. I am open to suggestions to improve this flow
NOTE: unless the user is actively developing the vue widget, the build bundle should be used. In file
protected/views/adminProject/_form.php
I added a $isDev flag that toggles this behavior. If a dev wants to work locally on the vue app, they shoud set it to true. I am open to suggestions to improve this flow.CREATE
files/dev/images/projects/temp/
folder, with a nested uuid folder which contains the image. You should also see the image showing up in the UIfiles/dev/images/projects/temp/
folder should be now empty, and there should be a new folder underfiles/dev/images/projects/
, looking like a uuid, with the logo within. This uuid is deterministically derived from the newly created project idEDIT
NOTE: as it is expected the logos are quite small, the image is displayed as is, rather than a thumbnail
NOTE: if you try to save the edits, you will see an error informing that duplicate name or url are not valid, which makes it impossible to just edit an image (without changing name and url). This is an issue that existed in the preexisting implementation, to reduce the scope of this issue, I suggest to create a new one for this (or fix in a separate PR, for the same issue). For testing purposes changing both fields works
files/dev/images/projects
should contain, within the same uuid folder, the new fileDELETE
How have functionalities been implemented?
ops/deployment/docker-compose.yml
, servicesvite-project-image-location-prod
and uses a setup similar to the one for the fuw service (js
service in the docker compose)protected/views/adminProject/_form.php
, in the element with idvue-client_project-image-logo
http://localhost:5173
and the main.ts is loaded directly along with vite client(cd vite && npm run build)
will create the bundles injs/vite-logo-upload
)More instructions about the build flow and testing scripts in
vite/README.md
Upload flow
actionUploadTempLogo
method, that saves the file to a temp folderactionCreate
method: A) first, project is created to get a project id, B) then, logo image is stored in a deterministic path (defined by project id), C) finally, project is updated with image url. Additionally, temp url is deletedIf used relaces the logo image for a project, the old image is deleted
NOTE: if user starts the process of uploading a logo to the point where a temp image is created, and they exist the process (reload page, abandon page, etc), the uploaded image will be left dangling in the temp folder. Generally speaking, an image will exist in the temp folder for a brief time, between uploading image with uppy dashboard and clicking on "save"
NOTE: in principle, when editing a project, since the project id is deteremined, it would be possible to directly store the image in the permanent folder rather than the temp folder when uploaded via the uppy dashboard. It is also not necessary to do that while adding more code, so I kept the same flow for both cases
Any issues with implementation?
Any changes to automated tests?
Added e2e tests for the logo upload flow in
playwright/tests/admin-project-logo-upload.spec.js
testing instructions added to
vite/README.md
NOTE: when running playwright tests from docker, this test fails. I think we can leave it for another ticket to fix that, if necessary
NOTE: I decided to implement playwright tests rather than adding gherkin acceptance tests because it is much easier and straightforward for me to do the former