diff --git a/api/funkwhale_api/common/factories.py b/api/funkwhale_api/common/factories.py index 85a441e85..d6a063603 100644 --- a/api/funkwhale_api/common/factories.py +++ b/api/funkwhale_api/common/factories.py @@ -16,14 +16,6 @@ class MutationFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): class Meta: model = "common.Mutation" - @factory.post_generation - def target(self, create, extracted, **kwargs): - if not create: - # Simple build, do nothing. - return - self.target = extracted - self.save() - @registry.register class AttachmentFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): diff --git a/api/funkwhale_api/common/migrations/0005_auto_20191125_1421.py b/api/funkwhale_api/common/migrations/0005_auto_20191125_1421.py new file mode 100644 index 000000000..b0984c14e --- /dev/null +++ b/api/funkwhale_api/common/migrations/0005_auto_20191125_1421.py @@ -0,0 +1,37 @@ +# Generated by Django 2.2.7 on 2019-11-25 14:21 + +import django.contrib.postgres.fields.jsonb +import django.core.serializers.json +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('common', '0004_auto_20191111_1338'), + ] + + operations = [ + migrations.AlterField( + model_name='mutation', + name='payload', + field=django.contrib.postgres.fields.jsonb.JSONField(encoder=django.core.serializers.json.DjangoJSONEncoder), + ), + migrations.AlterField( + model_name='mutation', + name='previous_state', + field=django.contrib.postgres.fields.jsonb.JSONField(default=None, encoder=django.core.serializers.json.DjangoJSONEncoder, null=True), + ), + migrations.CreateModel( + name='MutationAttachment', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('attachment', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='mutation_attachment', to='common.Attachment')), + ('mutation', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='mutation_attachment', to='common.Mutation')), + ], + options={ + 'unique_together': {('attachment', 'mutation')}, + }, + ), + ] diff --git a/api/funkwhale_api/common/models.py b/api/funkwhale_api/common/models.py index f764bf251..4e4fc14dd 100644 --- a/api/funkwhale_api/common/models.py +++ b/api/funkwhale_api/common/models.py @@ -167,7 +167,7 @@ def get_file_path(instance, filename): class AttachmentQuerySet(models.QuerySet): def attached(self, include=True): - related_fields = ["covered_album"] + related_fields = ["covered_album", "mutation_attachment"] query = None for field in related_fields: field_query = ~models.Q(**{field: None}) @@ -178,6 +178,12 @@ class AttachmentQuerySet(models.QuerySet): return self.filter(query) + def local(self, include=True): + if include: + return self.filter(actor__domain_id=settings.FEDERATION_HOSTNAME) + else: + return self.exclude(actor__domain_id=settings.FEDERATION_HOSTNAME) + class Attachment(models.Model): # Remote URL where the attachment can be fetched @@ -248,6 +254,25 @@ class Attachment(models.Model): return federation_utils.full_url(proxy_url + "?next=medium_square_crop") +class MutationAttachment(models.Model): + """ + When using attachments in mutations, we need to keep a reference to + the attachment to ensure it is not pruned by common/tasks.py. + + This is what this model does. + """ + + attachment = models.OneToOneField( + Attachment, related_name="mutation_attachment", on_delete=models.CASCADE + ) + mutation = models.OneToOneField( + Mutation, related_name="mutation_attachment", on_delete=models.CASCADE + ) + + class Meta: + unique_together = ("attachment", "mutation") + + @receiver(models.signals.post_save, sender=Attachment) def warm_attachment_thumbnails(sender, instance, **kwargs): if not instance.file or not settings.CREATE_IMAGE_THUMBNAILS: @@ -258,3 +283,22 @@ def warm_attachment_thumbnails(sender, instance, **kwargs): image_attr="file", ) num_created, failed_to_create = warmer.warm() + + +@receiver(models.signals.post_save, sender=Mutation) +def trigger_mutation_post_init(sender, instance, created, **kwargs): + if not created: + return + + from . import mutations + + try: + conf = mutations.registry.get_conf(instance.type, instance.target) + except mutations.ConfNotFound: + return + serializer = conf["serializer_class"]() + try: + handler = serializer.mutation_post_init + except AttributeError: + return + handler(instance) diff --git a/api/funkwhale_api/common/serializers.py b/api/funkwhale_api/common/serializers.py index 3f128c18d..fa889f9e8 100644 --- a/api/funkwhale_api/common/serializers.py +++ b/api/funkwhale_api/common/serializers.py @@ -23,9 +23,10 @@ class RelatedField(serializers.RelatedField): self.related_field_name = related_field_name self.serializer = serializer self.filters = kwargs.pop("filters", None) - kwargs["queryset"] = kwargs.pop( - "queryset", self.serializer.Meta.model.objects.all() - ) + try: + kwargs["queryset"] = kwargs.pop("queryset") + except KeyError: + kwargs["queryset"] = self.serializer.Meta.model.objects.all() super().__init__(**kwargs) def get_filters(self, data): diff --git a/api/funkwhale_api/common/views.py b/api/funkwhale_api/common/views.py index 1109589d0..b6f8d5a0a 100644 --- a/api/funkwhale_api/common/views.py +++ b/api/funkwhale_api/common/views.py @@ -157,7 +157,9 @@ class AttachmentViewSet( required_scope = "libraries" anonymous_policy = "setting" - @action(detail=True, methods=["get"]) + @action( + detail=True, methods=["get"], permission_classes=[], authentication_classes=[] + ) @transaction.atomic def proxy(self, request, *args, **kwargs): instance = self.get_object() diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 5a62f4ce3..977ab0bb1 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -5,9 +5,12 @@ import uuid from django.core.exceptions import ObjectDoesNotExist from django.core.paginator import Paginator +from django.db import transaction + from rest_framework import serializers from funkwhale_api.common import utils as funkwhale_utils +from funkwhale_api.common import models as common_models from funkwhale_api.music import licenses from funkwhale_api.music import models as music_models from funkwhale_api.music import tasks as music_tasks @@ -808,6 +811,7 @@ class MusicEntitySerializer(jsonld.JsonLdSerializer): child=TagSerializer(), min_length=0, required=False, allow_null=True ) + @transaction.atomic def update(self, instance, validated_data): attributed_to_fid = validated_data.get("attributedTo") if attributed_to_fid: @@ -815,6 +819,8 @@ class MusicEntitySerializer(jsonld.JsonLdSerializer): updated_fields = funkwhale_utils.get_updated_fields( self.updateable_fields, validated_data, instance ) + updated_fields = self.validate_updated_data(instance, updated_fields) + if updated_fields: music_tasks.update_library_entity(instance, updated_fields) @@ -828,6 +834,9 @@ class MusicEntitySerializer(jsonld.JsonLdSerializer): for item in sorted(instance.tagged_items.all(), key=lambda i: i.tag.name) ] + def validate_updated_data(self, instance, validated_data): + return validated_data + class ArtistSerializer(MusicEntitySerializer): updateable_fields = [ @@ -869,6 +878,7 @@ class AlbumSerializer(MusicEntitySerializer): ("musicbrainzId", "mbid"), ("attributedTo", "attributed_to"), ("released", "release_date"), + ("cover", "attachment_cover"), ] class Meta: @@ -912,6 +922,26 @@ class AlbumSerializer(MusicEntitySerializer): d["@context"] = jsonld.get_default_context() return d + def validate_updated_data(self, instance, validated_data): + try: + attachment_cover = validated_data.pop("attachment_cover") + except KeyError: + return validated_data + + if ( + instance.attachment_cover + and instance.attachment_cover.url == attachment_cover["href"] + ): + # we already have the proper attachment + return validated_data + # create the attachment by hand so it can be attached as the album cover + validated_data["attachment_cover"] = common_models.Attachment.objects.create( + mimetype=attachment_cover["mediaType"], + url=attachment_cover["href"], + actor=instance.attributed_to, + ) + return validated_data + class TrackSerializer(MusicEntitySerializer): position = serializers.IntegerField(min_value=0, allow_null=True, required=False) diff --git a/api/funkwhale_api/music/mutations.py b/api/funkwhale_api/music/mutations.py index b149f1963..aad3b0e96 100644 --- a/api/funkwhale_api/music/mutations.py +++ b/api/funkwhale_api/music/mutations.py @@ -1,4 +1,6 @@ +from funkwhale_api.common import models as common_models from funkwhale_api.common import mutations +from funkwhale_api.common import serializers as common_serializers from funkwhale_api.federation import routes from funkwhale_api.tags import models as tags_models from funkwhale_api.tags import serializers as tags_serializers @@ -74,11 +76,48 @@ class ArtistMutationSerializer(TagMutation): perm_checkers={"suggest": can_suggest, "approve": can_approve}, ) class AlbumMutationSerializer(TagMutation): + cover = common_serializers.RelatedField( + "uuid", queryset=common_models.Attachment.objects.all().local(), serializer=None + ) + + serialized_relations = {"cover": "uuid"} + previous_state_handlers = dict( + list(TagMutation.previous_state_handlers.items()) + + [ + ( + "cover", + lambda obj: str(obj.attachment_cover.uuid) + if obj.attachment_cover + else None, + ), + ] + ) + class Meta: model = models.Album - fields = ["title", "release_date", "tags"] + fields = ["title", "release_date", "tags", "cover"] def post_apply(self, obj, validated_data): routes.outbox.dispatch( {"type": "Update", "object": {"type": "Album"}}, context={"album": obj} ) + + def update(self, instance, validated_data): + if "cover" in validated_data: + validated_data["attachment_cover"] = validated_data.pop("cover") + return super().update(instance, validated_data) + + def mutation_post_init(self, mutation): + # link cover_attachment (if any) to mutation + if "cover" not in mutation.payload: + return + try: + attachment = common_models.Attachment.objects.get( + uuid=mutation.payload["cover"] + ) + except common_models.Attachment.DoesNotExist: + return + + common_models.MutationAttachment.objects.create( + attachment=attachment, mutation=mutation + ) diff --git a/api/tests/common/test_tasks.py b/api/tests/common/test_tasks.py index fc62d901b..cfb91470f 100644 --- a/api/tests/common/test_tasks.py +++ b/api/tests/common/test_tasks.py @@ -1,6 +1,7 @@ import pytest import datetime +from funkwhale_api.common import models from funkwhale_api.common import serializers from funkwhale_api.common import signals from funkwhale_api.common import tasks @@ -68,21 +69,27 @@ def test_cannot_apply_already_applied_migration(factories): def test_prune_unattached_attachments(factories, settings, now): settings.ATTACHMENTS_UNATTACHED_PRUNE_DELAY = 5 + prunable_date = now - datetime.timedelta( + seconds=settings.ATTACHMENTS_UNATTACHED_PRUNE_DELAY + ) attachments = [ # attached, kept factories["music.Album"]().attachment_cover, # recent, kept factories["common.Attachment"](), # too old, pruned - factories["common.Attachment"]( - creation_date=now - - datetime.timedelta(seconds=settings.ATTACHMENTS_UNATTACHED_PRUNE_DELAY) - ), + factories["common.Attachment"](creation_date=prunable_date), + # attached to a mutation, kept even if old + models.MutationAttachment.objects.create( + mutation=factories["common.Mutation"](payload={}), + attachment=factories["common.Attachment"](creation_date=prunable_date), + ).attachment, ] tasks.prune_unattached_attachments() attachments[0].refresh_from_db() attachments[1].refresh_from_db() + attachments[3].refresh_from_db() with pytest.raises(attachments[2].DoesNotExist): attachments[2].refresh_from_db() diff --git a/api/tests/conftest.py b/api/tests/conftest.py index a7fa02cc0..2d32f42cf 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -422,3 +422,8 @@ def clear_license_cache(db): licenses._cache = None yield licenses._cache = None + + +@pytest.fixture +def faker(): + return factory.Faker._get_faker() diff --git a/api/tests/federation/test_routes.py b/api/tests/federation/test_routes.py index 3bbdd4868..a632c5153 100644 --- a/api/tests/federation/test_routes.py +++ b/api/tests/federation/test_routes.py @@ -537,7 +537,7 @@ def test_inbox_update_album(factories, mocker): "funkwhale_api.music.tasks.update_library_entity" ) activity = factories["federation.Activity"]() - obj = factories["music.Album"](attributed=True) + obj = factories["music.Album"](attributed=True, attachment_cover=None) actor = obj.attributed_to data = serializers.AlbumSerializer(obj).data data["name"] = "New title" diff --git a/api/tests/federation/test_serializers.py b/api/tests/federation/test_serializers.py index 34460a5a6..b7c4c4a9e 100644 --- a/api/tests/federation/test_serializers.py +++ b/api/tests/federation/test_serializers.py @@ -610,6 +610,47 @@ def test_activity_pub_album_serializer_to_ap(factories): assert serializer.data == expected +def test_activity_pub_album_serializer_from_ap_update(factories, faker): + album = factories["music.Album"](attributed=True) + released = faker.date_object() + payload = { + "@context": jsonld.get_default_context(), + "type": "Album", + "id": album.fid, + "name": faker.sentence(), + "cover": {"type": "Link", "mediaType": "image/jpeg", "href": faker.url()}, + "musicbrainzId": faker.uuid4(), + "published": album.creation_date.isoformat(), + "released": released.isoformat(), + "artists": [ + serializers.ArtistSerializer( + album.artist, context={"include_ap_context": False} + ).data + ], + "attributedTo": album.attributed_to.fid, + "tag": [ + {"type": "Hashtag", "name": "#Punk"}, + {"type": "Hashtag", "name": "#Rock"}, + ], + } + serializer = serializers.AlbumSerializer(album, data=payload) + assert serializer.is_valid(raise_exception=True) is True + + serializer.save() + + album.refresh_from_db() + + assert album.title == payload["name"] + assert str(album.mbid) == payload["musicbrainzId"] + assert album.release_date == released + assert album.attachment_cover.url == payload["cover"]["href"] + assert album.attachment_cover.mimetype == payload["cover"]["mediaType"] + assert sorted(album.tagged_items.values_list("tag__name", flat=True)) == [ + "Punk", + "Rock", + ] + + def test_activity_pub_track_serializer_to_ap(factories): track = factories["music.Track"]( license="cc-by-4.0", diff --git a/api/tests/music/test_mutations.py b/api/tests/music/test_mutations.py index 3a86f3bf8..ff2982dff 100644 --- a/api/tests/music/test_mutations.py +++ b/api/tests/music/test_mutations.py @@ -176,3 +176,22 @@ def test_perm_checkers_can_approve( obj = factories["music.Track"](**obj_kwargs) assert mutations.can_approve(obj, actor=actor) is expected + + +def test_mutation_set_attachment_cover(factories, now, mocker): + new_attachment = factories["common.Attachment"](actor__local=True) + obj = factories["music.Album"]() + old_attachment = obj.attachment_cover + mutation = factories["common.Mutation"]( + type="update", target=obj, payload={"cover": new_attachment.uuid} + ) + + # new attachment should be linked to mutation, to avoid being pruned + # before being applied + assert new_attachment.mutation_attachment.mutation == mutation + + mutation.apply() + obj.refresh_from_db() + + assert obj.attachment_cover == new_attachment + assert mutation.previous_state["cover"] == old_attachment.uuid diff --git a/changes/changelog.d/588.enhancement b/changes/changelog.d/588.enhancement new file mode 100644 index 000000000..1ce18953d --- /dev/null +++ b/changes/changelog.d/588.enhancement @@ -0,0 +1 @@ +Support modifying album cover art through the web UI (#588) diff --git a/front/src/components/common/AttachmentInput.vue b/front/src/components/common/AttachmentInput.vue new file mode 100644 index 000000000..47b3253a2 --- /dev/null +++ b/front/src/components/common/AttachmentInput.vue @@ -0,0 +1,99 @@ + + diff --git a/front/src/components/library/EditCard.vue b/front/src/components/library/EditCard.vue index fc5efea55..20435a039 100644 --- a/front/src/components/library/EditCard.vue +++ b/front/src/components/library/EditCard.vue @@ -53,20 +53,37 @@ {{ field.id }} - - {{ part.value }} - + + N/A - - {{ part.value }} - + + + + + + - {{ field.newRepr }} @@ -171,6 +188,7 @@ export default { let getValueRepr = fieldConfig.getValueRepr || dummyRepr let d = { id: f, + config: fieldConfig } if (previousState && previousState[f]) { d.old = previousState[f] diff --git a/front/src/components/library/EditForm.vue b/front/src/components/library/EditForm.vue index ea9fc7696..6f4afccac 100644 --- a/front/src/components/library/EditForm.vue +++ b/front/src/components/library/EditForm.vue @@ -76,6 +76,17 @@ Clear + +