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

Require a community when uploading #183

Open
Tracked by #88
Samk13 opened this issue May 5, 2023 · 17 comments · Fixed by inveniosoftware/invenio-rdm-records#1813 · May be fixed by inveniosoftware/invenio-communities#1231
Open
Tracked by #88

Require a community when uploading #183

Samk13 opened this issue May 5, 2023 · 17 comments · Fixed by inveniosoftware/invenio-rdm-records#1813 · May be fixed by inveniosoftware/invenio-communities#1231
Assignees

Comments

@Samk13
Copy link
Member

Samk13 commented May 5, 2023

Is your feature request related to a problem?

Organizations may require at least one community to be associated with a record. Currently, there is no configuration option to enforce this constraint.

Describe the solution you'd like

  • Add a new configuration option that prevents users from deleting all communities in a record, except for the last one. By default, this feature should be off and can be activated in an instance by adding the following configuration:
RDM_RECORD_ALWAYS_IN_COMMUNITY = True

Describe alternatives you've considered

An alternative solution could involve implementing a more granular permission system that allows administrators to define and enforce specific rules for community association. However, this would require a more complex implementation and might introduce unnecessary complexity for most use cases.

Additional context

This feature request is related to the issue of disabling the "publish" button based on the value of a config variable and raising validation errors if the user hasn't selected a community when publishing. Implementing this configuration option will provide organizations with more flexibility in managing their records and community associations, as well as ensuring that the community managers have control over the publishing process.

This feature is related to CERNDocumentServer/cds-rdm#55
We can bring up the topic of introducing custom roles in place of granting full admin access for community creation. As community managers, we're interested in limiting their permissions to manage their respective communities only, rather than having complete control over the system. We discuss this further and explore potential solutions in separate issues when this one is merged.

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

This issue was automatically marked as stale.

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

This issue was automatically marked as stale.

@ntarocco ntarocco self-assigned this Mar 20, 2024
@ntarocco
Copy link

ntarocco commented Mar 24, 2024

@Samk13 @tmorrell (and anyone else interested in this feature) I started looking at the changes, and I realized that the implementation is more complex than expected.
After our discussion, the agreed solution is to implement this feature via permissions. This means that only configured users/roles via permissions will be able to publish without a community: by default, only super admins. However, this is configurable in your instance/overlay.

Because of this, it is not easy to control the behaviour of the Publish button in the upload form, and disable it when the current user cannot publish without selecting a community. In the upload form, the community selection happens in the UI only, client side, before publishing, and the can_publish permission is checked only after clicking on the Publish button.
Currently, there is no implementation that allows permissions evaluation on the UI/client side after an action (in this case, the selection of a community).

Is it acceptable to keep the Publish button always enabled, and return an understandable error after clicking on the Publish button when permission check fails?
If not, the implementation will become very complex. If you have other ideas, let me know.

@Samk13
Copy link
Member Author

Samk13 commented Mar 24, 2024

Thank you for the detailed explanation @ntarocco

TL;DR
Yes to the proposed implementation we can go with this approach.

Regarding the implementation, here are our preferences and requirements at KTH:

  1. The preferred approach would be to disable the Publish button if the user lacks the necessary permissions to publish without a community, similar to the example we created here. However, if this introduces significant complexity, displaying an understandable error message after clicking the Publish button is acceptable like in here.

  2. We need to have a specific role for community managers, distinct from the admin role. This role should allow admins to grant or restrict the community-manager role as demonstrated in the implementation we create here here as community managers or a better naming would be a community-creator should NOT have admin privileges as there role should be limited to create/manage communities only!

  3. It should not be possible for a user to remove the last community from a record. This requirement is crucial for our use case as it's been implemented here.

  4. This one can be taken in a later step: as the introduction of the GitHub integration, we need to consider changing the current scenario where the repository is been published directly without selecting a community or going through the deposit page.
    In such cases, the user is not been prompted to add a community to adhere to the publish with a community restriction.

Please let us know if you need any further clarification or how can we corporate on this.

@tmorrell
Copy link

I very much agree with the error after clicking the publish button approach. We pre-fill the community via the template, so for a user to encounter the error they would have to actively go and remove the community. An error seems reasonable since it was a result of user action. We might want to document that it's recommended to set a default community when using this configuration.

