diff --git a/swh/deposit/api/deposit_status.py b/swh/deposit/api/deposit_status.py --- a/swh/deposit/api/deposit_status.py +++ b/swh/deposit/api/deposit_status.py @@ -1,4 +1,4 @@ -# Copyright (C) 2017 The Software Heritage developers +# Copyright (C) 2017-2018 The Software Heritage developers # See the AUTHORS file at the top-level directory of this distribution # License: GNU General Public License version 3, or any later version # See top-level LICENSE file for more information @@ -12,6 +12,58 @@ from ..models import DEPOSIT_STATUS_DETAIL, Deposit +def convert_status_detail(status_detail): + """Given a status_detail dict, transforms it into a human readable + string. + + Dict has the following form (all first level keys are optional): + { + 'url': { + 'summary': , + 'fields': + }, + 'metadata': [{ + 'summary': , + 'fields': , + }], + 'archive': { + 'summary': , + 'fields': [], + } + + + } + + Args: + status_detail (dict): + + Returns: + Status detail as inlined string. + + """ + if not status_detail: + return None + + msg = [] + if 'metadata' in status_detail: + for data in status_detail['metadata']: + fields = ', '.join(data['fields']) + msg.append('- %s (%s)\n' % (data['summary'], fields)) + + for key in ['url', 'archive']: + if key in status_detail: + _detail = status_detail[key] + fields = _detail.get('fields') + suffix_msg = '' + if fields: + suffix_msg = ' (%s)' % ', '.join(fields) + msg.append('- %s%s\n' % (_detail['summary'], suffix_msg)) + + if not msg: + return None + return ''.join(msg) + + class SWHDepositStatus(SWHBaseDeposit): """Deposit status. @@ -35,10 +87,14 @@ 'deposit %s does not belong to collection %s' % ( deposit_id, collection_name)) + status_detail = convert_status_detail(deposit.status_detail) + if not status_detail: + status_detail = DEPOSIT_STATUS_DETAIL[deposit.status] + context = { 'deposit_id': deposit.id, 'status': deposit.status, - 'status_detail': DEPOSIT_STATUS_DETAIL[deposit.status], + 'status_detail': status_detail, 'swh_id': None, } diff --git a/swh/deposit/api/private/deposit_check.py b/swh/deposit/api/private/deposit_check.py --- a/swh/deposit/api/private/deposit_check.py +++ b/swh/deposit/api/private/deposit_check.py @@ -15,6 +15,14 @@ from ...models import Deposit, DepositRequest +MANDATORY_FIELDS_MISSING = 'Mandatory fields are missing' +ALTERNATE_FIELDS_MISSING = 'Mandatory alternate fields are missing' + +MANDATORY_ARCHIVE_UNREADABLE = 'Deposit was rejected because at least one of its associated archives was not readable' # noqa +MANDATORY_ARCHIVE_MISSING = 'Deposit without archive is rejected' +INCOMPATIBLE_URL_FIELDS = "At least one url field must be compatible with the client's domain name" # noqa + + class SWHChecksDeposit(SWHGetDepositAPI, SWHPrivateAPIView): """Dedicated class to read a deposit's raw archives content. @@ -55,8 +63,7 @@ if len(requests) == 0: # no associated archive is refused return False, { 'archive': { - 'summary': 'Deposit without archive is rejected.', - 'id': deposit.id, + 'summary': MANDATORY_ARCHIVE_MISSING, } } @@ -70,10 +77,8 @@ if rejected_dr_ids: return False, { 'archive': { - 'summary': 'Following deposit request ids are rejected ' - 'because their associated archive is not ' - 'readable', - 'ids': rejected_dr_ids, + 'summary': MANDATORY_ARCHIVE_UNREADABLE, + 'fields': rejected_dr_ids, }} return True, None @@ -144,19 +149,19 @@ mandatory_result = [k for k, v in required_fields.items() if not v] optional_result = [ - k for k, v in alternate_fields.items() if not v] + ' or '.join(k) for k, v in alternate_fields.items() if not v] if mandatory_result == [] and optional_result == []: return True, None detail = [] if mandatory_result != []: detail.append({ - 'summary': 'Mandatory fields are missing', + 'summary': MANDATORY_FIELDS_MISSING, 'fields': mandatory_result }) if optional_result != []: detail.append({ - 'summary': 'Mandatory alternate fields are missing', + 'summary': ALTERNATE_FIELDS_MISSING, 'fields': optional_result, }) return False, { @@ -178,17 +183,19 @@ """ url_fields = [] for field in metadata: - url_fields.append(field) - if 'url' in field and client_domain in metadata[field]: - return True, None + if 'url' in field: + if client_domain in metadata[field]: + return True, None + url_fields.append(field) - return False, { + detail = { 'url': { - 'summary': "At least one url field must be compatible with the" - "client's domain name. The following url fields " - "failed the check.", - 'fields': url_fields, - }} + 'summary': INCOMPATIBLE_URL_FIELDS, + } + } + if url_fields: + detail['url']['fields'] = url_fields + return False, detail def process_get(self, req, collection_name, deposit_id): """Build a unique tarball from the multiple received and stream that diff --git a/swh/deposit/tests/api/test_deposit_check.py b/swh/deposit/tests/api/test_deposit_check.py --- a/swh/deposit/tests/api/test_deposit_check.py +++ b/swh/deposit/tests/api/test_deposit_check.py @@ -12,12 +12,19 @@ from rest_framework import status from rest_framework.test import APITestCase -from ...models import Deposit -from ...config import DEPOSIT_STATUS_VERIFIED, PRIVATE_CHECK_DEPOSIT -from ...config import DEPOSIT_STATUS_DEPOSITED, DEPOSIT_STATUS_REJECTED +from swh.deposit.config import ( + DEPOSIT_STATUS_VERIFIED, PRIVATE_CHECK_DEPOSIT, + DEPOSIT_STATUS_DEPOSITED, DEPOSIT_STATUS_REJECTED +) +from swh.deposit.api.private.deposit_check import ( + SWHChecksDeposit, + MANDATORY_FIELDS_MISSING, INCOMPATIBLE_URL_FIELDS, + MANDATORY_ARCHIVE_UNREADABLE, ALTERNATE_FIELDS_MISSING +) +from swh.deposit.models import Deposit + from ..common import BasicTestCase, WithAuthTestCase, CommonCreationRoutine from ..common import FileSystemCreationRoutine -from ...api.private.deposit_check import SWHChecksDeposit @attr('fs') @@ -71,40 +78,22 @@ self.assertEqual(response.status_code, status.HTTP_200_OK) data = json.loads(response.content.decode('utf-8')) self.assertEqual(data['status'], DEPOSIT_STATUS_REJECTED) - expected_error = { - 'metadata': [ - { - 'fields': ['url', 'external_identifier', 'author'], - 'summary': 'Mandatory fields are missing' - }, - { - 'fields': [['name', 'title']], - 'summary': 'Mandatory alternate fields are missing' - }], - } details = data['details'] # archive checks failure - self.assertEqual(len(details['archive']['ids']), 1) + self.assertEqual(len(details['archive']['fields']), 1) self.assertEqual(details['archive']['summary'], - 'Following deposit request ids are ' - 'rejected because their associated archive' - ' is not readable') + MANDATORY_ARCHIVE_UNREADABLE) # metadata check failure self.assertEqual(len(details['metadata']), 2) mandatory = details['metadata'][0] - self.assertEqual(mandatory['summary'], 'Mandatory fields are missing') + self.assertEqual(mandatory['summary'], MANDATORY_FIELDS_MISSING) self.assertEqual(set(mandatory['fields']), set(['url', 'external_identifier', 'author'])) alternate = details['metadata'][1] - self.assertEqual(alternate['summary'], - 'Mandatory alternate fields are missing') - self.assertEqual(alternate['fields'], [['name', 'title']]) + self.assertEqual(alternate['summary'], ALTERNATE_FIELDS_MISSING) + self.assertEqual(alternate['fields'], ['name or title']) # url check failure - self.assertEqual(details['url']['summary'], - "At least one url field must be compatible with the" - "client's domain name. The following url fields " - "failed the check.") - self.assertEqual(details['url']['fields'], []) + self.assertEqual(details['url']['summary'], INCOMPATIBLE_URL_FIELDS) deposit = Deposit.objects.get(pk=deposit.id) self.assertEquals(deposit.status, DEPOSIT_STATUS_REJECTED) @@ -175,7 +164,7 @@ expected_error = { 'metadata': [{ 'summary': 'Mandatory alternate fields are missing', - 'fields': [('name', 'title')], + 'fields': ['name or title'], }] } self.assertFalse(actual_check) diff --git a/swh/deposit/tests/api/test_deposit_status.py b/swh/deposit/tests/api/test_deposit_status.py --- a/swh/deposit/tests/api/test_deposit_status.py +++ b/swh/deposit/tests/api/test_deposit_status.py @@ -10,6 +10,7 @@ from rest_framework import status from rest_framework.test import APITestCase +from swh.deposit.api.deposit_status import convert_status_detail from swh.deposit.models import Deposit, DEPOSIT_STATUS_DETAIL from swh.deposit.models import DEPOSIT_STATUS_LOAD_SUCCESS from swh.deposit.parsers import parse_xml @@ -112,3 +113,86 @@ HTTP_ACCEPT='text/html,application/xml;q=9,*/*,q=8') self.assertEqual(response.status_code, status.HTTP_200_OK) + + @istest + def convert_status_detail_empty(self): + actual_status_detail = convert_status_detail({}) + self.assertIsNone(actual_status_detail) + + actual_status_detail = convert_status_detail({'dummy-keys': []}) + self.assertIsNone(actual_status_detail) + + actual_status_detail = convert_status_detail(None) + self.assertIsNone(actual_status_detail) + + @istest + def convert_status_detail(self): + status_detail = { + 'url': { + 'summary': "At least one url field must be compatible with the client\'s domain name. The following url fields failed the check", # noqa + 'fields': ['blahurl', 'testurl'], + }, + 'metadata': [ + { + 'summary': 'Mandatory fields missing', + 'fields': ['url', 'title'], + }, + { + 'summary': 'Alternate fields missing', + 'fields': ['name or title', 'url or badurl'] + } + ], + 'archive': { + 'summary': 'Unreadable archive', + 'fields': ['1', '2'], + }, + } + + expected_status_detail = '''- Mandatory fields missing (url, title) +- Alternate fields missing (name or title, url or badurl) +- At least one url field must be compatible with the client's domain name. The following url fields failed the check (blahurl, testurl) +- Unreadable archive (1, 2) +''' # noqa + + actual_status_detail = convert_status_detail(status_detail) + + self.assertEqual(actual_status_detail, expected_status_detail) + + @istest + def convert_status_detail_2(self): + status_detail = { + 'url': { + 'summary': 'At least one compatible url field. Failed', + 'fields': ['testurl'], + }, + 'metadata': [ + { + 'summary': 'Mandatory fields missing', + 'fields': ['name'], + }, + ], + } + + expected_status_detail = '''- Mandatory fields missing (name) +- At least one compatible url field. Failed (testurl) +''' + + actual_status_detail = convert_status_detail(status_detail) + + self.assertEqual(actual_status_detail, expected_status_detail) + + @istest + def convert_status_detail_3(self): + status_detail = { + 'url': { + 'summary': 'At least one compatible url field', + }, + } + + expected_status_detail = '- At least one compatible url field\n' + actual_status_detail = convert_status_detail(status_detail) + self.assertEqual(actual_status_detail, expected_status_detail) + + @istest + def status_on_deposit_rejected(self): + pass