From d189443430302a90756aefcec5acd0dcddfa19d2 Mon Sep 17 00:00:00 2001 From: zzacharo Date: Mon, 26 Oct 2026 09:28:14 +0100 Subject: [PATCH 1/6] feat(rules.experiments): route ISOLDE experiment IDs to studies ISOLDE experiment identifiers (IS01*) arrive in the 693 experiments field but are conceptually studies. Route the configured set to cern:studies instead of cern:experiments, and add the ISOLDE-related experiment aliases (CERN ISOLDE, MISTRAL, WITCH, ISOLTRAP, REX-ISOLDE) so legacy names resolve to their vocabulary ids. Co-Authored-By: Claude Opus 4.8 --- .../rdm/records/transform/config.py | 57 +++++++++++++++++++ .../transform/xml_processing/rules/base.py | 10 ++++ 2 files changed, 67 insertions(+) diff --git a/cds_migrator_kit/rdm/records/transform/config.py b/cds_migrator_kit/rdm/records/transform/config.py index 156f4a72..4bf222b1 100644 --- a/cds_migrator_kit/rdm/records/transform/config.py +++ b/cds_migrator_kit/rdm/records/transform/config.py @@ -159,6 +159,57 @@ FILE_SUBFORMATS_TO_DROP = ["pdfa", "unstamped"] +# Experiment IDs that live in the experiments MARC field but should be +# stored as studies (cern:studies) rather than experiments (cern:experiments). +EXPERIMENTS_AS_STUDIES = { + "IS01", + "IS01-1", + "IS01-2", + "IS01-3", + "IS01-4", + "IS01-5", + "IS01-6", + "IS01-7", + "IS01-8", + "IS01-9", + "IS01-10", + "IS01-11", + "IS01-12", + "IS01-13", + "IS01-15", + "IS01-16", + "IS01-18", + "IS01-20", + "IS01-21", + "IS01-22", + "IS01-23", + "IS01-24", + "IS01-26", + "IS01-28", + "IS01-29", + "IS01-31", + "IS01-32", + "IS01-35", + "IS01-37", + "IS01-38", + "IS01-39", + "IS01-40", + "IS01-42", + "IS01-43", + "IS01-44", + "IS01-45", + "IS01-49", + "IS01-51", + "IS01-B", + "IS01-C", + "IS01-D", + "IS01-E", + # "IS250", + # "IS", + # "IS02", + # "IS142", +} + # Legacy experiment names remapped to vocabulary ids before lookup EXPERIMENT_ALIASES = { "t2k": "RE13", @@ -170,6 +221,12 @@ "dampe": "RE29", "compass": "NA58", "compass na58": "NA58", + "cern isolde": "ISOLDE", + "mistral is346": "IS346", + "mistral is373": "IS373", + "witch is433": "IS433", + "isoltrap": "IS394", + "is485 (rex-isolde)": "IS485", } # 693__e values that are keywords rather than real experiments (curated as diff --git a/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py b/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py index 4bd29e06..10f88f49 100644 --- a/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py +++ b/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py @@ -30,6 +30,7 @@ CONTROLLED_SUBJECTS_SCHEMES, IDENTIFIERS_SCHEMES_TO_DROP, KEYWORD_SCHEMES_TO_DROP, + EXPERIMENTS_AS_STUDIES, PID_SCHEMES_TO_STORE_IN_IDENTIFIERS, PID_SCHEMES_TO_STORE_IN_RELATED_IDENTIFIERS, RECOGNISED_KEYWORD_SCHEMES, @@ -241,6 +242,15 @@ def custom_fields_693(self, key, value): for custom_field_name, subfield in fields_map.items(): custom_field = _custom_fields.get(custom_field_name, []) new_values = [v for v in force_list(value.get(subfield, "")) if v] + + if custom_field_name == "cern:experiments": + studies = [v for v in new_values if v.upper() in EXPERIMENTS_AS_STUDIES] + new_values = [ + v for v in new_values if v.upper() not in EXPERIMENTS_AS_STUDIES + ] + if studies: + _custom_fields.setdefault("cern:studies", []).extend(studies) + custom_field += new_values _custom_fields[custom_field_name] = custom_field From 8bf049e113a50f64c26726314280bab0f64bcd0f Mon Sep 17 00:00:00 2001 From: zzacharo Date: Mon, 26 Oct 2026 09:28:25 +0100 Subject: [PATCH 2/6] feat(rules.journal): link errata via related identifiers Errata are now linked at the DOI level: a 0247_ $q "erratum" qualifier becomes a related identifier with relation "issupplementedby", and the 773__m note accepts "erratum" in addition to "publication" (the old single-value _773_m_seen bookkeeping is dropped since the linking no longer depends on it). Erratum/addendum files are excluded from the record's default_preview so the main document is shown. A repeated 773__c is ambiguous page/session data and is flagged for manual curation instead of crashing downstream on the tuple. Co-Authored-By: Claude Opus 4.8 --- .../rdm/records/load/entities/record.py | 34 ++++++++++++++ .../transform/mappers/custom_fields.py | 3 -- .../transform/xml_processing/rules/base.py | 12 +++++ .../xml_processing/rules/research.py | 46 +++++++++++-------- 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/cds_migrator_kit/rdm/records/load/entities/record.py b/cds_migrator_kit/rdm/records/load/entities/record.py index e94fc617..b88222f2 100644 --- a/cds_migrator_kit/rdm/records/load/entities/record.py +++ b/cds_migrator_kit/rdm/records/load/entities/record.py @@ -156,6 +156,39 @@ def load_files( self.migration_logger.add_log(exc, record={"record": self.record_entry}) raise e + def set_default_preview(self, identity, draft, version_files, uow): + """Set default_preview to the first file not labelled erratum or addendum. + + Files whose 8564_y description contains 'erratum' or 'addendum' + (case-insensitive) are supplementary documents and must not become + the record's preview file. If every file carries such a label there + is nothing useful to promote, so we leave the field unset. + """ + if not version_files: + return + + _skip = {"erratum", "addendum"} + preview_key = next( + ( + file_data["key"] + for file_data in version_files.values() + if not any( + term in (file_data["metadata"].get("description") or "").lower() + for term in _skip + ) + ), + None, + ) + + if preview_key is None: + return + + draft_dict = draft.to_dict() + draft_dict.setdefault("files", {})["default_preview"] = preview_key + current_rdm_records_service.update_draft( + identity, draft["id"], data=draft_dict, uow=uow + ) + def load_access(self, draft, access_dict: VersionAccess, uow): """Set this version's access on the published record.""" record = draft._record @@ -250,6 +283,7 @@ def pre_publish(self, identity, versions, version, draft, uow): self.load_access(draft, access, uow) self.load_files(draft, files, uow) + self.set_default_preview(identity, draft, files, uow) return draft diff --git a/cds_migrator_kit/rdm/records/transform/mappers/custom_fields.py b/cds_migrator_kit/rdm/records/transform/mappers/custom_fields.py index 071e6dbd..84a1e56e 100644 --- a/cds_migrator_kit/rdm/records/transform/mappers/custom_fields.py +++ b/cds_migrator_kit/rdm/records/transform/mappers/custom_fields.py @@ -246,9 +246,6 @@ class JournalMapper(CustomFieldMapper): def apply(self, ctx): """Set ctx.custom_fields["journal:journal"].""" - # `_773_m_seen` is an internal bookkeeping field, just to check that we only have one 773__m - # per record as agreed with SIS. - ctx.dojson_entry.pop("_773_m_seen", None) journal = ctx.dojson_entry.get("custom_fields", {}).get("journal:journal", {}) if journal and not journal.get("title"): ctx.flag_curation( diff --git a/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py b/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py index 10f88f49..d3f8c96c 100644 --- a/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py +++ b/cds_migrator_kit/rdm/records/transform/xml_processing/rules/base.py @@ -503,6 +503,18 @@ def _pids(self, key, value): if not is_doi_id and is_handle_id and (scheme == "doi" or scheme == "urn/hdl"): scheme = "handle" + if qualifier == "erratum": + related_identifiers = self.get("related_identifiers", []) + new_id = { + "identifier": identifier, + "scheme": scheme, + "relation_type": {"id": "issupplementedby"}, + } + if new_id not in related_identifiers: + related_identifiers.append(new_id) + self["related_identifiers"] = related_identifiers + raise IgnoreKey("_pids") + if qualifier == "publication" or qualifier == "thesis": # if we have a qualifier for one of these two, we know it references # an external resource (checked in DB and individually on records) diff --git a/cds_migrator_kit/rdm/records/transform/xml_processing/rules/research.py b/cds_migrator_kit/rdm/records/transform/xml_processing/rules/research.py index 87a8f70a..9319c961 100644 --- a/cds_migrator_kit/rdm/records/transform/xml_processing/rules/research.py +++ b/cds_migrator_kit/rdm/records/transform/xml_processing/rules/research.py @@ -273,27 +273,35 @@ def journal(self, key, value): related_ids.append(isbn_related_id) self["related_identifiers"] = related_ids + # 773__m qualifies the journal reference (e.g. the main publication or an + # erratum). Per the curators' decision, the erratum linking is handled + # entirely at the DOI level (0247_ $q -> related identifier with + # "issupplementedby"), so this note carries no extra information and is + # ignored. Any other value is surfaced for curation. if "m" in value: - # As discussed with SIS, we only ignore 773__m if there is only one in the record - # and its value is `publication` - if self.get("_773_m_seen"): - raise UnexpectedValue( - "Multiple 773__m seen. Record requires manual curation.", - subfield="m", - field=key, - value=value, - ) - - m_value = value.get("m") - if m_value != "publication": - raise UnexpectedValue( - f'Only value "publication" can be ignored for 773__m. Value "{m_value}" requires manual curation.', - subfield="m", - field=key, - value=value, - ) + allowed_m_values = {"publication", "erratum"} + m_values = value.get("m") + if not isinstance(m_values, (list, tuple)): + m_values = (m_values,) + for m_value in m_values: + if StringValue(m_value).parse().lower() not in allowed_m_values: + raise UnexpectedValue( + "Unexpected 773__m value. Record requires manual curation.", + subfield="m", + field=key, + value=value, + ) - self["_773_m_seen"] = True + # A repeated 773__c (dojson returns a tuple) is ambiguous page/session data + # that can't be reduced to a single value - flag for manual curation instead + # of crashing on the tuple when it is parsed downstream. + if isinstance(value.get("c"), (list, tuple)): + raise UnexpectedValue( + "Multiple 773__c values. Record requires manual curation.", + subfield="c", + field=key, + value=value, + ) # p/n/v are journal-specific; c alone with w is a conference proceedings artid is_journal = any(f in value for f in ["p", "n", "v"]) From 3158fd17c5ed09bbb262a7c69a0ebcebbd77b095 Mon Sep 17 00:00:00 2001 From: zzacharo Date: Mon, 26 Oct 2026 09:42:39 +0100 Subject: [PATCH 3/6] fix(models.isolde): ignore system-only MARC keys Fold IGNORE_SYSTEM_KEYS into the ISOLDE model's ignore list (in addition to the shared ResearchModel baseline) and register 999C5e as a system-only key, so these internal fields no longer surface as unexpected values during migration. Co-Authored-By: Claude Opus 4.8 --- cds_migrator_kit/rdm/records/transform/models/_config.py | 1 + cds_migrator_kit/rdm/records/transform/models/isolde.py | 7 ++----- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/cds_migrator_kit/rdm/records/transform/models/_config.py b/cds_migrator_kit/rdm/records/transform/models/_config.py index 181bdf9f..467b4dbb 100644 --- a/cds_migrator_kit/rdm/records/transform/models/_config.py +++ b/cds_migrator_kit/rdm/records/transform/models/_config.py @@ -85,4 +85,5 @@ "999C5d", # old INSPIRE attr "999C69", # See decision log "999C6c", # See decision log + "999C5e", # See decision log } diff --git a/cds_migrator_kit/rdm/records/transform/models/isolde.py b/cds_migrator_kit/rdm/records/transform/models/isolde.py index b4b8a3cd..61368384 100644 --- a/cds_migrator_kit/rdm/records/transform/models/isolde.py +++ b/cds_migrator_kit/rdm/records/transform/models/isolde.py @@ -7,6 +7,7 @@ """CDS-RDM ISOLDE model.""" +from cds_migrator_kit.rdm.records.transform.models._config import IGNORE_SYSTEM_KEYS from cds_migrator_kit.rdm.records.transform.models.base_publication_record import ( rdm_base_publication_model, ) @@ -19,11 +20,7 @@ class ISOLDEModel(CdsOverdo): __query__ = '693__.a:"CERN ISOLDE" AND (980__:ARTICLE OR 980__:PREPRINT OR 980__:conferencepaper OR 980__:NOTE OR 980__:REPORT) -980__:DELETED -980__:DUMMY' - # ResearchModel.__ignore_keys__ covers the shared publication baseline - # (035__ oai tags, 852__h/c holdings, 999C5*/6* citations, 8564_ file - # subfields, 773__ SIS fields, contributor emails, etc.). - # Only ISOLDE-specific additions are listed here. - __ignore_keys__ = ResearchModel.__ignore_keys__ + __ignore_keys__ = IGNORE_SYSTEM_KEYS | ResearchModel.__ignore_keys__ _default_fields = { "custom_fields": {}, From 4076b0e4148cabaa104df82a1281c5218bec3885 Mon Sep 17 00:00:00 2001 From: zzacharo Date: Mon, 26 Oct 2026 09:42:39 +0100 Subject: [PATCH 4/6] chore(isolde): point to production community and refresh cleanup docs Switch the ISOLDE stream to the production community id, and update the cleanup instructions to drop the cdsrn/apprn pidstore rows and use the production (non-dev) OpenSearch index names. Co-Authored-By: Claude Opus 4.8 --- cds_migrator_kit/rdm/README.md | 6 ++++-- cds_migrator_kit/rdm/streams.yaml | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/cds_migrator_kit/rdm/README.md b/cds_migrator_kit/rdm/README.md index 5c23c6bc..d7bc090b 100644 --- a/cds_migrator_kit/rdm/README.md +++ b/cds_migrator_kit/rdm/README.md @@ -326,6 +326,8 @@ DELETE FROM files_files; DELETE FROM pidstore_pid WHERE pid_type = 'lrecid'; DELETE FROM pidstore_pid WHERE pid_type = 'recid'; DELETE FROM pidstore_pid WHERE pid_type = 'doi'; +DELETE FROM pidstore_pid WHERE pid_type = 'cdsrn'; +DELETE FROM pidstore_pid WHERE pid_type = 'apprn'; DELETE FROM cds_clc_record_sync; DELETE FROM cds_migration_legacy_records; ``` @@ -333,13 +335,13 @@ DELETE FROM cds_migration_legacy_records; - Cleanup indexed documents from opensearch ``` -POST /cds-rdm-dev-rdmrecords/_delete_by_query +POST /cds-rdm-rdmrecords/_delete_by_query { "query": { "match_all": {} } } -POST /cds-rdm-dev-communities/_delete_by_query +POST /cds-rdm-communities/_delete_by_query { "query": { "match_all": {} diff --git a/cds_migrator_kit/rdm/streams.yaml b/cds_migrator_kit/rdm/streams.yaml index 28d64b40..780a036d 100644 --- a/cds_migrator_kit/rdm/streams.yaml +++ b/cds_migrator_kit/rdm/streams.yaml @@ -339,7 +339,7 @@ records: files_dump_dir: cds_migrator_kit/rdm/data/isolde/files/ missing_users: cds_migrator_kit/rdm/data/users communities_ids: - - "77d6a84a-596e-414f-817b-1c43e9c66258" + - "0e0493e2-d312-48ac-93ad-a379cca428d5" load: legacy_pids_to_redirect: cds_migrator_kit/rdm/data/isolde/duplicated_pids.json hie_isolde: From d39045c9e5ce095f5fb4442ceb339ce8c8110845 Mon Sep 17 00:00:00 2001 From: zzacharo Date: Mon, 26 Oct 2026 09:54:59 +0100 Subject: [PATCH 5/6] test(rules): add unit tests for base and publication rules Co-Authored-By: Claude Opus 4.8 --- tests/cds-rdm/test_base_rules.py | 64 ++++++++++++++++++++++++ tests/cds-rdm/test_publications_rules.py | 57 +++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/tests/cds-rdm/test_base_rules.py b/tests/cds-rdm/test_base_rules.py index 70c3034c..55f76a0b 100644 --- a/tests/cds-rdm/test_base_rules.py +++ b/tests/cds-rdm/test_base_rules.py @@ -12,6 +12,7 @@ from cds_migrator_kit.errors import UnexpectedValue from cds_migrator_kit.rdm.records.transform.xml_processing.rules.base import ( + _pids, custom_fields_693, imprint_info, normalize, @@ -463,3 +464,66 @@ def test_empty_datafield_is_ignored(self): ) result = rdm_base_record_model.do(marc_record) assert result["rights"] == [{"title": {"en": "CC-BY"}}] + + +class TestPidsDOI: + """Test the _pids rule (0247_) DOI handling. + + Per the curators' decision the erratum linking is handled at the DOI + level: the main (non-erratum) DOI becomes the record DOI, while each + erratum DOI goes to related_identifiers with an "issupplementedby" + relation. + """ + + def test_non_erratum_doi_becomes_main_record_doi(self): + """A DOI without an erratum qualifier is stored as the record DOI.""" + record = {} + with pytest.raises(IgnoreKey): + _pids(record, "0247_", {"2": "DOI", "a": "10.1103/PhysRevLett.1.1"}) + assert record["_pids"]["doi"] == {"identifier": "10.1103/PhysRevLett.1.1"} + assert "related_identifiers" not in record + + def test_erratum_doi_goes_to_related_identifiers_as_supplement(self): + """An erratum DOI becomes a related identifier, not the record DOI.""" + record = {} + with pytest.raises(IgnoreKey): + _pids( + record, + "0247_", + {"2": "DOI", "a": "10.1103/PhysRevLett.1.2", "q": "erratum"}, + ) + assert record["related_identifiers"] == [ + { + "identifier": "10.1103/PhysRevLett.1.2", + "scheme": "doi", + "relation_type": {"id": "issupplementedby"}, + } + ] + # the erratum DOI must not be promoted to the main record DOI + assert "_pids" not in record or "doi" not in record.get("_pids", {}) + + def test_multiple_erratum_dois_each_added_as_supplement(self): + """One or more erratum DOIs are all kept as related identifiers. + + dojson yields repeated 0247_ fields separately, so the rule is + invoked once per field; each erratum DOI accumulates. + """ + record = {} + for doi in ("10.1/err.a", "10.1/err.b"): + with pytest.raises(IgnoreKey): + _pids(record, "0247_", {"2": "DOI", "a": doi, "q": "erratum"}) + + identifiers = [r["identifier"] for r in record["related_identifiers"]] + assert identifiers == ["10.1/err.a", "10.1/err.b"] + assert all( + r["relation_type"] == {"id": "issupplementedby"} + for r in record["related_identifiers"] + ) + + def test_erratum_doi_not_duplicated(self): + """The same erratum DOI seen twice is only stored once.""" + record = {} + for _ in range(2): + with pytest.raises(IgnoreKey): + _pids(record, "0247_", {"2": "DOI", "a": "10.1/err.a", "q": "erratum"}) + assert len(record["related_identifiers"]) == 1 diff --git a/tests/cds-rdm/test_publications_rules.py b/tests/cds-rdm/test_publications_rules.py index 9ef382a7..baaaed3b 100644 --- a/tests/cds-rdm/test_publications_rules.py +++ b/tests/cds-rdm/test_publications_rules.py @@ -705,6 +705,63 @@ def test_962_matching_real_journal_pages_preserves_journal(self): assert result[0]["resource_type"] == {"id": "publication-conferenceproceeding"} assert record["custom_fields"]["journal:journal"]["title"] == "Journal" + @pytest.mark.parametrize("m_value", ["publication", "erratum"]) + def test_773_m_known_value_is_ignored(self, m_value): + """773__m publication/erratum is ignored. + + The erratum linking is handled at the DOI level, so the note itself + carries no extra data: the record still migrates and the journal + fields are populated normally. + """ + record = {} + result = journal(record, "773__", {"p": "Nature", "v": "5", "m": m_value}) + + journal_info = result.get("journal:journal", {}) + assert journal_info["title"] == "Nature" + assert journal_info["volume"] == "5" + + def test_773_m_known_value_case_insensitive(self): + """773__m matching is case/whitespace insensitive.""" + record = {} + result = journal(record, "773__", {"p": "Nature", "m": " Erratum "}) + assert result["journal:journal"]["title"] == "Nature" + + def test_773_m_unexpected_value_raises(self): + """An unknown 773__m value is surfaced for manual curation.""" + with pytest.raises(UnexpectedValue, match="Unexpected 773__m value"): + journal({}, "773__", {"p": "Nature", "m": "something-else"}) + + def test_773_m_repeated_all_known_ignored(self): + """Repeated 773__m (a tuple) is accepted when every value is known.""" + record = {} + result = journal( + record, "773__", {"p": "Nature", "m": ("publication", "erratum")} + ) + assert result["journal:journal"]["title"] == "Nature" + + def test_773_m_repeated_with_one_unknown_raises(self): + """Repeated 773__m raises if any value is unknown.""" + with pytest.raises(UnexpectedValue, match="Unexpected 773__m value"): + journal({}, "773__", {"p": "Nature", "m": ("publication", "bogus")}) + + def test_773_c_repeated_raises_for_curation(self): + """A repeated 773__c (a tuple) is ambiguous and flagged for curation. + + dojson returns the subfield as a tuple; rather than crashing on it (or + silently picking one value) the record is surfaced for manual curation. + """ + with pytest.raises(UnexpectedValue, match="Multiple 773__c values"): + journal( + {}, + "773__", + {"p": "Phys. Lett. B", "m": "publication", "c": ("062701", "209904")}, + ) + + def test_773_c_repeated_in_conference_raises_for_curation(self): + """A repeated 773__c in a conference 773 is also flagged for curation.""" + with pytest.raises(UnexpectedValue, match="Multiple 773__c values"): + journal({}, "773__", {"w": "C05-03-12", "c": ("175-181", "WEC02")}) + class TestMeetingDateFrom518: """Test meeting date (518__r/__d) handling in related_identifiers. From 525fbad2e7278a04a3e246cd10aa1bc52e974bf2 Mon Sep 17 00:00:00 2001 From: zzacharo Date: Mon, 26 Oct 2026 09:54:59 +0100 Subject: [PATCH 6/6] test: cover version transform and access grant resolution Co-Authored-By: Claude Opus 4.8 --- tests/cds-rdm/test_resolve_grants.py | 75 ++++++++++++++++++++++++ tests/cds-rdm/test_transform_versions.py | 75 ++++++++++++++++++++++++ 2 files changed, 150 insertions(+) diff --git a/tests/cds-rdm/test_resolve_grants.py b/tests/cds-rdm/test_resolve_grants.py index b1e6e2f2..a8565d84 100644 --- a/tests/cds-rdm/test_resolve_grants.py +++ b/tests/cds-rdm/test_resolve_grants.py @@ -10,18 +10,45 @@ These characterise the current behaviour of every branch that turns a legacy file-restriction string (``VersionEntry["access"]["meta"]``) plus the record's own access grants into ``(groups, emails, grants_with_perms)``. +<<<<<<< HEAD They rely on the shared ``test_app`` fixture (conftest) for the application context and its ``CDS_ACCESS_GROUP_MAPPINGS`` config (``SSO`` -> ``cern-personnel``, ``HrDepRestrFile`` -> ``hr-dep`` + ``fap-dep-tpr-mi-staf``). +======= +They are intentionally self-contained (a minimal Flask app providing only +``CDS_ACCESS_GROUP_MAPPINGS``) so they run without Docker services. +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """ from types import SimpleNamespace import pytest +<<<<<<< HEAD +======= +from flask import Flask +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) from cds_migrator_kit.errors import ManualImportRequired from cds_migrator_kit.rdm.records.transform.entities.parent import RecordParent +<<<<<<< HEAD +======= +GROUP_MAPPINGS = { + "SSO": ["cern-personnel"], + "ITDepRestrFile": ["it-dep"], + "HrDepRestrFile": ["hr-dep", "fap-dep-tpr-mi-staf"], +} + + +@pytest.fixture() +def app_ctx(): + """Minimal app context exposing CDS_ACCESS_GROUP_MAPPINGS.""" + app = Flask("test_resolve_grants") + app.config["CDS_ACCESS_GROUP_MAPPINGS"] = GROUP_MAPPINGS + with app.app_context(): + yield app + +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) def make_parent(access_grants=None): """Build a RecordParent bypassing build(); set access_grants directly.""" @@ -39,7 +66,11 @@ def make_parent(access_grants=None): # --- File restriction (``specific_file_restrictions``) only ---------------- +<<<<<<< HEAD def test_empty_restriction_no_grants(test_app): +======= +def test_empty_restriction_no_grants(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """No file restriction and no record grants -> everything empty.""" groups, emails, grants = make_parent().resolve_grants("") assert groups == set() @@ -47,7 +78,11 @@ def test_empty_restriction_no_grants(test_app): assert grants == {} +<<<<<<< HEAD def test_mapped_status_resolves_to_group(test_app): +======= +def test_mapped_status_resolves_to_group(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A status in CDS_ACCESS_GROUP_MAPPINGS resolves to its mapped groups.""" groups, emails, grants = make_parent().resolve_grants("SSO") assert groups == {"cern-personnel"} @@ -55,20 +90,32 @@ def test_mapped_status_resolves_to_group(test_app): assert grants == {} +<<<<<<< HEAD def test_mapped_status_multiple_groups(test_app): +======= +def test_mapped_status_multiple_groups(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A status mapping to several groups adds all of them.""" groups, _, _ = make_parent().resolve_grants("HrDepRestrFile") assert groups == {"hr-dep", "fap-dep-tpr-mi-staf"} +<<<<<<< HEAD def test_literal_restricted_maps_to_cern_personnel(test_app): +======= +def test_literal_restricted_maps_to_cern_personnel(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """The literal 'restricted' status maps to cern-personnel.""" groups, emails, grants = make_parent().resolve_grants("restricted") assert groups == {"cern-personnel"} assert emails == set() +<<<<<<< HEAD def test_bare_cern_egroup_name(test_app): +======= +def test_bare_cern_egroup_name(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A bare ' [CERN]' e-group name is added, '[CERN]' stripped.""" groups, emails, _ = make_parent().resolve_grants( "cds-ph-ep-publications-referee-non-lhc [CERN]" @@ -77,7 +124,11 @@ def test_bare_cern_egroup_name(test_app): assert emails == set() +<<<<<<< HEAD def test_firerole_allow_group(test_app): +======= +def test_firerole_allow_group(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A firerole 'allow group' string parses out the group(s).""" groups, emails, _ = make_parent().resolve_grants( 'firerole: allow group "atlas-active-members [CERN]"\ndeny all' @@ -86,7 +137,11 @@ def test_firerole_allow_group(test_app): assert emails == set() +<<<<<<< HEAD def test_firerole_allow_multiple_groups(test_app): +======= +def test_firerole_allow_multiple_groups(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A firerole 'allow group' with several quoted groups parses all.""" groups, _, _ = make_parent().resolve_grants( 'firerole: allow group "atlas-active-members [CERN]", "cms-members [CERN]"\n' @@ -95,7 +150,11 @@ def test_firerole_allow_multiple_groups(test_app): assert groups == {"atlas-active-members", "cms-members"} +<<<<<<< HEAD def test_firerole_allow_email(test_app): +======= +def test_firerole_allow_email(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A firerole 'allow email' string parses out the email(s).""" groups, emails, _ = make_parent().resolve_grants( 'firerole: allow email "a@cern.ch"\ndeny all' @@ -104,7 +163,11 @@ def test_firerole_allow_email(test_app): assert emails == {"a@cern.ch"} +<<<<<<< HEAD def test_unmapped_non_firerole_status_raises(test_app): +======= +def test_unmapped_non_firerole_status_raises(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """An unknown status that is neither mapped nor firerole hard-raises.""" with pytest.raises(ManualImportRequired): make_parent().resolve_grants("TOTALLY_UNKNOWN_STATUS") @@ -113,7 +176,11 @@ def test_unmapped_non_firerole_status_raises(test_app): # --- Record access grants (``self.access_grants``) ------------------------- +<<<<<<< HEAD def test_record_group_grant_without_file_restriction(test_app): +======= +def test_record_group_grant_without_file_restriction(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """With no file restriction, a record group grant feeds groups + perms. Note the normalised name goes to ``groups`` while the raw subject is the @@ -127,7 +194,11 @@ def test_record_group_grant_without_file_restriction(test_app): assert grants == {"some-group [CERN]": "manage"} +<<<<<<< HEAD def test_record_email_grant_defaults_to_view(test_app): +======= +def test_record_email_grant_defaults_to_view(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """A record email grant with no permission defaults to 'view'.""" groups, emails, grants = make_parent([{"user@cern.ch": None}]).resolve_grants("") assert groups == set() @@ -135,7 +206,11 @@ def test_record_email_grant_defaults_to_view(test_app): assert grants == {"user@cern.ch": "view"} +<<<<<<< HEAD def test_file_restriction_withholds_record_subjects_from_access(test_app): +======= +def test_file_restriction_withholds_record_subjects_from_access(app_ctx): +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """When a file restriction exists, record subjects are kept out of groups/emails (least access) but still recorded in grants_with_perms.""" groups, emails, grants = make_parent([{"extra-group": "view"}]).resolve_grants( diff --git a/tests/cds-rdm/test_transform_versions.py b/tests/cds-rdm/test_transform_versions.py index 0f526f7f..82c51076 100644 --- a/tests/cds-rdm/test_transform_versions.py +++ b/tests/cds-rdm/test_transform_versions.py @@ -283,6 +283,7 @@ def test_versions_all_public_files_stay_public(transform): transform.migration_logger.add_information.assert_not_called() +<<<<<<< HEAD @pytest.mark.parametrize( "files", [ @@ -301,18 +302,31 @@ def test_versions_non_uniform_file_statuses_raise_and_report_all(transform, file All files must share a single status (all public, or all the same restriction). Any other mix is unrepresentable and raises - independent of dump order - reporting every file with its status for manual review. +======= +def test_versions_mixed_public_and_restricted_raises_restricted_first(transform): + """A version mixing a restricted and a public file hard-stops. + + All files in a version must share one status; a restricted file with a + public sibling is unrepresentable regardless of dump order, so it raises. +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """ raw_dump_entry = { "recid": 123, "files": [ +<<<<<<< HEAD _file_dump(full_name=name, bibdocid=i + 1, status=status) for i, (name, status) in enumerate(files) +======= + _file_dump(full_name="restricted.pdf", bibdocid=1, status="SSO"), + _file_dump(full_name="public.pdf", bibdocid=2, status=""), +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) ], } with pytest.raises(ManualImportRequired) as exc_info: transform._versions(raw_dump_entry, _record()) +<<<<<<< HEAD reported = exc_info.value.value for name, status in files: assert f"{name}: {status}" in reported @@ -350,12 +364,25 @@ def test_versions_carry_forward_restricted_into_public_version_raises(transform) status="", creation_date="2020-02-01T00:00:00+00:00", ), +======= + assert "restricted.pdf: SSO" in exc_info.value.value + + +def test_versions_mixed_public_and_restricted_raises_public_first(transform): + """Same mix, reversed order: still hard-stops (order-independent).""" + raw_dump_entry = { + "recid": 123, + "files": [ + _file_dump(full_name="public.pdf", bibdocid=1, status=""), + _file_dump(full_name="restricted.pdf", bibdocid=2, status="SSO"), +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) ], } with pytest.raises(ManualImportRequired) as exc_info: transform._versions(raw_dump_entry, _record()) +<<<<<<< HEAD assert "A.pdf: " in exc_info.value.value assert "B.pdf: SSO" in exc_info.value.value @@ -365,10 +392,22 @@ def test_versions_restriction_lifted_on_only_file_becomes_public(transform): v1 is restricted, v2 (the same file, now public) is public - the cumulative set per version uses the latest revision per file, so there is no mix. +======= + assert "restricted.pdf: SSO" in exc_info.value.value + + +def test_versions_multiple_distinct_restriction_statuses_raises(transform): + """A version with two different restriction statuses can't be represented. + + A version carries a single access state + meta, so it cannot express more + than one distinct restriction. Two restricted files with different statuses + must hard-stop (otherwise the second status is silently dropped). +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation) """ raw_dump_entry = { "recid": 123, "files": [ +<<<<<<< HEAD _file_dump( full_name="A.pdf", file_version=1, @@ -396,3 +435,39 @@ def test_versions_restriction_lifted_on_only_file_becomes_public(transform): "access_obj": {"record": "public", "files": "public"} } assert versions[2]["files"]["A.pdf"]["version"] == 2 +======= + _file_dump(full_name="a.pdf", bibdocid=1, status="SSO"), + _file_dump(full_name="b.pdf", bibdocid=2, status="ITDepRestrFile"), + ], + } + + with pytest.raises(ManualImportRequired) as exc_info: + transform._versions(raw_dump_entry, _record()) + + assert "a.pdf: SSO" in exc_info.value.value + assert "b.pdf: ITDepRestrFile" in exc_info.value.value + + +def test_versions_public_plus_two_distinct_restricted_statuses_raises(transform): + """One public + two restricted files with distinct statuses: hard-stop. + + The single merged error reports every file with its status, including the + public one, so a reviewer sees the full picture of the mix. + """ + raw_dump_entry = { + "recid": 123, + "files": [ + _file_dump(full_name="public.pdf", bibdocid=1, status=""), + _file_dump(full_name="a.pdf", bibdocid=2, status="SSO"), + _file_dump(full_name="b.pdf", bibdocid=3, status="ITDepRestrFile"), + ], + } + + with pytest.raises(ManualImportRequired) as exc_info: + transform._versions(raw_dump_entry, _record()) + + assert "a.pdf: SSO" in exc_info.value.value + assert "b.pdf: ITDepRestrFile" in exc_info.value.value + # every file is reported, including the public one + assert "public.pdf: " in exc_info.value.value +>>>>>>> 8214940 (fix(files-access): flag mixed file restrictions for curation)