-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Dashboard] Crop Probability Module #232
Conversation
…dashboard-crop-information
…dashboard-crop-information
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 @FaithDaka - looking through the data and changes I can definitely appreciate this is a much more complex set of tasks than originally communicated.
I really like that you have considered both a base set of crop information, and then station-specific changes/overrides.
I've made some small changes to the crop_data
table. I learnt it's possible to use multiple columns as the unique id - the benefit in doing this is it restricts the data that can be entered, now so that there is only a single crop-variety entry possible (e.g. maize-SC304
), along with some minor column edits.
I've also reversed the relationship, so now the crop_data
table has no reference to climate stations, but the climate_station_crop_data
table can be used to provide a station-specific overrides to default crop information. I think this should make it easier to allow work on crop data independent of station calculations, and it also removes the need for a 3rd station_data
table linking the two (so now just have two tables).
NOTE - I've applied all schema changes directly on top of existing migrations files, so you will need to reset your local db to re-apply the migration with updated definitions. I've also done my best to update frontend code where I've modified table names and routes, but apologies if I've missed anything which may need a quick fix when next working on 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.
Thanks again for all the updates. Find a few mostly minor comments in-line, I think the one thing that would be really nice to prioritise would be getting the data input validated using angular forms as they're quite a powerful tool which I'm slowly trying to implement more across the codebase. Otherwise looking good as a basis until we know exactly how we can interact with the climate api to generate probabilities for specific stations (and provide custom crop defintion overrides)
apps/picsa-apps/dashboard/src/app/modules/crop-information/crop-information.component.html
Outdated
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/crop-information/crop-information.page.ts
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/crop-information/crop-information.page.ts
Outdated
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/crop-information/crop-information.page.ts
Outdated
Show resolved
Hide resolved
...csa-apps/dashboard/src/app/modules/crop-information/pages/new_entry/new_entry.component.html
Outdated
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/crop-information/pages/new_entry/new_entry.page.ts
Outdated
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/crop-information/crop-information.service.ts
Outdated
Show resolved
Hide resolved
apps/picsa-apps/dashboard/src/app/modules/crop-information/crop-information.service.ts
Outdated
Show resolved
Hide resolved
…dashboard-crop-information
@chrismclarke Thank you for all the elaborate feedback in this PR. I've worked on all the requests and fixes. Screen.Recording.2024-03-10.at.11.54.09.mov |
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 for carefully addressing all the feedback points, very much appreciated. I think the form validation methods in particular really make the code a lot more maintainable in the long run and also simplifies some of the UI logic.
I still need need to add a global permissions system that provides more fine-grain control on who can/can't edit content, but for now I'll add a quick commit just to make it easier to edit existing rows.
Otherwise I'd say should be good to go!
Minor Updates:
a00447d - automatically navigate back to the crop list page after successful crop add
3ff5559 - additional form validation against db input
283070e - crop entry edit
e75b174 - add common crop picker input
Updated Demo
Screenity.video.-.Mar.11.2024.webm
label: 'Crop Information', | ||
href: '/crop-information', | ||
matIcon:'spa' | ||
}, |
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.
(+) Impressed you found an icon that looks like a crop, so weird mat-icons call it the spa icon...
water_upper: [0], | ||
length_lower: [0], | ||
length_upper: [0], | ||
}); |
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.
(+1) thanks for adding the form and validation methods, looks good to me both on db and UI validation.
I still haven't found a super clean way to generate angular forms from our existing db schemas, but will add a small follow-up just to validate the form output against the expected db input
Description
Discussion
Screenshots / Videos