Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lf 3281 handle and convert as needed incoming temperature readings from ensemble #2692

Conversation

Duncan-Brain
Copy link
Collaborator

@Duncan-Brain Duncan-Brain commented May 23, 2023

** Sayaka or Kevin **
Right after merge this should be run on the beta database:

UPDATE sensor_reading
SET unit = 'C'
WHERE unit = 'Celsius';

Description

This was originally meant to handle the fact that we like to store values in only one unit (metric), and store units as the display unit. But became: lets store Celsius as 'C' and verify incoming units are expected.

The first commit shows most of what would be done to convert and handle both imperial and metricunits. What is not shown in that commit is:

  • package and lock files using convert-units version 2.3.4 (as opposed to frontend v3 beta -- without typescript configuration v3 beta convert-units has proven to be tough to configure like the frontend in plain JS -- unknown as to why)
  • Making frontend utilities such as components/Map/PreviewPopup/utils.js >> getTemperatureValue() to be dynamic instead of hardcoded.

The second commit tries to keep most of this structure in place and available but is now much stricter in terms of allowed values.

Improvements abandoned as I was overthinking things:

  • Handle soil water content incoming reading type better
    • If an invalid reading type is added the api handles it well, but Soil water content(SWC) is technically a valid reading type. Now as far as I know esci has no immediate intention to send us SWC data. I decided instead of running a migration to remove the partner reading type SWC from the esci partner_id I would just allow reading types of SWC with units of "Percent" and to store the unit as "%". It is pretty fixable in the future if we don't want this.

Other Notes:

  • SensorController is only really capable of handling ESCI as a partner so I made no attempt at making the controller extensible to other partners
  • If ESCI changes the data they send us we are not flexible
  • I don't really love my solution -- addReading() needs some rework and frontend and backend units could get messy we may need to strategize that

Jira link: LF-3281 also LF-3282

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): Postman queries:
    Post on localhost:5001/sensor/reading/partner/:partner_id/farm/:farm_id
    Header: authorization: farm_id
    Body:
{
    "7XZPYL":{
        "data": [
            {
                "parameter_category": "Soil water content",
                "unit": "Percent",
                "values": [10],
                "timestamps": ["2023-03-16 12:06:24.188428-04"],
                "validated":[false]
            },
            {
                "parameter_category": "Temperature",
                "unit": "Celsius",
                "values": [10],
                "timestamps": ["2023-02-16 12:06:24.188428-04"],
                "validated":[false]
            }, {
                "parameter_category": "Soil water potential",
                "unit": "kPa",
                "values": [10],
                "timestamps": ["2023-02-16 12:06:24.188428-04"],
                "validated":[false]
            }
        ]
    }
}

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

Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Populating my database nicely on incoming sensor data 🙂

@SayakaOno
Copy link
Collaborator

@Duncan-Brain I feel that the query should be run when you are available, so I won't merge this yet! I don't have write access to the beta DB, so we need Kevin's help.

@kcussen kcussen merged commit 774d180 into integration May 25, 2023
@kcussen kcussen deleted the LF-3281-handle-and-convert-as-needed-incoming-temperature-readings-from-ensemble branch May 25, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants