Lf 4170 create or modify animal enum tables

[navDhammu]

Description

This PR includes some migrations, api for /animal_identifier_types and some small changes to the animal_identifier_placements api as specified in ticket: https://lite-farm.atlassian.net/browse/LF-4170

While working on this I had some issues with some of the tests. I'm not sure if its just me but when I create a new test database and run the migrations on it, I get some errors when trying to run individual files like animal_identifier_test (and some other files too). Its happening because initially the database has several constant tables with values already present in them like animal colors and placements, and the cleanup is done in the afterEach method which means that when the first test is executed the tables are not in an empty state and this is resulting in some errors. It doesn't happen when you run all the tests with npm run test because the test that runs first doesn't rely on any non-empty tables so when its finished, the cleanup is done in afterEach and all subsequent tests run with a clean state making all tests pass.

Going further on this topic, I had some ideas on improving on the overall testing strategy. Rather than running a cleanup of the whole db after each test, it may be better to run each test in a transaction and rollback once its complete, potentially eliminating the need to manually do the cleanup and maintain a list of tables that need to be cleaned in testEnvironment.js. Also it might make the tests run a bit faster.

If the dev team @Duncan-Brain , @SayakaOno , @kathyavini is alright with this, I can try to create a sample PR as a proof of concept.

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

[navDhammu]

This is to let the db auto generate the permission id for new rows because it's currently out of sync and you have to manually look it up and add it in.

[Duncan-Brain]

I also prefer your proposed way -- but I think there was some issue in the past as there was a conscious decision to explicitly set the id to make it consistent between prod/beta/local. My understanding is you will need to use the existing pattern .. sorry

[Duncan-Brain]

@navDhammu Sorry it is taking a little longer to review than normal! We have had a lot of unexpected coding challenges happen this month and have been focusing on the Soil Health V1 ready for release. Apologies if it take a little longer.

In my opinion -- I really dislike our existing backend tests haha. So I am definitely open to that POC/discussion sometime after this release! My pet peeves with the tests are:

• cleaning up enum tables unnecessarily
• not testing the routes directly -- testing the db too much (with knex insertions)
• knex is used liberally without the trx object
• generally hard to navigate

[navDhammu]

@Duncan-Brain No worries!

Btw, not related to this ticket but I worked on an experimental PR which I thought might be useful. Once you have the time, if you can have a look and provide some feedback that would be appreciated!

[Duncan-Brain]

Looks great and tests are passing!

I had to merge integration for migrations to work since we did the release.

I think the only change needed is the permissions -- I agree it is strange the way we do it...

[Duncan-Brain]

I also prefer your proposed way -- but I think there was some issue in the past as there was a conscious decision to explicitly set the id to make it consistent between prod/beta/local. My understanding is you will need to use the existing pattern .. sorry

[Duncan-Brain]

Some nice sql'ing!

I personally find sql harder to read than breaking this down into smaller js-like queries and insertions with knex methods. But just a personal preference as my sql ain't as good as I would like it to be!

[navDhammu]

I think I initially went with the knex approach but there was a lot of code with some nested loops which wasn't easy to read either so I just went with the shorter sql way.

[Duncan-Brain]

I noticed that this is our only instance of keepOpen() and close() What happens if something fails before reaching this line? Like db transactions is a try catch needed or chained to the promises in .then() like the chai example: Search for keepOpen to see this example

[navDhammu]

Since multiple requests were being made here I thought it's more performant to leave the server open, rather than shutting down and opening on each request. I'm assuming it automatically closes if something throws before reaching the line but I couldn't find anything in the chai docs.

[navDhammu]

@Duncan-Brain Thank you for reviewing, I've made the requested changes.

[SayakaOno]

Sorry for the delayed review.
I left a comment, but I'm unsure about one table schema. I'll confirm with the team and get back to you!

[SayakaOno]

If I'm not mistaken, adding ids manually is problematic, and we want to avoid it.
(@Duncan-Brain , was the problem that we couldn't insert a new row because id was conflicting?)

Here are the examples of how these were done before:

LiteFarm/packages/api/db/migration/20240112220609_create_animal_type_tables.js

Lines 33 to 40 in b03f4b0

	// Add initial default types
	const defaultTypeKeys = ['CATTLE', 'PIGS', 'CHICKEN_BROILERS', 'CHICKEN_LAYERS'];
	for (const typeKey of defaultTypeKeys) {
	await knex('default_animal_type').insert({
	key: typeKey,
	});
	}

LiteFarm/packages/api/db/migration/20240115182150_create_animal_breed_tables.js

Lines 62 to 95 in b03f4b0

	// Add initial default breeds
	const defaultBreedKeys = [
	{
	typeKey: 'CATTLE',
	breedKeys: ['ANGUS', 'HEREFORD', 'CHAROLAIS'],
	},
	{
	typeKey: 'PIGS',
	breedKeys: ['YORKSHIRE_LARGE_WHITE', 'LANDRACE', 'DUROC'],
	},
	{
	typeKey: 'CHICKEN_BROILERS',
	breedKeys: ['CORNISH_CROSS', 'ROSS_308', 'COBB_500'],
	},
	{
	typeKey: 'CHICKEN_LAYERS',
	breedKeys: ['LEGHORN', 'RHODE_ISLAND_RED', 'PLYMOUTH_ROCK'],
	},
	];
	const rows = [];
	for (const entry of defaultBreedKeys) {
	const { typeKey, breedKeys } = entry;
	const { id: typeId } = await knex('default_animal_type').where('key', typeKey).first();
	breedKeys.forEach((breedKey) =>
	rows.push({
	default_type_id: typeId,
	key: breedKey,
	}),
	);
	}
	await knex('default_animal_breed').insert(rows);

[Duncan-Brain]

It's funny because it is the opposite of the permission_id thing haha. 😢

@SayakaOno I think you are right for the enum table animal_identifier_type we would prefer to use the auto-increment index. And then map that to the correct place in animal_identifier_placement.

If I recall many of our problems with setting this would be migration/rollback/testing related. I think another issue was if it was a user interactable table or not...

Here is one such initial conversation: #2857 (comment) and The PR that repaired that id problem: #2882

I always prefer using the autoincrement but for permission_id both Jimmy and Anto have kept the existing way...

[SayakaOno]

Thank you Duncan for finding the discussions in the past!
I added a new section "Auto-Increment ID Conventions" with the links to the API package confluence page.
Please update as you see fit!

[navDhammu]

I'm still not quite sure I understand why manually setting id in the permissions table is done the way it is. Is it even possible for local/beta/production db to have different id's if using auto-increment? Especially since users cannot edit that table so I'm expecting that it remains stable across environments. I think there are other tables in the schema which aren't able to be edited by a user and yet they still use the auto-increment, so I'm unsure what makes permissions table a special case?

Regarding the id's on animal_identifier_type, I agree it should've been set with the auto-increment sequence. I think I was just lazy and thought that if its an enum table that won't change then it doesn't matter. But if it caused problems in the past then should probably be changed to auto-increment.

[Duncan-Brain]

So at least for me I have seen auto-increment differences because of failed migrations or undoing-redoing migrations. It seems like because of concurrent transactions the nextval can never be rolled back as part of a transaction. https://www.postgresql.org/docs/9.1/functions-sequence.html (see nextval section)

But agreed that we shouldn't be reliant on it.. maybe a SPIKE jira ticket can be made to explore fixing this for good and addressing all concerns.

[navDhammu]

Interesting I didn't know sequence couldn't be rolled back, thanks for sharing the link. In that case it makes sense for manually entering the permission keys, and also for tables like role where the id needs to remain stable. But for other tables where we don't care about the value of the id, and only that it's unique in the table, then I think it doesn't really matter if there are differences between environments.

[SayakaOno]

@Duncan-Brain this table animal_identifier_placement doesn't seem correct to me...

I'm not sure about storing the id in the animal table as identifier_placement_id. I would prefer to store the keys directly, or create an enum table like this:

1. LEFT_EAR
2. RIGHT_EAR
3. LEFT_LEG
4. RIGHT_LEG
5. OTHER

Then, replace the "key" column with "identifier_placement_id" then omit the id column.

Can you recall if we discussed this design? If not, I would like to discuss it next week.
(Sorry Navdeep 🙏 )

[Duncan-Brain]

For Nav, to me this looks exactly as described in the Jira ticket at least!

@SayakaOno I am not sure if I remember discussing this. Are you suggesting a relationship table(between type and placement)? Or two ids on the animal (one for type one for placement)?

My understanding is how this was meant to work is that a user will react select an indentifier_type (Ear_tag) then we will filter the identifier_placements (left ear, right ear) based on that original choice for a second react select. The placement_id is added I think because combination (ear_tag) and (left_leg) don't make sense.

The architecture we have chosen seems not great I agree. I would prefer some composite key enum like the following and deleting one of the tables:

Or just omit the "left ear" , "right_ear" selection altogether -- "ear tag" or "leg_band" seems plenty descriptive.

[SayakaOno]

Thank you for your input Duncan!

Nav, we discussed this with the team, and the decision was made to remove the tag placement input.

This means that we won't need the animal_identifier_placement table. I'm terribly sorry for the update on the requirement, and for the lack of clarity in the initial requirements as well.

In addition to dropping the animal_identifier_placement table, the expected changes are:

• remove identifier_placement_id column from animal table
• update models
• update tests
• update routes
• other references?

Since this requires a substantial amount of changes, we could create another ticket/PR for this. (In this case, we could have one PR for animal_identifier_type, and another for removing animal_identifier_placement.)
Please let us know what works best.

[navDhammu]

@SayakaOno That sounds reasonable and I think separate PR's for each requirement would be better.

[SayakaOno]

Just for reference, here is the ticket to remove animal_identifier_placement.

[Duncan-Brain]

Maybe something like this is more direct? await knex('rolePermissions').where('permission_id', 171).del();

[navDhammu]

I'm more familiar with sql than knex so I have a tendency to just use sql but that looks much better!

[Duncan-Brain]

Yeah true haha -- it made sense I think when we didnt set the id explicitly!

From what I can tell the principle behind orms is that by using the portable knex functions we could theoretically switch db providers easily just by changing knex config. Raw functions mayyy need to be updated based upon SQL flavour if we were to switch. That and avoiding some common errors like N+1 etc.

So generally my understanding is it is best to stick with knex where possible and use raw for only limited situations. We haven't been picky though about that at all though!

[Duncan-Brain]

Maybe best to use id? In the extremely unlikely event the name is not unique.

[Duncan-Brain]

Overall looks good to me just think we want to use ID in the down function too -- otherwise tests and routes all look great!

[SayakaOno]

Thank you!