From 4d5c649259fcdde84a805925abcade7378afc04a Mon Sep 17 00:00:00 2001 From: Anika Churilova Date: Thu, 5 Dec 2024 11:40:12 +0100 Subject: [PATCH 1/6] doi: handle UI for optional DOI feature * closes https://github.com/CERNDocumentServer/cds-rdm/issues/163 Co-authored-by: Zacharias Zacharodimos --- .../src/deposit/api/DepositApiClient.js | 7 +- .../controls/PublishButton/PublishButton.js | 58 +++++++-- .../deposit/fields/Identifiers/PIDField.js | 123 ++++++++++++++---- invenio_rdm_records/config.py | 1 + invenio_rdm_records/records/api.py | 32 ++++- .../resources/serializers/datacite/schema.py | 2 +- .../services/components/pids.py | 101 +++++++++++++- .../components/test_pids_component.py | 37 +++--- tests/services/test_rdm_service.py | 114 +++++++++++++++- 9 files changed, 408 insertions(+), 67 deletions(-) diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/api/DepositApiClient.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/api/DepositApiClient.js index 0d35c2c8d..7a443698a 100644 --- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/api/DepositApiClient.js +++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/api/DepositApiClient.js @@ -103,7 +103,12 @@ export class RDMDepositApiClient extends DepositApiClient { ); return new DepositApiClientResponse(data, errors); } catch (error) { - const errorData = error.response.data; + let errorData = error.response.data; + const errors = this.recordSerializer.deserializeErrors( + error.response.data.errors || [] + ); + // this is to serialize raised error from the backend on publish + if (errors) errorData = errors; throw new DepositApiClientResponse({}, errorData); } } diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/PublishButton.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/PublishButton.js index 71dafb704..34518b157 100644 --- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/PublishButton.js +++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/controls/PublishButton/PublishButton.js @@ -18,6 +18,8 @@ import { DepositFormSubmitContext, } from "../../api/DepositFormSubmitContext"; import { DRAFT_PUBLISH_STARTED } from "../../state/types"; +import { scrollTop } from "../../utils"; +import { DRAFT_PUBLISH_FAILED_WITH_VALIDATION_ERRORS } from "../../state/types"; class PublishButtonComponent extends Component { state = { isConfirmModalOpen: false }; @@ -30,14 +32,36 @@ class PublishButtonComponent extends Component { handlePublish = (event, handleSubmit, publishWithoutCommunity) => { const { setSubmitContext } = this.context; - - setSubmitContext( - publishWithoutCommunity - ? DepositFormSubmitActions.PUBLISH_WITHOUT_COMMUNITY - : DepositFormSubmitActions.PUBLISH - ); - handleSubmit(event); - this.closeConfirmModal(); + const { formik, raiseDOINeededButNotReserved, isDOIRequired } = this.props; + const noINeedOne = formik?.values?.noINeedOne; + // Check for explicit DOI reservation via the "GET DOI button" only when DOI is + // optional in the instance's settings. If it is required, backend will automatically + // mint one even if it was not explicitly reserved + const shouldCheckForExplicitDOIReservation = + isDOIRequired !== undefined && // isDOIRequired is undefined when no value was provided from Invenio-app-rdm + !isDOIRequired && + noINeedOne && + Object.keys(formik?.values?.pids).length === 0; + if (shouldCheckForExplicitDOIReservation) { + const errors = { + pids: { + doi: i18next.t("DOI is needed. Please click on the button to reserve it."), + }, + }; + formik.setErrors(errors); + raiseDOINeededButNotReserved(formik?.values, errors); + this.closeConfirmModal(); + } else { + setSubmitContext( + publishWithoutCommunity + ? DepositFormSubmitActions.PUBLISH_WITHOUT_COMMUNITY + : DepositFormSubmitActions.PUBLISH + ); + handleSubmit(event); + this.closeConfirmModal(); + } + // scroll top to show the global error + scrollTop(); }; isDisabled = (values, isSubmitting, filesState) => { @@ -67,6 +91,7 @@ class PublishButtonComponent extends Component { publishWithoutCommunity, formik, publishModalExtraContent, + raiseDOINeededButNotReserved, ...ui } = this.props; const { isConfirmModalOpen } = this.state; @@ -139,6 +164,8 @@ PublishButtonComponent.propTypes = { formik: PropTypes.object.isRequired, publishModalExtraContent: PropTypes.string, filesState: PropTypes.object, + raiseDOINeededButNotReserved: PropTypes.func.isRequired, + isDOIRequired: PropTypes.bool, }; PublishButtonComponent.defaultProps = { @@ -147,15 +174,22 @@ PublishButtonComponent.defaultProps = { actionState: undefined, publishModalExtraContent: undefined, filesState: undefined, + isDOIRequired: undefined, }; const mapStateToProps = (state) => ({ actionState: state.deposit.actionState, publishModalExtraContent: state.deposit.config.publish_modal_extra, filesState: state.files, + isDOIRequired: state.deposit.config.is_doi_required, }); -export const PublishButton = connect( - mapStateToProps, - null -)(connectFormik(PublishButtonComponent)); +export const PublishButton = connect(mapStateToProps, (dispatch) => { + return { + raiseDOINeededButNotReserved: (data, errors) => + dispatch({ + type: DRAFT_PUBLISH_FAILED_WITH_VALIDATION_ERRORS, + payload: { data: data, errors: errors }, + }), + }; +})(connectFormik(PublishButtonComponent)); diff --git a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/fields/Identifiers/PIDField.js b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/fields/Identifiers/PIDField.js index bd5b00cae..e49437ef1 100644 --- a/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/fields/Identifiers/PIDField.js +++ b/invenio_rdm_records/assets/semantic-ui/js/invenio_rdm_records/src/deposit/fields/Identifiers/PIDField.js @@ -33,7 +33,7 @@ const getFieldErrors = (form, fieldPath) => { */ class ReservePIDBtn extends Component { render() { - const { disabled, handleReservePID, label, loading } = this.props; + const { disabled, handleReservePID, label, loading, fieldError } = this.props; return ( {({ form: formik }) => ( @@ -44,6 +44,7 @@ class ReservePIDBtn extends Component { disabled={disabled || loading} onClick={(e) => handleReservePID(e, formik)} content={label} + error={fieldError} /> )} @@ -54,6 +55,7 @@ class ReservePIDBtn extends Component { ReservePIDBtn.propTypes = { disabled: PropTypes.bool, handleReservePID: PropTypes.func.isRequired, + fieldError: PropTypes.object, label: PropTypes.string.isRequired, loading: PropTypes.bool, }; @@ -61,6 +63,7 @@ ReservePIDBtn.propTypes = { ReservePIDBtn.defaultProps = { disabled: false, loading: false, + fieldError: null, }; /** @@ -110,11 +113,13 @@ class ManagedUnmanagedSwitch extends Component { handleChange = (e, { value }) => { const { onManagedUnmanagedChange } = this.props; const isManagedSelected = value === "managed"; - onManagedUnmanagedChange(isManagedSelected); + const isNoNeedSelected = value === "notneeded"; + onManagedUnmanagedChange(isManagedSelected, isNoNeedSelected); }; render() { - const { disabled, isManagedSelected, pidLabel } = this.props; + const { disabled, isManagedSelected, isNoNeedSelected, pidLabel, required } = + this.props; return ( @@ -123,28 +128,41 @@ class ManagedUnmanagedSwitch extends Component { pidLabel: pidLabel, })} - + - + + {!required && ( + + + + )} ); } @@ -153,8 +171,10 @@ class ManagedUnmanagedSwitch extends Component { ManagedUnmanagedSwitch.propTypes = { disabled: PropTypes.bool, isManagedSelected: PropTypes.bool.isRequired, + isNoNeedSelected: PropTypes.bool.isRequired, onManagedUnmanagedChange: PropTypes.func.isRequired, pidLabel: PropTypes.string, + required: PropTypes.bool.isRequired, }; ManagedUnmanagedSwitch.defaultProps = { @@ -198,6 +218,8 @@ class ManagedIdentifierComponent extends Component { identifier, pidPlaceholder, pidType, + form, + fieldPath, } = this.props; const hasIdentifier = identifier !== ""; @@ -209,6 +231,7 @@ class ManagedIdentifierComponent extends Component { actionState === RESERVE_PID_STARTED && actionStateExtra.pidType === pidType } handleReservePID={this.handleReservePID} + fieldError={getFieldErrors(form, fieldPath)} /> ); @@ -253,6 +276,8 @@ ManagedIdentifierComponent.propTypes = { btnLabelDiscardPID: PropTypes.string.isRequired, pidPlaceholder: PropTypes.string.isRequired, pidType: PropTypes.string.isRequired, + form: PropTypes.object.isRequired, + fieldPath: PropTypes.string.isRequired, /* from Redux */ actionState: PropTypes.string, actionStateExtra: PropTypes.object, @@ -307,7 +332,7 @@ class UnmanagedIdentifierCmp extends Component { render() { const { localIdentifier } = this.state; - const { form, fieldPath, helpText, pidPlaceholder } = this.props; + const { form, fieldPath, helpText, pidPlaceholder, disabled } = this.props; const fieldError = getFieldErrors(form, fieldPath); return ( <> @@ -318,6 +343,7 @@ class UnmanagedIdentifierCmp extends Component { placeholder={pidPlaceholder} width={16} error={fieldError} + disabled={disabled} /> {helpText && } @@ -333,10 +359,12 @@ UnmanagedIdentifierCmp.propTypes = { identifier: PropTypes.string.isRequired, onIdentifierChanged: PropTypes.func.isRequired, pidPlaceholder: PropTypes.string.isRequired, + disabled: PropTypes.bool, }; UnmanagedIdentifierCmp.defaultProps = { helpText: null, + disabled: false, }; /** @@ -349,11 +377,18 @@ class CustomPIDField extends Component { constructor(props) { super(props); - const { canBeManaged, canBeUnmanaged } = this.props; + const { canBeManaged, canBeUnmanaged, record, field } = this.props; this.canBeManagedAndUnmanaged = canBeManaged && canBeUnmanaged; + const value = field?.value; + const isInternalProvider = value?.provider !== PROVIDER_EXTERNAL; + const isDraft = record?.is_draft === true; + const hasIdentifier = value?.identifier; + const isManagedSelected = + isDraft && hasIdentifier && isInternalProvider ? true : undefined; this.state = { - isManagedSelected: undefined, + isManagedSelected: isManagedSelected, + isNoNeedSelected: undefined, }; } @@ -373,7 +408,7 @@ class CustomPIDField extends Component { }; render() { - const { isManagedSelected } = this.state; + const { isManagedSelected, isNoNeedSelected } = this.state; const { btnLabelDiscardPID, btnLabelGetPID, @@ -394,6 +429,8 @@ class CustomPIDField extends Component { record, } = this.props; + let { doiDefaultSelection } = this.props; + const value = field.value || {}; const currentIdentifier = value.identifier || ""; const currentProvider = value.provider || ""; @@ -407,19 +444,43 @@ class CustomPIDField extends Component { } const hasManagedIdentifier = managedIdentifier !== ""; + const hasUnmanagedIdentifier = unmanagedIdentifier !== ""; + const doi = record?.pids?.doi?.identifier || ""; + const parentDoi = record.parent?.pids?.doi?.identifier || ""; + + const hasDoi = doi !== ""; + const hasParentDoi = parentDoi !== ""; + const isDoiCreated = currentIdentifier !== ""; + const isDraft = record.is_draft; + + const _isUnmanagedSelected = + isManagedSelected === undefined + ? hasUnmanagedIdentifier || + (currentIdentifier === "" && doiDefaultSelection === "yes") + : !isManagedSelected; const _isManagedSelected = isManagedSelected === undefined - ? hasManagedIdentifier || currentProvider === "" // i.e pids: {} + ? hasManagedIdentifier || + (currentIdentifier === "" && doiDefaultSelection === "no") // i.e pids: {} : isManagedSelected; - const doi = record?.pids?.doi?.identifier || ""; - const hasDoi = doi !== ""; - const isDoiCreated = currentIdentifier !== ""; + const _isNoNeedSelected = + isNoNeedSelected === undefined + ? (!_isManagedSelected && !_isUnmanagedSelected) || + (isDraft !== true && + currentIdentifier === "" && + doiDefaultSelection === "not_needed") + : isNoNeedSelected; + const fieldError = getFieldErrors(form, fieldPath); + return ( <> - + @@ -427,20 +488,32 @@ class CustomPIDField extends Component { { + isNoNeedSelected={_isNoNeedSelected} + onManagedUnmanagedChange={(userSelectedManaged, userSelectedNoNeed) => { if (userSelectedManaged) { form.setFieldValue("pids", {}); + if (!required) { + // We set the + form.setFieldValue("noINeedOne", true); + } + } else if (userSelectedNoNeed) { + form.setFieldValue("pids", {}); + form.setFieldValue("noINeedOne", false); } else { this.onExternalIdentifierChanged(""); + form.setFieldValue("noINeedOne", false); } + form.setFieldError(fieldPath, false); this.setState({ isManagedSelected: userSelectedManaged, + isNoNeedSelected: userSelectedNoNeed, }); }} pidLabel={pidLabel} + required={required} /> )} @@ -450,6 +523,7 @@ class CustomPIDField extends Component { btnLabelDiscardPID={btnLabelDiscardPID} btnLabelGetPID={btnLabelGetPID} form={form} + fieldPath={fieldPath} identifier={managedIdentifier} helpText={managedHelpText} pidPlaceholder={pidPlaceholder} @@ -458,7 +532,7 @@ class CustomPIDField extends Component { /> )} - {canBeUnmanaged && !_isManagedSelected && ( + {canBeUnmanaged && (!_isManagedSelected || _isNoNeedSelected) && ( { @@ -468,6 +542,7 @@ class CustomPIDField extends Component { fieldPath={fieldPath} pidPlaceholder={pidPlaceholder} helpText={unmanagedHelpText} + disabled={_isNoNeedSelected || isEditingPublishedRecord} /> )} @@ -493,6 +568,7 @@ CustomPIDField.propTypes = { required: PropTypes.bool.isRequired, unmanagedHelpText: PropTypes.string, record: PropTypes.object.isRequired, + doiDefaultSelection: PropTypes.object.isRequired, }; CustomPIDField.defaultProps = { @@ -542,6 +618,7 @@ PIDField.propTypes = { required: PropTypes.bool, unmanagedHelpText: PropTypes.string, record: PropTypes.object.isRequired, + doiDefaultSelection: PropTypes.object.isRequired, }; PIDField.defaultProps = { diff --git a/invenio_rdm_records/config.py b/invenio_rdm_records/config.py index 9ea88dddd..fdb96df32 100644 --- a/invenio_rdm_records/config.py +++ b/invenio_rdm_records/config.py @@ -391,6 +391,7 @@ def always_valid(identifier): "validator": idutils.is_doi, "normalizer": idutils.normalize_doi, "is_enabled": providers.DataCitePIDProvider.is_enabled, + "ui": {"default_selected": "yes"}, # "yes", "no" or "not_needed" }, "oai": { "providers": ["oai"], diff --git a/invenio_rdm_records/records/api.py b/invenio_rdm_records/records/api.py index 83180c770..7646105d1 100644 --- a/invenio_rdm_records/records/api.py +++ b/invenio_rdm_records/records/api.py @@ -536,10 +536,40 @@ def get_latest_published_by_parent(cls, parent): published yet or all versions are deleted. """ latest_record = cls.get_latest_by_parent(parent) - if latest_record.deletion_status != RecordDeletionStatusEnum.PUBLISHED.value: + if ( + latest_record + and latest_record.deletion_status + != RecordDeletionStatusEnum.PUBLISHED.value + ): return None return latest_record + @classmethod + def get_previous_published_by_parent(cls, parent): + """Get the previous of latest published record for the specified parent record. + + It might return None if there is no latest published version i.e not + published yet or all versions are deleted or there is only one published record. + + This method is needed instead of `get_latest_published_by_parent` because during + publish the version state is updated before the record is actually published. + That means, that `get_latest_published_by_parent` returns always the record that + is about to be published and thus, we cannot use it in the `component.publish()` + method to retrieve the actual last published record. + + Check `services.components.pids.PIDsComponent.publish()` for how it is used. + """ + # We need no_autoflush because the record.versions access triggers automatically + # one + with db.session.no_autoflush: + records = cls.get_records_by_parent(parent) + for record in records: + latest_version_index = record.versions.latest_index + if latest_version_index > 1: + if record.versions.index == latest_version_index - 1: + return record + return None + RDMFileRecord.record_cls = RDMRecord diff --git a/invenio_rdm_records/resources/serializers/datacite/schema.py b/invenio_rdm_records/resources/serializers/datacite/schema.py index 27b65b4e7..ee9889d26 100644 --- a/invenio_rdm_records/resources/serializers/datacite/schema.py +++ b/invenio_rdm_records/resources/serializers/datacite/schema.py @@ -417,7 +417,7 @@ def get_related_identifiers(self, obj): params={"_source_includes": "pids.doi"}, ) for version in record_versions: - version_doi = version["pids"]["doi"] + version_doi = version.get("pids", {}).get("doi") id_scheme = get_scheme_datacite( "doi", "RDM_RECORDS_IDENTIFIERS_SCHEMES", diff --git a/invenio_rdm_records/services/components/pids.py b/invenio_rdm_records/services/components/pids.py index 8a08a67b2..20d50fed7 100644 --- a/invenio_rdm_records/services/components/pids.py +++ b/invenio_rdm_records/services/components/pids.py @@ -15,14 +15,76 @@ from flask import current_app from invenio_drafts_resources.services.records.components import ServiceComponent from invenio_drafts_resources.services.records.uow import ParentRecordCommitOp +from invenio_i18n import lazy_gettext as _ from invenio_records_resources.services.uow import TaskOp +from ..errors import ValidationErrorWithMessageAsList from ..pids.tasks import register_or_update_pid class PIDsComponent(ServiceComponent): """Service component for PIDs.""" + ALLOWED_DOI_PROVIDERS_TRANSITIONS = { + "datacite": { + "allowed_providers": ["datacite"], + "message": _( + "A previous version used a DOI registered from {sitename}. This version must also use a DOI from {sitename}." + ), + }, + "external": { + "allowed_providers": ["external", "not_needed"], + "message": _( + "A previous version was published with a DOI from an external provider or without one. You cannot use a DOI registered from {sitename} for this version." + ), + }, + "not_needed": { + "allowed_providers": ["external", "not_needed"], + "message": _( + "A previous version was published with a DOI from an external provider or without one. You cannot use a DOI registered from {sitename} for this version." + ), + }, + } + + def _validate_doi_transition( + self, new_provider, previous_published_provider, errors=None + ): + """If DOI is not required then we validate allowed DOI providers. + + Each new version that is published must follow the ALLOWED_DOI_PROVIDERS_TRANSITIONS. + """ + sitename = current_app.config.get("THEME_SITENAME", "this repository") + sitename = current_app.config.get("THEME_SITENAME", "this repository") + + valid_transitions = self.ALLOWED_DOI_PROVIDERS_TRANSITIONS.get( + previous_published_provider, {} + ) + if new_provider not in valid_transitions.get("allowed_providers", []): + error_message = { + "field": "pids.doi", + "messages": [ + valid_transitions.get("message").format(sitename=sitename) + ], + } + + if errors is not None: + errors.append(error_message) + else: + raise ValidationErrorWithMessageAsList(message=[error_message]) + + def _validate_optional_doi(self, record, previous_published, errors=None): + """Reusable method to validate optional DOI.""" + if previous_published: + previous_published_pids = previous_published.get("pids", {}) + doi_pid = [pid for pid in record.pids.values() if "doi" in record.pids] + previous_published_provider = previous_published_pids.get("doi", {}).get( + "provider", "not_needed" + ) + new_provider = "not_needed" if not doi_pid else doi_pid[0]["provider"] + self._validate_doi_transition( + new_provider, previous_published_provider, errors + ) + def create(self, identity, data=None, record=None, errors=None): """This method is called on draft creation. @@ -41,6 +103,16 @@ def update_draft(self, identity, data=None, record=None, errors=None): if "pids" in data: # there is new input data for PIDs pids_data = data["pids"] + required_schemes = set(self.service.config.pids_required) + + # if DOI is not required in an instance check validate allowed providers + # for each record version + if "doi" not in required_schemes: + previous_published = self.service.record_cls.get_latest_published_by_parent( + record.parent + ) + self._validate_optional_doi(record, previous_published, errors) + self.service.pids.pid_manager.validate(pids_data, record, errors) record.pids = pids_data @@ -75,7 +147,19 @@ def publish(self, identity, draft=None, record=None): record_schemes = set(record_pids.keys()) required_schemes = set(self.service.config.pids_required) - # Validate the draft PIDs + # if DOI is not required in an instance check validate allowed providers + # for each record version + if "doi" not in required_schemes: + # if a doi was ever minted for the parent record then we always require one + # for any version of the record that will be published + if draft.parent.get("pids", {}).get("doi"): + required_schemes.add("doi") + + previous_published = ( + self.service.record_cls.get_previous_published_by_parent(record.parent) + ) + self._validate_optional_doi(draft, previous_published) + self.service.pids.pid_manager.validate(draft_pids, draft, raise_errors=True) # Detect which PIDs on a published record that has been changed. @@ -129,12 +213,7 @@ def publish(self, identity, draft=None, record=None): def new_version(self, identity, draft=None, record=None): """A new draft should not have any pids from the previous record.""" - # This makes the draft use the same identifier as the previous - # version - if record.pids.get("doi", {}).get("provider") == "external": - draft.pids = {"doi": {"provider": "external", "identifier": ""}} - else: - draft.pids = {} + draft.pids = {} def edit(self, identity, draft=None, record=None): """Add current pids from the record to the draft. @@ -172,6 +251,14 @@ def publish(self, identity, draft=None, record=None): current_schemes = set(current_pids.keys()) required_schemes = set(self.service.config.parent_pids_required) + # Check if a doi was added in the draft and create a parent DOI independently if + # doi is required. + # Note: we don't have to check explicitely to the parent DOI creation only for + # datacite provider because we pass a `condition_func` below that it omits the + # minting if the pid selected is external + if draft.get("pids", {}).get("doi"): + required_schemes.add("doi") + conditional_schemes = self.service.config.parent_pids_conditional for scheme in set(required_schemes): condition_func = conditional_schemes.get(scheme) diff --git a/tests/services/components/test_pids_component.py b/tests/services/components/test_pids_component.py index cbd3dc841..dc102a6f0 100644 --- a/tests/services/components/test_pids_component.py +++ b/tests/services/components/test_pids_component.py @@ -123,12 +123,11 @@ class TestServiceConfigRequiredExternalPID(RDMRecordServiceConfig): @pytest.fixture(scope="module") -def no_pids_cmp(): +def no_pids_cmp(app): + service_config = TestServiceConfigNoPIDs.build(app) service = RDMRecordService( - config=TestServiceConfigNoPIDs, - pids_service=PIDsService( - config=TestServiceConfigNoPIDs, manager_cls=PIDManager - ), + config=service_config, + pids_service=PIDsService(config=service_config, manager_cls=PIDManager), ) c = PIDsComponent(service=service) c.uow = UnitOfWork() @@ -136,12 +135,11 @@ def no_pids_cmp(): @pytest.fixture(scope="module") -def no_required_pids_service(): +def no_required_pids_service(app): + service_config = TestServiceConfigNoRequiredPIDs.build(app) return RDMRecordService( - config=TestServiceConfigNoRequiredPIDs, - pids_service=PIDsService( - config=TestServiceConfigNoRequiredPIDs, manager_cls=PIDManager - ), + config=service_config, + pids_service=PIDsService(config=service_config, manager_cls=PIDManager), ) @@ -153,12 +151,11 @@ def no_required_pids_cmp(no_required_pids_service): @pytest.fixture(scope="module") -def required_managed_pids_cmp(): +def required_managed_pids_cmp(app): + service_config = TestServiceConfigRequiredManagedPID.build(app) service = RDMRecordService( - config=TestServiceConfigRequiredManagedPID, - pids_service=PIDsService( - config=TestServiceConfigRequiredManagedPID, manager_cls=PIDManager - ), + config=service_config, + pids_service=PIDsService(config=service_config, manager_cls=PIDManager), ) c = PIDsComponent(service=service) c.uow = UnitOfWork() @@ -166,12 +163,11 @@ def required_managed_pids_cmp(): @pytest.fixture(scope="module") -def required_external_pids_cmp(): +def required_external_pids_cmp(app): + service_config = TestServiceConfigRequiredExternalPID.build(app) service = RDMRecordService( - config=TestServiceConfigRequiredExternalPID, - pids_service=PIDsService( - config=TestServiceConfigRequiredExternalPID, manager_cls=PIDManager - ), + config=service_config, + pids_service=PIDsService(config=service_config, manager_cls=PIDManager), ) c = PIDsComponent(service=service) c.uow = UnitOfWork() @@ -318,6 +314,7 @@ def test_publish_no_pids(no_pids_cmp, minimal_record, identity_simple, location) ], ) def test_publish_no_required_pids( + app, pids, no_required_pids_service, no_required_pids_cmp, diff --git a/tests/services/test_rdm_service.py b/tests/services/test_rdm_service.py index ae1158d99..daf7aedb2 100644 --- a/tests/services/test_rdm_service.py +++ b/tests/services/test_rdm_service.py @@ -10,10 +10,15 @@ """Service level tests for Invenio RDM Records.""" +from copy import deepcopy + import pytest from invenio_rdm_records.proxies import current_rdm_records -from invenio_rdm_records.services.errors import EmbargoNotLiftedError +from invenio_rdm_records.services.errors import ( + EmbargoNotLiftedError, + ValidationErrorWithMessageAsList, +) def test_minimal_draft_creation(running_app, search_clear, minimal_record): @@ -50,7 +55,7 @@ def test_draft_w_languages_creation(running_app, search_clear, minimal_record): def test_publish_public_record_with_default_doi( - running_app, search_clear, minimal_record + running_app, search_clear, minimal_record, uploader ): superuser_identity = running_app.superuser_identity service = current_rdm_records.records_service @@ -68,10 +73,115 @@ def test_publish_public_record_with_optional_doi( draft = service.create(superuser_identity, minimal_record) record = service.publish(id_=draft.id, identity=superuser_identity) assert "doi" not in record._record.pids + assert "doi" not in record._record.parent.pids + # Reset the running_app config for next tests + running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = True + + +def test_publish_public_record_versions_no_or_external_doi_managed_doi( + running_app, search_clear, minimal_record, verified_user +): + running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = False + verified_user_identity = verified_user.identity + service = current_rdm_records.records_service + # Publish without DOI + draft = service.create(verified_user_identity, minimal_record) + record = service.publish(id_=draft.id, identity=verified_user_identity) + assert "doi" not in record._record.pids + assert "doi" not in record._record.parent.pids + + # create a new version with an external DOI + draft = service.new_version(verified_user_identity, record.id) + draft_data = deepcopy(draft.data) + draft_data["metadata"]["publication_date"] = "2023-01-01" + draft_data["pids"]["doi"] = { + "identifier": "10.4321/test.1234", + "provider": "external", + } + draft = service.update_draft(verified_user_identity, draft.id, data=draft_data) + record = service.publish(id_=draft.id, identity=verified_user_identity) + assert "doi" in record._record.pids + assert "doi" not in record._record.parent.pids + + # create a new version and and try to mint a managed DOI now when you publish + draft = service.new_version(verified_user_identity, record.id) + draft = service.pids.create(verified_user_identity, draft.id, "doi") + draft_data = deepcopy(draft.data) + draft_data["metadata"]["publication_date"] = "2023-01-01" + draft = service.update_draft(verified_user_identity, draft.id, data=draft_data) + + with pytest.raises(ValidationErrorWithMessageAsList): + record = service.publish(id_=draft.id, identity=verified_user_identity) + + # Reset the running_app config for next tests + running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = True + + +def test_publish_public_record_versions_managed_doi_external_doi( + running_app, search_clear, minimal_record, verified_user +): + running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = False + verified_user_identity = verified_user.identity + service = current_rdm_records.records_service + # Publish with locally managed DOI + draft = service.create(verified_user_identity, minimal_record) + draft = service.pids.create(verified_user_identity, draft.id, "doi") + record = service.publish(id_=draft.id, identity=verified_user_identity) + assert "doi" in record._record.pids + assert "doi" in record._record.parent.pids + + # create a new version with an external DOI + draft = service.new_version(verified_user_identity, record.id) + draft_data = deepcopy(draft.data) + draft_data["metadata"]["publication_date"] = "2023-01-01" + draft_data["pids"]["doi"] = { + "identifier": "10.4321/test.1234", + "provider": "external", + } + draft = service.update_draft(verified_user_identity, draft.id, data=draft_data) + with pytest.raises(ValidationErrorWithMessageAsList): + record = service.publish(id_=draft.id, identity=verified_user_identity) + # Reset the running_app config for next tests running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = True +def test_publish_public_record_versions_managed_doi_no_doi( + running_app, search_clear, minimal_record, verified_user +): + running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = False + verified_user_identity = verified_user.identity + service = current_rdm_records.records_service + # Publish with locally managed DOI + draft = service.create(verified_user_identity, minimal_record) + draft = service.pids.create(verified_user_identity, draft.id, "doi") + record = service.publish(id_=draft.id, identity=verified_user_identity) + assert "doi" in record._record.pids + assert "doi" in record._record.parent.pids + + # create a new version with no DOI + draft = service.new_version(verified_user_identity, record.id) + draft_data = deepcopy(draft.data) + draft_data["metadata"]["publication_date"] = "2023-01-01" + draft_data["pids"] = {} + draft = service.update_draft(verified_user_identity, draft.id, data=draft_data) + with pytest.raises(ValidationErrorWithMessageAsList): + record = service.publish(id_=draft.id, identity=verified_user_identity) + + # Reset the running_app config for next tests + running_app.app.config["RDM_PERSISTENT_IDENTIFIERS"]["doi"]["required"] = True + + +def test_publish_public_record_with_default_doi( + running_app, search_clear, minimal_record, uploader +): + superuser_identity = running_app.superuser_identity + service = current_rdm_records.records_service + draft = service.create(superuser_identity, minimal_record) + record = service.publish(id_=draft.id, identity=superuser_identity) + assert "doi" in record._record.pids + + def test_publish_restricted_record_without_default_doi( running_app, search_clear, minimal_restricted_record ): From 428e479441ebd026a86610661ecf0b1b7f80a5a8 Mon Sep 17 00:00:00 2001 From: Christoph Ladurner Date: Thu, 12 Dec 2024 11:32:43 +0100 Subject: [PATCH 2/6] setup: bump major dependencies --- setup.cfg | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/setup.cfg b/setup.cfg index 6c57b8c56..827ff868f 100644 --- a/setup.cfg +++ b/setup.cfg @@ -3,7 +3,7 @@ # Copyright (C) 2019-2024 CERN. # Copyright (C) 2019 Northwestern University. # Copyright (C) 2022 Universität Hamburg. -# Copyright (C) 2022 Graz University of Technology. +# Copyright (C) 2022-2024 Graz University of Technology. # # Invenio-RDM-Records is free software; you can redistribute it and/or modify it # under the terms of the MIT License; see LICENSE file for more details. @@ -38,17 +38,17 @@ install_requires = Faker>=2.0.3 flask-iiif>=1.0.0,<2.0.0 ftfy>=4.4.3,<5.0.0 - invenio-administration>=2.0.0,<3.0.0 - invenio-communities>=17.0.0,<18.0.0 - invenio-drafts-resources>=5.0.0,<6.0.0 - invenio-records-resources>=6.0.0,<7.0.0 - invenio-github>=1.0.0,<2.0.0 - invenio-i18n>=2.0.0,<3.0.0 - invenio-jobs>=1.0.0,<2.0.0 - invenio-oaiserver>=2.0.0,<3.0.0 - invenio-oauth2server>=2.0.0 - invenio-stats>=4.0.0,<5.0.0 - invenio-vocabularies>=6.0.0,<7.0.0 + invenio-administration>=3.0.0,<4.0.0 + invenio-communities>=18.0.0.dev1,<19.0.0 + invenio-drafts-resources>=6.0.0,<7.0.0 + invenio-records-resources>=7.0.0,<8.0.0 + invenio-github>=2.0.0,<3.0.0 + invenio-i18n>=3.0.0,<4.0.0 + invenio-jobs>=3.0.0.dev1,<4.0.0 + invenio-oaiserver>=3.0.0,<4.0.0 + invenio-oauth2server>=3.0.0,<4.0.0 + invenio-stats>=5.0.0,<6.0.0 + invenio-vocabularies>=7.0.0.dev1,<8.0.0 nameparser>=1.1.1 pycountry>=22.3.5 pydash>=6.0.0,<7.0.0 @@ -61,18 +61,18 @@ install_requires = [options.extras_require] tests = pytest-black-ng>=0.4.0 - invenio-app>=1.4.0,<2.0.0 - invenio-db[postgresql,mysql]>=1.0.14,<2.0.0 - pytest-invenio>=2.1.0,<3.0.0 + invenio-app>=2.0.0,<3.0.0 + invenio-db[postgresql,mysql]>=2.0.0,<3.0.0 + pytest-invenio>=3.0.0,<4.0.0 pytest-mock>=1.6.0 sphinx>=4.5.0 tripoli~=2.0.0 elasticsearch7 = - invenio-search[elasticsearch7]>=2.1.0,<3.0.0 + invenio-search[elasticsearch7]>=3.0.0,<4.0.0 opensearch1 = - invenio-search[opensearch1]>=2.1.0,<3.0.0 + invenio-search[opensearch1]>=3.0.0,<4.0.0 opensearch2 = - invenio-search[opensearch2]>=2.1.0,<3.0.0 + invenio-search[opensearch2]>=3.0.0,<4.0.0 [options.entry_points] flask.commands = From 56b18791458ef101f1410329a157e5fbb617e059 Mon Sep 17 00:00:00 2001 From: Christoph Ladurner Date: Thu, 12 Dec 2024 11:33:24 +0100 Subject: [PATCH 3/6] setup: change to reusable workflows --- .github/workflows/pypi-publish.yml | 24 ++------- .github/workflows/tests-feature.yml | 82 ----------------------------- .github/workflows/tests.yml | 6 ++- 3 files changed, 10 insertions(+), 102 deletions(-) delete mode 100644 .github/workflows/tests-feature.yml diff --git a/.github/workflows/pypi-publish.yml b/.github/workflows/pypi-publish.yml index f95887e28..5babc2011 100644 --- a/.github/workflows/pypi-publish.yml +++ b/.github/workflows/pypi-publish.yml @@ -2,6 +2,7 @@ # # Copyright (C) 2021 CERN. # +# Copyright (C) 2024 Graz University of Technology. # Invenio-RDM-Records is free software; you can redistribute it and/or modify # it under the terms of the MIT License; see LICENSE file for more details. @@ -14,22 +15,7 @@ on: jobs: build-n-publish: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - name: Set up Python 3.9 - uses: actions/setup-python@v2 - with: - python-version: 3.9 - - name: Install dependencies - run: | - python -m pip install --upgrade pip - pip install setuptools wheel babel - - name: Build package - run: | - python setup.py compile_catalog sdist bdist_wheel - - name: Publish - uses: pypa/gh-action-pypi-publish@v1.3.1 - with: - user: __token__ - password: ${{ secrets.pypi_token }} + uses: inveniosoftware/workflows/.github/workflows/pypi-publish.yml@master + secrets: inherit + with: + babel-compile-catalog: true diff --git a/.github/workflows/tests-feature.yml b/.github/workflows/tests-feature.yml deleted file mode 100644 index 6ad1823fe..000000000 --- a/.github/workflows/tests-feature.yml +++ /dev/null @@ -1,82 +0,0 @@ -# -*- coding: utf-8 -*- -# -# This file is part of Invenio. -# Copyright (C) 2022 CERN. -# -# Invenio is free software; you can redistribute it and/or modify it -# under the terms of the MIT License; see LICENSE file for more details. - -name: Feature development CI - -on: - pull_request: - branches: - - "feature/**" - schedule: - # * is a special character in YAML so you have to quote this string - - cron: "0 3 * * 6" - workflow_dispatch: - inputs: - reason: - description: "Reason" - required: false - default: "Manual trigger" - -jobs: - Tests: - runs-on: ubuntu-20.04 - strategy: - matrix: - python-version: [3.8] # for efficiency test only one specific python version - requirements-level: [pypi] - cache-service: [redis] - db-service: [postgresql14] - search-service: [opensearch2,elasticsearch7] - include: - - db-service: postgresql14 - DB_EXTRAS: "postgresql" - - - search-service: opensearch2 - SEARCH_EXTRAS: "opensearch2" - - - search-service: elasticsearch7 - SEARCH_EXTRAS: "elasticsearch7" - - env: - CACHE: ${{ matrix.cache-service }} - DB: ${{ matrix.db-service }} - SEARCH: ${{ matrix.search-service }} - EXTRAS: tests,${{ matrix.SEARCH_EXTRAS }} - steps: - - name: Checkout - uses: actions/checkout@v2 - - - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 - with: - python-version: ${{ matrix.python-version }} - - - name: Generate dependencies - run: | - pip install wheel requirements-builder - requirements-builder -e "$EXTRAS" --level=${{ matrix.requirements-level }} setup.py > .${{ matrix.requirements-level }}-${{ matrix.python-version }}-requirements.txt - cat .${{ matrix.requirements-level }}-${{ matrix.python-version }}-requirements.txt - - - name: Cache pip - uses: actions/cache@v2 - with: - path: ~/.cache/pip - key: ${{ runner.os }}-pip-${{ hashFiles('.${{ matrix.requirements-level }}-${{ matrix.python-version }}-requirements.txt') }} - - - name: Install dependencies - run: | - pip install -r .${{ matrix.requirements-level }}-${{ matrix.python-version }}-requirements.txt -c constraints-${{ matrix.requirements-level }}.txt - pip install -r requirements-feature.txt # this file is used only when targeting a feature/* branch - pip install ".[$EXTRAS]" - pip freeze - docker --version - docker-compose --version - - - name: Run tests - run: | - ./run-tests.sh diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a6fdbe823..cfb893846 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -2,6 +2,7 @@ # # Copyright (C) 2020-2024 CERN. # Copyright (C) 2020 Northwestern University. +# Copyright (C) 2024 Graz University of Technology. # # Invenio-RDM-Records is free software; you can redistribute it and/or modify # it under the terms of the MIT License; see LICENSE file for more details. @@ -10,11 +11,14 @@ name: CI on: push: - branches: master + branches: + - master + - "feature/*" pull_request: branches: - master - "maint-**" + - "feature/*" schedule: # * is a special character in YAML so you have to quote this string - cron: "0 3 * * 6" From 441c69c9992318420d2ba44938d668acd5cf5393 Mon Sep 17 00:00:00 2001 From: Christoph Ladurner Date: Thu, 12 Dec 2024 14:48:10 +0100 Subject: [PATCH 4/6] comp: make compatible to flask-sqlalchemy>=3.1 --- tests/test_alembic.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_alembic.py b/tests/test_alembic.py index 0b6df3237..1953ce9a8 100644 --- a/tests/test_alembic.py +++ b/tests/test_alembic.py @@ -2,6 +2,7 @@ # # This file is part of Invenio. # Copyright (C) 2021 TU Wien. +# Copyright (C) 2024 Graz University of Technology. # # Invenio is free software; you can redistribute it and/or modify it # under the terms of the MIT License; see LICENSE file for more details. @@ -10,8 +11,6 @@ import pytest from invenio_db.utils import alembic_test_context, drop_alembic_version_table -from sqlalchemy_continuum import version_class -from sqlalchemy_utils.functions import get_class_by_table @pytest.mark.skip(reason="Caused by mergepoint") @@ -26,7 +25,7 @@ def test_alembic(base_app, database): base_app.config["ALEMBIC_CONTEXT"] = alembic_test_context() # Check that this package's SQLAlchemy models have been properly registered - tables = [x.name for x in db.get_tables_for_bind()] + tables = [x for x in db.metadata.tables] assert "rdm_drafts_files" in tables assert "rdm_drafts_metadata" in tables assert "rdm_records_files" in tables From 890f9b4ebf32a5f5d9cbdee3a031b78daf004103 Mon Sep 17 00:00:00 2001 From: Christoph Ladurner Date: Sun, 15 Dec 2024 21:53:26 +0100 Subject: [PATCH 5/6] fix: flask-sqlalchemy.pagination >= 3.0.0 * this is a compatibility fix. the old compatibility commit was wrong. they are not interchangable --- .../oaiserver/services/services.py | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/invenio_rdm_records/oaiserver/services/services.py b/invenio_rdm_records/oaiserver/services/services.py index 516db9093..47f0dacea 100644 --- a/invenio_rdm_records/oaiserver/services/services.py +++ b/invenio_rdm_records/oaiserver/services/services.py @@ -10,6 +10,7 @@ import re from flask import current_app +from flask_sqlalchemy.pagination import Pagination from invenio_db import db from invenio_i18n import lazy_gettext as _ from invenio_oaiserver.models import OAISet @@ -31,12 +32,25 @@ ) from .uow import OAISetCommitOp, OAISetDeleteOp -try: - # flask_sqlalchemy<3.0.0 - from flask_sqlalchemy import Pagination -except ImportError: - # flask_sqlalchemy>=3.0.0 - from flask_sqlalchemy.pagination import Pagination + +class OAIPagination(Pagination): + """OAI Pagination.""" + + def _query_items(self): + """Return items.""" + try: + return self._query_args["items"] + except KeyError: + msg = "items not set in OAIPaginations constructor." + raise RuntimeError(msg) + + def _query_count(self): + """Return count.""" + try: + return self._query_args["total"] + except KeyError: + msg = "total not set in OAIPaginations constructor." + raise RuntimeError(msg) class OAIPMHServerService(Service): @@ -226,7 +240,7 @@ def read_all_formats(self, identity): for k, v in current_app.config.get("OAISERVER_METADATA_FORMATS").items() ] - results = Pagination( + results = OAIPagination( query=None, page=1, per_page=None, From f733033eb5d332ba7a321d75391788cd08d9005e Mon Sep 17 00:00:00 2001 From: Christoph Ladurner Date: Sun, 15 Dec 2024 22:01:55 +0100 Subject: [PATCH 6/6] release: v17.0.0.dev1 --- CHANGES.rst | 9 ++++++++- invenio_rdm_records/__init__.py | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 5b95b4c80..07ee3b556 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -3,7 +3,7 @@ Copyright (C) 2019-2024 CERN. Copyright (C) 2019-2024 Northwestern University. Copyright (C) 2024 KTH Royal Institute of Technology. - + Copyright (C) 2024 Graz University of Technology. Invenio-RDM-Records is free software; you can redistribute it and/or modify it under the terms of the MIT License; see LICENSE file for more @@ -12,6 +12,13 @@ Changes ======= +Version 17.0.0.dev1 (released 2024-12-16) + +- fix: flask-sqlalchemy.pagination >= 3.0.0 +- comp: make compatible to flask-sqlalchemy>=3.1 +- setup: change to reusable workflows +- setup: bump major dependencies + Version v16.4.1 (released 2024-12-11) - mappings: add missing `identifiers` to community orgs diff --git a/invenio_rdm_records/__init__.py b/invenio_rdm_records/__init__.py index 034902c72..2969bb10a 100644 --- a/invenio_rdm_records/__init__.py +++ b/invenio_rdm_records/__init__.py @@ -12,6 +12,6 @@ from .ext import InvenioRDMRecords -__version__ = "16.4.1" +__version__ = "17.0.0.dev1" __all__ = ("__version__", "InvenioRDMRecords")