versions: calculate record's active versions #274
Conversation
anikachurilova
commented
Nov 9, 2023
anikachurilova
commented
- closes https://github.com/zenodo/rdm-project/issues/515
anikachurilova
force-pushed
the
Search-result-1-more-version-exists-wrongly-added
branch
from
fbba67f to
9542bb8
Compare
ptamarit
left a comment
ptamarit
left a comment
There was a problem hiding this comment.
Should we add a test for versions_count?
| def versions_count(self): | ||
| """Get number of record's active versions""" | ||
| records = list( | ||
| self.get_record_versions(parent_id=self.parent.id, include_deleted=False) |
There was a problem hiding this comment.
Could we reuse get_records_by_parent with with_deleted=False, ids_only=True?
This would fix the tests which are failing due to the import from invenio_rdm_records which we cannot do in invenio-drafts-resources.
Not sure what is the difference between is_deleted and deletion_status in the model.
There was a problem hiding this comment.
After checking, I can confirm that we need to use the deletion_status and not is_deleted=False as is_deleted is checking for empty json (see here)
You are right tho, that we can not import invenio_rdm_records... I could hardcode the value "P" of the RecordDeletionStatusEnum.PUBLISHED..
There was a problem hiding this comment.
After checking, we should explore a different implementation.
I would add the property count inside the versions system field, and calculate the number of versions there.
The query should exclude both records mark as deleted (RecordDeletionStatusEnum.PUBLISHED.value) and the soft deleted (is_deleted=True).
However, there are a couple of issues:
RecordDeletionStatusEnum.PUBLISHED.valueis in rdm-records and cannot be imported here. Does it make sense to move some of that code here? Or inrecords-resources?- the query should be performant, maybe cached, to avoid multiple calculations and performance hits. However, it is not straightforward to understand when invalidating the cache.
3 participants
