From 7b71463ef8e9e88c8c8ee26277d8c3eb13cea02a Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Mon, 21 May 2018 19:55:06 +0200
Subject: [PATCH] Removed acoustid support, as the integration was buggy and
error-prone (#106)
---
api/funkwhale_api/music/tasks.py | 15 +++---
.../management/commands/import_files.py | 10 +---
api/tests/music/test_tasks.py | 48 +++++--------------
api/tests/test_import_audio_file.py | 24 ----------
changes/changelog.d/acoustid.misc | 1 +
docs/importing-music.rst | 10 ++--
front/src/views/admin/Settings.vue | 3 +-
7 files changed, 27 insertions(+), 84 deletions(-)
create mode 100644 changes/changelog.d/acoustid.misc
diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py
index 34345e47..e5426904 100644
--- a/api/funkwhale_api/music/tasks.py
+++ b/api/funkwhale_api/music/tasks.py
@@ -9,7 +9,7 @@ from funkwhale_api.federation import models as federation_models
from funkwhale_api.federation import serializers as federation_serializers
from funkwhale_api.taskapp import celery
from funkwhale_api.providers.acoustid import get_acoustid_client
-from funkwhale_api.providers.audiofile.tasks import import_track_data_from_path
+from funkwhale_api.providers.audiofile import tasks as audiofile_tasks
from django.conf import settings
from . import models
@@ -73,13 +73,15 @@ def import_track_from_remote(library_track):
library_track.title, artist=artist, album=album)[0]
-def _do_import(import_job, replace=False, use_acoustid=True):
+def _do_import(import_job, replace=False, use_acoustid=False):
from_file = bool(import_job.audio_file)
mbid = import_job.mbid
acoustid_track_id = None
duration = None
track = None
- use_acoustid = use_acoustid and preferences.get('providers_acoustid__api_key')
+ # use_acoustid = use_acoustid and preferences.get('providers_acoustid__api_key')
+ # Acoustid is not reliable, we disable it for now.
+ use_acoustid = False
if not mbid and use_acoustid and from_file:
# we try to deduce mbid from acoustid
client = get_acoustid_client()
@@ -91,11 +93,12 @@ def _do_import(import_job, replace=False, use_acoustid=True):
if mbid:
track, _ = models.Track.get_or_create_from_api(mbid=mbid)
elif import_job.audio_file:
- track = import_track_data_from_path(import_job.audio_file.path)
+ track = audiofile_tasks.import_track_data_from_path(
+ import_job.audio_file.path)
elif import_job.library_track:
track = import_track_from_remote(import_job.library_track)
elif import_job.source.startswith('file://'):
- track = import_track_data_from_path(
+ track = audiofile_tasks.import_track_data_from_path(
import_job.source.replace('file://', '', 1))
else:
raise ValueError(
@@ -151,7 +154,7 @@ def _do_import(import_job, replace=False, use_acoustid=True):
models.ImportJob.objects.filter(
status__in=['pending', 'errored']),
'import_job')
-def import_job_run(self, import_job, replace=False, use_acoustid=True):
+def import_job_run(self, import_job, replace=False, use_acoustid=False):
def mark_errored():
import_job.status = 'errored'
import_job.save(update_fields=['status'])
diff --git a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py
index a2757c69..70ff90ff 100644
--- a/api/funkwhale_api/providers/audiofile/management/commands/import_files.py
+++ b/api/funkwhale_api/providers/audiofile/management/commands/import_files.py
@@ -54,13 +54,6 @@ class Command(BaseCommand):
'import and not much disk space available.'
)
)
- parser.add_argument(
- '--no-acoustid',
- action='store_true',
- dest='no_acoustid',
- default=False,
- help='Use this flag to completely bypass acoustid completely',
- )
parser.add_argument(
'--noinput', '--no-input', action='store_false', dest='interactive',
help="Do NOT prompt the user for input of any kind.",
@@ -118,7 +111,6 @@ class Command(BaseCommand):
len(filtered['new'])))
self.stdout.write('Selected options: {}'.format(', '.join([
- 'no acoustid' if options['no_acoustid'] else 'use acoustid',
'in place' if options['in_place'] else 'copy music files',
])))
if len(filtered['new']) == 0:
@@ -201,4 +193,4 @@ class Command(BaseCommand):
job.save()
import_handler(
import_job_id=job.pk,
- use_acoustid=not options['no_acoustid'])
+ use_acoustid=False)
diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py
index c5839432..26cb9453 100644
--- a/api/tests/music/test_tasks.py
+++ b/api/tests/music/test_tasks.py
@@ -105,7 +105,7 @@ def test_run_import_skipping_accoustid(factories, mocker):
def test__do_import_skipping_accoustid(factories, mocker):
t = factories['music.Track']()
m = mocker.patch(
- 'funkwhale_api.music.tasks.import_track_data_from_path',
+ 'funkwhale_api.providers.audiofile.tasks.import_track_data_from_path',
return_value=t)
path = os.path.join(DATA_DIR, 'test.ogg')
job = factories['music.FileImportJob'](
@@ -121,7 +121,7 @@ def test__do_import_skipping_accoustid_if_no_key(
preferences['providers_acoustid__api_key'] = ''
t = factories['music.Track']()
m = mocker.patch(
- 'funkwhale_api.music.tasks.import_track_data_from_path',
+ 'funkwhale_api.providers.audiofile.tasks.import_track_data_from_path',
return_value=t)
path = os.path.join(DATA_DIR, 'test.ogg')
job = factories['music.FileImportJob'](
@@ -132,32 +132,14 @@ def test__do_import_skipping_accoustid_if_no_key(
m.assert_called_once_with(p)
-def test_import_job_can_be_skipped(
- artists, albums, tracks, factories, mocker, preferences):
- preferences['providers_acoustid__api_key'] = 'test'
+def test_import_job_skip_if_already_exists(
+ artists, albums, tracks, factories, mocker):
path = os.path.join(DATA_DIR, 'test.ogg')
mbid = '9968a9d6-8d92-4051-8f76-674e157b6eed'
track_file = factories['music.TrackFile'](track__mbid=mbid)
- acoustid_payload = {
- 'results': [
- {'id': 'e475bf79-c1ce-4441-bed7-1e33f226c0a2',
- 'recordings': [
- {
- 'duration': 268,
- 'id': mbid}],
- 'score': 0.860825}],
- 'status': 'ok'
- }
- mocker.patch(
- 'funkwhale_api.musicbrainz.api.artists.get',
- return_value=artists['get']['adhesive_wombat'])
mocker.patch(
- 'funkwhale_api.musicbrainz.api.releases.get',
- return_value=albums['get']['marsupial'])
- mocker.patch(
- 'funkwhale_api.musicbrainz.api.recordings.search',
- return_value=tracks['search']['8bitadventures'])
- mocker.patch('acoustid.match', return_value=acoustid_payload)
+ 'funkwhale_api.providers.audiofile.tasks.import_track_data_from_path',
+ return_value=track_file.track)
job = factories['music.FileImportJob'](audio_file__path=path)
f = job.audio_file
@@ -171,29 +153,23 @@ def test_import_job_can_be_skipped(
def test_import_job_can_be_errored(factories, mocker, preferences):
- preferences['providers_acoustid__api_key'] = 'test'
path = os.path.join(DATA_DIR, 'test.ogg')
mbid = '9968a9d6-8d92-4051-8f76-674e157b6eed'
track_file = factories['music.TrackFile'](track__mbid=mbid)
- acoustid_payload = {
- 'results': [
- {'id': 'e475bf79-c1ce-4441-bed7-1e33f226c0a2',
- 'recordings': [
- {
- 'duration': 268,
- 'id': mbid}],
- 'score': 0.860825}],
- 'status': 'ok'
- }
+
class MyException(Exception):
pass
- mocker.patch('acoustid.match', side_effect=MyException())
+
+ mocker.patch(
+ 'funkwhale_api.music.tasks._do_import',
+ side_effect=MyException())
job = factories['music.FileImportJob'](
audio_file__path=path, track_file=None)
with pytest.raises(MyException):
tasks.import_job_run(import_job_id=job.pk)
+
job.refresh_from_db()
assert job.track_file is None
diff --git a/api/tests/test_import_audio_file.py b/api/tests/test_import_audio_file.py
index 8217ffa0..de818607 100644
--- a/api/tests/test_import_audio_file.py
+++ b/api/tests/test_import_audio_file.py
@@ -1,5 +1,4 @@
import pytest
-import acoustid
import datetime
import os
import uuid
@@ -17,8 +16,6 @@ DATA_DIR = os.path.join(
def test_can_create_track_from_file_metadata(db, mocker):
- mocker.patch(
- 'acoustid.match', side_effect=acoustid.WebServiceError('test'))
metadata = {
'artist': ['Test artist'],
'album': ['Test album'],
@@ -94,24 +91,6 @@ def test_import_files_creates_a_batch_and_job(factories, mocker):
assert job.audio_file.read() == f.read()
assert job.source == 'file://' + path
- m.assert_called_once_with(
- import_job_id=job.pk,
- use_acoustid=True)
-
-
-def test_import_files_skip_acoustid(factories, mocker):
- m = mocker.patch('funkwhale_api.music.tasks.import_job_run')
- user = factories['users.User'](username='me')
- path = os.path.join(DATA_DIR, 'dummy_file.ogg')
- call_command(
- 'import_files',
- path,
- username='me',
- async=False,
- no_acoustid=True,
- interactive=False)
- batch = user.imports.latest('id')
- job = batch.jobs.first()
m.assert_called_once_with(
import_job_id=job.pk,
use_acoustid=False)
@@ -128,7 +107,6 @@ def test_import_files_skip_if_path_already_imported(factories, mocker):
path,
username='me',
async=False,
- no_acoustid=True,
interactive=False)
assert user.imports.count() == 0
@@ -142,7 +120,6 @@ def test_import_files_works_with_utf8_file_name(factories, mocker):
path,
username='me',
async=False,
- no_acoustid=True,
interactive=False)
batch = user.imports.latest('id')
job = batch.jobs.first()
@@ -162,7 +139,6 @@ def test_import_files_in_place(factories, mocker, settings):
username='me',
async=False,
in_place=True,
- no_acoustid=True,
interactive=False)
batch = user.imports.latest('id')
job = batch.jobs.first()
diff --git a/changes/changelog.d/acoustid.misc b/changes/changelog.d/acoustid.misc
new file mode 100644
index 00000000..7fc34feb
--- /dev/null
+++ b/changes/changelog.d/acoustid.misc
@@ -0,0 +1 @@
+Removed acoustid support, as the integration was buggy and error-prone (#106)
diff --git a/docs/importing-music.rst b/docs/importing-music.rst
index 97dd1385..4d256604 100644
--- a/docs/importing-music.rst
+++ b/docs/importing-music.rst
@@ -6,7 +6,8 @@ From music directory on the server
You can import music files in funkwhale assuming they are located on the server
and readable by the funkwhale application. Your music files should contain at
-least an ``artist``, ``album`` and ``title`` tags.
+least an ``artist``, ``album`` and ``title`` tags, but we recommend you tag
+it extensively using a proper tool, such as Beets or Musicbrainz Picard.
You can import those tracks as follows, assuming they are located in
``/srv/funkwhale/data/music``:
@@ -32,11 +33,6 @@ get details::
For the best results, we recommand tagging your music collection through
`Picard <http://picard.musicbrainz.org/>`_ in order to have the best quality metadata.
-.. note::
-
- Autotagging using acoustid is experimental now and can yield unexpected
- result. You can disable acoustid by passing the --no-acoustid flag.
-
.. note::
This command is idempotent, meaning you can run it multiple times on the same
@@ -44,7 +40,7 @@ get details::
.. note::
- At the moment, only OGG/Vorbis and MP3 files with ID3 tags are supported
+ At the moment, only Flac, OGG/Vorbis and MP3 files with ID3 tags are supported
.. _in-place-import:
diff --git a/front/src/views/admin/Settings.vue b/front/src/views/admin/Settings.vue
index 7174ab51..81eb97aa 100644
--- a/front/src/views/admin/Settings.vue
+++ b/front/src/views/admin/Settings.vue
@@ -93,8 +93,7 @@ export default {
label: this.$t('Imports'),
id: 'imports',
settings: [
- 'providers_youtube__api_key',
- 'providers_acoustid__api_key'
+ 'providers_youtube__api_key'
]
},
{
--
GitLab