fix(harvester): error on withdrawn inspire records - #915
Conversation
| ) | ||
|
|
||
| rdm_entry["_inspire_ctx"] = {"cds_id": cds_id, "versions": versions} | ||
| withdrawn = bool( |
There was a problem hiding this comment.
I would prefer to handle it like other fields, with a dedicated function and raise an error if the field exists - otherwise it is a bit "hidden" inside another code flow by not following the common convention
There was a problem hiding this comment.
Done, added a dedicated WithdrawnMapper like the other fields.
042ba44 to
03d7465
Compare
| return None | ||
|
|
||
| elif match_result.found: | ||
| if self._is_withdrawn(stream_entry): |
There was a problem hiding this comment.
why do we need the additional check? the mapper will throw an error anyways
There was a problem hiding this comment.
well i thought that already harvested records with the withdrawn flag should be logged but i didnt realize that while being harvested, if the record has the withdrawn flag then the mapper would raise an error and we would just never reach the writer, thanks for catching this, i have dropped the writer error
2e35a9c to
f521f25
Compare
| if ctx.cds_rdm_id: | ||
| ctx.errors.append( | ||
| "Record is withdrawn on INSPIRE. " | ||
| f"| details: cds_id={ctx.cds_rdm_id}" | ||
| ) | ||
| else: | ||
| ctx.errors.append("Record is withdrawn on INSPIRE.") |
There was a problem hiding this comment.
last tiny thing... why does it matter if it has cds id in this case or not? I would always raise. If the CDS PID is none, just display None :)
in general I would not add conditional statements to modify log lines, since it does not handle bussiness logic in any way
f521f25 to
55f9f95
Compare
2 participants
Part of #906
if inspire marks a record as withdrawn we error and dont create/update it. if its already in cds we also put the cds id in the error so curators can open that record. if its not harvested yet we just say its withdrawn.