diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 34345e47b49e9f44f41bb341ebdb059ff5185464..e5426904a7f2b16bca519f37c1e30a0a2d5d4489 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 a2757c692bd4607b0779ba112e4768a5881d8770..70ff90ffac349858a0f239eecf6b42373d2d9a3c 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 c5839432bf128a3a03387bb2894d8ad56615594b..26cb9453efacca6137f412bdd03c8df570f41a02 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 8217ffa0b27a26ac99183314f87d69feced35e7d..de81860754a7da6c667ef05302b08ff7310ee6f8 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/106.bugfix b/changes/changelog.d/106.bugfix new file mode 100644 index 0000000000000000000000000000000000000000..ff0f61609c9dc540fa835cda34516dc2ad137850 --- /dev/null +++ b/changes/changelog.d/106.bugfix @@ -0,0 +1 @@ +File-upload importer should now work properly, assuming files are tagged (#106) diff --git a/changes/changelog.d/213.bugfix b/changes/changelog.d/213.bugfix new file mode 100644 index 0000000000000000000000000000000000000000..d6ff593b98a221aed01fcdf83ff599bcb57b3c32 --- /dev/null +++ b/changes/changelog.d/213.bugfix @@ -0,0 +1,9 @@ +File-upload import now supports Flac files (#213) + +Flac files imports via upload +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +You have nothing to do to benefit from this, however, since Flac files +tend to be a lot bigger than other files, you may want to increase the +``client_max_body_size`` value in your Nginx configuration if you plan +to upload flac files. diff --git a/changes/changelog.d/acoustid.misc b/changes/changelog.d/acoustid.misc new file mode 100644 index 0000000000000000000000000000000000000000..7fc34febed09e672c522f7f4aaef043b01497b76 --- /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/docker/nginx/conf.dev b/docker/nginx/conf.dev index ab6714e60e1b6fec7a606cc6e820e65127af7346..673edd1a4b4422f94e12cd4886dc5296e8760ed7 100644 --- a/docker/nginx/conf.dev +++ b/docker/nginx/conf.dev @@ -36,7 +36,7 @@ http { server { listen 6001; charset utf-8; - client_max_body_size 20M; + client_max_body_size 30M; include /etc/nginx/funkwhale_proxy.conf; location /_protected/media { internal; diff --git a/docs/importing-music.rst b/docs/importing-music.rst index 97dd1385485c18eab10fe805e49ff829140d3654..4d256604b522190941a8bed69a0c507d6bf05f23 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/components/library/import/FileUpload.vue b/front/src/components/library/import/FileUpload.vue index 9a4b820e3fddac7a624999e49b64ed91ed84f777..7aa8adac0f00f72c9724603095977b6797aa4ea9 100644 --- a/front/src/components/library/import/FileUpload.vue +++ b/front/src/components/library/import/FileUpload.vue @@ -1,6 +1,10 @@ <template> <div> <div v-if="batch" class="ui container"> + <div class="ui message"> + {{ $t('Ensure your music files are properly tagged before uploading them.') }} + <a href="http://picard.musicbrainz.org/" target='_blank'>{{ $t('We recommend using Picard for that purpose.') }}</a> + </div> <file-upload-widget :class="['ui', 'icon', 'left', 'floated', 'button']" :post-action="uploadUrl" @@ -8,7 +12,7 @@ :size="1024 * 1024 * 30" :data="uploadData" :drop="true" - extensions="ogg,mp3" + extensions="ogg,mp3,flac" accept="audio/*" v-model="files" name="audio_file" @@ -21,7 +25,7 @@ </file-upload-widget> <button :class="['ui', 'right', 'floated', 'icon', {disabled: files.length === 0}, 'button']" - v-if="!$refs.upload || !$refs.upload.active" @click.prevent="$refs.upload.active = true"> + v-if="!$refs.upload || !$refs.upload.active" @click.prevent="startUpload()"> <i class="play icon" aria-hidden="true"></i> <i18next path="Start Upload"/> </button> @@ -88,7 +92,7 @@ export default { inputFilter (newFile, oldFile, prevent) { if (newFile && !oldFile) { let extension = newFile.name.split('.').pop() - if (['ogg', 'mp3'].indexOf(extension) < 0) { + if (['ogg', 'mp3', 'flac'].indexOf(extension) < 0) { prevent() } } @@ -114,6 +118,10 @@ export default { }, (response) => { logger.default.error('error while launching creating batch') }) + }, + startUpload () { + this.$emit('batch-created', this.batch) + this.$refs.upload.active = true } }, computed: { diff --git a/front/src/components/library/import/Main.vue b/front/src/components/library/import/Main.vue index de17e2afadaf655377896a029f01e4ab9945eefb..eac1239a836a60d4393545253db392dc2b9ac12b 100644 --- a/front/src/components/library/import/Main.vue +++ b/front/src/components/library/import/Main.vue @@ -24,16 +24,25 @@ <div class="ui hidden divider"></div> <div class="ui centered buttons"> <button @click="currentStep -= 1" :disabled="currentStep === 0" class="ui icon button"><i class="left arrow icon"></i><i18next path="Previous step"/></button> - <button @click="currentStep += 1" v-if="currentStep < 2" class="ui icon button"><i18next path="Next step"/><i class="right arrow icon"></i></button> + <button @click="nextStep()" v-if="currentStep < 2" class="ui icon button"><i18next path="Next step"/><i class="right arrow icon"></i></button> <button @click="$refs.import.launchImport()" - v-if="currentStep === 2" + v-if="currentStep === 2 && currentSource != 'upload'" :class="['ui', 'positive', 'icon', {'loading': isImporting}, 'button']" :disabled="isImporting || importData.count === 0" > <i18next path="Import {%0%} tracks">{{ importData.count }}</i18next> <i class="check icon"></i> </button> + <button + v-else-if="currentStep === 2 && currentSource === 'upload'" + @click="$router.push({name: 'library.import.batches.detail', params: {id: importBatch.id}})" + :class="['ui', 'positive', 'icon', {'disabled': !importBatch}, 'button']" + :disabled="!importBatch" + > + {{ $t('Finish import' )}} + <i class="check icon"></i> + </button> </div> <div class="ui hidden divider"></div> <div class="ui attached segment"> @@ -100,6 +109,7 @@ <div v-if="currentStep === 2"> <file-upload ref="import" + @batch-created="updateBatch" v-if="currentSource == 'upload'" ></file-upload> @@ -165,6 +175,7 @@ export default { currentSource: this.source, metadata: {}, isImporting: false, + importBatch: null, importData: { tracks: [] }, @@ -214,11 +225,22 @@ export default { updateId (newValue) { this.currentId = newValue }, + updateBatch (batch) { + this.importBatch = batch + }, fetchRequest (id) { let self = this axios.get(`requests/import-requests/${id}`).then((response) => { self.currentRequest = response.data }) + }, + nextStep () { + if (this.currentStep === 0 && this.currentSource === 'upload') { + // we skip metadata directly + this.currentStep += 2 + } else { + this.currentStep += 1 + } } }, computed: { diff --git a/front/src/views/admin/Settings.vue b/front/src/views/admin/Settings.vue index 7174ab516c1e438d0aa09b0491e803b26f5f4316..81eb97aa6ec303b3a3c411a0022aa7a49c1ade30 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' ] }, {