From 31a0f3f2de3f197e3fb29e9c0bca096cc158995e Mon Sep 17 00:00:00 2001 From: zzacharo Date: Wed, 17 Jun 2026 09:29:26 +0200 Subject: [PATCH] feat(ep): change reportnumber generation mechanism --- .../record_details/CreatePublicRecordModal.js | 28 ++------ .../components/record_details/EPApproval.js | 60 ++++++++-------- .../requests/EPApprovalRequestDetails.js | 22 ++---- invenio.cfg | 22 +++--- site/cds_rdm/components.py | 10 +-- site/cds_rdm/ext.py | 2 +- site/cds_rdm/requests/ep_approval.py | 72 +++++++++++-------- site/tests/conftest.py | 2 +- site/tests/test_ep_approval.py | 14 ++-- 9 files changed, 106 insertions(+), 126 deletions(-) diff --git a/assets/js/components/record_details/CreatePublicRecordModal.js b/assets/js/components/record_details/CreatePublicRecordModal.js index d12293c3..f30aa797 100644 --- a/assets/js/components/record_details/CreatePublicRecordModal.js +++ b/assets/js/components/record_details/CreatePublicRecordModal.js @@ -6,14 +6,7 @@ import React, { Component } from "react"; import PropTypes from "prop-types"; -import { - Button, - Checkbox, - Header, - Icon, - Message, - Modal, -} from "semantic-ui-react"; +import { Button, Checkbox, Header, Icon, Message, Modal } from "semantic-ui-react"; import { http } from "react-invenio-forms"; import { i18next } from "@translations/invenio_rdm_records/i18next"; @@ -120,9 +113,7 @@ export class CreatePublicRecordModal extends Component { ) : publicRecord ? ( - - {i18next.t("Public record created")} - + {i18next.t("Public record created")} {i18next.t("The public record has been created successfully.")}{" "} - this.setState({ agreedToTerms: checked }) - } + onChange={(_, { checked }) => this.setState({ agreedToTerms: checked })} label={ @@ -169,44 +169,44 @@ export class EPApprovalManageSection extends Component { } const { - can_submit, - can_create_public, - open_request, - approved_report_number, - receiver_group, - ep_approval, + can_submit: canSubmit, + can_create_public: canCreatePublicFlag, + open_request: openRequest, + approved_report_number: approvedReportNumber, + receiver_group: receiverGroup, + ep_approval: epApprovalField, } = epApproval; // Public record URL: prefer the URL captured at creation time; fall back to // the recid stored on the parent (approved_public_version) for page-load case. const resolvedPublicRecordUrl = publicRecordUrl || - (ep_approval?.approved_public_version - ? `/records/${ep_approval.approved_public_version}` + (epApprovalField?.approved_public_version + ? `/records/${epApprovalField.approved_public_version}` : null); - const isPending = open_request?.status === "submitted"; - const isDeclined = open_request?.status === "declined"; - const isAccepted = open_request?.status === "accepted"; - // approved_report_number is always populated by the backend (scans the full + const isPending = openRequest?.status === "submitted"; + const isDeclined = openRequest?.status === "declined"; + const isAccepted = openRequest?.status === "accepted"; + // approvedReportNumber is always populated by the backend (scans the full // parent if the current version doesn't carry the CF itself). const canResubmit = - can_submit && !isPending && !approved_report_number && !isAccepted; - // can_create_public comes from the backend and already encodes version-order + canSubmit && !isPending && !approvedReportNumber && !isAccepted; + // canCreatePublicFlag comes from the backend and already encodes version-order // eligibility (only versions >= the approved version may create a public record). - const canCreatePublic = can_create_public && !publicRecordId; + const canCreatePublic = canCreatePublicFlag && !publicRecordId; const requestLink = - open_request?.links?.self_html || - (open_request?.id ? `/requests/${open_request.id}` : null); + openRequest?.links?.self_html || + (openRequest?.id ? `/requests/${openRequest.id}` : null); // Timeline step states // Step 1 — Request for approval - const step1Completed = !!approved_report_number; + const step1Completed = !!approvedReportNumber; const step1Active = !step1Completed && !isPending; // Step 2 — EP Board review - const step2Completed = !!approved_report_number; + const step2Completed = !!approvedReportNumber; const step2Active = isPending; const step2Disabled = !isPending && !step2Completed; @@ -280,7 +280,7 @@ export class EPApprovalManageSection extends Component { this.setState({ submitModalOpen: false })} onSuccess={this.handleSubmitSuccess} /> @@ -300,15 +300,19 @@ export class EPApprovalManageSection extends Component { {step2Completed ? ( requestLink ? ( - + {i18next.t("Approved as {{rn}}", { - rn: approved_report_number, + rn: approvedReportNumber, })} ) : ( i18next.t("Approved as {{rn}}", { - rn: approved_report_number, + rn: approvedReportNumber, }) ) ) : ( @@ -384,7 +388,7 @@ export class EPApprovalManageSection extends Component { this.setState({ createPublicModalOpen: false })} onSuccess={this.handlePublicRecordCreated} /> diff --git a/assets/js/components/requests/EPApprovalRequestDetails.js b/assets/js/components/requests/EPApprovalRequestDetails.js index 1b997337..fb1ef9d2 100644 --- a/assets/js/components/requests/EPApprovalRequestDetails.js +++ b/assets/js/components/requests/EPApprovalRequestDetails.js @@ -11,9 +11,7 @@ import { i18next } from "@translations/invenio_app_rdm/i18next"; import { Timeline } from "@js/invenio_requests/timelineParent"; import RequestMetadata from "@js/invenio_requests/request/RequestMetadata"; -const BoolCell = ({ value }) => ( - <>{value ? i18next.t("Yes") : i18next.t("No")} -); +const BoolCell = ({ value }) => <>{value ? i18next.t("Yes") : i18next.t("No")}; BoolCell.propTypes = { value: PropTypes.bool.isRequired }; @@ -50,11 +48,7 @@ const EPApprovalPayloadCard = ({ request }) => { {i18next.t("Latest version at")} - + {payload.latest_version_url} @@ -127,18 +121,10 @@ export const EPApprovalAwareRequestDetails = ({ {isEPApproval && } - + - + ); diff --git a/invenio.cfg b/invenio.cfg index ae28dc5f..cec15ece 100644 --- a/invenio.cfg +++ b/invenio.cfg @@ -446,7 +446,7 @@ CDS_EOS_OFFLOAD_X509_KEY_PATH = "" # check nginx config for more details CDS_EOS_OFFLOAD_REDIRECT_BASE_PATH = "" -CDS_CERN_SCIENTIFIC_COMMUNITY_ID = "5ad76c90-e035-401e-ac52-9aab978955a1" +CDS_CERN_SCIENTIFIC_COMMUNITY_ID = "" """The id of the CERN Scientific community.""" # EP / Publication Approval Workflow @@ -456,19 +456,25 @@ CDS_EP_APPROVAL_COMMUNITIES = { # Map community UUID → workflow config. # UUIDs are used (not slugs) because slugs can be renamed. # - "8665a816-6bea-4afd-b605-66e676621f43": { - "label": "EP approval", # shown in UI buttons/headings - "referee_group": "ep-test", # CERN e-group slug - "report_number_pattern": "CERN-EP-{year}-{seq:03d}", - }, + # "4b121eb9-3559-487d-9f73-8af1027668e9": { + # "label": "EP approval", # shown in UI buttons/headings + # "referee_group": "cds-ph-ep-publication", # CERN e-group slug + # "report_number": { + # "prefix": "CERN-EP", # literal prefix, e.g. "CERN-EP" + # "include_year": True, # append the current year after prefix + # "counter_digits": 3, # zero-padding width, e.g. 3 → "001" + # }, + # }, } """Communities enrolled in the Publication Approval Workflow. Keyed by community UUID. Each entry must have: - ``label``: human-readable name shown in the UI (e.g. "EP approval") - ``referee_group``: CERN e-group whose members act as referees - - ``report_number_pattern``: Python format string with ``{year}`` and - ``{seq:03d}`` placeholders (e.g. ``"CERN-EP-{year}-{seq:03d}"``). + - ``report_number``: dict with: + - ``prefix`` (str): fixed prefix, e.g. ``"CERN-EP"`` + - ``include_year`` (bool, default True): append ``-{year}`` after prefix + - ``counter_digits`` (int, default 3): zero-padding for the counter All generated numbers share the ``apprn`` PID type, so uniqueness is enforced across all enrolled communities. diff --git a/site/cds_rdm/components.py b/site/cds_rdm/components.py index 32302920..68a9be6d 100644 --- a/site/cds_rdm/components.py +++ b/site/cds_rdm/components.py @@ -8,8 +8,6 @@ """CDS RDM service components.""" -import re - from flask import current_app from flask_principal import ActionNeed from invenio_access import Permission @@ -158,17 +156,15 @@ def _is_privileged(self, identity): ).allows(identity) def _ep_approval_prefixes(self): - """Return the set of fixed prefixes from all configured EP patterns. + """Return the set of fixed prefixes from all configured EP communities. - E.g. pattern "CERN-EP-{year}-{seq:03d}" → prefix "CERN-EP-". + E.g. config ``{"prefix": "CERN-EP"}`` → prefix ``"CERN-EP"``. Used to detect cdsrn values that collide with EP report numbers. """ communities = current_app.config.get("CDS_EP_APPROVAL_COMMUNITIES", {}) prefixes = set() for cfg in communities.values(): - pattern = cfg.get("report_number_pattern", "") - # Extract the literal part before the first placeholder. - prefix = re.split(r"\{", pattern)[0] + prefix = cfg.get("report_number", {}).get("prefix") if prefix: prefixes.add(prefix) return prefixes diff --git a/site/cds_rdm/ext.py b/site/cds_rdm/ext.py index fa4c2619..e6bf97ea 100644 --- a/site/cds_rdm/ext.py +++ b/site/cds_rdm/ext.py @@ -10,13 +10,13 @@ from cds_rdm.clc_sync.resources.config import CLCSyncResourceConfig from cds_rdm.clc_sync.resources.resource import CLCSyncResource from cds_rdm.clc_sync.resources.utils import get_clc_sync_entry -from cds_rdm.requests.ep_approval_state import get_ep_approval_state from cds_rdm.clc_sync.services.config import CLCSyncServiceConfig from cds_rdm.clc_sync.services.service import CLCSyncService from cds_rdm.harvester_download.resources import ( HarvesterDownloadResource, HarvesterDownloadResourceConfig, ) +from cds_rdm.requests.ep_approval_state import get_ep_approval_state from . import config from .utils import evaluate_permissions diff --git a/site/cds_rdm/requests/ep_approval.py b/site/cds_rdm/requests/ep_approval.py index de4bf31e..e444461e 100644 --- a/site/cds_rdm/requests/ep_approval.py +++ b/site/cds_rdm/requests/ep_approval.py @@ -17,9 +17,11 @@ from flask_principal import Identity from invenio_access.permissions import system_identity from invenio_db import db +from invenio_drafts_resources.services.records.uow import ParentRecordCommitOp from invenio_i18n import lazy_gettext as _ from invenio_notifications.services.uow import NotificationOp from invenio_pidstore.models import PersistentIdentifier, PIDStatus +from invenio_rdm_records.proxies import current_rdm_records_service from invenio_rdm_records.records.api import RDMRecord from invenio_rdm_records.requests.base import BaseRequest as RDMBaseRequest from invenio_records_resources.services.errors import PermissionDeniedError @@ -37,7 +39,6 @@ # PID type stored in pidstore_pid.pid_type (VARCHAR(6) — keep ≤ 6 chars). # The identifier scheme name in record metadata is "apprn" (no length limit). -# TODO: what is apprn? can we name this better? APPRN_PID_TYPE = "apprn" @@ -157,40 +158,46 @@ def _community_config(self): """Resolve the community config for this request (guaranteed enrolled at submit).""" return _resolve_community_config(self.request) - def _generate_report_number(self, pattern: str) -> str: - """Auto-generate the next sequential report number for the given pattern. + def _next_report_number(self, rn: dict) -> str: + """Return the next report number for this community without minting a PID. - Derives the next sequence number from the MAX existing apprn PID value - for this pattern prefix and year, not a COUNT. This is robust to gaps - (deleted PIDs, failed accepts) — the sequence only ever goes forward. + Builds the scan prefix directly from the structured ``report_number`` + config — no string-format parsing needed. - The prefix is extracted by splitting on ``{seq`` so that zero-padded - formats like ``{seq:03d}`` do not produce a truncated prefix that misses - PIDs >= 010 (e.g. "CERN-EP-2026-00" would miss "CERN-EP-2026-010"). - - Pattern example: "CERN-EP-{year}-{seq:03d}" + Args: + rn: the ``report_number`` sub-dict from the community config, with keys: + - ``prefix`` (str): fixed prefix, e.g. ``"CERN-EP"`` + - ``include_year`` (bool, default ``True``): append ``-{year}`` + - ``counter_digits`` (int, default ``3``): zero-padding width """ + prefix = rn["prefix"] + digits = rn.get("counter_digits", 3) year = date.today().year - # Split on "{seq" to get everything before the sequence placeholder, - # then format only the year part → "CERN-EP-2026-" - # TODO: I don't understand what this is doing - prefix = pattern.split("{seq")[0].format(year=year) + scan_prefix = ( + f"{prefix}-{year}-" if rn.get("include_year", True) else f"{prefix}-" + ) + existing = PersistentIdentifier.query.filter( PersistentIdentifier.pid_type == APPRN_PID_TYPE, - PersistentIdentifier.pid_value.like(f"{prefix}%"), + PersistentIdentifier.pid_value.like(f"{scan_prefix}%"), ).all() max_seq = max( ( - int(p.pid_value[len(prefix) :]) + int(p.pid_value[len(scan_prefix) :]) for p in existing - if p.pid_value[len(prefix) :].isdigit() + if p.pid_value[len(scan_prefix) :].isdigit() ), default=0, ) - return pattern.format(year=year, seq=max_seq + 1) + return f"{scan_prefix}{max_seq + 1:0{digits}d}" - def _mint_apprn_pid(self, report_number: str, record_uuid: str) -> None: - """Mint the apprn PID pointing at the given record UUID.""" + def _issue_report_number(self, config: dict, record_uuid: str) -> str: + """Generate the next report number and mint its PID. + + Delegates number generation to ``_next_report_number`` (testable + independently) then registers the result in pidstore. + """ + report_number = self._next_report_number(config["report_number"]) PersistentIdentifier.create( pid_type=APPRN_PID_TYPE, pid_value=report_number, @@ -198,6 +205,7 @@ def _mint_apprn_pid(self, report_number: str, record_uuid: str) -> None: object_uuid=record_uuid, status=PIDStatus.REGISTERED, ) + return report_number def execute(self, identity: Identity, uow: UnitOfWork) -> None: """Execute accept: mint report number and write ep_approval to the parent. @@ -209,17 +217,12 @@ def execute(self, identity: Identity, uow: UnitOfWork) -> None: (detected by the presence of source_internal_version on the public parent). """ config = self._community_config() - pattern = config["report_number_pattern"] - report_number = self._generate_report_number(pattern) - topic = self.request.topic.resolve() submitted_version_recid = ( self.request["payload"].get("submitted_version_id") or topic["id"] ) - # TODO: do we not need to mint/reserve the report number when the request is created - # rather than accepted? E.g. maybe the curators need the number before publication - self._mint_apprn_pid(report_number, str(topic.id)) + report_number = self._issue_report_number(config, str(topic.id)) # Write ep_approval into permission_flags — single source of truth. pf = topic.parent.get("permission_flags") or {} @@ -228,11 +231,18 @@ def execute(self, identity: Identity, uow: UnitOfWork) -> None: "datetime": datetime.now(timezone.utc).isoformat(), "approved_internal_version": submitted_version_recid, } - # TODO: why is this in `permission_flags`? It is instance-specific metadata so makes sense to go in a dict field, - # but why not create a generic `metadata` field or something. topic.parent["permission_flags"] = pf - topic.parent.commit() - db.session.commit() + + # Flush the parent and defer the DB commit to the UoW so that the + # parent write, the PID insert, and the request status update all land + # in the same transaction. The advisory lock acquired in + # _issue_report_number is held until this transaction commits. + uow.register( + ParentRecordCommitOp( + topic.parent, + indexer_context=dict(service=current_rdm_records_service), + ) + ) # Store on the request payload so the UI can display it. self.request["payload"]["approved_report_number"] = report_number diff --git a/site/tests/conftest.py b/site/tests/conftest.py index df3bb5b9..ef57c019 100644 --- a/site/tests/conftest.py +++ b/site/tests/conftest.py @@ -1675,7 +1675,7 @@ def ep_enrolled_community(community_service, running_app): current_app.config["CDS_EP_APPROVAL_COMMUNITIES"][str(community.id)] = { "label": "EP approval", "referee_group": "cds-ph-ep-publication", - "report_number_pattern": "CERN-EP-{year}-{seq:03d}", + "report_number": {"prefix": "CERN-EP", "include_year": True, "counter_digits": 3}, } return community._record diff --git a/site/tests/test_ep_approval.py b/site/tests/test_ep_approval.py index 54700f4d..12a5fa31 100644 --- a/site/tests/test_ep_approval.py +++ b/site/tests/test_ep_approval.py @@ -29,7 +29,8 @@ # --------------------------------------------------------------------------- YEAR = date.today().year -EP_PATTERN = "CERN-EP-{year}-{seq:03d}" +EP_RN_CONFIG = {"prefix": "CERN-EP", "include_year": True, "counter_digits": 3} +TH_RN_CONFIG = {"prefix": "CERN-TH", "include_year": True, "counter_digits": 3} EP_GROUP_NAME = "cds-ph-ep-publication" @@ -170,7 +171,7 @@ def test_ep_approval_request_type_is_registered(app): def test_generate_report_number_first_of_year(app, db): """First number minted in a year produces seq=1.""" action = EPApprovalAcceptAction.__new__(EPApprovalAcceptAction) - assert action._generate_report_number(EP_PATTERN) == f"CERN-EP-{YEAR}-001" + assert action._next_report_number(EP_RN_CONFIG) == f"CERN-EP-{YEAR}-001" def test_generate_report_number_sequential_increment(app, db): @@ -188,7 +189,7 @@ def test_generate_report_number_sequential_increment(app, db): ) db.session.commit() - assert action._generate_report_number(EP_PATTERN) == f"CERN-EP-{YEAR}-003" + assert action._next_report_number(EP_RN_CONFIG) == f"CERN-EP-{YEAR}-003" def test_generate_report_number_independent_prefix_counters(app, db): @@ -204,11 +205,8 @@ def test_generate_report_number_independent_prefix_counters(app, db): ) db.session.commit() - assert action._generate_report_number(EP_PATTERN) == f"CERN-EP-{YEAR}-002" - assert ( - action._generate_report_number("CERN-TH-{year}-{seq:03d}") - == f"CERN-TH-{YEAR}-001" - ) + assert action._next_report_number(EP_RN_CONFIG) == f"CERN-EP-{YEAR}-002" + assert action._next_report_number(TH_RN_CONFIG) == f"CERN-TH-{YEAR}-001" # ---------------------------------------------------------------------------