The other KTH requirements all sound reasonable but I think they are slightly separate from the community requirement (we don't need any of them for our use case)

@zguillen
Copy link

We’ve implemented the general requirement of ensuring a community is assigned for every record in a very different approach. It’s been over a year now so I can’t remember all of the technical details but it’s easy for me to show you how the UI works.

  1. We’ve overwritten the /uploads/new route to render our own react component that is simply a modal. (NOTE: in our UI we’ve renamed “community” to “project”) We force the user to select a “reviewing project(community)” first, before filling anything out on the deposit form:

image

  1. We POST from that modal to /api/records to create the draft and then PUT to a URL like this using the reviewing project’s id: api/records/zwtga-c9702/draft/review
  2. Last, we redirect the browser to the original RDM deposit page with the newly created draft as if the user clicked edit next to a draft (/uploads/zwtga-c9702)
  3. Here’s what the draft UI looks like with our customized project (community) UI:

image

  1. I think I remember having to hook into a little bit of the buttons and react state to get the draft’s buttons to render like so (but it wasn’t that much work):

image

You’re welcome to play around with this UI at https://data.dev.msdlive.org/ but you’ll need to create an account and request to join a project first. I’ll get an email and approve the request if I see any come in.

@ntarocco
Copy link

Thanks a lot for the comments and discussion: we will actively work on this in Q2, it is now in our roadmap.

@ntarocco ntarocco changed the title Ensure a Community Link for Every Record Require a community when uploading Mar 28, 2024
@ntarocco ntarocco transferred this issue from inveniosoftware/invenio-app-rdm Mar 28, 2024
@ntarocco ntarocco moved this to Planned 🔮 in InvenioRDM Roadmap Apr 16, 2024
@ntarocco ntarocco moved this from Planned 🔮 to Near-term 🗺 in InvenioRDM Roadmap Apr 16, 2024
@tzubaki
Copy link

tzubaki commented Apr 26, 2024

For JRC, this feature is needed for the first production release, as we should not allow people to publish out of communities. The solution proposed by Nicola, when the button Publish is still enable, but a meaningfull error message will prevent from publishing is ok. Could this still be included in V12?

@ntarocco
Copy link

For JRC, this feature is needed for the first production release, as we should not allow people to publish out of communities. The solution proposed by Nicola, when the button Publish is still enable, but a meaningfull error message will prevent from publishing is ok. Could this still be included in V12?

If not included in v12, it will be part of a minor release, e.g. v12.1

@Samk13
Copy link
Member Author

Samk13 commented Oct 16, 2024

@ntarocco Great progress so far, but should we reopen this issue because the community-creator role has not been enforced.
While the merged PR implements the requirement for records to be associated with a community before publishing, it does not address the need for a distinct role to control who can create communities see example here. As of now, any user can still create a community and submit records, which does not meet the original request.

Samk13 added a commit to Samk13/invenio-communities that referenced this issue Oct 16, 2024
* The `can_create` function will now check if `RDM_COMMUNITY_REQUIRED_TO_PUBLISH` is set to True, requiring the user to have the community manager role to create new communities.
* The community creator role can be overridden from the user side.
* Closes inveniosoftware/product-rdm#183
Samk13 added a commit to Samk13/invenio-communities that referenced this issue Oct 19, 2024
* The `can_create` function will now check if `RDM_COMMUNITY_REQUIRED_TO_PUBLISH` is set to True, requiring the user to have the community manager role to create new communities.
* The community creator role can be overridden from the user side.
* Closes inveniosoftware/product-rdm#183
@ntarocco
Copy link

@ntarocco Great progress so far, but should we reopen this issue because the community-creator role has not been enforced. While the merged PR implements the requirement for records to be associated with a community before publishing, it does not address the need for a distinct role to control who can create communities see example here. As of now, any user can still create a community and submit records, which does not meet the original request.

This can be achieved simply overriding the service permissions, see an example here.

@Samk13
Copy link
Member Author

Samk13 commented Oct 30, 2024

@ntarocco Thank you for your response and for pointing me to the example of overriding service permissions.
I understand that we can restrict who can create communities by customizing permissions at the instance level. However, I believe we should reconsider and address this issue within the core codebase for a few reasons:

User Interface Consistency: Even if we restrict permissions, users still have access to the community settings page where they can attempt to change options like flipping a community from closed to open. While the permissions will prevent the action see here, having the option visible can lead to confusion and a less streamlined user experience. Removing or conditionally displaying these options based on permissions would enhance usability.

Future Maintainability: By integrating conditional code directly into the communities module, we ensure that future changes will naturally adapt to this mode. It's easy for developers to overlook instance-specific permission overrides during updates, which could lead to conflicts or security issues down the line. Embedding this logic in the core codebase makes it more robust and less error-prone.

Alignment with Original Requirements: The original request emphasized the need for a distinct role controlling who can create communities, separate from the admin role. While instance-level permission overrides provide a workaround, they don't fully meet the requirement of having this role managed within the system's core functionality. Implementing this directly addresses the initial use case more effectively.

I understand that this might require additional work now, but it could prevent multiple conflicts and issues in the future. Given the active development of the communities module with subcommunities etc..., incorporating this case would ensure that the system remains adaptable and secure as it evolves.

I'm more than willing to assist with the necessary changes to implement this functionality. Let's work together to enhance the platform for all users. Please let me know your thoughts on this approach.

@ntarocco
Copy link

@Samk13 there is one point that is not very clear to me:

Even if we restrict permissions, users still have access to the community settings page where they can attempt to change options like flipping a community from closed to open.

What is the exact use case? If I can create a community, and I therefore owner, I should be able to adjust its visibility too.

I checked the PR, it feels like that we are mixing the feature to submit a record without a review with the community required flag.

@Samk13
Copy link
Member Author

Samk13 commented Oct 31, 2024

@ntarocco Isn't the whole point of requiring "community on publish" to enforce a review process for each record?

It looks like your GPT summary didn’t accurately capture my Copilot AI comment ;) so to clarify, I’m not talking about visibility settings here, I’m specifically suggesting that we set the "review policy" to "closed," keeping the focus on enforcing a review process.

