Closed mahieakhtar closed 4 years ago
Please update fk_crops_id to fk_crop_id in your knex migration and controller if it's being used because fk_crops_id has been updated to fk_crop_id to make it consistent and to avoid confusion on migrations schema and DB diagram. So please replace fk_crops_id to fk_crop_id if your application is crashing and you want to save time.
@moonflare Thanks for your review. We refactored this PR based on your feedback, can you please review it again. @sowmya1408 Thanks for your review. We added the middleware, can you please review it again? @mahieakhtar fk_crops_id is changed to fk_crop_id - but be aware endpoints will not work, untill PR #180 is not merged to develop
@moonflare Thanks for your review. We refactored this PR based on your feedback, can you please review it again. @sowmya1408 Thanks for your review. We added the middleware, can you please review it again? @mahieakhtar fk_crops_id is changed to fk_crop_id - but be aware endpoints will not work, untill PR #180 is not merged to develop
@Daniel Its time to review and merge PR#180, Thank you.
@moonflare Thanks for your review. We refactored this PR based on your feedback, can you please review it again. @sowmya1408 Thanks for your review. We added the middleware, can you please review it again? @mahieakhtar fk_crops_id is changed to fk_crop_id - but be aware endpoints will not work, untill PR #180 is not merged to develop
@daniel Its time to review and merge PR#180, Thank you.
I can have a look at #180 later tonight.
Nice work overall. But there are some performance improvements we should do before we merge this one. There is a lot of over fetching data. We fire the same queries over and over. If we call one query once, we already have that data, no need to call it again. For example the query below, we run it 3 times I think.
knex('batches') .select('seeding_date', 'fk_crop_id') .where('id', batchId);
We should get all the data we need first, in one go if it makes sense, and then start calculating different metrics with that data. 🙌
Done 🙂
Approved
resolves #142 Status label is not part of this PR.