Collections #1810

quimmrc · 2025-01-16T09:28:56Z

Issue(s)
#1029

Description
App creation and first implementations of basic functionalitites for collections.

improve url handling and visualization

ffont · 2025-01-16T09:42:41Z

fscollections/models.py

+class Collection(models.Model):
+
+    author = models.ForeignKey(User, on_delete=models.CASCADE) 
+    name = models.CharField(max_length=128, default="BookmarkCollection") #add restrictions


No need to specify a default

freesound/settings.py

fscollections/models.py

ffont · 2025-02-03T12:17:32Z

fscollections/views.py

+def delete_collection(request, collection_id):
+    collection = get_object_or_404(Collection, id=collection_id)
+
+    if request.user==collection.user:


Consider else case, return 404 or reload edit page but with a message saying that user is not allowed

… server)

…l and bulk_sounds_for_collection

…ssary code

quimmrc · 2025-04-01T09:11:07Z

freesound/static/bw-frontend/src/components/addSoundsModal.js

@@ -73,11 +88,14 @@ const prepareAddSoundsModalAndFields = (container) => {
                const newSoundIds = serializedIdListToIntList(selectedSoundIds);
                const combinedIds = combineIdsLists(currentSoundIds, newSoundIds);
                selectedSoundsHiddenInput.value = combinedIds.join(',');
+                if(selectedSoundsHiddenInput.value.split(',').length >= 4 && selectedSoundsHiddenInput.id === "collection_sounds"){


there's an error here, this 4 should take the value of settings.MAX_SOUNDS_PER_COLLECTION (something that happens again a few lines of code above)

You should take the value from some data property as I commented above. Is there a problem with that?

quimmrc · 2025-04-01T09:15:43Z

freesound/static/bw-frontend/src/components/collections.js

+            const modalUrlSplitted = element.dataset.modalContentUrl.split('/')
+            const soundId = parseInt(modalUrlSplitted[modalUrlSplitted.length - 3], 10)
+            if (!evt.altKey) {
+                handleGenericModalWithForm(element.dataset.modalContentUrl, () => {


in here, bookmarks used a custom saveCollection function, however I found more intuitive and standard to use a handleGenericModalWithForm to properly handle the submission of the SelectCollectionOrNewCollection form, which is the one used to add sounds to Collections from Sound URLS.

quimmrc · 2025-04-01T09:17:22Z

freesound/static/bw-frontend/src/components/collections.js

+        updateObjectSelectorDataProperties, the "call stack" states that originally, the function is called from here (line 175).
+        Therefore, the definition of selectedMaintainersDestinationElement somehow is wrong or confused with the selectedSoundsDestinationElement.
+        I tried to filter this using the above queryselector but it does not work. I guess this might have something to do with the file
+        collectionEdit.js.


This piece of comment might be interesting but the issue prompted by this is already solved. Basically the query for users does not exclude any, and instead "validation" for users who are added as maintaienrs is done from server-side.

quimmrc · 2025-04-01T09:22:17Z

fscollections/forms.py

+        fields = CollectionEditForm.Meta.fields + ['collection_sounds'] + ['maintainers']
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)


this might need a good review, it was a topic of discussion in our last meeting and I think that my proposal differs a bit from your main idea. In here this forms inherits everything so that it can be displayed in the edit url but all fields get disabled as soon as the form is initalized.

quimmrc · 2025-04-01T09:25:02Z

fscollections/models.py

+   status = models.CharField(db_index=True, max_length=2, choices=STATUS_CHOICES, default="PE")
+   #sound won't be added to collection until maintainers approve the sound
+
+@receiver(post_save, sender=CollectionSound)


num_sounds are now updated using CollectionSounds triggers, not only in the save collection method

quimmrc · 2025-04-01T09:25:31Z

fscollections/templatetags/custom_form_display.py

+
+register = template.Library()
+
+@register.simple_tag


I should've deleted this but I forgot, my mistake

quimmrc · 2025-04-01T09:27:20Z

fscollections/views.py

+    # the collections_for_user can be reused to display ONE collection so give it a thought on full collections display
+    return render(request, 'collections/your_collections.html', tvars)
+
+def collection_stats_section(request, collection_id):


In here I tried to imitate the pack stats behaviour but I was not much familiar with it and how the cache is handled so this is not properly updated (needs further review by me)

I'll have a look when testing the PR. This async loading of stats has to do with caching computation of stats, but maybe if stats computation is fast enough, then this might not be needed. We added some recent changes to compute some of the stats in DB rather than in python with good results, so we might apply this here and not do the stats section async.

quimmrc · 2025-04-01T09:28:23Z

fscollections/views.py

+        if form.is_valid():
+            form.save(user_adding_sound=request.user)
+            return HttpResponseRedirect(reverse('collection', args=[collection.id]))
+        else:


this part might need a good review since I did something unusual here, but I still believe it's solid enough

I'll have a detailed look into this when I can test the PR. At first sight I don't think we need to anything unusual here.

quimmrc · 2025-04-01T09:30:44Z

templates/collections/modal_collect_sound.html

+{% load static %}
+{% load bw_templatetags %}
+
+{% block id %}collectSoundModal{% endblock %}


This should be named "AddSoundToCollectionModal" or similar, I forgot to change this, my bad

quimmrc · 2025-04-01T09:32:30Z

freesound/static/bw-frontend/src/components/addSoundsModal.js

this allows to differentiate the sound selector in the edit collections page from the users selector (for maintainers)

…max_sounds for django templates

ffont

Thanks @quimmrc for the PR! I had a quick look, but couldn't test it yet, I'll do it soon. This is long and complex code so some things still need to be looked at in detail, but it looks good generally! I'll did leave some comments of some small things to improve. You might want to look at those in the next days before I do a test and further review (if you have time).

ffont · 2025-04-23T07:44:01Z

accounts/templatetags/users_selector.py

+def users_selector(context, users, selected_user_ids=[], show_select_all_buttons=False):
+    if users:
+        if not isinstance(users[0], User):
+            # users are passed as a list of user ids, retrieve the Sound objects from DB


...retrieve the User objects...

ffont · 2025-04-23T08:43:02Z

freesound/context_processor.py

-            'system_prefers_dark_theme': request.COOKIES.get('systemPrefersDarkTheme', 'no') == 'yes'  # Determine the user's system preference for dark/light theme (for non authenticated users, always use light theme)
+            'system_prefers_dark_theme': request.COOKIES.get('systemPrefersDarkTheme', 'no') == 'yes',  # Determine the user's system preference for dark/light theme (for non authenticated users, always use light theme)
+            'enable_collections': settings.ENABLE_COLLECTIONS,
+            'max_sounds_per_collection': settings.MAX_SOUNDS_PER_COLLECTION,


I don't think we should add enable_collections and max_sounds_per_collection in the global context processor as this will only be used in collections-related pages. This should probably only be added in tvars where needed.

I agree for max_sounds_per_collection, however, for the enable_collections case it is used in a couple of html templates which are the navigation bar and the sound url ones, which have a more global behavior rather than collections-related-pages, do you think we could leave enable_collections as a context processor?

ffont · 2025-04-23T08:47:27Z

templates/collections/edit_collection.html

+
+    </p>
+</div>
+<div id="config" data-max-sounds="{{max_sounds_per_collection}}"></div>


This is quite ugly here :)
data-max-sounds could be a property of the object selector, and place it somewhere there. I left other comments that should clarify this.

ffont · 2025-04-23T08:50:04Z

freesound/static/bw-frontend/src/components/addSoundsModal.js

+        }
+
+        if(soundsInput.id === "collection_sounds"){
+            const maxSounds = document.getElementById('config').dataset.maxSounds;


Don't hardcode this thing here if(soundsInput.id === "collection_sounds"). I think you could set data-max-elements maybe as a property of selectedSoundsDestinationElement(because it already has the data-type property), and here you check if there is any maximum set, and if there is, act accordingly and disable the add button if selected elements is above the maximum.

ffont · 2025-04-23T08:51:33Z

freesound/static/bw-frontend/src/components/addSoundsModal.js

@@ -62,6 +78,9 @@ const prepareAddSoundsModalAndFields = (container) => {
            updateObjectSelectorDataProperties(selectedSoundsDestinationElement);
            const selectedSoundsHiddenInput = document.getElementById(addSoundsButton.dataset.selectedSoundsHiddenInputId);
            selectedSoundsHiddenInput.value = selectedSoundsDestinationElement.dataset.unselectedIds;
+            if(selectedSoundsHiddenInput.value.split(',').length < maxSounds && selectedSoundsHiddenInput.id === "collection_sounds"){
+                addSoundsButton.disabled = false;


This should also be updated per the comments above.

ffont · 2025-04-23T09:47:55Z

fscollections/views.py

+    # the collections_for_user can be reused to display ONE collection so give it a thought on full collections display
+    return render(request, 'collections/your_collections.html', tvars)
+
+def collection_stats_section(request, collection_id):


I'll have a look when testing the PR. This async loading of stats has to do with caching computation of stats, but maybe if stats computation is fast enough, then this might not be needed. We added some recent changes to compute some of the stats in DB rather than in python with good results, so we might apply this here and not do the stats section async.

ffont · 2025-04-23T09:49:19Z

fscollections/views.py

+    collection = get_object_or_404(Collection, id=collection_id)
+
+    if request.method=="POST" and request.user==collection.user:
+        collection.delete()


Add a message like "collection successfully deleted"? See how django messages app works, we do it in many other places

ffont · 2025-04-23T09:50:55Z

fscollections/views.py

+        collection.delete()
+        return HttpResponseRedirect(reverse('your-collections'))
+    else:
+        return HttpResponseRedirect(reverse('collection', args=[collection.id]))


Maybe we should return "not allowed" response. Did you check how similar things are handled elsewhere in Freesound?

ffont · 2025-04-23T09:52:40Z

fscollections/views.py

+        if form.is_valid():
+            form.save(user_adding_sound=request.user)
+            return HttpResponseRedirect(reverse('collection', args=[collection.id]))
+        else:


I'll have a detailed look into this when I can test the PR. At first sight I don't think we need to anything unusual here.

ffont · 2025-04-23T09:55:07Z

sounds/models.py

+        qs = qs.filter(
+            moderation_state="OK",
+            processing_state="OK",
+            collections=collection_id


Does this work?? I guess it is some feature of the through model, but I'm a bit surprised here... I'd like to look into this with @alastair

I think it does, so far it is only used to render the sounds selector for the edit collection URL

yes, this should be fine. it's not specifically related to the through, just the ManyToManyField

quimmrc · 2025-04-24T12:54:32Z

freesound/static/bw-frontend/src/components/addSoundsModal.js

                }
+                if (soundsLabel){
+                    soundsLabel.innerHTML = "Sounds in collection (" + selectedSoundsDestinationElement.children.length + ")"}


A part from removing the hard-coded part and retrieving the number of elements from the sound selector, I automated the display of the help text for this field (informing of maximum number of sounds permitted) and the label of this field (which shows the number of sounds in collection). Although this is also used for pack edits, it has no conflicts (queries for help text and label won't find anything).

quimmrc · 2025-04-24T12:55:27Z

fscollections/forms.py

-
-        if self.instance.num_sounds >= settings.MAX_SOUNDS_PER_COLLECTION: 
-           self.fields['collection_sounds'].help_text=f"You have reached the maximum number of sounds available for a collection ({settings.MAX_SOUNDS_PER_COLLECTION}). " \
-           "In order to add new sounds, first remove some of the current ones."


this could not be updated if done from server-side, which was solved with the comment above

alastair · 2025-05-12T09:07:20Z

accounts/templatetags/display_user.py

+@register.inclusion_tag('accounts/display_user_selectable.html', takes_context=True)
+def display_user_small_selectable(context, user, selected=False):
+    context = context.get('original_context', context)  # This is to allow passing context in nested inclusion tags
+    tvars = display_user(context, user, size='basic')


double-check this - to me it seems a bit weird to call a function which is registered as a templatetag that returns HTML. Does this actually work?
It might be better to take the code for display_user and move it into a separate util function that can be used by both of these template tags

alastair · 2025-05-12T09:07:26Z

accounts/templatetags/display_user.py

+
+@register.inclusion_tag('accounts/display_user_selectable.html', takes_context=True)
+def display_user_small_selectable(context, user, selected=False):
+    context = context.get('original_context', context)  # This is to allow passing context in nested inclusion tags


unnecessary comment

alastair · 2025-05-12T09:07:50Z

accounts/templatetags/users_selector.py

@@ -0,0 +1,27 @@
+# Authors:


a bit weird to have just the authors block but no copyright?

alastair · 2025-05-12T09:08:15Z

accounts/templatetags/users_selector.py

+
+
+@register.inclusion_tag('molecules/object_selector.html', takes_context=True)
+def users_selector(context, users, selected_user_ids=[], show_select_all_buttons=False):


using =[] in a function isn't a good idea, set this to None and only iterate through it if it's not NOne

alastair · 2025-05-12T09:09:50Z

accounts/templatetags/users_selector.py

+        'objects': users,
+        'type': 'users',
+        'show_select_all_buttons': show_select_all_buttons,
+        'original_context': context  # This will be used so a nested inclusion tag can get the original context


unneeded comment

alastair · 2025-05-12T10:07:09Z

templates/collections/your_collections.html

+                </div>
+                {% else %}
+                <div class="v-spacing-top-3">
+                    <span class=text-light-grey> You don't have any collection yet...</span>


Suggested change

 You don't have any collection yet...

 You don't have any collections yet...

alastair · 2025-05-12T10:07:24Z

templates/collections/your_collections.html

+                </div>
+                {% else %}
+                <div class="v-spacing-top-3">
+                    <span class=text-light-grey> You're not a maintainer in any collection yet...</span>


Suggested change

 You're not a maintainer in any collection yet...

 You're not a maintainer of any collection yet...

alastair · 2025-05-12T10:15:46Z

fscollections/models.py

+   #sound won't be added to collection until maintainers approve the sound
+
+@receiver(post_save, sender=CollectionSound)
+def update_collection_num_sounds(**kwargs):


It'd be better to have the explicit list of parameters to these signals: https://docs.djangoproject.com/en/5.2/ref/signals/#django.db.models.signals.pre_save

Suggested change

def update_collection_num_sounds(**kwargs):

def update_collection_num_sounds(sender, instance, raw, using, update_fields):

Then you don't need to set isntance to False in the next line (this will already be None if you're in this situation)

alastair · 2025-05-12T10:18:10Z

fscollections/models.py

+        Collection.objects.filter(collectionsound=collectionsound).update(num_sounds=Greatest(F('num_sounds') + 1, 0))
+
+@receiver(post_delete, sender=CollectionSound)
+def update_collection_num_sounds_sound_removal(**kwargs):


Note that django doesn't send post_save/post_delete signals when you directly update a ManyToMany field: see https://docs.djangoproject.com/en/4.2/ref/signals/#m2m-changed
I see that you create these objects manually anyway (this is necessary if you have a through model with extra data, so this is fine), but we may be in a situation where we bulk remove sounds from a collection and this signal isn't triggered. Review if this might happen in any place

alastair · 2025-05-12T10:27:11Z

fscollections/forms.py

+        for snd in current_sounds:
+            sound = Sound.objects.get(id=snd)
+            CollectionSound.objects.get(collection=collection, sound=sound).delete()


this makes 3 queries per item to delete. better to do either:

sounds = Sound.objects.filter(id__in=current_sounds) collection.sounds.remove(*sounds)

(but careful with this option, because it won't trigger a delete signal, because it modifies the m2m field)

or maybe

sounds = Sound.objects.filter(id__in=current_sounds) CollectionSound.objects.filter(collection=collection, sound__in=sounds).delete()

because the filter/delete will do only a delete query and no SELECT.

quimmrc added 3 commits January 8, 2025 10:12

collections app creation and first basic implementations

813bd48

migration: add collection attributes

52543c4

improve url handling and visualization

attempt to implement collection modals

5f137eb

ffont reviewed Jan 16, 2025

View reviewed changes

quimmrc added 16 commits January 21, 2025 09:41

models reformulation

51adc61

Merge branch 'master' into collections

13a90da

add sound modal behavior tbd

2780959

add sound modal collection selector

4c7c190

add sound to col from sound url

0f47090

delete sound from collection func.

fc55074

deletion for CollectionSound + restrict add duplicates

b2e0277

minor details correction

5ba088d

delete and create collection functionalities

ae9950b

Edit collection permissions

d679849

add collection parameter to settings.py

58d845f

add maintainer modal (fails)

3d7a668

add maintainers interface

5ffea0b

adequate variable namings for collection modals

d5fed09

maintainers display in edit collection url

04ba23d

Merge branch 'master' into collections

e2bc70a

ffont reviewed Feb 3, 2025

View reviewed changes

freesound/settings.py Show resolved Hide resolved

ffont reviewed Feb 3, 2025

View reviewed changes

fscollections/models.py Show resolved Hide resolved

ffont reviewed Feb 3, 2025

View reviewed changes

fscollections/models.py Show resolved Hide resolved

ffont reviewed Feb 3, 2025

View reviewed changes

quimmrc added 6 commits February 4, 2025 08:58

changes from github review

18c9548

Merge branch 'master' into collections

034528c

db update + review + collect sound small player

f62d388

add sound to collection for all sound displays

81affae

remove maintainers from edit page

79787d1

add maintainers

82b3e2a

quimmrc added 12 commits February 25, 2025 11:54

users selectable for maintainers display

21cce70

add and remove maintainers through modal

33bcc39

collections display approach

fb104f3

update user permissions for maintainers and regular users (client and…

02c50dc

… server)

maintainer form and modal improvements

21fedc7

num_sounds, downloads and max-sounds-collection

b56c9b0

add sound to collection rework + edit collection errors display

53d4e1d

tests + adjustments in selectCollection form cleaning method

f4682e2

Merge branch 'master' into collections + collectionsound through mode…

f5d8bab

…l and bulk_sounds_for_collection

collectionsound through model + bulk_sounds_for_collection

d6953ec

update num sounds by triggers and remove unnecessary view

68b615a

collection info display approach + tests, comments, and remove unnece…

65cfda7

…ssary code

quimmrc commented Apr 1, 2025

View reviewed changes

quimmrc added 2 commits April 2, 2025 11:12

remove unnecessary code, rename files, avatar adjustments and export …

fbadeb7

…max_sounds for django templates

user creation test solution

6e9a5db

ffont reviewed Apr 23, 2025

View reviewed changes

quimmrc added 2 commits April 24, 2025 11:13

Merge branch 'master' into collections

822953c

review improvements + parameters update in edit collection url

2c39aef

quimmrc commented Apr 24, 2025

View reviewed changes

alastair reviewed May 12, 2025

View reviewed changes



		@register.inclusion_tag('molecules/object_selector.html', takes_context=True)
		def users_selector(context, users, selected_user_ids=[], show_select_all_buttons=False):

	<span class=text-light-grey> You don't have any collection yet...</span>
	<span class=text-light-grey> You don't have any collections yet...</span>

	<span class=text-light-grey> You're not a maintainer in any collection yet...</span>
	<span class=text-light-grey> You're not a maintainer of any collection yet...</span>

	def update_collection_num_sounds(**kwargs):
	def update_collection_num_sounds(sender, instance, raw, using, update_fields):


		register = template.Library()

		@register.simple_tag

Collections #1810

Are you sure you want to change the base?

Collections #1810

Uh oh!

Conversation

quimmrc commented Jan 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quimmrc Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffont left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quimmrc Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

quimmrc Apr 1, 2025 •

edited

Loading

quimmrc Apr 24, 2025 •

edited

Loading