rcpch / rcpch-audit-engine

Epilepsy12 Audit Platform
https://e12.rcpch.ac.uk/
GNU Affero General Public License v3.0
5 stars 5 forks source link

Eatyourpeas/aggregation-shmaggregation-fix #916

Closed eatyourpeas closed 4 months ago

eatyourpeas commented 4 months ago

Overview

This brings together two observations of errors in the console, errors in the UI and a request that all table values represented only completed audit records. The issue related to a filtering error with an empy aggregation_level key being added to a the interpolated string of a query parameter. This led to a database matching error and django console errors with every aggregation. The fix is a simple one - it tests for the aggregation_level key being None and terminates the query at that point.

Code changes

  1. Complete refactor of the aggregate_by file which will be difficult to review because I have reorganised the functions with comments to make it clearer what they all do. Those related to spreadsheet download are at the bottom.
  2. There is a new function: filter_completed_cases_at_one_year_by_abstraction_level which generates the list of cases. This function is called from within update_kpi_aggregation_model, itself called from within update_kpi_aggregation_models. It ensures only completed audit cases are included.
  3. get_filtered_cases_queryset_for has been refactored also to include only completed cases. The conditionals relating to Wales vs England have been removed. Note this function is only used in the tests and when pulling aggregation results from disk. It is not used in the aggregation queries themselves.
  4. Refactor tests. Not all tests explicitly expected fully completed audit cases, so this has been updated
  5. An obvious error in a test relating to ComorbidityList which arose after has been fixed. It is not clear why it was not failing previously

Documentation changes (done or required as a result of this PR)

Bug fixes only

Related Issues

Hopefully will address #907 king's lynn issue but will only be clear once merged to live