Discussion - Radio Options

[kathyavini]

Description

Not for merging, just for discussion, based on the conversation in #3397

NOT URGENT, just documenting before I forget.

Background

A RadioGroup option (unlike a React Select option) is set up to only store a single primitive value (string/number) in React Hook Form. We have a RadioGroup option in the Add Animals (and later Readonly/Edit animals) which stores an Origin id (numeric key) from the AnimalOrigins enum table. But when we want to perform logic on AnimalOrigins (e.g. showing/hiding fields in the component or cleaning up submission data), we need the KEY, which has to be either 1) stored and passed down in some way in data otherwise available to the component and then re-matched to the numeric id (this was done in the component), or 2) re-fetched from the API (which luckily is not a network request though, as it should be cached) and then passed down to where the logic is needed.

To me, it read kind of messily in both places, so the idea was to discuss some alternatives and/or check if this was indeed the way it has to be done.

Options

1. (Currently pushed to this branch) My first question was how "wrong" is it to make radio values optionally an object with { id, key } and update Radios to accept these values alongside primitives. Personally, I kind of like how this idea reads more than any of the others. We can continue to use the AnimalOrigins enum and the logic is trivial
2. Similarly, a pattern closer to the React Select could be implemented, using a Controller to wrap RadioGroup (see Sayaka's screenshot here). Potentially the update to Radios/RadioGroup would be more substantial. I like this idea but I don't know if we have the bandwidth to implement.
3. More or less equivalently to (1), the key could be stored by using a compound key which is still a primitive value, e.g. 1:BROUGHT_IN and parsed with split:

const [originId, originKey] = data[DetailsFields.ORIGIN].split(':')`

but Sayaka mentioned not liking compound string keys and I also don't find that split very beautiful so I'm happy to discard that idea.

4. containers/AddAnimals/utils.ts could be refactored into a hook in order to call the origins useQuery hook directly, see comment here which would save the passing down of broughtInId through so many functions. However, this wouldn't work for the component, if we are keeping it pure (as I think we should).
5. (By default) leave as is. Although it's a little unwieldy to have to re-match up the key and the id like this in two places, it's logical. Maybe it's even the best pattern?

So, just wanted to discuss that. No rush though!

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

[antsgar]

This is an interesting discussion, thank you for putting this together @kathyavini!

The problem with the approach in the current branch is that in the HTML standard, the value of a radio input is always a string. So when we pass down an object, in the DOM we'll actually see a string representation of it, [object Object] (see screenshot). If we went with this, we wouldn't be following HTML and we would also be making it harder for ourselves to debug potential issues since all radios would technically have the same value at the HTML level.

The other disadvantage that I can think of is that, with the RadioGroup component setup as is right now, making the radios prop more flexible could lead to confusion in usage. With code as is in this PR, we're allowing value property be a primitive or an object. If it is an object, though, the component is expecting it to have an id property to check for. This would mean whoever's consuming the component would likely have to remember to go and check the component's code to see what the right shape should be for that property -- nothing would stop them of passing an object with an incorrect shape, without that id property. If we had TypeScript throughout, this wouldn't be an issue, but as is, I think this kind of flexibility might be prone to misusage.

Have we considered/tried using only the keys on the UI side, and only looking up for the IDs when we need to send data back to the back end? That is, using the keys as values instead of the numerical IDs. What I'm thinking of is the IDs matter at the database level, but the UI probably shouldn't care about them since they're not semantic. Using numbers as the radio values makes things harder to read at the HTML level, too, I think it'd be cleaner if the inputs had actual meaningful strings as values. I might be missing something though 🤔 there's a ton of new code on animals so trying to get up to speed right now!

[kathyavini]

Have we considered/tried using only the keys on the UI side, and only looking up for the IDs when we need to send data back to the back end? That is, using the keys as values instead of the numerical IDs. What I'm thinking of is the IDs matter at the database level, but the UI probably shouldn't care about them since they're not semantic.

@antsgar I agree with this soooo much! Do you think there is even the possibility leaving the IDs entirely on the backend and working only with keys on the front? Even at the point of sending data to the backend we do need access to both the semantic value (to do the right data processing) and the numeric id (to put in the actual request), and I would say the current question of how to deal with this radio is a microcosm of issues we've had with matching up numeric ids to semantic keys even at this step (sending data back).

In soil amendment this came up with the handling of both purpose_id and method_id (both specifically to be able to know when "other" has been selected), and if you look through the diff of these PRs you can see some of the code solutions we have gone through to get those semantic keys available to the formatting functions

• LF-4309 Complete form breaks when not completed fast enough allowing page to time out or something #3324
• LF-4348: The 400 error will be displayed once the user will try to add an other purpose in the list of purposes during the task completion or creation #3338
• LF-4348 Demo branch -- Pass other_purpose_id as a form field #3339
• LF-4351: Other purpose text is not saved #3344

(It's a lot of code so I can also definitely go over it in tech daily next week!)

In animals there is the origin_id (which you can see in this PR), use, and tag_type.

If I were to put it in a nutshell, I would say that mostly I have wanted to hack together something (anything!) to deliver the semantic data alongside the numeric id, while Sayaka has mostly re-fetched the enum and passed the key through whatever it takes to put it in the right place -- so in that sense, this question of the broughtInId here really is a true microcosm of what also happened in soil amendment 😄 Although this is a radio, it has applied equally well to React Select options because we have been making the value of those options the numeric id.

It would very good I think to have a more consistent approach and I would love as a starting point to discuss making the value of both radios and React Select options semantic!

[antsgar]

Yes! That would be ideal, to separate concerns and have the UI be entirely agnostic to the IDs. The only reason the numerical IDs exist is to keep the database layer performant, so we can draw a line at the back end and forget about them from there on. We could have the API receive the key instead when writing an animal, and have the controller do the lookup for the right ID to write. In that first soil PR, I see that we're even looking up for the key in the back end https://github.com/LiteFarmOrg/LiteFarm/pull/3324/files#diff-b5cc8699bfe63f32843d007de630ff3401e9e3cebceea7c07988705fea31a466R34, which we wouldn't have to do if we had the UI pass down the key and not the ID in the first place if I'm understanding correctly. Something to take into account, if we aren't yet we would need to make the key column unique at the database level to ensure that we're getting unique strings all the way through to the UI, but honestly we should be doing that anyway -- it wouldn't make sense to have two rows with the same key.

[Duncan-Brain]

I am also on board for that removal of ids from frontend!

...I know I have floated this idea a few times.. but do we even need enum ids on the backend?

I am curious about what the performance of a small and finite enum table with just string keys as primary key, since there will never be thousands of enums on animal_origin. And the only time it would check foreign key is on insert and update which we also intend to have max limits on.

I tested it when I first started on nomination table architechture:

A side benefit is it makes for very clean reading of a single row, with no joins necessary!

[antsgar]

It's a good point! But I think it's a good idea to try to aim for consistency across tables, so even though it might not be a problem with a table this size in particular, doing numerical IDs in general prevents someone from thinking string IDs in a bigger table might be a good idea in the future. Also if we do the same thing for IDs throughout the entire database it's easier to know what your query needs to look like. The other thing is strings will take up more space to store, and since these primary keys are actually foreign keys in other tables which will be big (like animals), that could become a problem.

[Duncan-Brain]

Fair point about copy pasting new architecture. But I think for enum tables I would hope it might be more clear and why using it for big resources like animals would be a bad idea.

Strings do take up more space but we save strings all the time like 'notes' with no issues. The question I am really interested in is: how expensive really is the Foreign Key relationship. I would be interested to see how much of a problem that actually is! Because we could save a lot of uneccessary joining and relationships on the backend too as well as the frontend.

Edit : Doing a quick google -- we have used uuid -- which is a string so probably fine?

[antsgar]

Yeah, that's a fair point! Tbh it might not be a huge problem either way in this scenario, it's probably a matter of just picking something and moving forward. Another disadvantage that I can think of for using "semantic" strings as keys on the database side is that if for some reason we need to change that translation key, we'll need to update thousands of records throughout to reflect the change instead of just updating the lookup table, but that's probably not likely to happen since it's the actual translation that would need to change and not the key? If that's the case I think it'd be reasonable to get rid of the numerical IDs and the need to lookup joined tables altogether.

[kathyavini]

@Duncan-Brain Happy birthday! Regarding specifically uuids as PKs, I thought we did discuss this previously with Iván and we established that incrementing integers were almost, in every conceivable situation, better. Not saying one way or another about KEY, but uuid string PKs were an anti-pattern that we have been trying to avoid since, so I wouldn't use them as a precendent!

@antsgar we have indeed at times changed the translation key and not just the translation, e.g. farmExpenseTable, and most recent one certifications, which makes me terrified of your example of having to update foreign keys on thousands of records.

So actually I guess I am declaring for team integer for PK 😂

[Duncan-Brain]

@kathyavini agreed no to uuid pk , but that context was for large resources like animals not small ones like enum tables. I am fairly confident he just agreed with the general content of an article about how to decide ( not as an antipattern) that I shared during that discussion https://www.bytebase.com/blog/choose-primary-key-uuid-or-auto-increment/