Add eitem_type field to EItem and 'MULTIMEDIA' to DOCUMENT_TYPES #1207
Conversation
* documents: Add MULTIMEDIA to DOCUMENT_TYPES
invenio_app_ils/config.py
Outdated
| @@ -829,6 +829,7 @@ def _(x): | |||
| eitems=dict( | |||
| aggs=dict( | |||
| access=dict(terms=dict(field="open_access")), | |||
| eitemtype=dict(terms=dict(field="eitem_type")), | |||
There was a problem hiding this comment.
for naming consistency:
| eitemtype=dict(terms=dict(field="eitem_type")), | |
| eitem_type=dict(terms=dict(field="eitem_type")), |
invenio_app_ils/config.py
Outdated
| @@ -841,6 +842,7 @@ def _(x): | |||
| ), | |||
| ), | |||
| post_filters=dict( | |||
| eitemtype=terms_filter("eitem_type"), | |||
There was a problem hiding this comment.
| eitemtype=terms_filter("eitem_type"), | |
| eitem_type=terms_filter("eitem_type"), |
| @@ -57,6 +57,12 @@ def validate(self, record, **kwargs): | |||
| class EItem(IlsRecord): | |||
| """EItem record class.""" | |||
|
|
|||
| EITEM_TYPES = [ | |||
There was a problem hiding this comment.
not a strong opinion, I would like to also check with the others (could you discuss on a standup?): would it be more flexible to store these types in a vocabulary?
There was a problem hiding this comment.
Sure, I will discuss
But wouldn't keeping the type of the record in its class provide better visibility and readability and also be easier to maintain
if we make it a vocabulary, then we would also need do the same for all other record types, and that will require checking if there might be some other flow that'll be affected by this change
| mediums.append("E-BOOK") | ||
| eitems = eitem_search.search_by_document_pid(document_pid) | ||
| for type in EItem.EITEM_TYPES: | ||
| type_count = eitems.filter("term", eitem_type=type).count() |
There was a problem hiding this comment.
Did you check if eitems is immutable? When you use .filter, does it change the eitems object or simply return a new version with the filter applied?
There was a problem hiding this comment.
Yes, it returns a new version of the object with the filter applied on it, it doesn't apply it in-place
| @@ -61,6 +61,9 @@ | |||
| "document_pid": { | |||
| "type": "keyword" | |||
| }, | |||
| "eitem_type": { | |||
There was a problem hiding this comment.
do we need the prefix eitem_, given that we are already in the e-item?
There was a problem hiding this comment.
My reasoning for the prefix eitem_ was to be consistent with naming like document_type for documents. Wouldn't it be confusing with just type?
❤️ Thank you for your contribution!
Description
closes: CERNDocumentServer/cds-ils#805
closes: CERNDocumentServer/cds-ils#865