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-4626 Automatically detect field for sensor array #3653

Conversation

kathyavini
Copy link
Collaborator

@kathyavini kathyavini commented Jan 15, 2025

Description

The main content of this PR is geoUtils.ts. It contains two functions:

  1. getAreaLocationsContainingPoint -- Given a point and an array of polygon locations (e.g. the output of a locations selector), return those whose polygons (grid_points) contain the point being queried. On the current screens this is the one that would be used from the list of sensors page or an individual sensor's page
  2. getPointLocationsWithinPolygon -- Given a polygon (location object with grid_points) and a an array of point locations (e.g. the return from sensorSelector), return those whose points are contained within the queried polygon. On the current screens this is one to call from the Field Technology tab of a given location.

The second file (the little hook) is just some example code that can be discarded entirely OR adapted to our needs based on the need of the calling component -- I pictured this as the easiest place to configure the desired selectors for the locations we'll query (particularly if this will be called against the same locations multiple times / from different components) but it's also fine for the calling component to call one of the util functions directly.

To use the example hooks, you can call like this from within <SensorDetail>:

const areasContainingSensor = useFarmAreasContainingPoint(sensorInfo?.point);

And like this from within, e.g. <PureLocationDetailLayout /> -- there isn't a great component for this, but this is a stand in for the field technology tab until that is built

const containedSensors = useFarmSensorsContainedWithinArea(persistedFormData?.grid_points);

The returned value is the same format as using a location selector like e.g. animalLocationSelector

Why turf.js over maps.geometry.poly.containsLocation()

We only use the Google Maps functions (including the other use in our codebase of .containsLocation() within the context of a rendered map component, handled by google-map-react. As far as I understand it (although I'm realizing now we don't have screens yet for the new "confirm location" functionality so I'm not even sure this will remain true 😬 !!) we want to be able to detect a location to populate a multi-select without having to render a map.

We could create an independent API call to Google Maps outside of google-map-react to use .containsLocation() BUT there is really no reason to do that when we can do this independently. turf is a tiny package to begin with, that also lets us selectively install the functionality we need (here only @turf/boolean-point-in-polygon and the helpers to create the point and polygon features)

Jira link: https://lite-farm.atlassian.net/browse/LF-4626

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)

I logged the values in the example code above for some different farm map setups; nothing exhaustive though.

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 kathyavini added the enhancement New feature or request label Jan 15, 2025
@kathyavini kathyavini self-assigned this Jan 15, 2025
@kathyavini kathyavini requested review from a team as code owners January 15, 2025 23:54
@kathyavini kathyavini requested review from antsgar and Duncan-Brain and removed request for a team January 15, 2025 23:54
@kathyavini kathyavini marked this pull request as draft January 15, 2025 23:54
@kathyavini kathyavini changed the title LF-4626 [Nice to have] automatically detect field for sensor array LF-4626 Automatically detect field for sensor array Jan 22, 2025
Wrap (only) each call to the fixture-creating helpers in try/catch, and make sure other locations are still processed if one location errors
@kathyavini kathyavini marked this pull request as ready for review January 23, 2025 21:44
Copy link
Collaborator

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

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

Nice ! love the use of non-google utils.

Copy link
Collaborator

@antsgar antsgar left a comment

Choose a reason for hiding this comment

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

Love this!! Good call using Turf instead of Google Maps 👏

Not because I want to include them now, but do you think there'd be a way to add unit tests for these utilities?

@kathyavini
Copy link
Collaborator Author

Not because I want to include them now, but do you think there'd be a way to add unit tests for these utilities?

@antsgar Hmm, I think they should be fairly trivial to unit test (would just require some fake polygons and points). There isn't isn't too much logic in the files beyond the package call, though, do you think it's still worth testing (as we don't really want to test the package)?

@antsgar
Copy link
Collaborator

antsgar commented Jan 29, 2025

@kathyavini they're pretty lean but, you never know, sometimes the simplest chunks of code lead to someone making a mistake just because they think the code is too simple for that to happen 😂 I don't think we need to add tests right now though or that it's a blocker to have them

@kathyavini
Copy link
Collaborator Author

code lead to someone making a mistake just because they think the code is too simple for that to happen

I believe it; I could definitely be that someone 😂

@kathyavini kathyavini added this pull request to the merge queue Jan 29, 2025
Merged via the queue into integration with commit 93d7cf7 Jan 29, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants