-
Notifications
You must be signed in to change notification settings - Fork 144
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
record files: enable file modifications depending on external DOi #2264
record files: enable file modifications depending on external DOi #2264
Conversation
* remove "New version" button from landing page for records with external DOI * remove "New version" button from record edit form for records with external DOI * closes inveniosoftware#2259
@@ -400,6 +411,8 @@ def deposit_edit(pid_value, draft=None, draft_files=None): | |||
"delete_draft", | |||
"manage_files", | |||
"manage_record_access", | |||
# "draft_modify_files", |
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.
Note: commented line through the PR - the approach using permissions.
Question for the reviewers: which approach seems better?
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.
In my opinion permissions with IfConfig
generator taking RDM_LOCK_FILES_FOR_EXTERNAL_DOI
as parameter would be cleaner. ping @slint
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 permissions make this cleaner, but with two separate permissions for the deposit form and landing page.
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.
On second thought, I don't think we need to rely on permissions, the record status (published or draft) or even the DOI provider (external or DataCite) for this (at least for display purposes).
The reason is that at the end of the day, the one that controls if one can modify files of a deposit is the bucket (i.e. RDMDraft.bucket
), and more specifically, if it's locked/unlocked. And the one that controls the draft bucket is the DraftFilesComponent.edit
component method.
As a consequence, the active/inactive/hidden state of the files upload section in the form should be controlled by passing something like filesEditable=draft.bucket.locked
. This also goes a bit in the direction of making React components "dumb presentation containers" and not aware of business-logic rules (like e.g. if the record is published or has an external DOI).
Of course, we will still use config flags to control the features, and the defaults will not break the current behaviour, i.e. by default:
- you CANNOT edit files of published external DOI records
- you CAN create new versions of external DOI records and have mixed record versions of external/managed DOIs
Note: will add this feature to |
"update_draft", | ||
"read_files", | ||
"review", | ||
# "draft_modify_files", |
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'd call this "show_version_button". Though I'd check what the "new_version" permission does to see if that might be duplicative.
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.
very nice job! once the comments on rdm-records are agreed on, I think this is good to go!
closing, decided to implement an alternative solution |
Landing page preview for record with external DOI:
For comparison: rec with our DOI: