bookmark category download implementation #1800

quimmrc · 2024-12-02T11:20:02Z

Issue(s)
#1290

Description
Implementation of new view for bookmarkcategory downloads, new method to get the sound attributions for a bookmark category and download_sounds adaptation to make it viable for both packs and bookmark categories

Errors: download_sounds redirects to a url with the text content of the download instead of downloading the desired zip file

ffont

I left a couple of comments to make code a bit cleaner and better documented, but other than that the PR looks great. The "error" you mention (that clicking the download link redirects to a file list), is also happening with pack downloads right? This is because to turn this into a zip file we use some specific server tricks which are not available in the development server. If the behaviour is the same as for the pack downloads, then it should be working when deployed.

ffont · 2024-12-02T11:29:44Z

bookmarks/views.py

+def download_bookmark_category(request, category_id):
+    category = get_object_or_404(BookmarkCategory, id=category_id)
+    licenses_url = (reverse('bookmark-category-licenses', args=[category_id]))
+    #missing: cache checking done in packdownload


I would suggest to improve this comment, something like: # NOTE: unlike pack downloads, here we are not doing any cache check to...

Make it clear, so if we look at it in the future we understand it easily.

ffont · 2024-12-02T11:36:18Z

utils/downloads.py

 from utils.nginxsendfile import prepare_sendfile_arguments_for_sound_download


-def download_sounds(licenses_url, pack):
+def download_sounds(licenses_url, batch):


I kind of don't like the name batch. Currently it should be some type of object that implements a method called get_attribution which should return the same thing that is returned in the licenses_url argument. But this is not well explained. It wasn't well explained before either. But a good practice we try to follow is to take the opportunity to improve code documentation when we refactor things, so maybe it could be documented now.

I might consider having 3 arguments licneses_file_url, licenses_file_content and sound_list, and then add proper documentation for these arguments. Copilot should probably help you. We follow google style for python documentation, see for example this function as an example.

Sending the sound_list into the function was an option I had also considered and implemented previously, I will look back into it as well as the licenses data. I will work on proper documentation for the further push. Thanks.

ffont · 2024-12-02T11:40:27Z

templates/bookmarks/bookmark_category_attribution.txt

@@ -0,0 +1,21 @@
+{% load absurl %}Bookmark Category downloaded from Freesound


I was thinking if it would make sense to reuse some template/method code between packs and bookmark categories. Did you think about that?

I am reusing the pack_attributions.txt file for bookmark categories as well, shall I modify the "pack" naming of the variable in the txt file to find something suitable for both kind of classes? I have made it work to return a proper .txt file in either case using "pack" naming but I don't know if it might be confusing in the future:

I did not realize you were reusing the same file. Then I might rename the file to multiple_sounds_download_attribution.txt, and pass pack or bookmark_category as needed, then check type to see if you should use pack.name or bookmark_category.name.

I was not reusing the same file in fact, I just tried after you suggested it xD

ffont · 2024-12-02T11:42:42Z

Oh, and please edit the title of the PR, don't say this is an "attempt". I guess everything that we do is just initially an attempt :)

…ies and packs + attributions.txt adaptation

…th packs and bookmark categories

ffont

Thanks @quimmrc! I left some other comments, this is almost there!

ffont · 2024-12-09T10:34:21Z

apiv2/views.py

@@ -702,7 +702,9 @@ def get(self, request,  *args, **kwargs):
                msg='Sounds in pack %i have not yet been described or moderated' % int(pack_id), resource=self)

        licenses_url = (reverse('pack-licenses', args=[pack.user.username, pack.id]))
-        return download_sounds(licenses_url, pack)
+        licenses_content = pack.get_attribution()
+        sound_list = pack.sounds.filter(processing_state="OK", moderation_state="OK").select_related('user', 'license')


I imagine that inside get_attribution we already do this query pack.sounds.filter(processing_state="OK", moderation_state="OK").select_related('user', 'license') so we are doing it twice. We should improve that. Maybe get_attribution could get an optional argument sound_qs (qsstands for queryset), and when the argument is provided, it does not do the query inside get_attribution and it uses the provided queryset. Simlar logic should be applied for the bookmarks case. Do you think it makes sense?

ffont · 2024-12-09T10:34:52Z

bookmarks/views.py

@@ -84,20 +84,25 @@ def delete_bookmark_category(request, category_id):
        return HttpResponseRedirect(next)
    else:
        return HttpResponseRedirect(reverse("bookmarks-for-user", args=[request.user.username]))
-
+
+@login_required    


good catch!

ffont · 2024-12-09T10:35:34Z

bookmarks/views.py

+    licenses_url = (reverse('category-licenses', args=[category_id]))
+
+    bookmarked_sounds = Bookmark.objects.filter(category_id=category.id).values("sound_id")
+    sounds_list = Sound.objects.filter(id__in=bookmarked_sounds, processing_state="OK", moderation_state="OK").select_related('user','license')


Same comment as before, try to do it in a way in which this query is only performed once.

ffont · 2024-12-09T10:36:59Z

sounds/models.py

-            dict(users=users,
-                pack=self,
+        attribution = render_to_string("sounds/multiple_sounds_attribution.txt",
+            dict(type=self.__class__.__name__,


wow, this is ugly :S
just hardcode the name, e.g. type="pack", it is clearer.

ffont · 2024-12-09T10:38:40Z

utils/downloads.py

+
+    Args:
+        licenses_file_url (str): url to the sound Pack or BookmarkCategory licenses
+        licenses_file_content (django SafeString): attributions for the different sounds in the Pack or BookmarkCategory 


These django things were suggested by copilot or some other AI, or did you find about that elsewhere?

It wasn't provided by neither copilot nor an AI tool, I can't remember exactly how I got that but probably by looking at Django official documentation or by looking into the type of the variable. I'll check it again and see what it really is.

definetly took it from the "type" of the variable "licenses_file_content", as it returns:

<class 'django.utils.safestring.SafeString'>

However I would agree that it's not a very friendly name. Attributions come from the "render_to_string" from a .txt file. Any proposals on other name type this could take? Maybe just string would be enough.

ffont · 2024-12-09T10:40:01Z

bookmarks/models.py

-            dict(users=users,
-                category=self,
+        attribution = render_to_string(("sounds/multiple_sounds_attribution.txt"),
+            dict(type=self.__class__.__name__,


type="bookmark_category" (or whatever you use to compare in the template). It will be clearer for our future selves this way.

… to .txt file

bookmark category download implementation attempt

a9d503b

ffont requested changes Dec 2, 2024

View reviewed changes

ffont reviewed Dec 2, 2024

View reviewed changes

quimmrc changed the title ~~bookmark category download implementation attempt~~ bookmark category download implementation Dec 2, 2024

quimmrc added 3 commits December 3, 2024 11:23

bookmark category download: download_sounds for both bookmark categor…

3cf0512

…ies and packs + attributions.txt adaptation

multiple_sound_attribution.txt file coding to make it suitable for bo…

c5d0d48

…th packs and bookmark categories

download_sounds test correction

879c49a

ffont requested changes Dec 9, 2024

View reviewed changes

quimmrc and others added 5 commits December 10, 2024 12:12

get_attribution improvement and variable type change in the dict sent…

6788de0

… to .txt file

Remove unneeded print

d587ecc

Reduce num queries bookmarks page

eff0984

Merge branch 'master' into issue1290

4024444

Fix wrong no-bookmarks check

598f0ce

ffont approved these changes Dec 10, 2024

View reviewed changes

ffont merged commit 762b267 into master Dec 10, 2024
1 check passed

ffont deleted the issue1290 branch December 10, 2024 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bookmark category download implementation #1800

bookmark category download implementation #1800

quimmrc commented Dec 2, 2024 •

edited

Loading

ffont left a comment

ffont Dec 2, 2024

ffont Dec 2, 2024

quimmrc Dec 2, 2024

ffont Dec 2, 2024

quimmrc Dec 3, 2024

ffont Dec 3, 2024

quimmrc Dec 3, 2024

ffont commented Dec 2, 2024

ffont left a comment

ffont Dec 9, 2024

ffont Dec 9, 2024

ffont Dec 9, 2024

ffont Dec 9, 2024

ffont Dec 9, 2024

quimmrc Dec 9, 2024

quimmrc Dec 9, 2024 •

edited

Loading

ffont Dec 9, 2024

		@@ -0,0 +1,21 @@
		{% load absurl %}Bookmark Category downloaded from Freesound

bookmark category download implementation #1800

bookmark category download implementation #1800

Conversation

quimmrc commented Dec 2, 2024 • edited Loading

ffont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffont commented Dec 2, 2024

ffont left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quimmrc Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quimmrc commented Dec 2, 2024 •

edited

Loading

quimmrc Dec 9, 2024 •

edited

Loading