Fix animal uses api to act on single resource

[Duncan-Brain]

Description
Originally branched from: #3391

Uses the normal endpoint structure of returning a single unformatted resource for animal use and animal type use relationships.

If we start formatting data on the backend

• it could get really inconsistent with what is returned
• It might only be usable by the one component in the formatting provided -- and then need to be reformatted again in another case.
• hard to write consistent APIs,
• hard to write consistent webapp queries

Other options than formatting to specific use cases and to use less resources on the frontend:

• use createEntityAdapter to manipulate enums to be directly queriable by id
• Instead of enum tables with key and id --> I like just use key enum table (see nomination and nomination_type table) as key is provided in every row without joining to get it.

Jira link:

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Passes test case
• UI components visually reviewed on desktop view
• UI components visually reviewed on mobile view
• Other (please explain)

Checklist:

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• The precommit and linting ran successfully
• I have added or updated language tags for text that's part of the UI
• I have added "MISSING" for all new language tags to languages I don't speak
• I have added the GNU General Public License to all new files

[kathyavini]

But wasn't it a very deliberate decision on our part with the animal endpoints to 1) move more data processing to the server (particularly to models) and 2) start returning more bespoke data for the frontend (other example being types count endpoint)? Why are we going back on that so soon?

it could get really inconsistent with what is returned
hard to write consistent APIs

Could you elaborate on what would become inconsistent?

From the perspective of consuming the query within webapp, I loved having this lookup table-like structure that was designed for this React Select use case. And of course I sooo appreciate a single network request instead of two requests for resources that have to then be merged on the frontend (historically a very problematic LF pattern...)

[Duncan-Brain]

@kathyavini

There is definitely value in bespoke endpoints for specific purpose! Count is a query param, and also animals/remove is now used to remove an animal. I could maybe see value in a sortBy='column' or something along those lines.

hard to write consistent APIs

For example if all get requests acting on a resource return the resources then it is trivial to set up the api and we could probably even abstract away all enum tables to just be .get(). I am sure we will use animal_uses somewhere else and it should be its own resource.

loved having this lookup table-like structure

Why not let RTK do it like our other FE code: https://redux-toolkit.js.org/rtk-query/usage/customizing-queries#normalizing-data-with-createentityadapter

I sooo appreciate a single network request

I agree! I didn't know it causes too many problems for an enum like this but I would love to improve it by using just keys on enum tables as the foreign key (see nomination and nomination_type table). Then there is no need to join.

[Duncan-Brain]

Essentially we remove all of the controller logic and only have to add in this filter, and the all uses option below.

[kathyavini]

@Duncan-Brain It's still hard for me feel really enthusiastic about 220 additional lines of code (almost all boilerplate) and basically identical logic running in the client (hook) instead of on the server (animalUseModel.js), to create the same result. Is it also the first time we've put a straight .get() on a _relationship table??!

It just... already worked before and the design was in the spirit of what we decided on in the Spring 🤷 But I don't feel strongly enough to put my foot down, and I particularly don't think -- given the fact that the original endpoint was working and that the data involved is not large -- that a migration or any more redesign is warranted for this given the other tasks we have to complete. So I'll give it neutral thumb; please merge as is if you guys prefer it.

---

To address your specific comments though:

I am sure we will use animal_uses somewhere else and it should be its own resource.

Doesn't look like it within the scope of this release which is all I was thinking (would prefer not over-anticipate). Ditto with an abstracted enum .get(), although I like that idea a lot!

I could maybe see value in a sortBy='column' or something along those lines.

Maybe the error was in giving this data structure the plain .get() route on a table name instead of something more specific to what it actually returns? Would it work for you to just rename it, leaving open the possibility of making a different .get() route when required?

Why not let RTK do it like our other FE code

