metadata: add internal notes #1858
Conversation
kpsherva
commented
Oct 25, 2024
•
kpsherva
commented
- closes sspn: migration rules CERNDocumentServer/cds-rdm#227
- closes fields: add internal notes CERNDocumentServer/cds-rdm#240
| @@ -341,6 +341,22 @@ | |||
| } | |||
| } | |||
| } | |||
| }, | |||
| "_internal_notes": { | |||
There was a problem hiding this comment.
question: may i ask, what is the leading underscore for?
There was a problem hiding this comment.
because of https://github.com/inveniosoftware/invenio-rdm-records/blob/master/invenio_rdm_records/records/mappings/v7/rdmrecords/records/record-v6.0.0.json#L654
it will be still discussed, that's why it is a draft PR, too early to review since it is not the final form of the code
| """Schema for internal notes.""" | ||
| timestamp = TZDateTime(timezone=timezone.utc, format="iso", dump_only=True) | ||
| user = fields.Nested(Agent, dump_only=True) | ||
| note = SanitizedUnicode() |
* components: add internal notes cmp
kpsherva
force-pushed
the
internal-notes
branch
from
c418ffb
to
ad96939
Compare
| @@ -231,6 +239,11 @@ class RDMSearchOptions(SearchOptions, SearchOptionsMixin): | |||
| MetricsParam, | |||
| ] | |||
|
|
|||
| query_parser_cls = QueryParser.factory( | |||
There was a problem hiding this comment.
for reviewers: I find this very confusing to have to define the query_parser_cls both in RDM_SEARCH config variable and here. Does anyone know the reason for this config being spread in many places?
| class InternalNoteSchema(Schema): | ||
| """Schema for internal notes.""" | ||
|
|
||
| id = SanitizedUnicode(load_default=lambda: RecordIdProviderV2.generate_id()) |
There was a problem hiding this comment.
I have doubts about using the RecordIDProvider in the schema of the record, maybe someone has an idea for something more elegant
I need to identify the notes within the record, otherwise there is no way of keeping track
| @@ -261,6 +269,18 @@ def always_valid(identifier): | |||
| "mostviewed", | |||
| "mostdownloaded", | |||
| ], | |||
| "query_parser_cls": QueryParser.factory( | |||
| mapping={ | |||
| "internal_notes.note": RestrictedTerm(administration_permission), | |||
There was a problem hiding this comment.
I would gladly align on using the correct permission here, I used admin for self explaining PR
There was a problem hiding this comment.
agreed with @ntarocco to settle on super-user permission for now
| existing_ids = set([note["id"] for note in notes]) | ||
| new_ids = set([note["id"] for note in notes_updated]) | ||
| to_delete = existing_ids - new_ids | ||
| if sorted(existing_ids) != sorted(new_ids): |
There was a problem hiding this comment.
there is a bit of manual checks since the field was designed as a list, each new or removed element need to be checked by its unique id
There was a problem hiding this comment.
With the discussion we had IRL, the id was not needed. Why adding it now?
If not needed, it might be confusing.
| existing_ids = set([note["id"] for note in notes]) | ||
| new_ids = set([note["id"] for note in notes_updated]) | ||
| to_delete = existing_ids - new_ids | ||
| if sorted(existing_ids) != sorted(new_ids): |
There was a problem hiding this comment.
With the discussion we had IRL, the id was not needed. Why adding it now?
If not needed, it might be confusing.
| def create(self, identity, data=None, record=None, **kwargs): | ||
| """Inject note to the record.""" | ||
| notes = data.get(self.field, []) | ||
| for note in notes: |
There was a problem hiding this comment.
Given that it is a new record, do we need to use the setdefault?
record["internal_notes"] = [{
"added_by": {"user": identity.id}),
"timestamp": datetime.now(timezone.utc).isoformat(),
"note": note["note"],
} for note in data.data.get(self.field, [])
] | "internal_notes.id": RestrictedTerm(administration_permission), | ||
| "internal_notes.added_by": RestrictedTerm(administration_permission), | ||
| "internal_notes.timestamp": RestrictedTerm(administration_permission), | ||
| "_exists_": RestrictedTerm( |
There was a problem hiding this comment.
this requires some explanations :)
|
|
||
|
|
||
| def word_internal_notes(node): | ||
| """Quote DOIs.""" |
| @@ -40,6 +39,8 @@ | |||
| from marshmallow_utils.schemas import GeometryObjectSchema, IdentifierSchema | |||
| from werkzeug.local import LocalProxy | |||
|
|
|||
| from invenio_rdm_records.services.schemas.parent.access import Agent | |||
| class InternalNoteSchema(Schema): | ||
| """Schema for internal notes.""" | ||
|
|
||
| id = SanitizedUnicode(load_default=lambda: RecordIdProviderV2.generate_id()) |
| # Invenio-RDM-Records is free software; you can redistribute it and/or modify | ||
| # it under the terms of the MIT License; see LICENSE file for more details. | ||
|
|
||
| """Tests for the service Metadata component.""" |
There was a problem hiding this comment.
I don't know if here or in invenio-records-resources, but we need to see tests for searching text that is in the internal_notes field, without specifying the field name in the search query.
