Refactor of GetDigitalSpecimenComplete service and setup of data mapper to UI#244
Conversation
MelanieTheLion
requested review from
Eleni-Theodoridou,
JontySponselee-Naturalis,
Ycreak,
david-auk and
southeo
| * @param ds The digital specimen object | ||
| * @returns Either the accepted identification or, if there is none, the first identification it can find | ||
| */ | ||
| const getAcceptedIdentification = (ds: any) => { |
There was a problem hiding this comment.
This can return undefined when identifications is falsy, is this expected? Because the @returns suggests it will always return an identification
There was a problem hiding this comment.
I adjusted the function to also return a null in case of no identifications whatsoever. Probably a bit of an edge case, but my mapper works also if no identifications are found.
| mappedFields[fieldKey] = { | ||
| label: config.label, | ||
| value: config.resolve(ds, acceptedIdentification), | ||
| isHtml: !!config.isHtml, |
There was a problem hiding this comment.
I know !! is a quite a common trick, but it is not explicit, !Boolean(config.isHtml), makes it more clear that it converts it to an boolean and invert it
There was a problem hiding this comment.
Changed it to Boolean(config.isHtml). Turned out to be the right one :)
| return Object.entries(DIGITAL_SPECIMEN_SCHEMA_MAP as SchemaMap).reduce((accumulator, [groupKey, fields]) => { | ||
| const mappedFields: Record<string, UIProperty> = {}; | ||
|
|
||
| Object.entries(fields).forEach(([fieldKey, config]) => { |
There was a problem hiding this comment.
You could make your code a bit more compact and immutable, but this is optional and opinionated.
There was a problem hiding this comment.
I like this approach! I addressed it as well as I could. Let me know what you think.
|
|
||
| accumulator[groupKey as keyof DigitalSpecimenUIModel] = mappedFields; | ||
| return accumulator; | ||
| }, {} as DigitalSpecimenUIModel); |
There was a problem hiding this comment.
.reduce accepts a generic what it should return, this is more typesafe
There was a problem hiding this comment.
What it should return added.
| console.error('Error fetching the digital specimen:', error); | ||
|
|
||
| /* Rethrow error for useQuery */ | ||
| throw error; |
There was a problem hiding this comment.
why catch here just to log it? why not just add the logging on line 28?
There was a problem hiding this comment.
In the future, this will be caught by some form of error notification system I still have to implement. The error in the catch can be any kind of error, 500s, 404s. The error on line 28 is only thrown if the status is OK (200, 201), but the response that we get does not match what we expect at all.
It is an edge case, because if it is OK, your response would probably (assumption) be what we expect.
The catch takes care of my error codes. So separate error instances.
| resolve: (_, { acceptedIdentification }) => acceptedIdentification?.["dwc:specificEpithet"] | ||
| }, | ||
| nomenClaturalCode: { | ||
| label: 'Nomen/Clatural Code', |
There was a problem hiding this comment.
Haha yes. Sometimes I don't know what I am talking about
|
| CVE-2026-33636 No newline at end of file | ||
| CVE-2026-33636 | ||
|
|
||
| # lodash-es is pinned as a dependency on a vulnerable version in formik and cannot be upgraded by us Apr-3-2026 |
There was a problem hiding this comment.
are you sure this cannot be changed with overrides ? Or are all the earlier packages vulnerable and new versions not compatible with formik ?
https://docs.npmjs.com/cli/v8/configuring-npm/package-json#overrides
There was a problem hiding this comment.
I think I could. I have a follow up user story to address all these trivyignores, because the list is getting long and I'd like to clean them up.
3 participants
In this PR:
This service and data mapper is not used yet, but is tested. Implementation to UI will follow this PR due to complexity.