Pass "agreed with" params (if applicable) when creating an identification #266
Conversation
✅ Deploy Preview for ami-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ami-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| return `${this._occurrence.determination.id}` | ||
| } | ||
|
|
||
| get determinationIdentificationId(): string | undefined { |
There was a problem hiding this comment.
Not super happy with this, I think a separate model representation for identifications/predictions could simplify, but perhaps not in this PR
There was a problem hiding this comment.
Okay yes. I'm curious to know more about why you need to make additional prop functions like determinationPredictionId() instead of accessing determination_details directly
There was a problem hiding this comment.
I want to keep the interface separated from BE as far as possible. That is why I have setup a separate layer to handle the all backend communication, including converting data. This means, when you are making an API update (like changing the structure of data), I don't have make updates all over the FE app. Instead I can just update the FE model class.
Also I'm using this layer to simplify a bit. Sometimes I'm getting more than needed from BE and somtimes in a format that requires a bit of processing before rendering (I think taxa is one good example of this). In this way, I can choose what to expose to the interface layer and how.
I do think the classes are sometimes a bit overkill and there are definitely room for improvements here and there (like in code above), but at least this is how I have been thinking about it :) While BE has been changing a lot, it has been pretty nice for me, but now when things are more stable, maybe the FE app will not have the same need to adapt to BE updates.
Happy to discuss more, maybe you see things from a different perspective?
There was a problem hiding this comment.
Thank you for that explanation! I really appreciate you keeping the frontend working while the backend has lagged behind. It makes sense to keep the objects in the frontend specific and simpler when they can be. But I also like the idea of eventually making types/models more consistent between the frontend & backend when the API is stable. At some point would even like to try using the types generated by the OpenAPI SDK directly in React. We can have multiple "view" types for a model (OccurrenceIdentificationAgreeDetails), but it would be awesome to define them in one place. Especially for validation, and for when we are all full stack developers!
There was a problem hiding this comment.
I would love that, it would make the FE app less fragile also, if we can use a more type safe approach!
annavik
changed the title
annavik
changed the title
mihow
left a comment
mihow
left a comment
There was a problem hiding this comment.
This looks great, but I am not seeing either agreed_with field saved in the API response. Were you able to see it working? For example:
https://api.dev.insectai.org/api/v2/identifications/210/
Hmm, no I did not test that values are saved on BE, just that I pass them. Let's have a closer look at that! |
Strange! I had a closer look and works fine for me, I wonder what it might be? Example where value got saved: https://api.dev.insectai.org/api/v2/identifications/219/ |
mihow
left a comment
mihow
left a comment
There was a problem hiding this comment.
Perhaps I was looking a the wrong identification before
Fixes #251