As it seems like you see the "community creator" role is currently treated as an admin, but this shouldn’t be the case. they should still follow the organization’s publish under review rule.

I also invite others here to share their thoughts on this, as this feature has been requested by many other organizations not only us.

@ntarocco
Copy link

ntarocco commented Nov 1, 2024

@ntarocco Isn't the whole point of requiring "community on publish" to enforce a review process for each record?

I don't think that we should mix and merge these 2. I think that it is a realistic use case to require a community, so you are sure that community curators can curate all records in your repository. And, at the same time, allow community owners to decide if each submission requires a review or not.
The auto-inclusion is a community setting, a community policy. The community required feature is a global system setting.

If your instance requires these 2 together, then we can discuss what is the best way to implement this in your instance.

We should keep InvenioRDM simple, understandable. This also means keeping under control the amount of possible available configurations.

I re-opened the discussion and it would be indeed nice to have other users expressing their opinions on this topic ☺️

@ntarocco ntarocco reopened this Nov 1, 2024
@mugraph
Copy link

mugraph commented Nov 1, 2024

This may be slightly OT but I felt I may add this observation here while the issue is re-opened.

Trying out the current state of development on this feature, I came to wonder if the RDM_RECORD_ALWAYS_IN_COMMUNITY = True config should not also disable (or hide) the "Publish without community" button that is shown for declined drafts, as this may result in confusion on the user side with no clear way to recover, besides the "Change community" option.

@ntarocco
Copy link

ntarocco commented Nov 1, 2024

This may be slightly OT but I felt I may add this observation here while the issue is re-opened.

Trying out the current state of development on this feature, I came to wonder if the RDM_RECORD_ALWAYS_IN_COMMUNITY = True config should not also disable (or hide) the "Publish without community" button that is shown for declined drafts, as this may result in confusion on the user side with no clear way to recover, besides the "Change community" option.

@mugraph the answer to this question is up here in this conversation (already many comments :)): the short answer is that, at the moment, there is no easy way to implement this in InvenioRDM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment