-
Notifications
You must be signed in to change notification settings - Fork 15
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
Uppy logo upload vue widget #2070
Open
luistoptal
wants to merge
39
commits into
gigascience:develop
Choose a base branch
from
luistoptal:new-feature/376-upload-update-project-images
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Uppy logo upload vue widget #2070
luistoptal
wants to merge
39
commits into
gigascience:develop
from
luistoptal:new-feature/376-upload-update-project-images
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
#376 has some interesting comments that should serve to improve this PR I will apply them |
@rija as for this comment: #376 (comment) 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request for issue: #376
NOTE: A follow up for this PR implements use of the /tmp folder to store temporary images #2092
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 user replaces 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 exit 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" --> this flow is changed in follow up PR #2092
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