-
Notifications
You must be signed in to change notification settings - Fork 83
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-4695 Create GET endpoint to fetch list of Ensemble devices #3665
base: LF-4625a-update-controller-for-sensor-addition
Are you sure you want to change the base?
LF-4695 Create GET endpoint to fetch list of Ensemble devices #3665
Conversation
This is not part of our original sensor implementation; it was a junk controller that only errors (it queries a non-existent property on the sensor table)
…ng data This includes a mocking function that can be deleted once we have real sensor and profile data coming in
Ultimately only one of the two is required -- probably on login makes more sense -- but for development the refresh on home screen is useful.
…n map menu and old sensor views
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description and comments were super helpful! And thank you for the frontend setup ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the only maybe errors I saw were:
- All the mocked sensors seem to show on all my farms - I only expected it with successful farm_addon row in table. (I manually inserted mine)
- Console errors (maybe these are grandfather sensor related so up to you to fix) but I believe happen when there are no sensors present on farm: using
undefined
to query old endpoint and type error on the slice.
ENSEMBLE_BRAND, | ||
); | ||
|
||
const { access_token } = await IntegratingPartnersModel.getAccessAndRefreshTokens( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding this to ensembleAPICall()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do that? I thought all the models had to be called from the controller as a rule 😂 (the ensemble API calls are in /util
). I was following what was written previously, but also thought that was the correct way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries if you aren't into it! I think its nice to keep it all in one spot rather than repeat it. Anto mentioned lean controllers and model services last year sometime and I thought services could be used by any backend file cuz why not?
We also use them in middleware/pre-checks, tests, other models/services... I think if jobs or 3rd party util files wanted to use them it seems fine. digitalOceanSpaces.js
seems analogous but we keep the keys in our env instead of our addon db table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think following the code pattern from the other thread, it's gonna to end up as simple as
const { sensors, sensor_arrays } = await getEnsembleSensors(farm_id)
in the controller! In which case yeah, why not throw this into the other function in that file 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omg it touches so much code to move this it into the API call 😂 But I think it's an improvement 👍
@@ -152,6 +157,107 @@ const sensorController = { | |||
}); | |||
} | |||
}, | |||
async getSensors(req, res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about getAddonSensors()
to be more specific that this would not get grandfathered/custom sensors?
Feel free to ignore since we are focusing on ESCI but if we are now trying for lighter controllers I might maybe expect a utility function or two to be addon agnostic like:
const [sensors, profiles] = getAndMapAddonSensors(farm_id)
or
farmAddon.forEach((addon) => {
const data = getAddonSensors(farm_id);
const [sensors,profiles] = mapAddonSensors(data.partner_id);
array.push(...)
etc.
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me (although it's not my preference), if rest of the team is onboard with the naming terminology (new sensors = addon sensors). Would you change the terminology when the old sensor flow is deleted, or maintain it as addon sensors? I don't think it's worth it if it's just a temporary distinction while the old data exists.
Re: the utility function looping through addons -- I like the idea and I bet we will want to do it when we have more partners. Where were you thinking of calling it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup feel free to ignore. I would maintain it as addonSensors()
long term probably.
I think getSensors implies all sensors but custom ones might be taken through the location route or something -- I forget exactly but you said the previous getSensors()
was unused so would never include custom sensors unless we changed that pattern... tldr I still see custom sensors like arduinos or manual measuring devices as important (long term) and they wouldn't use this function is my understanding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: the utility function looping through addons -- I like the idea and I bet we will want to do it when we have more partners. Where were you thinking of calling it?
Probably something unimaginative like /util/addonSensors.js
😂 . I just always like to build the loop and setup the mappings since its not too consuming and so the next person doesn't have to overthink it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting we would eventually extract the ensemble specific code!
But I was imagining something like this:
getSensors() {
// get partners' sensors
getAddonSensors()
// get custom sensors
getCustomSensors()
}
To see less diff later, I agree with moving the code to a util file!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antsgar could you take a look at this? I think in this code example directly above getAddonSensors()
would be an example of a service as you defined below? If getAddonSensors()
is defined in utils/ensemble.js
and wraps all the functionality here, it would require moving all the model calls into the getAddonSensors()
. This is different than how utils/ensemble.js
was set up, but I see actually now that @SayakaOno and @Duncan-Brain have set a precedent of calling models in util files with animal.js
and copyCropPlan.js
-- are those files are already services then, in a sense? Is utils/
still the right place for ensemble.js
if we go this direction? Or is it time to establish a /src/services
?
Edit: Yeah, the more I look at our utils the more I realize they are chock full of models anyway 😳 So I think it's totally fine to move everything into the ensemble
file, which might... be a service then?
@@ -27,7 +27,7 @@ const upload = multer({ storage }); | |||
|
|||
const router = express.Router(); | |||
|
|||
router.get('/:farm_id', SensorController.getSensorsByFarmId); | |||
router.get('/', SensorController.getSensors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we need some sort of auth check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree! But there is no get:sensors
in the permissions table -- should we migrate? Or let me know if you had a different check in mind.
And just to clarify you mean a check at that level but not necessarily limiting based on user role? I think here a case could even be made to limit on user roles (and we could grab the old add:sensors
permission record), but I don't think I've heard that discussed yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't thinking user role no. Weird there is no get:sensors
! Maybe I should have known that as the db migrator.
Should I make a separate ticket to add get:sensors and check the auth farm_id against integration farm id? Maybe thats not possible without a db resource to check against.... maybe the basic check with auth farm_id against header farm_id should be enough? Which one is that again maybe hasFarmAccess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkScope
is the one that creates a userFarm out of auth user_id
and headers farm_id
and queries that properly. I think it's the best one we have actually. hasFarmAccess
I believe we decided was worthless unless it is checking an actual entity -- although every time I look at that one I have to re-learn what the heck it does 😝
Since it's not merged yet maybe the permissions additions can be appended to the open migration ticket? If you don't mind extra migrations I can add it here too (if we do want all user roles get on sensors). I'm wondering if we also want to create user role permissions on adding farm integrating partner / integrating addon? To be honest I had forgotten permissions entirely and we should double check requirements there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah lets check it out monday. It makes sense to add the migration in the other migrration file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For getting the sensors, correct, we do need a
get:sensors
permission, even if all user roles will be entitled to it. Thiw will allow us to add thecheckScope
middleware here which will ensure that the farm_id sent in the header is a farm that the user has access to. - We will also need an
add:farm_addon
permission like you mention @kathyavini. - We'll probably also need permissions for other new endpoints -- like the endpoint to get an existing farn addon (something like
get:farm_addons
I assume, and for the endpoints that will retrieve a sensor's reading. We could add an extra migration down the line though, that's okay. - I completely forgot about permissions too 😅 thanks for remembering!!
packages/api/src/util/ensemble.js
Outdated
let selectedSensor = sensors[0]; | ||
|
||
for (const sensor of sensors) { | ||
if (sensor.latest_position.depth < selectedSensor.latest_position.depth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sayaka made a utility for custom comparators for JS .sort()
function on the frontend.. could consider that here or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll post in Slack but I don't think this is the right comparison anyway!
@@ -30,7 +31,10 @@ export default function Home({ history }) { | |||
fileUrls: [userFarm.farm_image_url], | |||
}); | |||
|
|||
const { refetch: refetchSensors } = useGetSensorsQuery(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its temporary for devs maybe a //TODO comment or something? If it does it on login I can suffer through re-logs haha. Also wondering if it should be only done on /map
so the data isnt stale (last seen = last login datetime)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So re: /map in particular -- that one doesn't work because with the API calls involved, the data doesn't update in time to see it reflected in that render. That was the rationale for /Home
. It's not necessarily only for development if stale data (like you just described with wanting to update before showing map) is a concern of ours; logins are not generally that frequent.
But this is an example of the you can take it or leave it code! For instance, I think given our need to display of "latest reading date" this refetch might find a happier home in a different component that doesn't exist yet?
@@ -238,6 +241,11 @@ export const api = createApi({ | |||
}), | |||
}, | |||
), | |||
getSensors: build.query<SensorData, void>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would consider being specific throughout the stack getAddonSensors()
.
@@ -238,6 +241,11 @@ export const api = createApi({ | |||
}), | |||
}, | |||
), | |||
getSensors: build.query<SensorData, void>({ | |||
query: () => `${sensorUrl}`, | |||
keepUnusedDataFor: 60 * 60 * 24 * 365, // 1 year |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Request for comment on - Are you thinking this is a good standard for 3rd party addons? Is the default 60s and would other endpoints benefit from this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good question. I started to explain this one but it was end of day yesterday so I left it!
This aggressive caching is because the sensors will otherwise disappear from the map when the cache expires (which is yes, 60 sec by default). I can't speak to whether it should apply to other addons as I have no idea what other addon data we would be fetching!
Even for sensors I could also see it argued that one year is a bit excessive; if the query is re-run on login probably a week is not quite enough (logins can be less frequent), so the year is meant as a basic "don't purge it" setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yeah makes me wonder if we should set some new default value for all our endpoints?
…r standalone sensors
…e depth values, select based on absolute value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the prompt reviews @SayakaOno and @Duncan-Brain ❤️
I think I responded to all the comments in line other than the errors you mentioned Duncan -- I'll address those below! What I picked out as a few open questions:
- Do we want to name this sensor flow (and so the endpoint, and the RTK Query endpoint and hook)
addonSensor
(and sogetAddonSensors
)? I didn't feel comfortable implementing that without checking with team because it will affect a lot of frontend code. I also wonder if naming it like that would suggest we should be sending thepartner_id
as a parameter? Curious for @SayakaOno and @antsgar's input - Do we want all users have this information available? I forget if we ever discussed limiting based on user role?
Re: errors
- Yes, thank you for catching that! So apparently by default RTK query doesn't invalidate the cache when the endpoint returns an error! Surprising actually! Probably could be configured OR we could manually clear on farm change as with other data, but I loved @SayakaOno's suggestion to return empty data instead of 404 which will fix this issue automagically
- That second one I have never seen -- I didn't touch that flow (the old sensors via locations > sensorSlice) so I'm surprised you wouldn't see that same error on integration. To me it reads like either your old sensor data has a bad record or maybe you have the new tables migrated locally so your old sensor couldn't get its integrating partner id? But in either case yeah, it's an error of the old flow so I don't think it's in scope for this PR. Interesting, though. It's a full type error crash?
@@ -152,6 +157,107 @@ const sensorController = { | |||
}); | |||
} | |||
}, | |||
async getSensors(req, res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me (although it's not my preference), if rest of the team is onboard with the naming terminology (new sensors = addon sensors). Would you change the terminology when the old sensor flow is deleted, or maintain it as addon sensors? I don't think it's worth it if it's just a temporary distinction while the old data exists.
Re: the utility function looping through addons -- I like the idea and I bet we will want to do it when we have more partners. Where were you thinking of calling it?
@@ -27,7 +27,7 @@ const upload = multer({ storage }); | |||
|
|||
const router = express.Router(); | |||
|
|||
router.get('/:farm_id', SensorController.getSensorsByFarmId); | |||
router.get('/', SensorController.getSensors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree! But there is no get:sensors
in the permissions table -- should we migrate? Or let me know if you had a different check in mind.
And just to clarify you mean a check at that level but not necessarily limiting based on user role? I think here a case could even be made to limit on user roles (and we could grab the old add:sensors
permission record), but I don't think I've heard that discussed yet.
packages/api/src/util/ensemble.js
Outdated
let selectedSensor = sensors[0]; | ||
|
||
for (const sensor of sensors) { | ||
if (sensor.latest_position.depth < selectedSensor.latest_position.depth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll post in Slack but I don't think this is the right comparison anyway!
@@ -30,7 +31,10 @@ export default function Home({ history }) { | |||
fileUrls: [userFarm.farm_image_url], | |||
}); | |||
|
|||
const { refetch: refetchSensors } = useGetSensorsQuery(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So re: /map in particular -- that one doesn't work because with the API calls involved, the data doesn't update in time to see it reflected in that render. That was the rationale for /Home
. It's not necessarily only for development if stale data (like you just described with wanting to update before showing map) is a concern of ours; logins are not generally that frequent.
But this is an example of the you can take it or leave it code! For instance, I think given our need to display of "latest reading date" this refetch might find a happier home in a different component that doesn't exist yet?
@@ -238,6 +241,11 @@ export const api = createApi({ | |||
}), | |||
}, | |||
), | |||
getSensors: build.query<SensorData, void>({ | |||
query: () => `${sensorUrl}`, | |||
keepUnusedDataFor: 60 * 60 * 24 * 365, // 1 year |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good question. I started to explain this one but it was end of day yesterday so I left it!
This aggressive caching is because the sensors will otherwise disappear from the map when the cache expires (which is yes, 60 sec by default). I can't speak to whether it should apply to other addons as I have no idea what other addon data we would be fetching!
Even for sensors I could also see it argued that one year is a bit excessive; if the query is re-run on login probably a week is not quite enough (logins can be less frequent), so the year is meant as a basic "don't purge it" setting.
ENSEMBLE_BRAND, | ||
); | ||
|
||
const { access_token } = await IntegratingPartnersModel.getAccessAndRefreshTokens( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do that? I thought all the models had to be called from the controller as a rule 😂 (the ensemble API calls are in /util
). I was following what was written previously, but also thought that was the correct way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and I really like the idea of mocking the data as needed so that we can unblock the UI tasks! Left a few comments
@@ -591,6 +591,8 @@ export function* fetchAllSaga() { | |||
const { has_consent, user_id, farm_id } = yield select(userFarmSelector); | |||
if (!has_consent) return history.push('/consent'); | |||
|
|||
yield put(api.endpoints.getSensors.initiate()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is just to make the UI work for now, correct? Eventually I'd hope all sensor related API calls to go through RTK Query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is going through RTK Query (RTK Query's apiSlice is api.endpoints
), it's just that the RTK Query call is initiated by the saga instead of in a component hook call. It doesn't have anything to do with UI, it's just a question of timing -- I placed it here because this is our "fetch data on login" code block.
But if you mean that you prefer to keep the query calls in hooks/components (the useGetSensorsQuery
format), that's totally possible, e.g. we could keep the component call in /home
and remove this, it would just be slightly different from when we fetch the other farm data on login, but honestly I don't think it would be a perceptible difference.
const sensor = { | ||
name: device.name, | ||
external_id: device.esid, | ||
sensor_reading_types: device.parameter_types.map( | ||
(type) => ENSEMBLE_READING_TYPES_MAPPING[type], | ||
), | ||
last_seen: device.last_seen, | ||
point: { | ||
lat: device.latest_position.coordinates.lat, | ||
lng: device.latest_position.coordinates.lng, | ||
}, | ||
depth: device.latest_position.depth, | ||
depth_unit: 'cm', // to be confirmed | ||
profile_id: device.profile_id, | ||
|
||
// The following only for backwards compatibility until old sensor flow is removed | ||
location_id: device.esid, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT but perhaps this could be modularized to its own function so that this function is a bit leaner/easier to read, something like mapDeviceToSensor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, and it's also breaking the pattern of keeping Ensemble-related code in utils/ensemble.js
so I will definitely move that 🙏
@@ -27,7 +27,7 @@ const upload = multer({ storage }); | |||
|
|||
const router = express.Router(); | |||
|
|||
router.get('/:farm_id', SensorController.getSensorsByFarmId); | |||
router.get('/', SensorController.getSensors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For getting the sensors, correct, we do need a
get:sensors
permission, even if all user roles will be entitled to it. Thiw will allow us to add thecheckScope
middleware here which will ensure that the farm_id sent in the header is a farm that the user has access to. - We will also need an
add:farm_addon
permission like you mention @kathyavini. - We'll probably also need permissions for other new endpoints -- like the endpoint to get an existing farn addon (something like
get:farm_addons
I assume, and for the endpoints that will retrieve a sensor's reading. We could add an extra migration down the line though, that's okay. - I completely forgot about permissions too 😅 thanks for remembering!!
({ uuid }) => uuid === farmEnsembleOrganizationid, | ||
); | ||
|
||
const devices = await getOrganizationDevices(organization.pk, access_token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of related to the comment above -- from this point forward, the code is pretty Ensemble specific, correct? This doesn't need to be part of this ticket and I mentioned it in the technical notes but don't think I actually included it in any of the tickets yet -- my thinking was that we could build an Ensemble service, which would be a class with different methods destined to connect to Ensemble, fetch the data and parse it as needed. Anything related to the Ensemble integration specifically would go in that service, which would include all of this code. The Ensemble service could use the existing token/auth/API call utils to make the calls to their API, and the rest of the utils would probably end up being deleted or moved/refactored to the new service if needed.
In general, I'd like for us to move towards this sort of architecure where we have business logic live in a separate layer from the controller. Right now, our controllers feel really bloated and hard to read, and it seems controllers should actually only be in charge of parsing requests and sending an appropriate response instead of having to deal with the ins and outs of the logic in between.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antsgar For whoever starts off this new architecture, do you have an idea or resources about the service method arch?
Basically - Folder/file structure and general constraints (don't do's)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a couple of articles that explain the service concept
https://www.coreycleary.me/what-is-the-difference-between-controllers-and-services-in-node-rest-apis
https://medium.com/codex/better-than-mvc-537e61928c14
You can find more info if you search for the MVCS pattern -- most of it will be framework/language agnostic.
In terms of folder/file structure, services would be as important as models or controllers, so I imagine we'd just create a new services folder in /src and create a new file for each new service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my thinking was that we could build an Ensemble service, which would be a class with different methods destined to connect to Ensemble, fetch the data and parse it as needed. Anything related to the Ensemble integration specifically would go in that service, which would include all of this code. The Ensemble service could use the existing token/auth/API call utils to make the calls to their API, and the rest of the utils would probably end up being deleted or moved/refactored to the new service if needed.
I wonder if the setup was actually already pretty closer to MVCS and I complicated the situation by doing the device -> sensor mapping in the controller? Other than the mapping (which I'll move), the data fetching and parsing functions, although I import them into this controller, all live in one module (utils/ensemble.js
) and get imported here, a bit like the code example from the first link above, and I'm curious how that file differs from a service.
Is it particularly the question of who calls the models?
…x conflicts in sensor controller
…-4695-create-get-endpoint-to-fetch-list-of-ensemble-devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you guys, I think this PR is ready for re-review 🙏 It's a huge diff so I want to re-iterate that the backend changes are the important ones!
Here are the main changes since initial reviews:
- All Ensemble-specific code has been removed from the controllers and into
ensemble.js
- because of the nature of the dependency between branches, I did this same refactor for
farmAddonController
in this branch as well
- because of the nature of the dependency between branches, I did this same refactor for
- implemented the organization --> organisation change we discussed in tech daily (this is most of the diff!)
- moved fetching the access token into
ensembleAPICall
as @Duncan-Brain suggested. This also created quite a diff since it had previously been passed as a parameter to most functions - more minor refactors as suggested in PR comments. There were quite a few PR comments so please let me know if I missed one 🙏 Thank you!
- profiles are now referred to as sensorArrays (or in the response object key
sensor_arrays
) - updates to reflect the new migration + models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its coming together nice work!
I like everything in ensemble there, it helps me really picture copying and reusing this structure for use with other manufactures. Manufacturer.mapDeviceToSensor, Manufacturer.mapSensorsToProfiles.. etc good stuff 👍
@@ -91,9 +91,9 @@ class FarmAddon extends Model { | |||
return FarmAddon.query().patch({ webhook_id: webhookId }).where('farm_id', farmId); | |||
} | |||
|
|||
static async getOrganizationId(farmId, addonPartnerId) { | |||
static async getOrganisationIds(farmId, addonPartnerId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have kept it singular because .first()
but I see why you did it, maybe theres a better name? No biggie either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that "organisation" is still in the singular is the part that corresponds to the .first()
😂 But I'm not hugely into that name myself... getOrganisationIdentifiers
? getOrganisationKeys
?
This refers to the /farm_addon POST controller which was refactored on this branch
I didn’t review all the changes, but I’ve confirmed that my comments were addressed. Please go ahead and have it merged once you’ve received approvals from others. Thank you! |
const getValidEnsembleOrg = async (org_uuid) => { | ||
const allRegisteredOrganisations = await getEnsembleOrganisations(); | ||
|
||
const organisation = allRegisteredOrganisations.find(({ uuid }) => uuid === org_uuid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Teeny thing if you end up making more commits.. could probably return here without const and rename function away from "valid"
Description
This is the GET endpoint for Ensemble devices, written with the aim of unblocking the frontend (sensor list and map view tickets). Therefore, I have temporarily mocked the necessary data (location, depth, profile assignment) that we need for building our UI but that Ensemble is not returning to us yet. This is done in a dedicated mocking function that should be removed before release.
This PR has three components that vary a lot in importance!
Most important: backend controller and helper functions
THIS IS THE ACTUAL PR CONTENT; PLEASE REVIEW THIS IN PARTICULAR 🙏 I have indicated in comments what is temporary (e.g. the mocking function and the use of
location_id
because it is integral to the old sensor logic) and what still needs confirmation from Ensemble like the shape of incoming location data.Integration into the frontend (needed but feel free to take or leave the particular implementations)
Note re-fetching sensor data on the Home page is not necessary at all, but as it will randomly re-regenerate the mocked data I found it very useful for development and left it in. Only if we wanted to aggressively re-fetch would we want to leave that in for release (edit: sorry I take that back, our need for
last_seen
means we do need to do this! But doesn't have to necessarily be in Home).If any of this code is useful please go ahead and use it for the frontend work but feel free to take or discard any part, or adjust as needed in your tickets. It is offered only in case it is useful.
Backwards compatibility code (please skip in terms of review)
I am on the fence about whether I even want to include this -- it's in a single commit for ease of revert -- but the last commit (3b07ad6) changes are just what's needed to keep the old sensor frontend from Type Error crashing. I mainly included it so @Duncan-Brain could click through the map flow without encountering a crash every two clicks.
For the time being, these adjusts allow the old sensor code to co-exist with the new sensor code and the new sensors to work within the old sensor views. But please don't spend any time reviewing this code, it's just for easing our development work right now and I expect all of those files to be removed in the "remove old sensor flow" task.
Jira link: https://lite-farm.atlassian.net/browse/LF-4695
Type of change
How Has This Been Tested?
I will send a good organization_uuid in Slack that returns up to 12 sensors.
Checklist: