diff --git a/cds_migrator_kit/rdm/records/transform/transform.py b/cds_migrator_kit/rdm/records/transform/transform.py index 1750b597..6b91988e 100644 --- a/cds_migrator_kit/rdm/records/transform/transform.py +++ b/cds_migrator_kit/rdm/records/transform/transform.py @@ -14,11 +14,14 @@ import arrow import yaml +from cds_dojson.marc21.utils import create_record from cds_rdm.legacy.models import CDSMigrationAffiliationMapping +from cds_rdm.legacy.resolver import get_pid_by_legacy_recid from dateutil.parser import ParserError, parse from flask import current_app from idutils import normalize_ror from idutils.validators import is_doi, is_ror +from invenio_access.permissions import system_identity from invenio_accounts.models import User, UserIdentity from invenio_db import db from invenio_pidstore.models import PersistentIdentifier, PIDStatus @@ -26,6 +29,7 @@ RDMRecordEntry, RDMRecordTransform, ) +from invenio_rdm_records.proxies import current_rdm_records_service, current_record_communities_service from invenio_vocabularies.contrib.affiliations.models import AffiliationsMetadata from invenio_vocabularies.contrib.names.models import NamesMetadata from sqlalchemy.exc import NoResultFound @@ -622,7 +626,7 @@ def field_departments(record_json, custom_fields_dict): value=department, field="department", message=f"Department {department} not found. " - f"Added as unit and subject", + f"Added as unit and subject", stage="vocabulary match", ) @@ -907,12 +911,12 @@ def _transform(self, entry): "_clc_sync": clc_sync, } except ( - LossyConversion, - RestrictedFileDetected, - UnexpectedValue, - ManualImportRequired, - MissingRequiredField, - MultipleModelsMatched, + LossyConversion, + RestrictedFileDetected, + UnexpectedValue, + ManualImportRequired, + MissingRequiredField, + MultipleModelsMatched, ) as e: migration_logger.add_log(e, record=entry) @@ -1014,7 +1018,7 @@ def compute_files(file_dump, versions_dict): { file_dump["full_name"]: { "eos_tmp_path": tmp_eos_root - / full_path.relative_to(legacy_path_root), + / full_path.relative_to(legacy_path_root), "id_bibdoc": file_dump["bibdocid"], "key": file_dump["full_name"], "metadata": { @@ -1100,17 +1104,77 @@ def _load_migrated_recids(self): def should_skip(self, entry): return str(entry["recid"]) in self._migrated_recids + def _existing_record_is_restricted(self, record_id): + """Check the current access state of an already-migrated RDM record. + + Reads live from the record service (DB) rather than the legacy + MARCXML, since access restrictions may have been changed in RDM + after migration and the legacy record data would be stale. + """ + record = current_rdm_records_service.read_latest( + system_identity, id_=record_id + ) + access = record.data.get("access", {}) + return access.get("record") != "public" or access.get("files") != "public" + def run(self, entries): """Run transformation step.""" self._migrated_recids = self._load_migrated_recids() - for entry in entries: if self.should_skip(entry): recid = entry["recid"] + try: + parent_pid = get_pid_by_legacy_recid(str(recid)) + # we don't check here if the record has 980:MIGRATED + # because this does not add anything and it should not be deciding factor. + # if the legacy recid has been minted - we know already the record has been migrated + if self._existing_record_is_restricted(parent_pid.pid_value): + # checking if we need to be more careful while assigning access + # to various communities + raise ManualImportRequired( + message=( + "Existing record is restricted or has " + "restricted files; not adding to " + "communities automatically" + ), + field="access", + stage="transform", + recid=recid, + priority="warning", + ) + # bulk_add resolves record_ids via RDMRecord.pid.resolve(), + # which -- unlike read_latest -- has no fallback for a + # parent-level recid (what get_pid_by_legacy_recid returns). + # We must pass the record's own recid instead. + record_item = current_rdm_records_service.read_latest( + system_identity, id_=parent_pid.pid_value + ) + for community_id in self.communities_ids: + current_record_communities_service.bulk_add( + system_identity, + community_id, + [record_item.id], + ) + except NoResultFound: + self.migration_logger.add_information( + recid, + { + "message": ( + "Problem with PIDs - minted legacy recid but" + "no corresponding parent record found." + ), + "value": recid, + }, + ) + except ManualImportRequired as exc: + self.migration_logger.add_log(exc, record=entry) + self.migration_logger.add_information( recid, { - "message": "Record already migrated, skipping", + "message": "Record already migrated, skipping," + " added existing {} to communities {}".format( + recid, self.communities_ids), "value": recid, }, ) diff --git a/cds_migrator_kit/rdm/streams.yaml b/cds_migrator_kit/rdm/streams.yaml index b8e3890e..b5af11d3 100644 --- a/cds_migrator_kit/rdm/streams.yaml +++ b/cds_migrator_kit/rdm/streams.yaml @@ -458,6 +458,160 @@ records: - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" load: legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_ab/duplicated_pids.json + ac_dep: + data_dir: cds_migrator_kit/rdm/data/dep_ac + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_ac/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_ac/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "dfebd855-d156-4428-9b08-fe8ab8171799" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_ab/duplicated_pids.json + ar_dep: + data_dir: cds_migrator_kit/rdm/data/dep_ar + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_ar/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_ar/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "2bf2998b-b310-4f36-9344-71e47560ae16" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_ar/duplicated_pids.json + isr_dep: + data_dir: cds_migrator_kit/rdm/data/dep_isr + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_isr/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_isr/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "c2fdb4ec-201c-4ec7-a9f0-23e4380a3fce" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_ab/duplicated_pids.json + lep_dep: + data_dir: cds_migrator_kit/rdm/data/dep_lep + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_lep/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_lep/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "f49a960d-94bf-432c-9ce6-c2fb9115ff9a" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_lep/duplicated_pids.json + ps_dep: + data_dir: cds_migrator_kit/rdm/data/dep_ps + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_ps/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_ps/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "5b634108-c93c-4740-af5c-31fb32e548eb" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_ps/duplicated_pids.json + sc_dep: + data_dir: cds_migrator_kit/rdm/data/dep_sc + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_sc/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_sc/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "369f4c3d-10fc-41e3-8cce-681fad5fcafe" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_sc/duplicated_pids.json + si_dep: + data_dir: cds_migrator_kit/rdm/data/dep_si + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_si/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_si/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "db44a69b-babc-48e8-9c03-29436b5bff07" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_si/duplicated_pids.json + sl_dep: + data_dir: cds_migrator_kit/rdm/data/dep_sl + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_sl/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_sl/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "bcc34ae6-3a02-44cd-b08d-316a9d3e27e6" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_sl/duplicated_pids.json + sps_dep: + data_dir: cds_migrator_kit/rdm/data/dep_sps + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/dep_sps/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/dep_sps/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "4bdb0cb3-b5f0-4383-a621-44b6e287438a" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/dep_sps/duplicated_pids.json + mps_div: + data_dir: cds_migrator_kit/rdm/data/div_mps + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/div_mps/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/div_mps/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "510206fb-2f80-44ee-8944-57f5dcfae5f0" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/div_mps/duplicated_pids.json + msc_div: + data_dir: cds_migrator_kit/rdm/data/div_msc + plots: true + create_inclusion_request: true + extract: + dirpath: cds_migrator_kit/rdm/data/div_msc/dump/ + transform: + files_dump_dir: cds_migrator_kit/rdm/data/div_msc/files/ + missing_users: cds_migrator_kit/rdm/data/users + communities_ids: + - "7bd98561-4985-434a-aed1-74301f865c24" + - "c2c46ab3-5fb4-4d86-83c6-5d9dc8392d6f" + load: + legacy_pids_to_redirect: cds_migrator_kit/rdm/data/div_msc/duplicated_pids.json comments: faser-drafts: dir_path: /migration/faser-drafts/comments/ diff --git a/setup.cfg b/setup.cfg index bc39231b..aedb5a9c 100644 --- a/setup.cfg +++ b/setup.cfg @@ -17,7 +17,7 @@ zip_safe = False install_requires = sentry-sdk>=1.45,<2.0.0 cds-dojson @ git+https://github.com/CERNDocumentServer/cds-dojson@master#egg=cds-dojson - invenio-rdm-migrator>=5.0.0 + invenio-rdm-migrator @ git+https://github.com/CERNDocumentServer/invenio-rdm-migrator@master#egg=invenio-rdm-migrator lxml>=4.6.5 ipython!=8.1.0 uwsgi>=2.0 @@ -32,11 +32,10 @@ install_requires = [options.extras_require] rdm = - invenio-app-rdm[opensearch2]==14.0.0b10.dev4 + invenio-app-rdm[opensearch2]>=v15.0.0b2.dev0 cds-rdm @ git+https://github.com/CERNDocumentServer/cds-rdm@master#egg=cds-rdm&subdirectory=site - invenio-preservation-sync==0.3.0 - invenio-cern-sync @ git+https://github.com/CERNDocumentServer/invenio-cern-sync@v0.6.0#egg=invenio-cern-sync - invenio-rdm-migrator @ git+https://github.com/CERNDocumentServer/invenio-rdm-migrator@master#egg=invenio-rdm-migrator + invenio-preservation-sync==0.5.0 + invenio-cern-sync @ git+https://github.com/CERNDocumentServer/invenio-cern-sync@main#egg=invenio-cern-sync invenio-query-parser @ git+https://github.com/CERNDocumentServer/invenio-query-parser@master#egg=invenio-query-parser videos = diff --git a/tests/cds-rdm/test_transform_prod_migration_check.py b/tests/cds-rdm/test_transform_prod_migration_check.py new file mode 100644 index 00000000..062508a3 --- /dev/null +++ b/tests/cds-rdm/test_transform_prod_migration_check.py @@ -0,0 +1,208 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2026 CERN. +# +# CDS-RDM is free software; you can redistribute it and/or modify it under +# the terms of the MIT License; see LICENSE file for more details. + +"""Tests for the "already migrated" skip/community-add logic. + +Covers ``CDSToRDMRecordTransform.should_skip``, +``_existing_record_is_restricted``, and the corresponding branches of +``run()`` -- all exercised as unit tests (external DB/service calls mocked) +to mirror the style of ``test_transform_versions.py``. +""" + +from unittest.mock import MagicMock + +import pytest +from sqlalchemy.exc import NoResultFound + +from cds_migrator_kit.errors import ManualImportRequired +from cds_migrator_kit.rdm.records.transform.transform import ( + CDSToRDMRecordTransform, +) + + +@pytest.fixture +def transform(tmp_path): + """Transform instance, mirroring test_transform_versions.py.""" + return CDSToRDMRecordTransform( + files_dump_dir=tmp_path, + missing_users=tmp_path, + communities_ids=["community-a"], + migration_logger=MagicMock(), + ) + + +@pytest.fixture +def app_context(base_app): + """Push an app context, without DB/search, just to resolve service proxies. + + ``current_rdm_records_service`` et al. are Flask-app-bound LocalProxy + objects: reading/patching them (even under mock) requires an active app + context with the relevant extensions registered, but not a live DB or + search connection -- so the lightweight, module-scoped ``base_app`` + (from pytest-invenio) is enough, avoiding the full ``test_app`` DB setup. + """ + with base_app.app_context(): + yield base_app + + +# -- should_skip ---------------------------------------------------------- + + +def test_should_skip_true_when_recid_in_migrated_set(transform): + """Recid already registered as an lrecid PID is skipped.""" + transform._migrated_recids = {"123"} + + assert transform.should_skip({"recid": 123}) is True + + +def test_should_skip_false_when_recid_not_in_migrated_set(transform): + """Recid not yet registered is not skipped.""" + transform._migrated_recids = {"456"} + + assert transform.should_skip({"recid": 123}) is False + + +# -- _existing_record_is_restricted ---------------------------------------- + + +def _mock_read_latest(mocker, transform, access): + """Patch current_rdm_records_service.read_latest to return given access. + + Uses ``new_callable=MagicMock`` explicitly: ``mock.patch`` auto-detects + the LocalProxy target as async-like otherwise, wrapping it in an + ``AsyncMock`` whose calls return coroutines instead of the configured + return value. + """ + service = mocker.patch( + "cds_migrator_kit.rdm.records.transform.transform.current_rdm_records_service", + new_callable=MagicMock, + ) + service.read_latest.return_value = MagicMock(data={"access": access}) + return service + + +def test_existing_record_is_restricted_false_when_public( + transform, mocker, app_context +): + """A fully public record/files is not restricted.""" + _mock_read_latest(mocker, transform, {"record": "public", "files": "public"}) + + assert transform._existing_record_is_restricted("42") is False + + +def test_existing_record_is_restricted_true_when_record_restricted( + transform, mocker, app_context +): + """A restricted record (metadata) is flagged, regardless of files.""" + _mock_read_latest(mocker, transform, {"record": "restricted", "files": "public"}) + + assert transform._existing_record_is_restricted("42") is True + + +def test_existing_record_is_restricted_true_when_files_restricted( + transform, mocker, app_context +): + """Restricted files alone are enough to flag the record.""" + _mock_read_latest(mocker, transform, {"record": "public", "files": "restricted"}) + + assert transform._existing_record_is_restricted("42") is True + + +# -- run(): already-migrated branch ---------------------------------------- + + +def _patch_run_deps(mocker, parent_pid=None, restricted=False, record_id="record-99"): + """Patch run()'s external collaborators for the should_skip branch.""" + get_pid = mocker.patch( + "cds_migrator_kit.rdm.records.transform.transform.get_pid_by_legacy_recid" + ) + if parent_pid is None: + get_pid.side_effect = NoResultFound() + else: + get_pid.return_value = parent_pid + + mocker.patch.object( + CDSToRDMRecordTransform, + "_existing_record_is_restricted", + return_value=restricted, + ) + # read_latest resolves the parent-level recid (what get_pid_by_legacy_recid + # returns) to the actual latest record, whose own id is what bulk_add needs. + read_latest = mocker.patch( + "cds_migrator_kit.rdm.records.transform.transform.current_rdm_records_service", + new_callable=MagicMock, + ).read_latest + read_latest.return_value = MagicMock(id=record_id) + bulk_add = mocker.patch( + "cds_migrator_kit.rdm.records.transform.transform." + "current_record_communities_service" + ).bulk_add + return get_pid, bulk_add + + +def test_run_logs_when_no_matching_rdm_record(transform, mocker, app_context): + """No lrecid->recid PID found: logs info, doesn't touch communities.""" + transform._load_migrated_recids = MagicMock(return_value={"123"}) + _, bulk_add = _patch_run_deps(mocker, parent_pid=None) + + list(transform.run([{"recid": 123}])) + + bulk_add.assert_not_called() + transform.migration_logger.add_log.assert_not_called() + messages = [ + call.args[1]["message"] + for call in transform.migration_logger.add_information.call_args_list + ] + assert any("Problem with PIDs" in m for m in messages) + transform.migration_logger.finalise_record.assert_called_once_with(123) + + +def test_run_adds_existing_record_to_communities_when_not_restricted( + transform, mocker, app_context +): + """Should-skip + not restricted -> added to all communities.""" + transform._load_migrated_recids = MagicMock(return_value={"123"}) + parent_pid = MagicMock(pid_value="42") + _, bulk_add = _patch_run_deps(mocker, parent_pid=parent_pid, restricted=False) + + list(transform.run([{"recid": 123}])) + + bulk_add.assert_called_once_with(mocker.ANY, "community-a", ["record-99"]) + transform.migration_logger.add_log.assert_not_called() + transform.migration_logger.finalise_record.assert_called_once_with(123) + + +def test_run_skips_community_add_when_restricted(transform, mocker, app_context): + """Should-skip + restricted -> ManualImportRequired logged, no community add.""" + transform._load_migrated_recids = MagicMock(return_value={"123"}) + parent_pid = MagicMock(pid_value="42") + _, bulk_add = _patch_run_deps(mocker, parent_pid=parent_pid, restricted=True) + + list(transform.run([{"recid": 123}])) + + bulk_add.assert_not_called() + transform.migration_logger.add_log.assert_called_once() + exc = transform.migration_logger.add_log.call_args.args[0] + assert isinstance(exc, ManualImportRequired) + transform.migration_logger.finalise_record.assert_called_once_with(123) + + +def test_run_transforms_normally_when_not_already_migrated( + transform, mocker, app_context +): + """A record not in the migrated set goes through the normal transform path.""" + transform._load_migrated_recids = MagicMock(return_value=set()) + _, bulk_add = _patch_run_deps(mocker, parent_pid=MagicMock(pid_value="42")) + mocker.patch.object( + CDSToRDMRecordTransform, "_transform", return_value={"record": "ok"} + ) + + results = list(transform.run([{"recid": 999}])) + + assert results == [{"record": "ok"}] + bulk_add.assert_not_called() + transform.migration_logger.finalise_record.assert_not_called()