From 71b400a9b8225a8dc3730bf00c60e18bcd241e45 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Fri, 17 Jan 2020 16:27:11 +0100 Subject: [PATCH] See #170: cover on tracks and artists --- .../migrations/0007_auto_20200116_1610.py | 18 +++ api/funkwhale_api/common/models.py | 9 +- api/funkwhale_api/common/mutations.py | 12 +- api/funkwhale_api/common/utils.py | 42 +++++++ api/funkwhale_api/federation/serializers.py | 73 +++++++---- api/funkwhale_api/federation/views.py | 21 +++- api/funkwhale_api/manage/serializers.py | 4 + api/funkwhale_api/manage/views.py | 8 +- api/funkwhale_api/music/factories.py | 2 + api/funkwhale_api/music/metadata.py | 1 + .../migrations/0047_auto_20200116_1246.py | 30 +++++ api/funkwhale_api/music/models.py | 60 ++++----- api/funkwhale_api/music/mutations.py | 116 ++++++++++-------- api/funkwhale_api/music/serializers.py | 3 +- api/funkwhale_api/music/tasks.py | 69 +++++++---- api/funkwhale_api/music/views.py | 11 +- api/requirements/test.txt | 2 +- api/tests/common/test_models.py | 2 +- api/tests/common/test_mutations.py | 5 +- api/tests/common/test_utils.py | 49 ++++++++ api/tests/federation/test_routes.py | 4 +- api/tests/federation/test_serializers.py | 90 +++++++++++++- api/tests/manage/test_serializers.py | 2 + api/tests/music/test_metadata.py | 12 +- api/tests/music/test_models.py | 8 -- api/tests/music/test_music.py | 20 --- api/tests/music/test_mutations.py | 5 +- api/tests/music/test_serializers.py | 2 + api/tests/music/test_tasks.py | 115 +++++++++++------ front/src/components/audio/artist/Card.vue | 3 + front/src/components/library/ArtistBase.vue | 3 + front/src/edits.js | 29 +++-- .../src/views/admin/library/ArtistDetail.vue | 3 +- front/src/views/admin/library/TrackDetail.vue | 3 +- 34 files changed, 582 insertions(+), 254 deletions(-) create mode 100644 api/funkwhale_api/common/migrations/0007_auto_20200116_1610.py create mode 100644 api/funkwhale_api/music/migrations/0047_auto_20200116_1246.py diff --git a/api/funkwhale_api/common/migrations/0007_auto_20200116_1610.py b/api/funkwhale_api/common/migrations/0007_auto_20200116_1610.py new file mode 100644 index 000000000..5032d719c --- /dev/null +++ b/api/funkwhale_api/common/migrations/0007_auto_20200116_1610.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.9 on 2020-01-16 16:10 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('common', '0006_content'), + ] + + operations = [ + migrations.AlterField( + model_name='attachment', + name='url', + field=models.URLField(max_length=500, null=True), + ), + ] diff --git a/api/funkwhale_api/common/models.py b/api/funkwhale_api/common/models.py index 993b1cef1..1b70304ed 100644 --- a/api/funkwhale_api/common/models.py +++ b/api/funkwhale_api/common/models.py @@ -175,7 +175,12 @@ def get_file_path(instance, filename): class AttachmentQuerySet(models.QuerySet): def attached(self, include=True): - related_fields = ["covered_album", "mutation_attachment"] + related_fields = [ + "covered_album", + "mutation_attachment", + "covered_track", + "covered_artist", + ] query = None for field in related_fields: field_query = ~models.Q(**{field: None}) @@ -195,7 +200,7 @@ class AttachmentQuerySet(models.QuerySet): class Attachment(models.Model): # Remote URL where the attachment can be fetched - url = models.URLField(max_length=500, unique=True, null=True) + url = models.URLField(max_length=500, null=True) uuid = models.UUIDField(unique=True, db_index=True, default=uuid.uuid4) # Actor associated with the attachment actor = models.ForeignKey( diff --git a/api/funkwhale_api/common/mutations.py b/api/funkwhale_api/common/mutations.py index 586e86ec1..66a16e4b4 100644 --- a/api/funkwhale_api/common/mutations.py +++ b/api/funkwhale_api/common/mutations.py @@ -85,8 +85,6 @@ class MutationSerializer(serializers.Serializer): class UpdateMutationSerializer(serializers.ModelSerializer, MutationSerializer): - serialized_relations = {} - def __init__(self, *args, **kwargs): # we force partial mode, because update mutations are partial kwargs.setdefault("partial", True) @@ -105,13 +103,14 @@ class UpdateMutationSerializer(serializers.ModelSerializer, MutationSerializer): return super().validate(validated_data) def db_serialize(self, validated_data): + serialized_relations = self.get_serialized_relations() data = {} # ensure model fields are serialized properly for key, value in list(validated_data.items()): if not isinstance(value, models.Model): data[key] = value continue - field = self.serialized_relations[key] + field = serialized_relations[key] data[key] = getattr(value, field) return data @@ -120,7 +119,7 @@ class UpdateMutationSerializer(serializers.ModelSerializer, MutationSerializer): # we use our serialized_relations configuration # to ensure we store ids instead of model instances in our json # payload - for field, attr in self.serialized_relations.items(): + for field, attr in self.get_serialized_relations().items(): try: obj = data[field] except KeyError: @@ -139,10 +138,13 @@ class UpdateMutationSerializer(serializers.ModelSerializer, MutationSerializer): return get_update_previous_state( obj, *list(validated_data.keys()), - serialized_relations=self.serialized_relations, + serialized_relations=self.get_serialized_relations(), handlers=self.get_previous_state_handlers(), ) + def get_serialized_relations(self): + return {} + def get_previous_state_handlers(self): return {} diff --git a/api/funkwhale_api/common/utils.py b/api/funkwhale_api/common/utils.py index 9b230f411..9fb9f64e2 100644 --- a/api/funkwhale_api/common/utils.py +++ b/api/funkwhale_api/common/utils.py @@ -1,6 +1,8 @@ +from django.core.files.base import ContentFile from django.utils.deconstruct import deconstructible import bleach.sanitizer +import logging import markdown import os import shutil @@ -13,6 +15,8 @@ from django.conf import settings from django import urls from django.db import models, transaction +logger = logging.getLogger(__name__) + def rename_file(instance, field_name, new_name, allow_missing_file=False): field = getattr(instance, field_name) @@ -306,3 +310,41 @@ def attach_content(obj, field, content_data): setattr(obj, field, content_obj) obj.save(update_fields=[field]) return content_obj + + +@transaction.atomic +def attach_file(obj, field, file_data, fetch=False): + from . import models + from . import tasks + + existing = getattr(obj, "{}_id".format(field)) + if existing: + getattr(obj, field).delete() + + if not file_data: + return + + extensions = {"image/jpeg": "jpg", "image/png": "png", "image/gif": "gif"} + extension = extensions.get(file_data["mimetype"], "jpg") + attachment = models.Attachment(mimetype=file_data["mimetype"]) + + filename = "cover-{}.{}".format(obj.uuid, extension) + if "url" in file_data: + attachment.url = file_data["url"] + else: + f = ContentFile(file_data["content"]) + attachment.file.save(filename, f, save=False) + + if not attachment.file and fetch: + try: + tasks.fetch_remote_attachment(attachment, filename=filename, save=False) + except Exception as e: + logger.warn("Cannot download attachment at url %s: %s", attachment.url, e) + attachment = None + + if attachment: + attachment.save() + + setattr(obj, field, attachment) + obj.save(update_fields=[field]) + return attachment diff --git a/api/funkwhale_api/federation/serializers.py b/api/funkwhale_api/federation/serializers.py index 2f593111f..c55c99ed0 100644 --- a/api/funkwhale_api/federation/serializers.py +++ b/api/funkwhale_api/federation/serializers.py @@ -22,7 +22,7 @@ logger = logging.getLogger(__name__) class LinkSerializer(jsonld.JsonLdSerializer): - type = serializers.ChoiceField(choices=[contexts.AS.Link]) + type = serializers.ChoiceField(choices=[contexts.AS.Link, contexts.AS.Image]) href = serializers.URLField(max_length=500) mediaType = serializers.CharField() @@ -817,6 +817,17 @@ def include_content(repr, content_obj): repr["mediaType"] = "text/html" +def include_image(repr, attachment): + if attachment: + repr["image"] = { + "type": "Image", + "href": attachment.download_url_original, + "mediaType": attachment.mimetype or "image/jpeg", + } + else: + repr["image"] = None + + class TruncatedCharField(serializers.CharField): def __init__(self, *args, **kwargs): self.truncate_length = kwargs.pop("truncate_length") @@ -877,6 +888,23 @@ class MusicEntitySerializer(jsonld.JsonLdSerializer): ] 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 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 def validate(self, data): @@ -890,15 +918,26 @@ class MusicEntitySerializer(jsonld.JsonLdSerializer): class ArtistSerializer(MusicEntitySerializer): + image = LinkSerializer( + allowed_mimetypes=["image/*"], allow_null=True, required=False + ) updateable_fields = [ ("name", "name"), ("musicbrainzId", "mbid"), ("attributedTo", "attributed_to"), + ("image", "attachment_cover"), ] class Meta: model = music_models.Artist - jsonld_mapping = MUSIC_ENTITY_JSONLD_MAPPING + jsonld_mapping = common_utils.concat_dicts( + MUSIC_ENTITY_JSONLD_MAPPING, + { + "released": jsonld.first_val(contexts.FW.released), + "artists": jsonld.first_attr(contexts.FW.artists, "@list"), + "image": jsonld.first_obj(contexts.AS.image), + }, + ) def to_representation(self, instance): d = { @@ -913,6 +952,7 @@ class ArtistSerializer(MusicEntitySerializer): "tag": self.get_tags_repr(instance), } include_content(d, instance.description) + include_image(d, instance.attachment_cover) if self.context.get("include_ap_context", self.parent is None): d["@context"] = jsonld.get_default_context() return d @@ -921,6 +961,7 @@ class ArtistSerializer(MusicEntitySerializer): class AlbumSerializer(MusicEntitySerializer): released = serializers.DateField(allow_null=True, required=False) artists = serializers.ListField(child=ArtistSerializer(), min_length=1) + # XXX: 1.0 rename to image cover = LinkSerializer( allowed_mimetypes=["image/*"], allow_null=True, required=False ) @@ -970,30 +1011,12 @@ class AlbumSerializer(MusicEntitySerializer): "href": instance.attachment_cover.download_url_original, "mediaType": instance.attachment_cover.mimetype or "image/jpeg", } + include_image(d, instance.attachment_cover) + if self.context.get("include_ap_context", self.parent is None): 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) @@ -1002,6 +1025,9 @@ class TrackSerializer(MusicEntitySerializer): album = AlbumSerializer() license = serializers.URLField(allow_null=True, required=False) copyright = serializers.CharField(allow_null=True, required=False) + image = LinkSerializer( + allowed_mimetypes=["image/*"], allow_null=True, required=False + ) updateable_fields = [ ("name", "title"), @@ -1011,6 +1037,7 @@ class TrackSerializer(MusicEntitySerializer): ("position", "position"), ("copyright", "copyright"), ("license", "license"), + ("image", "attachment_cover"), ] class Meta: @@ -1024,6 +1051,7 @@ class TrackSerializer(MusicEntitySerializer): "disc": jsonld.first_val(contexts.FW.disc), "license": jsonld.first_id(contexts.FW.license), "position": jsonld.first_val(contexts.FW.position), + "image": jsonld.first_obj(contexts.AS.image), }, ) @@ -1054,6 +1082,7 @@ class TrackSerializer(MusicEntitySerializer): "tag": self.get_tags_repr(instance), } include_content(d, instance.description) + include_image(d, instance.attachment_cover) if self.context.get("include_ap_context", self.parent is None): d["@context"] = jsonld.get_default_context() return d diff --git a/api/funkwhale_api/federation/views.py b/api/funkwhale_api/federation/views.py index b732712dc..b57a88261 100644 --- a/api/funkwhale_api/federation/views.py +++ b/api/funkwhale_api/federation/views.py @@ -222,9 +222,12 @@ class MusicLibraryViewSet( queryset=music_models.Track.objects.select_related( "album__artist__attributed_to", "artist__attributed_to", + "artist__attachment_cover", + "attachment_cover", "album__attributed_to", "attributed_to", "album__attachment_cover", + "album__artist__attachment_cover", "description", ).prefetch_related( "tagged_items__tag", @@ -283,6 +286,9 @@ class MusicUploadViewSet( "track__album__artist", "track__description", "track__album__attachment_cover", + "track__album__artist__attachment_cover", + "track__artist__attachment_cover", + "track__attachment_cover", ) serializer_class = serializers.UploadSerializer lookup_field = "uuid" @@ -303,7 +309,9 @@ class MusicArtistViewSet( ): authentication_classes = [authentication.SignatureAuthentication] renderer_classes = renderers.get_ap_renderers() - queryset = music_models.Artist.objects.local().select_related("description") + queryset = music_models.Artist.objects.local().select_related( + "description", "attachment_cover" + ) serializer_class = serializers.ArtistSerializer lookup_field = "uuid" @@ -314,7 +322,7 @@ class MusicAlbumViewSet( authentication_classes = [authentication.SignatureAuthentication] renderer_classes = renderers.get_ap_renderers() queryset = music_models.Album.objects.local().select_related( - "artist__description", "description" + "artist__description", "description", "artist__attachment_cover" ) serializer_class = serializers.AlbumSerializer lookup_field = "uuid" @@ -326,7 +334,14 @@ class MusicTrackViewSet( authentication_classes = [authentication.SignatureAuthentication] renderer_classes = renderers.get_ap_renderers() queryset = music_models.Track.objects.local().select_related( - "album__artist", "album__description", "artist__description", "description" + "album__artist", + "album__description", + "artist__description", + "description", + "attachment_cover", + "album__artist__attachment_cover", + "album__attachment_cover", + "artist__attachment_cover", ) serializer_class = serializers.TrackSerializer lookup_field = "uuid" diff --git a/api/funkwhale_api/manage/serializers.py b/api/funkwhale_api/manage/serializers.py index 20c1d0238..7adb9c863 100644 --- a/api/funkwhale_api/manage/serializers.py +++ b/api/funkwhale_api/manage/serializers.py @@ -390,6 +390,7 @@ class ManageArtistSerializer( tracks = ManageNestedTrackSerializer(many=True) attributed_to = ManageBaseActorSerializer() tags = serializers.SerializerMethodField() + cover = music_serializers.cover_field class Meta: model = music_models.Artist @@ -398,6 +399,7 @@ class ManageArtistSerializer( "tracks", "attributed_to", "tags", + "cover", ] def get_tags(self, obj): @@ -447,6 +449,7 @@ class ManageTrackSerializer( attributed_to = ManageBaseActorSerializer() uploads_count = serializers.SerializerMethodField() tags = serializers.SerializerMethodField() + cover = music_serializers.cover_field class Meta: model = music_models.Track @@ -456,6 +459,7 @@ class ManageTrackSerializer( "attributed_to", "uploads_count", "tags", + "cover", ] def get_uploads_count(self, obj): diff --git a/api/funkwhale_api/manage/views.py b/api/funkwhale_api/manage/views.py index 1754a8970..24e09de47 100644 --- a/api/funkwhale_api/manage/views.py +++ b/api/funkwhale_api/manage/views.py @@ -64,7 +64,7 @@ class ManageArtistViewSet( queryset = ( music_models.Artist.objects.all() .order_by("-id") - .select_related("attributed_to") + .select_related("attributed_to", "attachment_cover",) .prefetch_related( "tracks", Prefetch( @@ -164,7 +164,11 @@ class ManageTrackViewSet( music_models.Track.objects.all() .order_by("-id") .select_related( - "attributed_to", "artist", "album__artist", "album__attachment_cover" + "attributed_to", + "artist", + "album__artist", + "album__attachment_cover", + "attachment_cover", ) .annotate(uploads_count=Coalesce(Subquery(uploads_subquery), 0)) .prefetch_related(music_views.TAG_PREFETCH) diff --git a/api/funkwhale_api/music/factories.py b/api/funkwhale_api/music/factories.py index 07f0d9aa3..4247af3a2 100644 --- a/api/funkwhale_api/music/factories.py +++ b/api/funkwhale_api/music/factories.py @@ -64,6 +64,7 @@ class ArtistFactory( mbid = factory.Faker("uuid4") fid = factory.Faker("federation_url") playable = playable_factory("track__album__artist") + attachment_cover = factory.SubFactory(common_factories.AttachmentFactory) class Meta: model = "music.Artist" @@ -111,6 +112,7 @@ class TrackFactory( album = factory.SubFactory(AlbumFactory) position = 1 playable = playable_factory("track") + attachment_cover = factory.SubFactory(common_factories.AttachmentFactory) class Meta: model = "music.Track" diff --git a/api/funkwhale_api/music/metadata.py b/api/funkwhale_api/music/metadata.py index 78ca79168..ee24995e7 100644 --- a/api/funkwhale_api/music/metadata.py +++ b/api/funkwhale_api/music/metadata.py @@ -723,6 +723,7 @@ class TrackMetadataSerializer(serializers.Serializer): continue if v in ["", None, []]: validated_data.pop(field) + validated_data["album"]["cover_data"] = validated_data.pop("cover_data", None) return validated_data diff --git a/api/funkwhale_api/music/migrations/0047_auto_20200116_1246.py b/api/funkwhale_api/music/migrations/0047_auto_20200116_1246.py new file mode 100644 index 000000000..9981cacfd --- /dev/null +++ b/api/funkwhale_api/music/migrations/0047_auto_20200116_1246.py @@ -0,0 +1,30 @@ +# Generated by Django 2.2.9 on 2020-01-16 12:46 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('common', '0006_content'), + ('music', '0046_auto_20200113_1018'), + ] + + operations = [ + migrations.AddField( + model_name='artist', + name='attachment_cover', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='covered_artist', to='common.Attachment'), + ), + migrations.AddField( + model_name='track', + name='attachment_cover', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='covered_track', to='common.Attachment'), + ), + migrations.AlterField( + model_name='album', + name='attachment_cover', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='covered_album', to='common.Attachment'), + ), + ] diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 9165e4658..0d02236dd 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -230,6 +230,13 @@ class Artist(APIModelMixin): description = models.ForeignKey( "common.Content", null=True, blank=True, on_delete=models.SET_NULL ) + attachment_cover = models.ForeignKey( + "common.Attachment", + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name="covered_artist", + ) api = musicbrainz.api.artists objects = ArtistQuerySet.as_manager() @@ -248,6 +255,10 @@ class Artist(APIModelMixin): kwargs.update({"name": name}) return cls.objects.get_or_create(name__iexact=name, defaults=kwargs) + @property + def cover(self): + return self.attachment_cover + def import_artist(v): a = Artist.get_or_create_from_api(mbid=v[0]["artist"]["id"])[0] @@ -358,44 +369,6 @@ class Album(APIModelMixin): } objects = AlbumQuerySet.as_manager() - def get_image(self, data=None): - from funkwhale_api.common import tasks as common_tasks - - attachment = None - if data: - extensions = {"image/jpeg": "jpg", "image/png": "png", "image/gif": "gif"} - extension = extensions.get(data["mimetype"], "jpg") - attachment = common_models.Attachment(mimetype=data["mimetype"]) - f = None - filename = "{}.{}".format(self.uuid, extension) - if data.get("content"): - # we have to cover itself - f = ContentFile(data["content"]) - attachment.file.save(filename, f, save=False) - elif data.get("url"): - attachment.url = data.get("url") - # we can fetch from a url - try: - common_tasks.fetch_remote_attachment( - attachment, filename=filename, save=False - ) - except Exception as e: - logger.warn( - "Cannot download cover at url %s: %s", data.get("url"), e - ) - return - - elif self.mbid: - image_data = musicbrainz.api.images.get_front(str(self.mbid)) - f = ContentFile(image_data) - attachment = common_models.Attachment(mimetype="image/jpeg") - attachment.file.save("{0}.jpg".format(self.mbid), f, save=False) - if attachment and attachment.file: - attachment.save() - self.attachment_cover = attachment - self.save(update_fields=["attachment_cover"]) - return self.attachment_cover.file - @property def cover(self): return self.attachment_cover @@ -518,6 +491,13 @@ class Track(APIModelMixin): description = models.ForeignKey( "common.Content", null=True, blank=True, on_delete=models.SET_NULL ) + attachment_cover = models.ForeignKey( + "common.Attachment", + null=True, + blank=True, + on_delete=models.SET_NULL, + related_name="covered_track", + ) federation_namespace = "tracks" musicbrainz_model = "recording" @@ -572,6 +552,10 @@ class Track(APIModelMixin): except AttributeError: return "{} - {}".format(self.artist.name, self.title) + @property + def cover(self): + return self.attachment_cover + def get_activity_url(self): if self.mbid: return "https://musicbrainz.org/recording/{}".format(self.mbid) diff --git a/api/funkwhale_api/music/mutations.py b/api/funkwhale_api/music/mutations.py index d158857f0..c208d93a1 100644 --- a/api/funkwhale_api/music/mutations.py +++ b/api/funkwhale_api/music/mutations.py @@ -59,55 +59,15 @@ class DescriptionMutation(mutations.UpdateMutationSerializer): return r -@mutations.registry.connect( - "update", - models.Track, - perm_checkers={"suggest": can_suggest, "approve": can_approve}, -) -class TrackMutationSerializer(TagMutation, DescriptionMutation): - serialized_relations = {"license": "code"} - - class Meta: - model = models.Track - fields = ["license", "title", "position", "copyright", "tags", "description"] - - def post_apply(self, obj, validated_data): - routes.outbox.dispatch( - {"type": "Update", "object": {"type": "Track"}}, context={"track": obj} - ) - - -@mutations.registry.connect( - "update", - models.Artist, - perm_checkers={"suggest": can_suggest, "approve": can_approve}, -) -class ArtistMutationSerializer(TagMutation, DescriptionMutation): - class Meta: - model = models.Artist - fields = ["name", "tags", "description"] - - def post_apply(self, obj, validated_data): - routes.outbox.dispatch( - {"type": "Update", "object": {"type": "Artist"}}, context={"artist": obj} - ) - - -@mutations.registry.connect( - "update", - models.Album, - perm_checkers={"suggest": can_suggest, "approve": can_approve}, -) -class AlbumMutationSerializer(TagMutation, DescriptionMutation): +class CoverMutation(mutations.UpdateMutationSerializer): cover = common_serializers.RelatedField( "uuid", queryset=common_models.Attachment.objects.all().local(), serializer=None ) - serialized_relations = {"cover": "uuid"} - - class Meta: - model = models.Album - fields = ["title", "release_date", "tags", "cover", "description"] + def get_serialized_relations(self): + serialized_relations = super().get_serialized_relations() + serialized_relations["cover"] = "uuid" + return serialized_relations def get_previous_state_handlers(self): handlers = super().get_previous_state_handlers() @@ -116,11 +76,6 @@ class AlbumMutationSerializer(TagMutation, DescriptionMutation): ) return handlers - 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") @@ -140,3 +95,64 @@ class AlbumMutationSerializer(TagMutation, DescriptionMutation): common_models.MutationAttachment.objects.create( attachment=attachment, mutation=mutation ) + + +@mutations.registry.connect( + "update", + models.Track, + perm_checkers={"suggest": can_suggest, "approve": can_approve}, +) +class TrackMutationSerializer(CoverMutation, TagMutation, DescriptionMutation): + class Meta: + model = models.Track + fields = [ + "license", + "title", + "position", + "copyright", + "tags", + "description", + "cover", + ] + + def get_serialized_relations(self): + serialized_relations = super().get_serialized_relations() + serialized_relations["license"] = "code" + return serialized_relations + + def post_apply(self, obj, validated_data): + routes.outbox.dispatch( + {"type": "Update", "object": {"type": "Track"}}, context={"track": obj} + ) + + +@mutations.registry.connect( + "update", + models.Artist, + perm_checkers={"suggest": can_suggest, "approve": can_approve}, +) +class ArtistMutationSerializer(CoverMutation, TagMutation, DescriptionMutation): + class Meta: + model = models.Artist + fields = ["name", "tags", "description", "cover"] + + def post_apply(self, obj, validated_data): + routes.outbox.dispatch( + {"type": "Update", "object": {"type": "Artist"}}, context={"artist": obj} + ) + + +@mutations.registry.connect( + "update", + models.Album, + perm_checkers={"suggest": can_suggest, "approve": can_approve}, +) +class AlbumMutationSerializer(CoverMutation, TagMutation, DescriptionMutation): + class Meta: + model = models.Album + fields = ["title", "release_date", "tags", "cover", "description"] + + def post_apply(self, obj, validated_data): + routes.outbox.dispatch( + {"type": "Update", "object": {"type": "Album"}}, context={"album": obj} + ) diff --git a/api/funkwhale_api/music/serializers.py b/api/funkwhale_api/music/serializers.py index dcaeaadd0..34025f297 100644 --- a/api/funkwhale_api/music/serializers.py +++ b/api/funkwhale_api/music/serializers.py @@ -121,6 +121,7 @@ class ArtistWithAlbumsSerializer(OptionalDescriptionMixin, serializers.Serialize name = serializers.CharField() creation_date = serializers.DateTimeField() is_local = serializers.BooleanField() + cover = cover_field def get_tags(self, obj): tagged_items = getattr(obj, "_prefetched_tagged_items", []) @@ -266,7 +267,7 @@ class TrackSerializer(OptionalDescriptionMixin, serializers.Serializer): disc_number = serializers.IntegerField() copyright = serializers.CharField() license = serializers.SerializerMethodField() - + cover = cover_field get_attributed_to = serialize_attributed_to def get_artist(self, o): diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index e6b632072..3e95e5325 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -11,6 +11,7 @@ from django.dispatch import receiver from musicbrainzngs import ResponseError from requests.exceptions import RequestException +from funkwhale_api import musicbrainz from funkwhale_api.common import channels, preferences from funkwhale_api.common import utils as common_utils from funkwhale_api.federation import routes @@ -28,33 +29,34 @@ from . import signals logger = logging.getLogger(__name__) -def update_album_cover( - album, source=None, cover_data=None, musicbrainz=True, replace=False -): +def populate_album_cover(album, source=None, replace=False): if album.attachment_cover and not replace: return - if cover_data: - return album.get_image(data=cover_data) - if source and source.startswith("file://"): # let's look for a cover in the same directory path = os.path.dirname(source.replace("file://", "", 1)) logger.info("[Album %s] scanning covers from %s", album.pk, path) cover = get_cover_from_fs(path) - if cover: - return album.get_image(data=cover) - if musicbrainz and album.mbid: + return common_utils.attach_file(album, "attachment_cover", cover) + if album.mbid: + logger.info( + "[Album %s] Fetching cover from musicbrainz release %s", + album.pk, + str(album.mbid), + ) try: - logger.info( - "[Album %s] Fetching cover from musicbrainz release %s", - album.pk, - str(album.mbid), - ) - return album.get_image() + image_data = musicbrainz.api.images.get_front(str(album.mbid)) except ResponseError as exc: logger.warning( "[Album %s] cannot fetch cover from musicbrainz: %s", album.pk, str(exc) ) + else: + return common_utils.attach_file( + album, + "attachment_cover", + {"content": image_data, "mimetype": "image/jpeg"}, + fetch=True, + ) IMAGE_TYPES = [("jpg", "image/jpeg"), ("jpeg", "image/jpeg"), ("png", "image/png")] @@ -274,10 +276,8 @@ def process_upload(upload, update_denormalization=True): # update album cover, if needed if not track.album.attachment_cover: - update_album_cover( - track.album, - source=final_metadata.get("upload_source"), - cover_data=final_metadata.get("cover_data"), + populate_album_cover( + track.album, source=final_metadata.get("upload_source"), ) broadcast = getter( @@ -299,6 +299,12 @@ def process_upload(upload, update_denormalization=True): ) +def get_cover(obj, field): + cover = obj.get(field) + if cover: + return {"mimetype": cover["mediaType"], "url": cover["href"]} + + def federation_audio_track_to_metadata(payload, references): """ Given a valid payload as returned by federation.serializers.TrackSerializer.validated_data, @@ -315,6 +321,7 @@ def federation_audio_track_to_metadata(payload, references): "mbid": str(payload.get("musicbrainzId")) if payload.get("musicbrainzId") else None, + "cover_data": get_cover(payload, "image"), "album": { "title": payload["album"]["name"], "fdate": payload["album"]["published"], @@ -324,6 +331,7 @@ def federation_audio_track_to_metadata(payload, references): "mbid": str(payload["album"]["musicbrainzId"]) if payload["album"].get("musicbrainzId") else None, + "cover_data": get_cover(payload["album"], "cover"), "release_date": payload["album"].get("released"), "tags": [t["name"] for t in payload["album"].get("tags", []) or []], "artists": [ @@ -331,6 +339,7 @@ def federation_audio_track_to_metadata(payload, references): "fid": a["id"], "name": a["name"], "fdate": a["published"], + "cover_data": get_cover(a, "image"), "description": a.get("description"), "attributed_to": references.get(a.get("attributedTo")), "mbid": str(a["musicbrainzId"]) if a.get("musicbrainzId") else None, @@ -348,6 +357,7 @@ def federation_audio_track_to_metadata(payload, references): "attributed_to": references.get(a.get("attributedTo")), "mbid": str(a["musicbrainzId"]) if a.get("musicbrainzId") else None, "tags": [t["name"] for t in a.get("tags", []) or []], + "cover_data": get_cover(a, "image"), } for a in payload["artists"] ], @@ -356,9 +366,6 @@ def federation_audio_track_to_metadata(payload, references): "fdate": payload["published"], "tags": [t["name"] for t in payload.get("tags", []) or []], } - cover = payload["album"].get("cover") - if cover: - new_data["cover_data"] = {"mimetype": cover["mediaType"], "url": cover["href"]} return new_data @@ -427,11 +434,7 @@ def get_track_from_import_metadata( ): track = _get_track(data, attributed_to=attributed_to, **forced_values) if update_cover and track and not track.album.attachment_cover: - update_album_cover( - track.album, - source=data.get("upload_source"), - cover_data=data.get("cover_data"), - ) + populate_album_cover(track.album, source=data.get("upload_source")) return track @@ -513,6 +516,9 @@ def _get_track(data, attributed_to=None, **forced_values): common_utils.attach_content( artist, "description", artist_data.get("description") ) + common_utils.attach_file( + artist, "attachment_cover", artist_data.get("cover_data") + ) if "album" in forced_values: album = forced_values["album"] @@ -550,6 +556,11 @@ def _get_track(data, attributed_to=None, **forced_values): common_utils.attach_content( album_artist, "description", album_artist_data.get("description") ) + common_utils.attach_file( + album_artist, + "attachment_cover", + album_artist_data.get("cover_data"), + ) # get / create album album_data = data["album"] @@ -583,6 +594,9 @@ def _get_track(data, attributed_to=None, **forced_values): common_utils.attach_content( album, "description", album_data.get("description") ) + common_utils.attach_file( + album, "attachment_cover", album_data.get("cover_data") + ) # get / create track track_title = ( @@ -643,6 +657,7 @@ def _get_track(data, attributed_to=None, **forced_values): ) tags_models.add_tags(track, *tags) common_utils.attach_content(track, "description", data.get("description")) + common_utils.attach_file(track, "attachment_cover", data.get("cover_data")) return track diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index f7fb6e653..28babcc96 100644 --- a/api/funkwhale_api/music/views.py +++ b/api/funkwhale_api/music/views.py @@ -113,7 +113,7 @@ class ArtistViewSet( ): queryset = ( models.Artist.objects.all() - .prefetch_related("attributed_to") + .prefetch_related("attributed_to", "attachment_cover") .prefetch_related( Prefetch( "tracks", @@ -295,7 +295,7 @@ class TrackViewSet( queryset = ( models.Track.objects.all() .for_nested_serialization() - .prefetch_related("attributed_to") + .prefetch_related("attributed_to", "attachment_cover") .order_by("-creation_date") ) serializer_class = serializers.TrackSerializer @@ -558,7 +558,12 @@ class UploadViewSet( queryset = ( models.Upload.objects.all() .order_by("-creation_date") - .prefetch_related("library", "track__artist", "track__album__artist") + .prefetch_related( + "library", + "track__artist", + "track__album__artist", + "track__attachment_cover", + ) ) serializer_class = serializers.UploadForOwnerSerializer permission_classes = [ diff --git a/api/requirements/test.txt b/api/requirements/test.txt index 051a117df..508b1c689 100644 --- a/api/requirements/test.txt +++ b/api/requirements/test.txt @@ -1,7 +1,7 @@ # Test dependencies go here. flake8 -pytest>=5 +pytest>=5,<5.3.3 pytest-django>=3.5.1 pytest-mock pytest-sugar diff --git a/api/tests/common/test_models.py b/api/tests/common/test_models.py index 9fdde0378..f60ee3ef5 100644 --- a/api/tests/common/test_models.py +++ b/api/tests/common/test_models.py @@ -64,7 +64,7 @@ def test_attachment(factories, now): @pytest.mark.parametrize("args, expected", [([], [0]), ([True], [0]), ([False], [1])]) def test_attachment_queryset_attached(args, expected, factories, queryset_equal_list): attachments = [ - factories["music.Album"]().attachment_cover, + factories["music.Album"](artist__attachment_cover=None).attachment_cover, factories["common.Attachment"](), ] diff --git a/api/tests/common/test_mutations.py b/api/tests/common/test_mutations.py index 877128fac..d006f83f7 100644 --- a/api/tests/common/test_mutations.py +++ b/api/tests/common/test_mutations.py @@ -63,12 +63,13 @@ def test_db_serialize_update_mutation(factories, mutations_registry, mocker): user = factories["users.User"](email="hello@test.email", with_actor=True) class S(mutations.UpdateMutationSerializer): - serialized_relations = {"actor": "full_username"} - class Meta: model = user.__class__ fields = ["actor"] + def get_serialized_relations(self): + return {"actor": "full_username"} + expected = {"actor": user.actor.full_username} assert S().db_serialize({"actor": user.actor}) == expected diff --git a/api/tests/common/test_utils.py b/api/tests/common/test_utils.py index def5434a4..478a9e8cc 100644 --- a/api/tests/common/test_utils.py +++ b/api/tests/common/test_utils.py @@ -1,3 +1,4 @@ +import io import pytest from funkwhale_api.common import utils @@ -124,3 +125,51 @@ def test_join_url(start, end, expected): def test_render_html(text, content_type, expected): result = utils.render_html(text, content_type) assert result == expected + + +def test_attach_file_url(factories): + album = factories["music.Album"]() + existing_attachment = album.attachment_cover + assert existing_attachment is not None + + data = {"mimetype": "image/jpeg", "url": "https://example.com/test.jpg"} + new_attachment = utils.attach_file(album, "attachment_cover", data) + + album.refresh_from_db() + + with pytest.raises(existing_attachment.DoesNotExist): + existing_attachment.refresh_from_db() + + assert album.attachment_cover == new_attachment + assert not new_attachment.file + assert new_attachment.url == data["url"] + assert new_attachment.mimetype == data["mimetype"] + + +def test_attach_file_url_fetch(factories, r_mock): + album = factories["music.Album"]() + + data = {"mimetype": "image/jpeg", "url": "https://example.com/test.jpg"} + r_mock.get(data["url"], body=io.BytesIO(b"content")) + new_attachment = utils.attach_file(album, "attachment_cover", data, fetch=True) + + album.refresh_from_db() + + assert album.attachment_cover == new_attachment + assert new_attachment.file.read() == b"content" + assert new_attachment.url == data["url"] + assert new_attachment.mimetype == data["mimetype"] + + +def test_attach_file_content(factories, r_mock): + album = factories["music.Album"]() + + data = {"mimetype": "image/jpeg", "content": b"content"} + new_attachment = utils.attach_file(album, "attachment_cover", data) + + album.refresh_from_db() + + assert album.attachment_cover == new_attachment + assert new_attachment.file.read() == b"content" + assert new_attachment.url is None + assert new_attachment.mimetype == data["mimetype"] diff --git a/api/tests/federation/test_routes.py b/api/tests/federation/test_routes.py index 995cb1e44..83f10e5cd 100644 --- a/api/tests/federation/test_routes.py +++ b/api/tests/federation/test_routes.py @@ -540,7 +540,7 @@ def test_inbox_update_artist(factories, mocker): "funkwhale_api.music.tasks.update_library_entity" ) activity = factories["federation.Activity"]() - obj = factories["music.Artist"](attributed=True) + obj = factories["music.Artist"](attributed=True, attachment_cover=None) actor = obj.attributed_to data = serializers.ArtistSerializer(obj).data data["name"] = "New name" @@ -602,7 +602,7 @@ def test_inbox_update_track(factories, mocker): "funkwhale_api.music.tasks.update_library_entity" ) activity = factories["federation.Activity"]() - obj = factories["music.Track"](attributed=True) + obj = factories["music.Track"](attributed=True, attachment_cover=None) actor = obj.attributed_to data = serializers.TrackSerializer(obj).data data["name"] = "New title" diff --git a/api/tests/federation/test_serializers.py b/api/tests/federation/test_serializers.py index e158b6041..4f31dcbdf 100644 --- a/api/tests/federation/test_serializers.py +++ b/api/tests/federation/test_serializers.py @@ -575,6 +575,11 @@ def test_activity_pub_artist_serializer_to_ap(factories): "attributedTo": artist.attributed_to.fid, "mediaType": "text/html", "content": common_utils.render_html(content.text, content.content_type), + "image": { + "type": "Image", + "mediaType": "image/jpeg", + "href": utils.full_url(artist.attachment_cover.file.url), + }, "tag": [ {"type": "Hashtag", "name": "#Punk"}, {"type": "Hashtag", "name": "#Rock"}, @@ -601,6 +606,11 @@ def test_activity_pub_album_serializer_to_ap(factories): "mediaType": "image/jpeg", "href": utils.full_url(album.attachment_cover.file.url), }, + "image": { + "type": "Image", + "mediaType": "image/jpeg", + "href": utils.full_url(album.attachment_cover.file.url), + }, "musicbrainzId": album.mbid, "published": album.creation_date.isoformat(), "released": album.release_date.isoformat(), @@ -622,6 +632,44 @@ def test_activity_pub_album_serializer_to_ap(factories): assert serializer.data == expected +def test_activity_pub_artist_serializer_from_ap_update(factories, faker): + artist = factories["music.Artist"](attributed=True) + payload = { + "@context": jsonld.get_default_context(), + "type": "Artist", + "id": artist.fid, + "name": faker.sentence(), + "musicbrainzId": faker.uuid4(), + "published": artist.creation_date.isoformat(), + "attributedTo": artist.attributed_to.fid, + "mediaType": "text/html", + "content": common_utils.render_html(faker.sentence(), "text/html"), + "image": {"type": "Image", "mediaType": "image/jpeg", "href": faker.url()}, + "tag": [ + {"type": "Hashtag", "name": "#Punk"}, + {"type": "Hashtag", "name": "#Rock"}, + ], + } + + serializer = serializers.ArtistSerializer(artist, data=payload) + assert serializer.is_valid(raise_exception=True) is True + + serializer.save() + + artist.refresh_from_db() + + assert artist.name == payload["name"] + assert str(artist.mbid) == payload["musicbrainzId"] + assert artist.attachment_cover.url == payload["image"]["href"] + assert artist.attachment_cover.mimetype == payload["image"]["mediaType"] + assert artist.description.text == payload["content"] + assert artist.description.content_type == "text/html" + assert sorted(artist.tagged_items.values_list("tag__name", flat=True)) == [ + "Punk", + "Rock", + ] + + def test_activity_pub_album_serializer_from_ap_update(factories, faker): album = factories["music.Album"](attributed=True) released = faker.date_object() @@ -699,6 +747,11 @@ def test_activity_pub_track_serializer_to_ap(factories): {"type": "Hashtag", "name": "#Punk"}, {"type": "Hashtag", "name": "#Rock"}, ], + "image": { + "type": "Image", + "mediaType": "image/jpeg", + "href": utils.full_url(track.attachment_cover.file.url), + }, } serializer = serializers.TrackSerializer(track) @@ -726,6 +779,11 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): "disc": 1, "content": "Hello there", "attributedTo": track_attributed_to.fid, + "image": { + "type": "Link", + "href": "https://cover.image/track.png", + "mediaType": "image/png", + }, "album": { "type": "Album", "id": "http://hello.album", @@ -753,6 +811,11 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): "published": published.isoformat(), "attributedTo": album_artist_attributed_to.fid, "tag": [{"type": "Hashtag", "name": "AlbumArtistTag"}], + "image": { + "type": "Link", + "href": "https://cover.image/album-artist.png", + "mediaType": "image/png", + }, } ], }, @@ -767,6 +830,11 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): "attributedTo": artist_attributed_to.fid, "published": published.isoformat(), "tag": [{"type": "Hashtag", "name": "ArtistTag"}], + "image": { + "type": "Link", + "href": "https://cover.image/artist.png", + "mediaType": "image/png", + }, } ], "tag": [ @@ -774,7 +842,6 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): {"type": "Hashtag", "name": "World"}, ], } - r_mock.get(data["album"]["cover"]["href"], body=io.BytesIO(b"coucou")) serializer = serializers.TrackSerializer(data=data, context={"activity": activity}) assert serializer.is_valid(raise_exception=True) @@ -793,10 +860,12 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): assert str(track.mbid) == data["musicbrainzId"] assert track.description.text == data["content"] assert track.description.content_type == "text/html" + assert track.attachment_cover.url == data["image"]["href"] + assert track.attachment_cover.mimetype == data["image"]["mediaType"] assert album.from_activity == activity - assert album.attachment_cover.file.read() == b"coucou" - assert album.attachment_cover.file.path.endswith(".png") + assert album.attachment_cover.url == data["album"]["cover"]["href"] + assert album.attachment_cover.mimetype == data["album"]["cover"]["mediaType"] assert album.title == data["album"]["name"] assert album.fid == data["album"]["id"] assert str(album.mbid) == data["album"]["musicbrainzId"] @@ -814,6 +883,8 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): assert artist.attributed_to == artist_attributed_to assert artist.description.text == data["artists"][0]["content"] assert artist.description.content_type == data["artists"][0]["mediaType"] + assert artist.attachment_cover.url == data["artists"][0]["image"]["href"] + assert artist.attachment_cover.mimetype == data["artists"][0]["image"]["mediaType"] assert album_artist.from_activity == activity assert album_artist.name == data["album"]["artists"][0]["name"] @@ -826,6 +897,14 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): album_artist.description.content_type == data["album"]["artists"][0]["mediaType"] ) + assert ( + album_artist.attachment_cover.url + == data["album"]["artists"][0]["image"]["href"] + ) + assert ( + album_artist.attachment_cover.mimetype + == data["album"]["artists"][0]["image"]["mediaType"] + ) add_tags.assert_any_call(track, *["Hello", "World"]) add_tags.assert_any_call(album, *["AlbumTag"]) @@ -833,7 +912,7 @@ def test_activity_pub_track_serializer_from_ap(factories, r_mock, mocker): add_tags.assert_any_call(artist, *["ArtistTag"]) -def test_activity_pub_track_serializer_from_ap_update(factories, r_mock, mocker): +def test_activity_pub_track_serializer_from_ap_update(factories, r_mock, mocker, faker): set_tags = mocker.patch("funkwhale_api.tags.models.set_tags") content = factories["common.Content"]() track_attributed_to = factories["federation.Actor"]() @@ -853,6 +932,7 @@ def test_activity_pub_track_serializer_from_ap_update(factories, r_mock, mocker) "attributedTo": track_attributed_to.fid, "album": serializers.AlbumSerializer(track.album).data, "artists": [serializers.ArtistSerializer(track.artist).data], + "image": {"type": "Link", "mediaType": "image/jpeg", "href": faker.url()}, "tag": [ {"type": "Hashtag", "name": "#Hello"}, # Ensure we can handle tags without a leading # @@ -873,6 +953,8 @@ def test_activity_pub_track_serializer_from_ap_update(factories, r_mock, mocker) assert track.description.content_type == "text/html" assert track.description.text == "hello there" assert str(track.mbid) == data["musicbrainzId"] + assert track.attachment_cover.url == data["image"]["href"] + assert track.attachment_cover.mimetype == data["image"]["mediaType"] set_tags.assert_called_once_with(track, *["Hello", "World"]) diff --git a/api/tests/manage/test_serializers.py b/api/tests/manage/test_serializers.py index a80015ea9..412a7a58c 100644 --- a/api/tests/manage/test_serializers.py +++ b/api/tests/manage/test_serializers.py @@ -303,6 +303,7 @@ def test_manage_artist_serializer(factories, now, to_api_date): artist.attributed_to ).data, "tags": [], + "cover": common_serializers.AttachmentSerializer(artist.attachment_cover).data, } s = serializers.ManageArtistSerializer(artist) @@ -412,6 +413,7 @@ def test_manage_track_serializer(factories, now, to_api_date): ).data, "uploads_count": 44, "tags": [], + "cover": common_serializers.AttachmentSerializer(track.attachment_cover).data, } s = serializers.ManageTrackSerializer(track) diff --git a/api/tests/music/test_metadata.py b/api/tests/music/test_metadata.py index d842d3318..485502ac7 100644 --- a/api/tests/music/test_metadata.py +++ b/api/tests/music/test_metadata.py @@ -440,7 +440,7 @@ def test_track_metadata_serializer(path, expected, mocker): path = os.path.join(DATA_DIR, path) data = metadata.Metadata(path) get_picture = mocker.patch.object(data, "get_picture") - expected["cover_data"] = get_picture.return_value + expected["album"]["cover_data"] = get_picture.return_value serializer = metadata.TrackMetadataSerializer(data=data) assert serializer.is_valid(raise_exception=True) is True @@ -566,13 +566,13 @@ def test_fake_metadata_with_serializer(): "mbid": uuid.UUID("5b4d7d2d-36df-4b38-95e3-a964234f520f"), }, ], + "cover_data": None, }, "position": 1, "disc_number": 1, "mbid": uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656"), "license": "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/", "copyright": "Someone", - "cover_data": None, } serializer = metadata.TrackMetadataSerializer(data=metadata.FakeMetadata(data)) assert serializer.is_valid(raise_exception=True) is True @@ -594,8 +594,8 @@ def test_serializer_album_artist_missing(): "mbid": None, "release_date": None, "artists": [], + "cover_data": None, }, - "cover_data": None, } serializer = metadata.TrackMetadataSerializer(data=metadata.FakeMetadata(data)) assert serializer.is_valid(raise_exception=True) is True @@ -622,8 +622,8 @@ def test_serializer_album_default_title_when_missing_or_empty(data): "mbid": None, "release_date": None, "artists": [], + "cover_data": None, }, - "cover_data": None, } serializer = metadata.TrackMetadataSerializer(data=metadata.FakeMetadata(data)) assert serializer.is_valid(raise_exception=True) is True @@ -649,8 +649,8 @@ def test_serializer_empty_fields(field_name): "mbid": None, "release_date": None, "artists": [], + "cover_data": None, }, - "cover_data": None, } serializer = metadata.TrackMetadataSerializer(data=metadata.FakeMetadata(data)) assert serializer.is_valid(raise_exception=True) is True @@ -701,8 +701,8 @@ def test_acquire_tags_from_genre(genre, expected_tags): "mbid": None, "release_date": None, "artists": [], + "cover_data": None, }, - "cover_data": None, } if expected_tags: expected["tags"] = expected_tags diff --git a/api/tests/music/test_models.py b/api/tests/music/test_models.py index d8022c964..74d5e9604 100644 --- a/api/tests/music/test_models.py +++ b/api/tests/music/test_models.py @@ -187,14 +187,6 @@ def test_track_get_file_size_in_place(factories): assert upload.get_file_size() == 297745 -def test_album_get_image_content(factories): - album = factories["music.Album"]() - album.get_image(data={"content": b"test", "mimetype": "image/jpeg"}) - album.refresh_from_db() - - assert album.attachment_cover.file.read() == b"test" - - def test_library(factories): now = timezone.now() actor = factories["federation.Actor"]() diff --git a/api/tests/music/test_music.py b/api/tests/music/test_music.py index 0709ab340..79402b47c 100644 --- a/api/tests/music/test_music.py +++ b/api/tests/music/test_music.py @@ -121,23 +121,3 @@ def test_can_get_or_create_track_from_api(artists, albums, tracks, mocker, db): track2, created = models.Track.get_or_create_from_api(mbid=data["id"]) assert not created assert track == track2 - - -def test_can_download_image_file_for_album(binary_cover, mocker, factories): - mocker.patch( - "funkwhale_api.musicbrainz.api.images.get_front", return_value=binary_cover - ) - # client._api.get_image_front('55ea4f82-b42b-423e-a0e5-290ccdf443ed') - album = factories["music.Album"](mbid="55ea4f82-b42b-423e-a0e5-290ccdf443ed") - album.get_image() - album.save() - - assert album.attachment_cover.file.read() == binary_cover - - -def test_album_get_image_doesnt_crash_with_empty_data(mocker, factories): - album = factories["music.Album"](mbid=None, attachment_cover=None) - assert ( - album.get_image(data={"content": "", "url": "", "mimetype": "image/png"}) - is None - ) diff --git a/api/tests/music/test_mutations.py b/api/tests/music/test_mutations.py index eff6925d3..a0f684769 100644 --- a/api/tests/music/test_mutations.py +++ b/api/tests/music/test_mutations.py @@ -179,9 +179,10 @@ def test_perm_checkers_can_approve( assert mutations.can_approve(obj, actor=actor) is expected -def test_mutation_set_attachment_cover(factories, now, mocker): +@pytest.mark.parametrize("factory_name", ["music.Artist", "music.Track", "music.Album"]) +def test_mutation_set_attachment_cover(factory_name, factories, now, mocker): new_attachment = factories["common.Attachment"](actor__local=True) - obj = factories["music.Album"]() + obj = factories[factory_name]() old_attachment = obj.attachment_cover mutation = factories["common.Mutation"]( type="update", target=obj, payload={"cover": new_attachment.uuid} diff --git a/api/tests/music/test_serializers.py b/api/tests/music/test_serializers.py index a35180cef..156abe33b 100644 --- a/api/tests/music/test_serializers.py +++ b/api/tests/music/test_serializers.py @@ -71,6 +71,7 @@ def test_artist_with_albums_serializer(factories, to_api_date): "tags": [], "attributed_to": federation_serializers.APIActorSerializer(actor).data, "tracks_count": 42, + "cover": common_serializers.AttachmentSerializer(artist.attachment_cover).data, } serializer = serializers.ArtistWithAlbumsSerializer(artist) assert serializer.data == expected @@ -217,6 +218,7 @@ def test_track_serializer(factories, to_api_date): "is_local": upload.track.is_local, "tags": [], "attributed_to": federation_serializers.APIActorSerializer(actor).data, + "cover": common_serializers.AttachmentSerializer(track.attachment_cover).data, } serializer = serializers.TrackSerializer(track) assert serializer.data == expected diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 3f09c5378..053f62ede 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -1,5 +1,4 @@ import datetime -import io import os import pytest import uuid @@ -270,7 +269,7 @@ def test_can_create_track_from_file_metadata_distinct_position(factories): assert new_track != track -def test_can_create_track_from_file_metadata_federation(factories, mocker, r_mock): +def test_can_create_track_from_file_metadata_federation(factories, mocker): metadata = { "artists": [ {"name": "Artist", "fid": "https://artist.fid", "fdate": timezone.now()} @@ -279,6 +278,7 @@ def test_can_create_track_from_file_metadata_federation(factories, mocker, r_moc "title": "Album", "fid": "https://album.fid", "fdate": timezone.now(), + "cover_data": {"url": "https://cover/hello.png", "mimetype": "image/png"}, "artists": [ { "name": "Album artist", @@ -291,9 +291,7 @@ def test_can_create_track_from_file_metadata_federation(factories, mocker, r_moc "position": 4, "fid": "https://hello", "fdate": timezone.now(), - "cover_data": {"url": "https://cover/hello.png", "mimetype": "image/png"}, } - r_mock.get(metadata["cover_data"]["url"], body=io.BytesIO(b"coucou")) track = tasks.get_track_from_import_metadata(metadata, update_cover=True) @@ -301,10 +299,11 @@ def test_can_create_track_from_file_metadata_federation(factories, mocker, r_moc assert track.fid == metadata["fid"] assert track.creation_date == metadata["fdate"] assert track.position == 4 - assert track.album.attachment_cover.file.read() == b"coucou" - assert track.album.attachment_cover.file.path.endswith(".png") - assert track.album.attachment_cover.url == metadata["cover_data"]["url"] - assert track.album.attachment_cover.mimetype == metadata["cover_data"]["mimetype"] + assert track.album.attachment_cover.url == metadata["album"]["cover_data"]["url"] + assert ( + track.album.attachment_cover.mimetype + == metadata["album"]["cover_data"]["mimetype"] + ) assert track.album.fid == metadata["album"]["fid"] assert track.album.title == metadata["album"]["title"] @@ -328,7 +327,9 @@ def test_sort_candidates(factories): def test_upload_import(now, factories, temp_signal, mocker): outbox = mocker.patch("funkwhale_api.federation.routes.outbox.dispatch") - update_album_cover = mocker.patch("funkwhale_api.music.tasks.update_album_cover") + populate_album_cover = mocker.patch( + "funkwhale_api.music.tasks.populate_album_cover" + ) get_picture = mocker.patch("funkwhale_api.music.metadata.Metadata.get_picture") get_track_from_import_metadata = mocker.spy(tasks, "get_track_from_import_metadata") track = factories["music.Track"](album__attachment_cover=None) @@ -348,8 +349,8 @@ def test_upload_import(now, factories, temp_signal, mocker): assert upload.import_status == "finished" assert upload.import_date == now get_picture.assert_called_once_with("cover_front", "other") - update_album_cover.assert_called_once_with( - upload.track.album, cover_data=get_picture.return_value, source=upload.source + populate_album_cover.assert_called_once_with( + upload.track.album, source=upload.source ) assert ( get_track_from_import_metadata.call_args[-1]["attributed_to"] @@ -557,46 +558,33 @@ def test_upload_import_error_metadata(factories, now, temp_signal, mocker): def test_upload_import_updates_cover_if_no_cover(factories, mocker, now): - mocked_update = mocker.patch("funkwhale_api.music.tasks.update_album_cover") + populate_album_cover = mocker.patch( + "funkwhale_api.music.tasks.populate_album_cover" + ) album = factories["music.Album"](attachment_cover=None) track = factories["music.Track"](album=album) upload = factories["music.Upload"]( track=None, import_metadata={"funkwhale": {"track": {"uuid": track.uuid}}} ) tasks.process_upload(upload_id=upload.pk) - mocked_update.assert_called_once_with(album, source=None, cover_data=None) - - -def test_update_album_cover_mbid(factories, mocker): - album = factories["music.Album"](attachment_cover=None) - - mocked_get = mocker.patch("funkwhale_api.music.models.Album.get_image") - tasks.update_album_cover(album=album) - - mocked_get.assert_called_once_with() - - -def test_update_album_cover_file_data(factories, mocker): - album = factories["music.Album"](attachment_cover=None, mbid=None) - - mocked_get = mocker.patch("funkwhale_api.music.models.Album.get_image") - tasks.update_album_cover(album=album, cover_data={"hello": "world"}) - mocked_get.assert_called_once_with(data={"hello": "world"}) + populate_album_cover.assert_called_once_with(album, source=None) @pytest.mark.parametrize("ext,mimetype", [("jpg", "image/jpeg"), ("png", "image/png")]) -def test_update_album_cover_file_cover_separate_file(ext, mimetype, factories, mocker): +def test_populate_album_cover_file_cover_separate_file( + ext, mimetype, factories, mocker +): mocker.patch("funkwhale_api.music.tasks.IMAGE_TYPES", [(ext, mimetype)]) image_path = os.path.join(DATA_DIR, "cover.{}".format(ext)) with open(image_path, "rb") as f: image_content = f.read() album = factories["music.Album"](attachment_cover=None, mbid=None) - mocked_get = mocker.patch("funkwhale_api.music.models.Album.get_image") + attach_file = mocker.patch("funkwhale_api.common.utils.attach_file") mocker.patch("funkwhale_api.music.metadata.Metadata.get_picture", return_value=None) - tasks.update_album_cover(album=album, source="file://" + image_path) - mocked_get.assert_called_once_with( - data={"mimetype": mimetype, "content": image_content} + tasks.populate_album_cover(album=album, source="file://" + image_path) + attach_file.assert_called_once_with( + album, "attachment_cover", {"mimetype": mimetype, "content": image_content} ) @@ -623,6 +611,11 @@ def test_federation_audio_track_to_metadata(now, mocker): "attributedTo": "http://track.attributed", "tag": [{"type": "Hashtag", "name": "TrackTag"}], "content": "hello there", + "image": { + "type": "Link", + "href": "http://cover.test/track", + "mediaType": "image/png", + }, "album": { "published": published.isoformat(), "type": "Album", @@ -645,6 +638,11 @@ def test_federation_audio_track_to_metadata(now, mocker): "musicbrainzId": str(uuid.uuid4()), "attributedTo": "http://album-artist.attributed", "tag": [{"type": "Hashtag", "name": "AlbumArtistTag"}], + "image": { + "type": "Link", + "href": "http://cover.test/album-artist", + "mediaType": "image/png", + }, } ], "cover": { @@ -664,6 +662,11 @@ def test_federation_audio_track_to_metadata(now, mocker): "musicbrainzId": str(uuid.uuid4()), "attributedTo": "http://artist.attributed", "tag": [{"type": "Hashtag", "name": "ArtistTag"}], + "image": { + "type": "Link", + "href": "http://cover.test/artist", + "mediaType": "image/png", + }, } ], } @@ -681,6 +684,10 @@ def test_federation_audio_track_to_metadata(now, mocker): "attributed_to": references["http://track.attributed"], "tags": ["TrackTag"], "description": {"content_type": "text/html", "text": "hello there"}, + "cover_data": { + "mimetype": serializer.validated_data["image"]["mediaType"], + "url": serializer.validated_data["image"]["href"], + }, "album": { "title": payload["album"]["name"], "attributed_to": references["http://album.attributed"], @@ -690,6 +697,10 @@ def test_federation_audio_track_to_metadata(now, mocker): "fdate": serializer.validated_data["album"]["published"], "tags": ["AlbumTag"], "description": {"content_type": "text/plain", "text": "album desc"}, + "cover_data": { + "mimetype": serializer.validated_data["album"]["cover"]["mediaType"], + "url": serializer.validated_data["album"]["cover"]["href"], + }, "artists": [ { "name": a["name"], @@ -704,6 +715,14 @@ def test_federation_audio_track_to_metadata(now, mocker): "text": "album artist desc", }, "tags": ["AlbumArtistTag"], + "cover_data": { + "mimetype": serializer.validated_data["album"]["artists"][i][ + "image" + ]["mediaType"], + "url": serializer.validated_data["album"]["artists"][i][ + "image" + ]["href"], + }, } for i, a in enumerate(payload["album"]["artists"]) ], @@ -719,13 +738,15 @@ def test_federation_audio_track_to_metadata(now, mocker): "attributed_to": references["http://artist.attributed"], "tags": ["ArtistTag"], "description": {"content_type": "text/html", "text": "artist desc"}, + "cover_data": { + "mimetype": serializer.validated_data["artists"][i]["image"][ + "mediaType" + ], + "url": serializer.validated_data["artists"][i]["image"]["href"], + }, } for i, a in enumerate(payload["artists"]) ], - "cover_data": { - "mimetype": serializer.validated_data["album"]["cover"]["mediaType"], - "url": serializer.validated_data["album"]["cover"]["href"], - }, } result = tasks.federation_audio_track_to_metadata( @@ -764,7 +785,7 @@ def test_scan_page_fetches_page_and_creates_tracks(now, mocker, factories, r_moc scan_page = mocker.patch("funkwhale_api.music.tasks.scan_library_page.delay") scan = factories["music.LibraryScan"](status="scanning", total_files=5) uploads = [ - factories["music.Upload"].build( + factories["music.Upload"]( fid="https://track.test/{}".format(i), size=42, bitrate=66, @@ -780,7 +801,9 @@ def test_scan_page_fetches_page_and_creates_tracks(now, mocker, factories, r_moc "page": Paginator(uploads, 3).page(1), "item_serializer": federation_serializers.UploadSerializer, } + uploads[0].__class__.objects.filter(pk__in=[u.pk for u in uploads]).delete() page = federation_serializers.CollectionPageSerializer(page_conf) + r_mock.get(page.data["id"], json=page.data) tasks.scan_library_page(library_scan_id=scan.pk, page_url=page.data["id"]) @@ -1129,3 +1152,15 @@ def test_tag_artists_from_tracks(queryset_equal_queries, factories, mocker): add_tags_batch.assert_called_once_with( get_tags_from_foreign_key.return_value, model=models.Artist, ) + + +def test_can_download_image_file_for_album_mbid(binary_cover, mocker, factories): + mocker.patch( + "funkwhale_api.musicbrainz.api.images.get_front", return_value=binary_cover + ) + # client._api.get_image_front('55ea4f82-b42b-423e-a0e5-290ccdf443ed') + album = factories["music.Album"](mbid="55ea4f82-b42b-423e-a0e5-290ccdf443ed") + tasks.populate_album_cover(album, replace=True) + + assert album.attachment_cover.file.read() == binary_cover + assert album.attachment_cover.mimetype == "image/jpeg" diff --git a/front/src/components/audio/artist/Card.vue b/front/src/components/audio/artist/Card.vue index 71192914e..6c9d54ecc 100644 --- a/front/src/components/audio/artist/Card.vue +++ b/front/src/components/audio/artist/Card.vue @@ -51,6 +51,9 @@ export default { return url }, cover () { + if (this.artist.cover) { + return this.artist.cover + } return this.artist.albums.map((a) => { return a.cover }).filter((c) => { diff --git a/front/src/components/library/ArtistBase.vue b/front/src/components/library/ArtistBase.vue index 2c5d5284a..1c9054efd 100644 --- a/front/src/components/library/ArtistBase.vue +++ b/front/src/components/library/ArtistBase.vue @@ -230,6 +230,9 @@ export default { ) }, cover() { + if (this.object.cover) { + return this.object.cover + } return this.object.albums .filter(album => { return album.cover diff --git a/front/src/edits.js b/front/src/edits.js index 0757ed0d4..baa9bbb32 100644 --- a/front/src/edits.js +++ b/front/src/edits.js @@ -19,6 +19,19 @@ export default { getValue: (obj) => { return obj.description || {text: null, content_type: 'text/markdown'}}, getValueRepr: getContentValueRepr } + const cover = { + id: 'cover', + type: 'attachment', + required: false, + label: this.$pgettext('Content/*/*/Noun', 'Cover'), + getValue: (obj) => { + if (obj.cover) { + return obj.cover.uuid + } else { + return null + } + } + } return { artist: { fields: [ @@ -30,6 +43,7 @@ export default { getValue: (obj) => { return obj.name } }, description, + cover, { id: 'tags', type: 'tags', @@ -57,19 +71,7 @@ export default { label: this.$pgettext('Content/*/*/Noun', 'Release date'), getValue: (obj) => { return obj.release_date } }, - { - id: 'cover', - type: 'attachment', - required: false, - label: this.$pgettext('Content/*/*/Noun', 'Cover'), - getValue: (obj) => { - if (obj.cover) { - return obj.cover.uuid - } else { - return null - } - } - }, + cover, { id: 'tags', type: 'tags', @@ -90,6 +92,7 @@ export default { getValue: (obj) => { return obj.title } }, description, + cover, { id: 'position', type: 'text', diff --git a/front/src/views/admin/library/ArtistDetail.vue b/front/src/views/admin/library/ArtistDetail.vue index 72e1bcb08..0104431ea 100644 --- a/front/src/views/admin/library/ArtistDetail.vue +++ b/front/src/views/admin/library/ArtistDetail.vue @@ -9,7 +9,8 @@

- + +
{{ object.name | truncate(100) }}
diff --git a/front/src/views/admin/library/TrackDetail.vue b/front/src/views/admin/library/TrackDetail.vue index d7f836d3a..e85acef02 100644 --- a/front/src/views/admin/library/TrackDetail.vue +++ b/front/src/views/admin/library/TrackDetail.vue @@ -9,7 +9,8 @@

- + +
{{ object.title | truncate(100) }}