Resolve "Adding cover art to my albums"

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):

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')},
+            },
+        ),
+    ]

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)

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):

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()

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)

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
+        )

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()

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()

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"

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",

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

changes/changelog.d/588.enhancement

@@ -0,0 +1 @@
+Support modifying album cover art through the web UI (#588)

front/src/components/common/AttachmentInput.vue

@@ -0,0 +1,99 @@
+<template>
+  <div>
+    <div v-if="errors.length > 0" class="ui negative message">
+      <div class="header"><translate translate-context="Content/*/Error message.Title">Your attachment cannot be saved</translate></div>
+      <ul class="list">
+        <li v-for="error in errors">{{ error }}</li>
+      </ul>
+    </div>
+    <div class="ui stackable two column grid">
+      <div class="column" v-if="value && value === initialValue">
+        <h3 class="ui header"><translate translate-context="Content/*/Title/Noun">Current file</translate></h3>
+        <img class="ui image" v-if="value" :src="$store.getters['instance/absoluteUrl'](`api/v1/attachments/${value}/proxy?next=medium_square_crop`)" />
+      </div>
+      <div class="column" v-else-if="attachment">
+        <h3 class="ui header"><translate translate-context="Content/*/Title/Noun">New file</translate></h3>
+        <img class="ui image" v-if="attachment && attachment.square_crop" :src="$store.getters['instance/absoluteUrl'](attachment.medium_square_crop)" />
+      </div>
+      <div class="column" v-if="!attachment">
+        <div class="ui basic segment">
+          <h3 class="ui header"><translate translate-context="Content/*/Title/Noun">New file</translate></h3>
+          <p><translate translate-context="Content/*/Paragraph">PNG or JPG. At most 5MB. Will be downscaled to 400x400px.</translate></p>
+          <input class="ui input" ref="attachment" type="file" accept="image/x-png,image/jpeg" @change="submit" />
+          <div v-if="isLoading" class="ui active inverted dimmer">
+            <div class="ui indeterminate text loader">
+              <translate translate-context="Content/*/*/Noun">Uploading file…</translate>
+            </div>
+          </div>
+        </div>
+      </div>
+
+    </div>
+  </div>
+</template>
+<script>
+import axios from 'axios'
+
+export default {
+  props: ['value', 'initialValue'],
+  data () {
+    return {
+      attachment: null,
+      isLoading: false,
+      errors: [],
+    }
+  },
+  methods: {
+    submit() {
+      this.isLoading = true
+      this.errors = []
+      let self = this
+      this.file = this.$refs.attachment.files[0]
+      let formData = new FormData()
+      formData.append("file", this.file)
+      axios
+        .post(`attachments/`, formData, {
+          headers: {
+            "Content-Type": "multipart/form-data"
+          }
+        })
+        .then(
+          response => {
+            this.isLoading = false
+            self.attachment = response.data
+            self.$emit('input', self.attachment.uuid)
+          },
+          error => {
+            self.isLoading = false
+            self.errors = error.backendErrors
+          }
+        )
+    },
+    remove() {
+      this.isLoading = true
+      this.errors = []
+      let self = this
+      axios.delete(`attachments/${this.attachment.uuid}/`)
+        .then(
+          response => {
+            this.isLoading = false
+            self.attachment = null
+            self.$emit('delete')
+          },
+          error => {
+            self.isLoading = false
+            self.errors = error.backendErrors
+          }
+        )
+    },
+  },
+  watch: {
+    value (v) {
+      if (this.attachment && v === this.initialValue) {
+        // we had a reset to initial value
+        this.remove()
+      }
+    }
+  }
+}
+</script>

front/src/components/library/EditCard.vue

@@ -53,20 +53,37 @@
             <td>{{ field.id }}</td>
 
             <td v-if="field.diff">
-              <span v-if="!part.added" v-for="part in field.diff" :class="['diff', {removed: part.removed}]">
-                {{ part.value }}
-              </span>
+              <template v-if="field.config.type === 'attachment' && field.oldRepr">
+                <img class="ui image" :src="$store.getters['instance/absoluteUrl'](`api/v1/attachments/${field.oldRepr}/proxy?next=medium_square_crop`)" />
+              </template>
+              <template v-else>
+                <span v-if="!part.added" v-for="part in field.diff" :class="['diff', {removed: part.removed}]">
+                  {{ part.value }}
+                </span>
+              </template>
             </td>
             <td v-else>
               <translate translate-context="*/*/*">N/A</translate>
             </td>
 
             <td v-if="field.diff" :title="field.newRepr">
-              <span v-if="!part.removed" v-for="part in field.diff" :class="['diff', {added: part.added}]">
-                {{ part.value }}
-              </span>
+              <template v-if="field.config.type === 'attachment' && field.newRepr">
+                <img class="ui image" :src="$store.getters['instance/absoluteUrl'](`api/v1/attachments/${field.newRepr}/proxy?next=medium_square_crop`)" />
+              </template>
+              <template v-else>
+                <span v-if="!part.removed" v-for="part in field.diff" :class="['diff', {added: part.added}]">
+                  {{ part.value }}
+                </span>
+              </template>
+            </td>
+            <td v-else :title="field.newRepr">
+              <template v-if="field.config.type === 'attachment' && field.newRepr">
+                <img class="ui image" :src="$store.getters['instance/absoluteUrl'](`api/v1/attachments/${field.newRepr}/proxy?next=medium_square_crop`)" />
+              </template>
+              <template v-else>
+                {{ field.newRepr }}
+              </template>
             </td>
-            <td v-else :title="field.newRepr">{{ field.newRepr }}</td>
           </tr>
         </tbody>
       </table>
@@ -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]

front/src/components/library/EditForm.vue

@@ -76,6 +76,17 @@
             <translate translate-context="Content/Library/Button.Label">Clear</translate>
           </button>
 
+        </template>
+        <template v-else-if="fieldConfig.type === 'attachment'">
+          <label :for="fieldConfig.id">{{ fieldConfig.label }}</label>
+          <attachment-input
+            v-model="values[fieldConfig.id]"
+            :initial-value="initialValues[fieldConfig.id]"
+            :required="fieldConfig.required"
+            :name="fieldConfig.id"
+            :id="fieldConfig.id"
+            @delete="values[fieldConfig.id] = initialValues[fieldConfig.id]"></attachment-input>
+
         </template>
         <template v-else-if="fieldConfig.type === 'tags'">
           <label :for="fieldConfig.id">{{ fieldConfig.label }}</label>
@@ -120,6 +131,7 @@
 import $ from 'jquery'
 import _ from '@/lodash'
 import axios from "axios"
+import AttachmentInput from '@/components/common/AttachmentInput'
 import EditList from '@/components/library/EditList'
 import EditCard from '@/components/library/EditCard'
 import TagsSelector from '@/components/library/TagsSelector'
@@ -132,7 +144,8 @@ export default {
   components: {
     EditList,
     EditCard,
-    TagsSelector
+    TagsSelector,
+    AttachmentInput
   },
   data() {
     return {

front/src/edits.js

@@ -43,6 +43,19 @@ 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
+              }
+            }
+          },
           {
             id: 'tags',
             type: 'tags',