Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optional-doi: fix new upload disabled states #1932

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import { i18next } from "@translations/invenio_rdm_records/i18next";

import PropTypes from "prop-types";
import React, { Component } from "react";
import { connect } from "react-redux";
import { Form, Radio, Popup } from "semantic-ui-react";
import _isEmpty from "lodash/isEmpty";
import { DepositStatus } from "../../../../state/reducers/deposit";

/**
* Manage radio buttons choices between managed (i.e. datacite), unmanaged (i.e. external) and no need for a PID.
*/
export class OptionalDOIoptions extends Component {
class OptionalDOIoptionsCmp extends Component {
handleChange = (e, { value }) => {
const { onManagedUnmanagedChange } = this.props;
const isManagedSelected = value === "managed";
Expand All @@ -23,24 +24,39 @@ export class OptionalDOIoptions extends Component {
};

_render = (cmp, shouldWrapPopup, message) =>
shouldWrapPopup ? <Popup content={message} trigger={cmp} /> : cmp;
shouldWrapPopup && message ? <Popup content={message} trigger={cmp} /> : cmp;

render() {
const { isManagedSelected, isNoNeedSelected, pidLabel, optionalDOItransitions } =
this.props;

const allDOIoptionsAllowed = _isEmpty(optionalDOItransitions);
const isUnManagedDisabled =
isManagedSelected ||
(!allDOIoptionsAllowed &&
!optionalDOItransitions.allowed_providers.includes("external"));
const isNoNeedDisabled =
isManagedSelected ||
(!allDOIoptionsAllowed &&
!optionalDOItransitions.allowed_providers.includes("not_needed"));
const {
isManagedSelected,
isNoNeedSelected,
pidLabel,
optionalDOItransitions,
record,
} = this.props;

const doi = record?.pids?.doi?.identifier || "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: see my comment below(can be replacing empty string) why the empty string is needed in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty string is the initial state when the form is rendered. This was carried over from the existing implementation and I didn't refactor it. If you don't think it is a major blocker I would avoid refactoring more now...

const alreadyPublished = [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrcastro2 I changes to this. Better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks! more readable IMO 🚀

DepositStatus.PUBLISHED,
DepositStatus.NEW_VERSION_DRAFT,
].includes(record.status);

const hasDoi = doi !== "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: wouldn't isEmpty be safer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why safer? I agree it would be more elegant but I think also if we were to replace it would need more testing as the isEmpty would catch all falsy values...If you think is major, I could have a look!

const isUnManagedDisabled = alreadyPublished
? !optionalDOItransitions.allowed_providers.includes("external")
: hasDoi && isManagedSelected;

const isNoNeedDisabled = alreadyPublished
? !optionalDOItransitions.allowed_providers.includes("not_needed")
: hasDoi && isManagedSelected;

const isManagedTransition = optionalDOItransitions?.allowed_providers?.some(
(val) => !["external", "not_needed"].includes(val)
);
// The locally managed DOI is disabled either if the external provider is allowed or if the no need option is allowed
const isManagedDisabled =
!allDOIoptionsAllowed && (!isUnManagedDisabled || !isNoNeedDisabled);
const isManagedDisabled = alreadyPublished
? !isManagedTransition
: hasDoi && isManagedSelected;

const yesIHaveOne = (
<Form.Field width={4}>
Expand Down Expand Up @@ -93,28 +109,31 @@ export class OptionalDOIoptions extends Component {
</Form.Field>
{this._render(
yesIHaveOne,
isUnManagedDisabled && !isManagedSelected,
isUnManagedDisabled,
optionalDOItransitions?.message
)}
{this._render(noINeedOne, isManagedDisabled, optionalDOItransitions?.message)}
{this._render(
noNeed,
isNoNeedDisabled && !isManagedSelected,
optionalDOItransitions?.message
)}
{this._render(noNeed, isNoNeedDisabled, optionalDOItransitions?.message)}
</Form.Group>
);
}
}

OptionalDOIoptions.propTypes = {
OptionalDOIoptionsCmp.propTypes = {
isManagedSelected: PropTypes.bool.isRequired,
isNoNeedSelected: PropTypes.bool.isRequired,
onManagedUnmanagedChange: PropTypes.func.isRequired,
pidLabel: PropTypes.string,
optionalDOItransitions: PropTypes.array.isRequired,
record: PropTypes.object.isRequired,
};

OptionalDOIoptions.defaultProps = {
OptionalDOIoptionsCmp.defaultProps = {
pidLabel: undefined,
};

const mapStateToProps = (state) => ({
record: state.deposit.record,
});

export const OptionalDOIoptions = connect(mapStateToProps, null)(OptionalDOIoptionsCmp);
Loading