-
Notifications
You must be signed in to change notification settings - Fork 175
fix: set files_locked to .locked rather than permission #3223
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
base: master
Are you sure you want to change the base?
Conversation
* currently the permission is whether you can unlock the files * but the files are only unlocked when you create the draft * as such as an admin when you edit a published draft, if it was created by a user it will look like the files are unlocked but this is false this PR correctly sets files_locked to whether the files are locked
| ) | ||
| kwargs["draft"] = draft | ||
| kwargs["files_locked"] = ( | ||
| record_service.config.lock_edit_published_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.
We cannot easily remove this feature, first because it is a breaking change, and also because we rely on it.
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.
We also use it on Zenodo like that. We incorrectly pass the permission to the UI here. @slint can maybe elaborate or you two can talk :)
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.
After a discussion IRL, summarized here:
Right now when admins edit a published record, we "accidentally" also unlock the file bucket (based on our config). This gives record owners technically the ability to modify the draft files, but it's hidden by UI bugs so nobody notices. This PR fixes how the UI reflects the bucket state for the file modification feature, and this is going to become obvious and cause problems.
We decided that admins should instead have to explicitly unlock buckets using the same file modification request flow as regular users. This means clicking "Edit" won't automatically unlock files anymore. If an admin needs to unlock files, they use the request button, which:
A. is an explicit action.
B. creates an audit trail and makes it clear what happened and by who
This way, we avoid accidentally unlocking buckets when admins are just checking/fixing draft metadata, and we have a proper trace of who unlocked what and when. If an admin forgets to re-lock, that's on them - but at least there's a trace of it.
Before deploying this fix, it's worth checking in our instances if there are published records with a draft in progress that have unlocked files. This has to be an SQL query though, since we're not indexing the bucket lock state in OpenSearch:
-- Count of unlocked draft buckets
SELECT COUNT(*) as unlocked_draft_buckets
FROM rdm_records_metadata r
INNER JOIN rdm_drafts_metadata d ON r.id = d.id
INNER JOIN files_bucket b ON d.bucket_id = b.id
WHERE b.locked = false
AND r.deletion_status != 'D';
-- Unlocked draft buckets details
SELECT
r.id as record_id,
r.updated as record_last_updated,
r.deletion_status,
d.bucket_id as draft_bucket_id,
d.updated as draft_last_updated,
d.expires_at as draft_expires_at,
b.locked as bucket_locked,
b.size as bucket_size,
b.updated as bucket_last_updated
FROM rdm_records_metadata r
INNER JOIN rdm_drafts_metadata d ON r.id = d.id
INNER JOIN files_bucket b ON d.bucket_id = b.id
WHERE b.locked = false
AND r.deletion_status != 'D'
ORDER BY r.updated DESC;
❤️ Thank you for your contribution!
Description
this PR correctly sets files_locked to whether the files are locked
Checklist
Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:
Frontend
Reminder
By using GitHub, you have already agreed to the GitHub’s Terms of Service including that: