new-download-fix #1057
new-download-fix #1057
Conversation
|
@derneuere the pre-commit hooks does not work for me in local while commiting code i run it manually with |
|
I think you just have to install with pip install pre-commit and install the dev dependencies and then it should complain on all commits :) |
|
Oh, and you need to do a pre-commit install once it's installed! |
|
You don't have to change the proxy.conf folder as protected_media and its subfolders get already rerouted :) |
|
hey i am a little confused here as in what are all the things that are pending for this issue. I have listed some below pls let me know if there are more
|
|
Just think feature first. We are still trying to fix the bug and handle large downloads. Right now, we should see a new long-running job, when we click on download with new api. This starts zipping the file. When the job is done, indicated with the long-running job, we should then call api/media/zip/.zip, to start downloading. There are still two issues, feature wise. The user wants the zip to get deleted after the download, because he wants to conserve space. For this, we have now two new cases:
|
|
i was thinking of making something like a cron job which schedules the delete_zip_from_local job one day after it has been created |
|
You can use this schedule function for it! Just schedule the job, when you create the zipping job https://django-q2.readthedocs.io/en/master/schedules.html Yes this all belongs in the same PR as it is one feature :) |
api/views/views.py
Outdated
| try: | ||
| job=LongRunningJob.objects.get(job_id=request.data["job_id"]) | ||
| if job.finished: | ||
| zip_file_path = os.path.join("/protected_media/all_zip_folder/", request.data["url"]) |
There was a problem hiding this comment.
use constant from the settings (MEDIA_ROOT)
|
@derneuere @sickelap after using nginx its pretty fast and i loved it !! can you pls tell me how did you set nginx up for librephotos also the test file is remaining now can you pls guide me through it not sure how to go about it also should i start working on the frontend pr ? |
|
Ideally, we should have a managed task that we could run as a cronjob. As a one of the tasks, we could do a cleanup for whatever we need to cleanup... |
so should i remove the delete function i schedule during creation ? or this cron job is a different one which admins can use ? |
|
I have added a test file which tests 2 scenarios :-
|
|
hey @derneuere can we have the label "HACKTOBERFEST-ACCEPTED" for both of my prs #1057 and #778 as hacktober will end soon and i just want 2 prs to be accepted/approved/merged it would really mean a lot and would motivate me, thank-you |
|
Added the labels to the pull requests and to the ones, that were already merged :) The nginx proxy gets configured in here: https://github.com/LibrePhotos/librephotos-docker/blob/main/proxy/nginx.conf I don't think that you have to add another location, as the zip folder is a subfolder of protected_media. You can either use cronjobs or the schedule function in django q2. Both mechanism can be used to solve the issue, that the zip gets deleted after a time period. The scheduled async task or the cronjob should get added once a zip gets created. It als should handle it gracefully, if the zip is already missing. I would prefer the schedule function as the user can see this in the django-admin dashboard. The testcases look good! If you can think of other cases, which could break this function, then add cases for it too :) |
Hey thank-you soo much for this and all the time you have given for guiding me really ...means a lot <3
thankyou
i did not understand this i am using the protected_media constant
I have used schedule once the file is created in the file
i have addressed this in
sure |
|
@derneuere @sickelap can we discuss what is exactly missing/remaining here was planning to close this soon |
|
i schedule the delete method once its created do you want to also delete it if it has been downloaded by the user ? |
|
Yes, either by triggering an endpoint or by some other mechanisms. If I download multiple albums or have multiple users, I do not want my hard drive to be filled up, if it is not necessary. |
|
One more thing. It seems that the archive will be accessible by anyone who know the link. Is this intentional? Should we have a check to validate that only an owner can download the archive? Just a thought... |
|
Hmm, true. The easiest fix is, to put the user id into the filename, when zipping and checking for it when downloading. To enhance the security a bit more, the download name should not be guessable. so something like a <uuid + user id>.zip should work fine. |
i just saw that we are storing everything(images,videos) locally right ? so for multiple users everyone will have photos on their local disk then why do we have a download option why are we not just pointing the disk location of the zip file instead of sending it to the user through frontend ? |
|
In this case locally means a self-hosted system, which may or may not be the same system as the client. You can have users, which do not have direct access to the server like for example friends, which want to download the pictures from the last party. In that case a download function is still needed. |
oh okay so if i use libre photos and want to share access with my friends my friends can download images but those images will be from my device storage ? so if any other user uploads something it will be stored in my device ? asking these because i really wanted to know more about the product |
|
It's similar to other photo management systems. The systems which hosts the docker container acts as a cloud for the other devices. Other devices can upload images to your cloud, if they have an account and can share them from your cloud. LibrePhotos acts as a multi user system, which means it can have multiple accounts. The images will then just be placed in a subfolder for each user. |
|
My opinion LP is not a file sharing platform. As I understood the feature is to download your photos to local machine, not to create an archive for others to download. I agree with @derneuere and photo sharing feature within the LP should be enough for sharing. |
|
@derneuere @sickelap i have created a delete api which we can call from front-end as soon as the download is successfull |
| return Response(status=404) | ||
|
|
||
|
|
||
| class DeleteZipView(APIView): |
There was a problem hiding this comment.
This can be part of ZipListPhotosView_V2, I think :)
There was a problem hiding this comment.
oh okay cool then will implement the delete inside the zip view only
There was a problem hiding this comment.
hey i can do this but then i need the file path so sending it through body wont be good for a delete request moreover would have to edit the urls.py for make call to a function based view but then the pattern of all class based views would not continue so i was thinking of making it as a separate view class
api/views/views.py
Outdated
| @@ -251,6 +253,7 @@ class MediaAccessView(APIView): | |||
| permission_classes = (AllowAny,) | |||
|
|
|||
| def _get_protected_media_url(self, path, fname): | |||
| print(fname) | |||
There was a problem hiding this comment.
I think you forgot to remove this :)
| lrj.save() | ||
| # scheduling a task to delete the zip file after a day | ||
| execution_time = timezone.now() + timezone.timedelta(days=1) | ||
| schedule("api.all_tasks.delete_zip_file", filename, next_run=execution_time) |
There was a problem hiding this comment.
@sickelap The first point is already implemented. Right now, the zip file will get deleted after a day. The delete endpoint is the happy path, this clean up job is the edge case, where the user never downloads the zip.
There was a problem hiding this comment.
will start with the front-end pr 🚀
There was a problem hiding this comment.
yea, I have missed this one. I think this should be enough for automated cleanup.
|
I think we thought of most edge cases and looks pretty good. Found some smaller things and it would be great if you could remove the old view. Next step is to create the frontend pr, so that we can test it end to end :) |
|
@sickelap @derneuere
|
|
I would recommend not over-complicate things and don't think about redux. Focus on the more simple download option first. When that's done you could think of other use case scenarios. |
|
and be sure that all checks are green ;) |
okay got it |
|
Also, just use the notifications (https://v5.mantine.dev/core/notification/) to communicate, that zipping started or that the download is now starting. No need to make it work as a new component on the side. |
AnkurPrabhu
force-pushed
the
new-download-fix
branch
from
4dcdba8
to
75e3f70
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@sickelap commit hooks were failing because of some other files have fixed it mostly were because of unused imports |
|
Great work @AnkurPrabhu !!! 🎉 |
|
@derneuere hey just wanted to know is it working as expected ? |
|
@AnkurPrabhu I think so :) It will be part of the next release! |
Issue Link #375 @derneuere
New Pr for #1055
what has been done :
have created a job type in long_running_job for download
created two method one for making the zip file i.e zip_photos_task and second which call the zip function i.e create_download_job
created a get api for the same view class which checks the status of the running job
Pending:
checking file system storage before starting the zipping process
test file