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

Adds enablePublishOnAllNonPublic configuration #2203

Closed
wants to merge 1 commit into from

Conversation

helbashandy
Copy link
Collaborator

@helbashandy helbashandy commented Oct 17, 2023

Description

Adds enablePublishOnAllNonPublic configuration. If true, users can see a "Publish" button in the MetadataView on all non-public datasets regardless if a DOI is assigned to the dataset. If false, the default behavior would take place based on the AppConfig#enablePublishDOI config.

closes #2202

@helbashandy helbashandy force-pushed the issue_2202_adding_publish_doi_config branch 3 times, most recently from d583c02 to bca1b3f Compare October 17, 2023 21:26
Copy link
Member

@robyngit robyngit left a comment

Choose a reason for hiding this comment

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

@helbashandy this looks great, thanks! Could you please just update the comments above your code changes to describe the new decision process?

* a DOI is assigned to the dataset
* If false, the default behavior would take place based on the {@link AppConfig#enablePublishDOI} config.
* @type {boolean}
* @default false
Copy link
Member

Choose a reason for hiding this comment

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

When this gets released, I update all the @SInCE x.x.x with the new version.

Suggested change
* @default false
* @default false
* @since x.x.x

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@robyngit - Done! I added the version to be 2.27.0. Please let me know if you are planning to introduce this change with a different release version.

Copy link
Member

Choose a reason for hiding this comment

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

That's perfect @helbashandy, thank you! In the future, just use x.x.x as the version - these are all replaced with the actual version number at the time of release - that way there's flexibility if something has to get pushed to a different release, or the version # changes for any reason.

@robyngit
Copy link
Member

@rushirajnenuji I imagine that your work on hierarchical folders could involve lots of changes to the MetadataView. Does this PR conflict with any of your work?

@helbashandy helbashandy changed the title Adds enablePublishWithExistingDOI configuration Adds enablePublishOnAllNonPublic configuration Oct 18, 2023
@helbashandy helbashandy changed the title Adds enablePublishOnAllNonPublic configuration Adds enablePublishOnAllNonPublic configuration Oct 18, 2023
@helbashandy helbashandy force-pushed the issue_2202_adding_publish_doi_config branch from bca1b3f to 73d8e5c Compare October 18, 2023 17:33
@rushirajnenuji
Copy link
Member

Thank you @robyngit and @helbashandy -- I'll review the code to see any conflicts with MDV and get back.

Copy link
Member

@rushirajnenuji rushirajnenuji left a comment

Choose a reason for hiding this comment

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

Hi @robyngit and @helbashandy -- I reviewed this PR and it looks good. There are some minor conflicts with the MetaDataView, but it should be easy to merge into my branch. Thank you for checking.

@robyngit
Copy link
Member

@helbashandy not sure what the expected behaviour is for ESS-DIVE, but it seems intuitive that if enablePublishDOI is false, the publish button should never show up. Likewise if publishing is limited to those in the enablePublishDOIForSubjects list, and the user isn’t authorized, then the publish button should never show up. So if we were to move this decision into its own method, the logic would be something like...

canBePublished: function () {

  // The Publish feature has to be enabled in the app. 
  if (!MetacatUI.appModel.get("enablePublishDOI")) {
    return false;
  }

  // If the repository is configured to limit publishing to only certain
  // users and groups, then check if the logged-in user is one of the
  // subjects in the list or is in a group that is in the list. If not,
  // then this metadata cannot be published.
  const authorizedPublishers = MetacatUI.appModel.get("enablePublishDOIForSubjects");
  const authorizedOnly = Array.isArray(authorizedPublishers) &&
    (authorizedPublishers.length);
  const userIsAuthorized = authorizedOnly &&
    MetacatUI.appUserModel.hasIdentityOverlap(authorizedPublishers);
  if (authorizedOnly && !userIsAuthorized) {
    return false;
  }

  // Finally, if the setting to enable publishing on all private
  // metadata is enabled, then allow publishing if the metadata is not
  // yet public. Otherwise, allow publishing if there's not a DOI yet.
  const isPrivate = !this.get("isPublic")
  const isDOI = model.isDOI()
  const enablePublishOnAllPrivate = MetacatUI.appModel.get("enablePublishOnAllPrivate");
  if ((enablePublishOnAllPrivate && isPrivate) || !isDOI) {
    return true;
  }

  // Otherwise, this metadata cannot be published.
  return false;

},

What do you think?

@robyngit
Copy link
Member

robyngit commented Apr 1, 2024

Hey @helbashandy, wondering if ESS-Dive is still hoping to merge in these changes and add a enablePublishOnAllNonPublic config option?

@robyngit
Copy link
Member

robyngit commented Jun 4, 2024

I will close this PR for now since it hasn't had attention for 7+ months, but @helbashandy please re-open if you're still wanting to get this merged into MetacatUI!

@robyngit robyngit closed this Jun 4, 2024
@helbashandy
Copy link
Collaborator Author

Hi Robyn,

I apologize for the delayed response. Regarding the pending PR, we were in the process of finalizing design changes that impact the ESS-DIVE user journey for publication requests. After consideration of our current use-cases, we determined that the suggested configuration change is no longer necessary.

The current design includes an additional modal triggered by the publication button, which addresses various use cases beyond what the suggested configuration would have covered. As a result, integrating the config change is not currently a priority for ESS-DIVE's UI and user journey flow.

Thank you for your patience and for following up on the PR. Let me know if you have any further questions.

@robyngit
Copy link
Member

robyngit commented Jun 4, 2024

Thanks @helbashandy ! I figured you had taken a different path, but just didn't want to assume :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure MetacatUI to show publish button even if DOI is assigned.
3 participants