I think I like that transformation pattern in general, although it doesn't address the core issue of moving logic frontend --> backend. However, this particular data structure is not the same structure as a normalized Redux entity because we'd want the lookup keys to be default_type_ids, not ids for the uses. I think we could normalize in apiSlice (i.e. make a real lookup table instead of something just lookup table-like) if the backend returns the right general structure. Is this what the nomination table pattern can provide? If so, I'm definitely interested in learning about that for the future -- maybe as a tech daily topic?

[Duncan-Brain]

@kathyavini no worries -- I don't expect it to be merged -- we were just fixing logic to avoid a type error by type casting a string from a string key to make it a number -- and so removing the additional logic and following the reusable code pattern seemed to make more sense to me. If Sayaka doesn't like it that's okay !

I am down to explore what should be moved from frontend to backend but I am not sure that is good practice to make an endpoint for a react select or everything unless it is something the backend is particularly good at like counting or filtering.

It's still hard for me feel really enthusiastic about 220 additional lines of code (almost all boilerplate)

I am also very anti-lines of code haha -- but boilerplate I don't mind at all because if enough of it builds up someone will OOP the model, OOP the controller, OOP the test, and its gone!

To me I think the core problem for me was yes most about the get endpoint yes. But exploring this idea further it feels code smelly not REST-ful not that we must be strict about it.

[SayakaOno]

Sorry for jumping in late!

When I first created this API, I was unsure whether it was common practice to create a controller for a join table. Sorry to bring up a completely different idea, but it seems more natural to achieve what I wanted by joining default_animal_type, animal_type_use_relationship and animal_use (maybe with a route like /default_animal_types?animal_use=true???)... Do you think that’s a bad idea?

[Duncan-Brain]

@SayakaOno
This is the first time I can think of that we had a enum join table so it is a weird one to think through! (But I think it is fun and good practice to iron out the best solution!)

I think you make a great point that since default_type_id is the id and key that would be a more natural place for this shape of a return! ... IF you still think we really want this exact format from an endpoint!

I think it makes sense to just get() plain enum tables as they are (count is of course a useful addition on getting user created animals etc) even if it is a relationship. There is almost no extra logic needed on the frontend since we are already mapping anyways. The core idea being that following the same pattern for all leads to other generic reusable patterns vs specific needs based shapes of duplicate data.

I am interested in flat vs nested data. The problems I remember seeing on frontend with keeping store updated was when things were nested like tasks had nested management plans and management plans had nested tasks and keeping those two in sync was a problem.

Anyways if you think its valuable to keep the new get() on animal_use but not on relationship. I can re-add the get endpoint that formats to ReactSelect useful lookup table to the default_animal_type model as you suggest. What do you think about /default_animal_types?include[]=animal_use (not too long ago we had animal_identifier_placement which was also based on defualt_type_id).

TLDR: If I am dumb for even thinking about this please close PR 😂

[SayakaOno]

@Duncan-Brain
I agree that joining three tables won't be flexible. With the current API design, it's not possible to get an animal use key using use_id if we wanted to.

My preferred approach would be /default_animal_types?include[]=animal_use (joining two tables) API + animal_use API + some formatting on the frontend, I think...

For the nesting structure, I think we could invalid the API to trigger a refetch, so it should be fine with RTK query...?

I'm interested in how we could properly create an API that returns pre-cooked data!
Looking forward to learning more! :)

[Duncan-Brain]

Possible related other solution suggested by Anto is to replace enum ids with enum key on backend so that frontend doesn't use enum ids.

I like that solution since it is a half step towards the enum key table. Even if my second half is never implemented, that suggestion is a solid way to make frontend id agnostic moving forward for most other endpoints. So that would be cool!

So for this discussion I think what that would look like is that the /animal_type_use_relationship, or the /default_animal_type?include[]=animal_use or both returning one of:

[
{default_type: CATTLE, default_type_use: MILK},
{default_type: CATTLE, default_type_use: EGGS},
{default_type: CHICKEN, default_type_use: MILK},
...
]

{ 
CATTLE: {animal_use: [MILK, EGGS]}, 
CHICKEN: {animal_use: [MILK]},
DOG: {animal_use: []},
...
}

If we find time after priorities we could test one of those?