When @eatyourpeas, @dc2007git and I mob coded on https://github.com/rcpch/rcpch-audit-engine/pull/941 we felt like there was a simpler way to express the code but focused on fixing the report with the minimum possible changes.
This PR tries to capture as many simplifications as possible before the context inevitably falls out of my head!
The core change is to use the functions from aggregate_by.py consistently across all sheets we generate in the report. To achieve this I broke them out into two:
get_kpi_aggregation_rows
Extracts all the rows from the KPI aggregation model and adds an additional key_field (eg Country, NHS region...)
create_kpi_aggregation_dataframe
Now just loops through all the measures which a4rre hard-coded at the top of kpi.py as not all measures are included in the report
It then uses the existing create_kpi_report_row function to add the row to the dataframe. That function defends against missing/null/zero field values
These changes allow us to push the sheet specific logic back up into kpi.py, for example combining local health boards in Wales and trusts in England when generating the "HBT" sheet.
I've also linked the definition of the measures directly to the fields on the models. This doesn't change any functionality but makes it clearer they're linked and in the unlikely event we change them we would automatically update here.
That means we can pass the Django field definition all the way down to create_kpi_report_row, meaning one less parameter for all the function calls. This is maybe not clearer than before but felt like a logical follow on from linking directly to the model definitions at the top level of kpi.py.
Questions
[x] ~Our current code and the template spreadsheet group all the rows by measure except for the HBT sheet where it's grouped by ODS code. This refactor has broken that. Do we need to maintain that behaviour?~
When @eatyourpeas, @dc2007git and I mob coded on https://github.com/rcpch/rcpch-audit-engine/pull/941 we felt like there was a simpler way to express the code but focused on fixing the report with the minimum possible changes.
This PR tries to capture as many simplifications as possible before the context inevitably falls out of my head!
The core change is to use the functions from aggregate_by.py consistently across all sheets we generate in the report. To achieve this I broke them out into two:
get_kpi_aggregation_rowskey_field(eg Country, NHS region...)create_kpi_aggregation_dataframecreate_kpi_report_rowfunction to add the row to the dataframe. That function defends against missing/null/zero field valuesThese changes allow us to push the sheet specific logic back up into
kpi.py, for example combining local health boards in Wales and trusts in England when generating the "HBT" sheet.I've also linked the definition of the measures directly to the fields on the models. This doesn't change any functionality but makes it clearer they're linked and in the unlikely event we change them we would automatically update here.
That means we can pass the Django field definition all the way down to
create_kpi_report_row, meaning one less parameter for all the function calls. This is maybe not clearer than before but felt like a logical follow on from linking directly to the model definitions at the top level ofkpi.py.Questions