sfbrigade / bats-server

Routed is an app to help ambulances direct non-critical patients to hospital emergency rooms with the most availability.
https://routedapp.org/
GNU Affero General Public License v3.0
18 stars 11 forks source link

Bkretz/feature/celcius temp #275

Closed kretzbryan closed 1 year ago

kretzbryan commented 1 year ago

My initial attempt at this was to add new columns to the patient metadata(let me know if this is the correct terminology for adding a new field object to the metadata). Example: { name: 'temperature', type: 'decimal', label: 'Temperature', shortLabel: 'Temp.', unit: '°F', range: { min: 80, max: 150 }, conversion: 'celsius', }, { name: 'celsius', type: 'decimal', unit: '°C', range: { min: 26.5, max: 65.5 }, conversion: 'temperature', }

This didn't work because it no longer matched the patient model on the backend, to the app would crash.

You can see in the up-to-date branch that I added a conversion object to the temperature column of the patient metadata. That was my attempt to solve the issue of the app crashing, but since there was no specific column for the celsius field, it was never added to the Ringdown payload. I did add the celsius object statically to the Ringdown class file, which is most definitely not ideal. With the way the temperature column is currently, I could map through the patient metadata and return two FieldMetadata classes if there is a conversion field within the column (possibly in the ModelMetadata class), but I'm trying to get an understanding of how and when the ModelMetaData is being used on the backend.

fwextensions commented 1 year ago

The ModelMetadata instances are just thin wrappers that make it easy to convert the arrays of field specifications to forms needed by the server or client. That way, the field names, types, ranges, etc. are only defined once.

Adding a field to the metadata or even the server model doesn't automatically add it to the database, though. To do that, a migration script has to be added to server/migrations/, which can then add a column for the new field.

For the F/C fields, it may make sense to create a control like the one I made for blood pressure, so we don't need to force it into the generic field structure. I think it's probably simplest to keep using F for the temperature field in the database.

We could also look at using a composite field, as described in #273, to combine the temp value with a selected units.

kretzbryan commented 1 year ago

If i create a control for F/C fields, are we expecting to add a new field for celsius to the metadata? My biggest issue with this was the fact that the app crashed on login when I did this, and the error I got was a db related error (shown below).

Screenshot 2023-02-28 at 8 05 25 PM Screenshot 2023-02-28 at 8 01 21 PM
fwextensions commented 1 year ago

are we expecting to add a new field for celsius to the metadata?

I don't see any reason to. The F temp can remain the source of truth, and can be used to generate the values for both F/C fields.

kretzbryan commented 1 year ago

are we expecting to add a new field for celsius to the metadata?

I don't see any reason to. The F temp can remain the source of truth, and can be used to generate the values for both F/C fields.

Ah okay so just handle the conversion logic in a control for temp and using local state to store the celsius value as opposed to handling it in the Ringdown class?

kretzbryan commented 1 year ago

A couple more things. The stylesheet you created for bpfield works well with the temp field. Should I name the stylesheet/subsequent classes something a little more generic and use it for the temperature control as well? Also, since the input handling Fahrenheit is going to throw an error either way, is it safe to say we don't need validation state for Celsius?

fwextensions commented 1 year ago

Maybe the CSS and basic structure could be rolled up into a FormMultiField component or something, that would render the label and whatever child elements are passed to it. Then the BP and temp controls could both use it.

It may make sense to include validation on the Celsius field, if only because it would be slightly weird seeing an error on the F field if you're typing it in C. But then is it weird seeing the error on both fields? Maybe start without it and we can see how it feels.

kretzbryan commented 1 year ago

Maybe the CSS and basic structure could be rolled up into a FormMultiField component or something, that would render the label and whatever child elements are passed to it. Then the BP and temp controls could both use it.

Good idea. Let me know what you think of the Component wrapper I made for this.

It may make sense to include validation on the Celsius field, if only because it would be slightly weird seeing an error on the F field if you're typing it in C. But then is it weird seeing the error on both fields? Maybe start without it and we can see how it feels.

Sweet. I'll hold off on handling celsius error handling for the time being.

fwextensions commented 1 year ago

Looks good! Can you move the FormMultiField files into /Components, so they're with the other form components?