services: insure one community exists when remove

[Samk13]

❤️ Thank you for your contribution!

Description

Closes inveniosoftware/product-rdm#183

This PR adds an option to prevent deleting all communities in a record except for the last one, as our org requires at least one community per record. This feature is off by default and can be activated in your instance by adding:

RDM_ENSURE_RECORD_COMMUNITY_EXISTS = True

Then you will still be able to remove communities, but not the last one.

This PR is connected to this package we create, it prevents submitting any record without selecting a community from the API level. and prevents users from creating communities unless they have certain privileges that we set for the theme programmatically.
Future plans are to implement this as well with the master branch.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

• I'm aware of the code of conduct.
• I've created logical separate commits and followed the commit message format.
• I've added relevant test cases.
• I've added relevant documentation.
• I've marked translation strings (for relevant code).
• I've followed the CSS/JS and React guidelines (for relevant code).
• I've followed the web accessibility guidelines (for relevant code).
• I've followed the user interface guidelines (for relevant code).
• I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
• I've NOT included third-party code (copy/pasted source code or new dependencies).

Third-party code

If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

1. You license your contribution under the same terms as the current repository’s license.
2. You agree that you have the right to license your contribution under the current repository’s license.

[kpsherva]

we have discussed this PR internally with my colleagues and I have some more comments to follow up (sorry about that!)

• this check will not ensure the record being part of at least one community, however you can still create a record via deposit form and select no community. Is this a desired behaviour for the feature? If yes, what is the use case for allowing to create a record without a community but not allowing to remove one, once the community is added at least once? Could you explain us a bit more in what cases is this desired behaviour?

• this part seems to have reflection only in the backend, it should also be reflected in the frontend if we would like to consider it as a full feature, meaning, for example if RDM_ENSURE_RECORD_COMMUNITY_EXISTS and there is only one community attached to a record, the Remove button should be blocked in the UI, with a toolltip informing about the reason of blocking the button. Otherwise, the user is not informed early about the limitations in the instance (only when the error appears after an attempt of removing the last community)

[chokribr]

@kpsherva shall we move this PR to the Work In progress stage?

[Samk13]

Thank you for the valuable input @kpsherva.

• to document the answer to the first question, We are currently depending on invenio-config-kth that overrides permissions to prevent record publishing in the absence of a selected community.

After our discussion on Discord, we agreed to include this behavior in the current PR to make it a complete feature so that when this option is True, you as a user will be denied publishing a record without belonging to at least one community and denied the removal of the last record community.

• We have also decided to bring up this question during the next teleconference with the community to seek their feedback on what behavior we expect in edge cases like:

What happens if you have a restricted community attached and a public one and you try to remove the public one what should happen then?

Only the community manager of the community in question can remove the record from a community. A manager of community A cannot remove the record from community B.
so if community B is restricted (and the community manager A is not part of community B = he cannot see it), but in the interface, the button to remove community A is not disabled, it means (from the point of the manager A) there is a hidden last community.

This can be related to the question if we should make the "remove community" button visible in the UI or not.

[fenekku]

Yeah... it ends up awkward having this single feature -"require a community for every deposited record"- be broken up into pieces across a core module and an extension with neither being a coherent whole on its own.

InvenioRDM doesn't let you implement all the pieces in the extension, eh? 🤔 Are replaceable components only doable in an instance repo (not an extension)? They wouldn't be packageable

I would see 2 ways to go:

1. do it all in your instance for now . It might require some weird duplication and other hacks, but at least you can get the full functionality you need. This way you can iterate faster on better UX and devX, as well as identify the kind of hooks you would need InvenioRDM to give you.

2. move all the functionality in InvenioRDM. This will probably be slower to get in and iterate, but eventually more people could use it. Again the "metadata-only allowed switch" work (which I don't have a better reference than Files: Support for disabling metadata-only records product-rdm#107 ) is down the same alley anyway, so maybe it's not as bad.

Anyway, just my 2 cents - CERN people probably have a better solution.

[Samk13]

Thank you for your suggestions. I see your perspectives on both approaches. However, before finalizing our decision, I would also like to hear what the CERN team suggests on this matter. Their input could potentially provide additional insights or even more suitable solutions.

[ntarocco]

Suggestion: we can take advantage of permissions and avoid any code change, similarly to what done with can_manage_files:

can_remove_community = [
        IfConfig(
            "RDM_RECORD_ALWAYS_IN_COMMUNITY",
            then_=[IfOneCommunity(SystemProcess())],
            else_=[... as now ...],
        ),
    ]

The permission class IfOneCommunity could simply count how many communities are in the record.
In this way, not only the config var controls if this should be enabled or not, the config is also easily overridable and the code does not need changes.

It is also not clear to me how we ensure that, when submitting, a community is always selected.

[Samk13]

Implemented your suggested permission change, using a method similar to can_manage_files.

To ensure a community is always selected during submission, no separate PR is needed. Override permissions on instance level (site) and we update the documentation with instructions and the following code snippet:

from invenio_records_permissions.generators import ConditionalGenerator
from invenio_rdm_records.services import RDMRecordPermissionPolicy
from invenio_i18n import lazy_gettext as _
from flask import abort

class IfInCommunity(ConditionalGenerator):
    def _condition(self, record, **kwargs):
        if not record or not record.parent.communities.ids:
            abort(403, description=_("Please select a community before publishing."))
        return True

class KTHRecordPermissionPolicy(RDMRecordPermissionPolicy):
    can_publish = [
        IfInCommunity(then_=RDMRecordPermissionPolicy.can_publish, else_=Disable())
    ]

If we agree I can write this docs part.

In this PR, introduced can_publish_via_community to disable publishing unless a community is selected. Direct API attempts will also be denied.

Direct publish UI button disabled if the config is on in this PR. Only community route available for publishing.

Open for further feedback.

[github-actions]

This PR was automatically marked as stale.

[Samk13]

Closing this PR as the changes were mistakenly made on the master branch. I've created a new branch with these changes and opened a new PR here: #1719. Thanks for your understanding.