From dd83a91bbc7ad30528beff3ae0f5476e8ba4f154 Mon Sep 17 00:00:00 2001 From: Aly Badr Date: Wed, 6 Oct 2021 10:22:11 +0200 Subject: [PATCH] templates: suppress links from the data section * Removes all fields from the template data dictionary that are linked to other records or that causes issues with the circulation module and with data integrity at the creation of the templates. * Closes #2385. * Fixes problem when librarian can be deleted even if they own templates. Use the following command to clean up the current templates saved in database: The current templates will be backed-up in this file `templates.json` ``` poetry run tools.py tools migration clean_templates -o templates.json ``` Co-Authored-by: Aly Badr --- rero_ils/modules/patrons/api.py | 13 +++- rero_ils/modules/templates/api.py | 4 +- rero_ils/modules/templates/extensions.py | 23 +++++- tests/api/templates/test_templates_rest.py | 85 +++++++++++++++++++++- tests/data/data.json | 6 +- tests/fixtures/metadata.py | 20 +++++ 6 files changed, 141 insertions(+), 10 deletions(-) diff --git a/rero_ils/modules/patrons/api.py b/rero_ils/modules/patrons/api.py index 3bb8bc008e..513cf914f0 100644 --- a/rero_ils/modules/patrons/api.py +++ b/rero_ils/modules/patrons/api.py @@ -38,6 +38,7 @@ from ..organisations.api import Organisation from ..patron_transactions.api import PatronTransaction from ..providers import Provider +from ..templates.api import TemplatesSearch from ..users.api import User from ..utils import extracted_data_from_ref, get_patron_from_arguments, \ get_ref_for_pid, sorted_pids, trim_patron_barcode_for_record @@ -486,21 +487,27 @@ def get_links_to_me(self, get_pids=False): links = {} exclude_states = [LoanState.CANCELLED, LoanState.ITEM_RETURNED, LoanState.CREATED] - query = current_circulation.loan_search_cls()\ + loan_query = current_circulation.loan_search_cls()\ .filter('term', patron_pid=self.pid)\ .exclude('terms', state=exclude_states) + template_query = TemplatesSearch()\ + .filter('term', creator__pid=self.pid) if get_pids: - loans = sorted_pids(query) + loans = sorted_pids(loan_query) transactions = PatronTransaction \ .get_transactions_pids_for_patron(self.pid, status='open') + templates = sorted_pids(template_query) else: - loans = query.count() + loans = loan_query.count() transactions = PatronTransaction \ .get_transactions_count_for_patron(self.pid, status='open') + templates = template_query.count() if loans: links['loans'] = loans if transactions: links['transactions'] = transactions + if templates: + links['templates'] = templates return links def reasons_to_keep(self): diff --git a/rero_ils/modules/templates/api.py b/rero_ils/modules/templates/api.py index c40f6f7347..6f5eb4108b 100644 --- a/rero_ils/modules/templates/api.py +++ b/rero_ils/modules/templates/api.py @@ -19,7 +19,7 @@ """API for manipulating Templates.""" from functools import partial -from .extensions import RemoveDataPidExtension +from .extensions import CleanDataDictExtension from .models import TemplateIdentifier, TemplateMetadata from ..api import IlsRecord, IlsRecordsIndexer, IlsRecordsSearch from ..fetchers import id_fetcher @@ -56,7 +56,7 @@ class Meta: class Template(IlsRecord): """Templates class.""" - _extensions = [RemoveDataPidExtension()] + _extensions = [CleanDataDictExtension()] minter = template_id_minter fetcher = template_id_fetcher diff --git a/rero_ils/modules/templates/extensions.py b/rero_ils/modules/templates/extensions.py index 9dd13a82db..7c0bd49a22 100644 --- a/rero_ils/modules/templates/extensions.py +++ b/rero_ils/modules/templates/extensions.py @@ -20,14 +20,31 @@ from invenio_records.extensions import RecordExtension -class RemoveDataPidExtension(RecordExtension): +class CleanDataDictExtension(RecordExtension): """Defines the methods needed by an extension.""" def post_init(self, record, data, model=None, **kwargs): """Called after a record is initialized. + Removes fields that can have a link to other records in the database. + :param data: The dict passed to the record's constructor :param model: The model class used for initialization. """ - # force removing of record pid - record.get('data', {}).pop('pid', None) + fields = ['pid'] + if record.get('template_type') == 'items': + fields = fields + [ + 'barcode', 'status', 'document', 'holding', 'organisation', + 'library'] + elif record.get('template_type') == 'holdings': + fields = fields + ['organisation', 'library', 'document'] + elif record.get('template_type') == 'patrons': + fields = fields + [ + 'user_id', 'patron.subscriptions', 'patron.barcode'] + + for field in fields: + if '.' in field: + level_1, level_2 = field.split('.') + record.get('data', {}).get(level_1, {}).pop(level_2, None) + else: + record.get('data', {}).pop(field, None) diff --git a/tests/api/templates/test_templates_rest.py b/tests/api/templates/test_templates_rest.py index 7bc63035a1..773ae915e8 100644 --- a/tests/api/templates/test_templates_rest.py +++ b/tests/api/templates/test_templates_rest.py @@ -25,6 +25,8 @@ from utils import VerifyRecordPermissionPatch, get_json, postdata, \ to_relative_url +from rero_ils.modules.utils import get_ref_for_pid + @mock.patch('invenio_records_rest.views.verify_record_permission', mock.MagicMock(return_value=VerifyRecordPermissionPatch)) @@ -200,12 +202,16 @@ def test_template_secure_api(client, json_header, def test_template_secure_api_create(client, json_header, system_librarian_martigny, system_librarian_sion, - templ_doc_public_martigny_data): + templ_doc_public_martigny_data, + templ_item_public_martigny, + templ_hold_public_martigny, + templ_patron_public_martigny): """Test templates secure api create.""" # Martigny login_user_via_session(client, system_librarian_martigny.user) post_entrypoint = 'invenio_records_rest.tmpl_list' + # test template creation for documents del templ_doc_public_martigny_data['pid'] # add a pid to the record data templ_doc_public_martigny_data['data']['pid'] = 'toto' @@ -218,6 +224,83 @@ def test_template_secure_api_create(client, json_header, # ensure that pid is removed from recordds assert 'pid' not in res.json['metadata']['data'] + # test template creation for items + del templ_item_public_martigny['pid'] + # add fields that will be removed at the creation of the template. + templ_item_public_martigny['data']['pid'] = 'toto' + templ_item_public_martigny['data']['barcode'] = 'toto' + templ_item_public_martigny['data']['status'] = 'on_loan' + templ_item_public_martigny['data']['library'] = \ + {'$ref': get_ref_for_pid('lib', 'toto')} + templ_item_public_martigny['data']['document'] = \ + {'$ref': get_ref_for_pid('doc', 'toto')} + templ_item_public_martigny['data']['holding'] = \ + {'$ref': get_ref_for_pid('hold', 'toto')} + templ_item_public_martigny['data']['organisation'] = \ + {'$ref': get_ref_for_pid('org', 'toto')} + + res, _ = postdata( + client, + post_entrypoint, + templ_item_public_martigny + ) + assert res.status_code == 201 + # ensure that added fields are removed from record. + fields = [ + 'barcode', 'pid', 'status', 'document', 'holding', 'organisation', + 'library'] + for field in fields: + assert field not in res.json['metadata']['data'] + + # templates now prevent the deletion of its owner + assert system_librarian_martigny.get_links_to_me().get('templates') + + # test template creation for holdings + del templ_hold_public_martigny['pid'] + # add fields that will be removed at the creation of the template. + templ_hold_public_martigny['data']['pid'] = 'toto' + templ_hold_public_martigny['data']['organisation'] = \ + {'$ref': get_ref_for_pid('org', 'toto')} + templ_hold_public_martigny['data']['library'] = \ + {'$ref': get_ref_for_pid('lib', 'toto')} + templ_hold_public_martigny['data']['document'] = \ + {'$ref': get_ref_for_pid('doc', 'toto')} + + res, _ = postdata( + client, + post_entrypoint, + templ_hold_public_martigny + ) + assert res.status_code == 201 + # ensure that added fields are removed from record. + fields = ['organisation', 'library', 'document', 'pid'] + for field in fields: + assert field not in res.json['metadata']['data'] + + # test template creation for patrons + del templ_patron_public_martigny['pid'] + # add fields that will be removed at the creation of the template. + templ_patron_public_martigny['data']['pid'] = 'toto' + templ_patron_public_martigny['data']['user_id'] = 'toto' + templ_patron_public_martigny['data']['patron']['subscriptions'] = 'toto' + templ_patron_public_martigny['data']['patron']['barcode'] = ['toto'] + + res, _ = postdata( + client, + post_entrypoint, + templ_patron_public_martigny + ) + assert res.status_code == 201 + # ensure that added fields are removed from record. + fields = ['user_id', 'patron.subscriptions', 'patron.barcode', 'pid'] + json_data = res.json['metadata']['data'] + for field in fields: + if '.' in field: + level_1, level_2 = field.split('.') + assert level_2 not in json_data.get(level_1) + else: + assert field not in json_data + # Sion login_user_via_session(client, system_librarian_sion.user) diff --git a/tests/data/data.json b/tests/data/data.json index 7f06b0d1b9..ccd713b16c 100644 --- a/tests/data/data.json +++ b/tests/data/data.json @@ -3825,7 +3825,11 @@ }, "visibility": "public", "template_type": "patrons", - "data": {} + "data": { + "patron": { + "source": "imported" + } + } }, "illr1": { "$schema": "https://bib.rero.ch/schemas/ill_requests/ill_request-v0.0.1.json", diff --git a/tests/fixtures/metadata.py b/tests/fixtures/metadata.py index 5fe26f1525..bae616fcc9 100644 --- a/tests/fixtures/metadata.py +++ b/tests/fixtures/metadata.py @@ -1000,6 +1000,26 @@ def templ_item_public_martigny( return template +@pytest.fixture(scope="module") +def templ_hold_public_martigny_data(data): + """Load template for a public holding martigny data.""" + return deepcopy(data.get('tmpl4')) + + +@pytest.fixture(scope="module") +def templ_hold_public_martigny( + app, org_martigny, templ_hold_public_martigny_data, + system_librarian_martigny): + """Load template for a public holding martigny.""" + template = Template.create( + data=templ_hold_public_martigny_data, + delete_pid=False, + dbcommit=True, + reindex=True) + flush_index(TemplatesSearch.Meta.index) + return template + + @pytest.fixture(scope="module") def templ_patron_public_martigny_data(data): """Load template for a public patron martigny data."""