Lf 4765 create get endpoint to fetch list of i ps #3755
Conversation
…prescription to its own route
…that pr is merged
…d utility functions
…nd filter returned irrigation tasks by result of that query
… after mock no longer needed on endpoint
antsgar
left a comment
antsgar
left a comment
There was a problem hiding this comment.
Sorry this took me a while to review! I had started it the other day and had to switch to the Soil breakdown. I've added a few comments, but it's overall looking good -- I'll re-review once you've addressed Joyce's comment
| // @ts-expect-error - farm_id is guaranteed here by the checkScope middleware with single argument | ||
| const farmAddonPartnerIds = await FarmAddonModel.getDistinctFarmAddonPartnerIds(farm_id); |
There was a problem hiding this comment.
If you know for sure farm_id won't be undefined, instead of @ts-expect-error you can use a non-null assertion operator
| // @ts-expect-error - farm_id is guaranteed here by the checkScope middleware with single argument | |
| const farmAddonPartnerIds = await FarmAddonModel.getDistinctFarmAddonPartnerIds(farm_id); | |
| const farmAddonPartnerIds = await FarmAddonModel.getDistinctFarmAddonPartnerIds(farm_id!); |
There was a problem hiding this comment.
I like having a comment for unclear assertions. Assertions should be used sparingly.
I have a test branch for a specific fix for this open mentioned with Joyces comment: #3755 (comment)
There was a problem hiding this comment.
I'd say ts-expect-error should be used even more sparingly, and my understanding is that it's meant to be used temporarily. If you're absolutely sure farm_id would be present I think the non-null assertion would be okay. I did look at the other branch, and although I like the idea of adding a type guard I honestly feel it overcomplicates the code with not that much added benefit. But I'm curious to know what others think too
There was a problem hiding this comment.
I think this is a point we disagree on.
For code reuse and readability for devs, if I were to copy this code I would likely not notice a trailing ! present in the code vs a commented @ts-expect-error. It is a single character and assertions are dangerous because they override some typescript typings.
I am fairly certain we have been using @ts-expect-error as a stop gap when we have not found the correct typescript solution to make it work. The non-null assertion feels like a final 'correct' solution in limited circumstances. So, if I copied this controller code erroneously without a checkScope in the accompanying routes file -- the assertion could be incorrect.
In the other branch -- I don't know what type guard you are talking about -- but the explicit type assertion that overrides the express Request type is placed next to the checkScope to make it clear they go together. This is an assertion done in the routes context right next to the checkScope it relies upon... still not ideal but avoids the @ts-expect-error. In the link you provided from Typescript they also put the non-null assertion right next to some context claiming it is correctly validated:
function processEntity(e?: Entity) {
validateEntity(e);
let s = e!.name; // Assert that e is non-null and access name
}
It feels like the end best solution will be that express needs to make some sort of generic that goes along with type NextFunction or something like that but thats not our problem. Maybe the cleanest/safest way for us right now is to double up on certain type guards and put them in the controller as well as well as the middleware? Do you like that any better?
There was a problem hiding this comment.
This is the type guard I'm referring to https://github.com/LiteFarmOrg/LiteFarm/pull/3772/files#diff-31ffdd44c889de461e56828613f90ff13ba191db724cda9bdb7b6e90e8937d48R45-R52.
A type assertion is as bad as a non-null assertion (arguably worse?) so my feeling is the balance between having a +185 -42 diff to end up with a solution that isn't ideal vs achieving a solution that isn't good either but just uses one character doesn't add up. I'd be totally okay with keeping the @ts-expect-error as a non-ideal solution too, that's not a blocker for me!
There was a problem hiding this comment.
Great, I would like to keep the ts-expect error then since it has a comment explaining its existence and we may not know the best way to type it.
Thanks for sharing the type guard, I think the one you are pointing too has nothing to do with the farm_id, the query param type guard check has been moved to middleware in this PR but in the draft PR it is outdated.
To be fair the actual code change to assert the correct type is like ~ +20, the rest is changing a core file to typescript and adding more types which is generally useful. I am really not saying we should do it but it seems like we are striving to have mostly correct typings -- I personally think this solution is possibly the most correct we can get with express.
To continue the best practice discussion -- because I like chatting about it 😄 :
A type assertion is as bad as a non-null assertion (arguably worse?)
Its not that either asserting that is necessarily bad, it is asserting out of context or to use as a temporary solution that is bad. There are genuine cases for each but being extra careful about both is important I think. For this case I think we would be using it here as a temporary solution, out of context, that could lead to serious code reuse errors.
The farm_id type guard that already exists in checkScope is two files away from the controller. If it were right beside it and not working in context (like the TS example, and other examples on internet I can find) I would agree non-null assertion.
Type casting assertions are also useful when maybe you cannot type check like external package or endpoint.
packages/api/src/controllers/irrigationPrescriptionController.ts
Outdated
Show resolved
Hide resolved
Duncan-Brain
requested review from
antsgar and
kathyavini
and removed request for
SayakaOno
|
I think I have addressed all the comments. Just noting that when I tested the full flow including creating a task I think some parts of task flow may not be correct:
|
@Duncan-Brain did you maybe not pull the most recent version of integration before you merged it? Those adjusts were merged in #3762 but I don't see that code on this branch. It's going to help with my testing, so I'll just re-merge integration now if that's okay 🙏 |
|
@Duncan-Brain actually, is your local code definitely the same as what you have pushed to this branch? Can you double-check? You have a line here: return shouldSend ? ESciAddon : Mocks;that controls when the mocking happens, but |
|
@kathyavini Thanks for merging integration, yes I think you are right. Type and conditional is updated. Must have had my frontend cache why I didn't see it 🙏 . Damned attention to detail haha |
|
Anto gave her verbal approval for this one (and we'll be moving to her review not blocking this week anyhow). And it looks good to me! |
|
Great thanks @kathyavini for your review and merging! |
Description
** REBASED - in case you pulled locally already
Waiting for LF-4767 to merge so I can delete frontend mock and add after_date param to query function.This implements the backend endpoint to get irrigation prescriptions from esci.
It also adds tests for this endpoint.
Notes:
Jira link: LF-4765
Type of change
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
Checklist: