-
Notifications
You must be signed in to change notification settings - Fork 151
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
ui: Require records community #2215
Conversation
invenio_app_rdm/theme/assets/semantic-ui/js/invenio_app_rdm/deposit/RDMDepositForm.js
Outdated
Show resolved
Hide resolved
a83c9ec
to
51bce06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the conceptual clarification on the whole feature is more important than the code-side for this. See inveniosoftware/invenio-rdm-records#1283 (review) .
Again, I was just asked to review as an outsider, so I am not familiar/my take probably doesn't count for much. I'll let others chime in.
# Can't check "publish" permission as draft is not a record with an id yet. | ||
permissions["can_publish_restricted"] = current_app.config.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "can published restricted" mean? It reads as: current user can publish a restricted record. How does it relate to requiring a community? (same question as other pr).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your feedback regarding the potential confusion that this naming may cause, especially in relation to restricted records. To help resolve this, I'm open to suggestions for a more suitable name. Here are three alternative names that might be clearer:
can_publish_needs_community_config
can_not_publish_without_community_config
can_not_publish_config
Please share your thoughts or other naming suggestions you might have so we change it all at once @fenekku.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see hmm ...
can_publish_with_community_only
can_publish_w_community_only
but...
My take would be to move this feature outside of permissions altogether (but maybe that was recommended by an architect) and place it at the record validation level given all that 😅 . It's non-trivial to do, so it's not great advice on its own 🤔 ... I understand why permissions changes were easier at first... Let's see what CERN recommends 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your input!
I will go with can_publish_with_community_only
.
I see your point about moving this feature out of permissions and into record validation. However, the current approach is indeed similar to this one #2207, where the configuration is passed in a similar way.
Let's see what the CERN team recommends - whether they suggest a separate channel for passing this configuration or if it remains as a part of permissions or a completely different approach. In the meantime, if you can direct me to some resources or examples where this has been implemented at the record validation level, I'd look into it and explore the possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking for such an example this morning, without finding super clear ones. I think this schema: https://github.com/inveniosoftware/invenio-rdm-records/blob/master/invenio_rdm_records/services/config.py#L135 defined here https://github.com/inveniosoftware/invenio-rdm-records/blob/master/invenio_rdm_records/services/schemas/record_communities.py#L23 is maybe the kind of location where we would want to have that check reside (I don't think it's necessarily this exact space) i.e., schemas might be the place...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sharing the links, but still, they do not sufficiently clarify how I can attain the same outcomes in the services and UI without modifying the permissions. We must ensure there's absolutely no possibility for a user to publish both from the UI and API levels. Could you provide a more detailed example illustrating your approach to rectifying both the UI and service aspects?
dc0c35d
to
84dc8c6
Compare
11ff5ab
to
520a35a
Compare
2102bca
to
0d72878
Compare
This PR was automatically marked as stale. |
a2d4360
to
c6462a7
Compare
Closing this PR as the changes were mistakenly made on the |
❤️ Thank you for your contribution!
Description
depends on: inveniosoftware/invenio-rdm-records#1283
closes: inveniosoftware/product-rdm#183
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
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: