service: display tombstone for deleted record's files #511
Conversation
jrcastro2
force-pushed
the
fix-files-tombstone
branch
from
53270b3 to
7a0dcca
Compare
| @@ -70,6 +74,36 @@ def __init__(self, error): | |||
| super().__init__(code=500, description=_("Internal server error")) | |||
|
|
|||
|
|
|||
| class HTTPJSONException(_HTTPJSONException): | |||
There was a problem hiding this comment.
this was moved here from invenio-rdm-records as it's required in this module as well
| @@ -69,3 +69,12 @@ def __init__(self, recid, file_key): | |||
| ) | |||
| self.recid = recid | |||
| self.file_key = file_key | |||
|
|
|||
|
|
|||
| class RecordDeletedException(Exception): | |||
There was a problem hiding this comment.
this was moved here from invenio-rdm-records as it's required in this module as well
jrcastro2
force-pushed
the
fix-files-tombstone
branch
2 times, most recently
from
5020004 to
f01b418
Compare
| @@ -68,10 +68,11 @@ def _get_record(self, id_, identity, action, file_key=None): | |||
| # | |||
| # High-level API | |||
| # | |||
| def list_files(self, identity, id_): | |||
| def list_files(self, identity, id_, with_deleted=False): | |||
There was a problem hiding this comment.
are all the with_deleted params being checked against permissions? I guess we need a <action>_deleted permission to check if you can perform the action?
There was a problem hiding this comment.
jrcastro2
force-pushed
the
fix-files-tombstone
branch
4 times, most recently
from
6be91de to
8102172
Compare
| """List the files of a record.""" | ||
| record = self._get_record(id_, identity, "read_files") | ||
|
|
||
| self._check_record_deleted(record, identity, include_deleted) |
There was a problem hiding this comment.
improvement: read_deleted_files can potentially be consolidated with read_files permission, we could avoid double permission check
| @@ -63,11 +63,11 @@ def get_by_key(cls, record_id, key): | |||
| return cls(obj.data, model=obj) | |||
|
|
|||
| @classmethod | |||
| def list_by_record(cls, record_id, with_deleted=False): | |||
| def list_by_record(cls, record_id, include_deleted=False): | |||
There was a problem hiding this comment.
unfortunately this might cause issues.
the with_deleted is coming from invenio_records and I think we are now hiding this functionality
There was a problem hiding this comment.
see also (got it while testing):
There was a problem hiding this comment.
The error seems to be due to missing the changes from invenio-app-rdm, probably you are missing the changes of the following PR . If not, please describe how can I reproduce it and I will fix it.
| if errors: | ||
| body["errors"] = errors | ||
|
|
||
| # TODO: Revisit how to integrate error monitoring services. See issue #56 |
There was a problem hiding this comment.
What is the issue #56?
It is not easy to understand well what is the issue here.
There was a problem hiding this comment.
Right! Thank you for pointing this out. This code was moved here from invenio-rdm-records. And apparently that was copied from flask-resources, I have updated the comment to point to the correct issue
jrcastro2
force-pushed
the
fix-files-tombstone
branch
from
8102172 to
86ff1fa
Compare
| ].record_cls.deletion_status.is_deleted = True | ||
| recid = example_file_record["id"] | ||
| file_to_initialise = [ | ||
| { | ||
| "key": "article.txt", | ||
| "checksum": "md5:c785060c866796cc2a1708c997154c8e", | ||
| "size": 17, # 2kB | ||
| "metadata": { | ||
| "description": "Published article PDF.", | ||
| }, | ||
| } | ||
| ] | ||
| # Initialize file saving | ||
| result = file_service.init_files(identity_simple, recid, file_to_initialise) | ||
| assert result.to_dict()["entries"][0]["key"] == file_to_initialise[0]["key"] | ||
| # for to_file in to_files: | ||
| content = BytesIO(b"test file content") | ||
| result = file_service.set_file_content( | ||
| identity_simple, |
There was a problem hiding this comment.
normally I would say you should not be able to set_file_content on deleted record unless for some reason the admin's job requires it. I would ask the zenodo support team for their use cases
I see two options
- this endpoint should be protected by permissions, and only admin should be able to set content/commit - in this case we miss a test to check if normal user or owner of the record still can commit files
- nobody, even the admin/moderator (except superuse) should be able to commit or set content
There was a problem hiding this comment.
This feels a bit out of scope for this issue, the goal is to hide the files of deleted records. The behaviour of who can update files of deleted records was not changed, at the moment it's only admins.
If we want to change this we should add the tests in invenio-rdm-records as here we mocking the permissions.
| assert result.file_id == "article.txt" | ||
|
|
||
| # Delete file | ||
| result = file_service.delete_file(identity_simple, recid, "article.txt") |
There was a problem hiding this comment.
do we allow to an user (simple identity) to delete a file from deleted record? I don't think it should happen. Once the record is deleted, it should be "frozen", check if RFC has any info about this case: inveniosoftware/invenio-app-rdm#2289
There was a problem hiding this comment.
We are using mocked permissions here, that's why a simple identity can delete files, the real permissions set for rdm should be tested in invenio-rdm-records.
jrcastro2
force-pushed
the
fix-files-tombstone
branch
from
86ff1fa to
b434491
Compare
|
Closed as the functionality is implemented in invenio-rdm-records inveniosoftware/invenio-rdm-records#1490 |
The following endpoints display the tombstone page:
api/records/<pid>/filesapi/records/<pid>/files/<key>api/records/<pid>/media-filesapi/records/<pid>/files/<key>/contentapi/records/<pid>/files-archive